Skip to content

Commit

Permalink
[8.12] [RAM] Fix rule log aggregations when filtering >1k logs (#1…
Browse files Browse the repository at this point in the history
…72228) (#174557)

# Backport

This will backport the following commits from `main` to `8.12`:
- [[RAM] Fix rule log aggregations when filtering >1k logs
(#172228)](#172228)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Zacqary Adam
Xeper","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-01-09T23:17:13Z","message":"[RAM]
Fix rule log aggregations when filtering >1k logs (#172228)\n\n##
Summary\r\n\r\nFixes #171409 \r\n\r\nThis fixes weird behavior when
attempting to filter more than 1000 rule\r\nexecution logs.\r\n\r\nThe
execution log aggregator had two problems:\r\n\r\n- It had a max bucket
limit of 1000, while the KPI agg had a max bucket\r\nlimit of 10000,
leading to inconsistent results\r\n- For pagination, it reported the
total number of logs by using a\r\ncardinality aggregator, which wasn't
subject to any bucket limit at all\r\n\r\nThe endpoint responds with a
`total` (the cardinality) and an array of\r\n`data` (a paginated slice
of the aggregated logs). The way the data\r\ntable UI works is that it
takes the `total` value, caps it at 1000, and\r\nthen generates that
many blank rows to fill with whatever's in the\r\n`data`
array.\r\n\r\nSo as seen in the issue, we could easily run into a
situation where:\r\n\r\n1. User enters a filter query that last appeared
more than 1000 logs ago\r\n2. The cardinality agg accurately reports the
number of logs that should\r\nbe fetched, and generates enough blank
rows for these logs\r\n3. The actual execution log agg hits the 1000
bucket limit, and returns\r\nan empty `data` array\r\n\r\nThis PR fixes
this by using a bucket sum aggregation to determine the\r\ndata table's
`total` instead of a cardinality aggregation.\r\n\r\nIt also ups the
bucket limit from 1000 to 10000, to match the bucket\r\nlimit from the
summary KPI endpoint. This prevents a situation where:\r\n\r\n1. User
enters a filter query that last appeared in <1000 logs, but more\r\nthan
1000 logs ago\r\n2. Summary KPI widget, with a bucket limit of 10000,
accurately displays\r\na count of the <1000 logs this query
matches\r\n3. The data table is empty because the execution log bucket
limit tops\r\nout at 1000\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>\r\nCo-authored-by:
Xavier Mouligneau
<[email protected]>","sha":"9d32f889683cb6bc099d561d94e02af2feaf0d10","branchLabelMapping":{"^v8.13.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","Team:ResponseOps","Feature:Alerting/RulesManagement","v8.12.0","v8.13.0"],"title":"[RAM]
Fix rule log aggregations when filtering >1k
logs","number":172228,"url":"https://github.com/elastic/kibana/pull/172228","mergeCommit":{"message":"[RAM]
Fix rule log aggregations when filtering >1k logs (#172228)\n\n##
Summary\r\n\r\nFixes #171409 \r\n\r\nThis fixes weird behavior when
attempting to filter more than 1000 rule\r\nexecution logs.\r\n\r\nThe
execution log aggregator had two problems:\r\n\r\n- It had a max bucket
limit of 1000, while the KPI agg had a max bucket\r\nlimit of 10000,
leading to inconsistent results\r\n- For pagination, it reported the
total number of logs by using a\r\ncardinality aggregator, which wasn't
subject to any bucket limit at all\r\n\r\nThe endpoint responds with a
`total` (the cardinality) and an array of\r\n`data` (a paginated slice
of the aggregated logs). The way the data\r\ntable UI works is that it
takes the `total` value, caps it at 1000, and\r\nthen generates that
many blank rows to fill with whatever's in the\r\n`data`
array.\r\n\r\nSo as seen in the issue, we could easily run into a
situation where:\r\n\r\n1. User enters a filter query that last appeared
more than 1000 logs ago\r\n2. The cardinality agg accurately reports the
number of logs that should\r\nbe fetched, and generates enough blank
rows for these logs\r\n3. The actual execution log agg hits the 1000
bucket limit, and returns\r\nan empty `data` array\r\n\r\nThis PR fixes
this by using a bucket sum aggregation to determine the\r\ndata table's
`total` instead of a cardinality aggregation.\r\n\r\nIt also ups the
bucket limit from 1000 to 10000, to match the bucket\r\nlimit from the
summary KPI endpoint. This prevents a situation where:\r\n\r\n1. User
enters a filter query that last appeared in <1000 logs, but more\r\nthan
1000 logs ago\r\n2. Summary KPI widget, with a bucket limit of 10000,
accurately displays\r\na count of the <1000 logs this query
matches\r\n3. The data table is empty because the execution log bucket
limit tops\r\nout at 1000\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>\r\nCo-authored-by:
Xavier Mouligneau
<[email protected]>","sha":"9d32f889683cb6bc099d561d94e02af2feaf0d10"}},"sourceBranch":"main","suggestedTargetBranches":["8.12"],"targetPullRequestStates":[{"branch":"8.12","label":"v8.12.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.13.0","branchLabelMappingKey":"^v8.13.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172228","number":172228,"mergeCommit":{"message":"[RAM]
Fix rule log aggregations when filtering >1k logs (#172228)\n\n##
Summary\r\n\r\nFixes #171409 \r\n\r\nThis fixes weird behavior when
attempting to filter more than 1000 rule\r\nexecution logs.\r\n\r\nThe
execution log aggregator had two problems:\r\n\r\n- It had a max bucket
limit of 1000, while the KPI agg had a max bucket\r\nlimit of 10000,
leading to inconsistent results\r\n- For pagination, it reported the
total number of logs by using a\r\ncardinality aggregator, which wasn't
subject to any bucket limit at all\r\n\r\nThe endpoint responds with a
`total` (the cardinality) and an array of\r\n`data` (a paginated slice
of the aggregated logs). The way the data\r\ntable UI works is that it
takes the `total` value, caps it at 1000, and\r\nthen generates that
many blank rows to fill with whatever's in the\r\n`data`
array.\r\n\r\nSo as seen in the issue, we could easily run into a
situation where:\r\n\r\n1. User enters a filter query that last appeared
more than 1000 logs ago\r\n2. The cardinality agg accurately reports the
number of logs that should\r\nbe fetched, and generates enough blank
rows for these logs\r\n3. The actual execution log agg hits the 1000
bucket limit, and returns\r\nan empty `data` array\r\n\r\nThis PR fixes
this by using a bucket sum aggregation to determine the\r\ndata table's
`total` instead of a cardinality aggregation.\r\n\r\nIt also ups the
bucket limit from 1000 to 10000, to match the bucket\r\nlimit from the
summary KPI endpoint. This prevents a situation where:\r\n\r\n1. User
enters a filter query that last appeared in <1000 logs, but more\r\nthan
1000 logs ago\r\n2. Summary KPI widget, with a bucket limit of 10000,
accurately displays\r\na count of the <1000 logs this query
matches\r\n3. The data table is empty because the execution log bucket
limit tops\r\nout at 1000\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<[email protected]>\r\nCo-authored-by:
Xavier Mouligneau
<[email protected]>","sha":"9d32f889683cb6bc099d561d94e02af2feaf0d10"}}]}]
BACKPORT-->

Co-authored-by: Zacqary Adam Xeper <[email protected]>
  • Loading branch information
kibanamachine and Zacqary authored Jan 10, 2024
1 parent 47b78f0 commit cec6529
Show file tree
Hide file tree
Showing 12 changed files with 876 additions and 195 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import { getOAuthClientCredentialsAccessToken } from '../lib/get_oauth_client_cr
import { OAuthParams } from '../routes/get_oauth_access_token';
import { eventLogClientMock } from '@kbn/event-log-plugin/server/event_log_client.mock';
import { GetGlobalExecutionKPIParams, GetGlobalExecutionLogParams } from '../../common';
import { estypes } from '@elastic/elasticsearch';

jest.mock('@kbn/core-saved-objects-utils-server', () => {
const actual = jest.requireActual('@kbn/core-saved-objects-utils-server');
Expand Down Expand Up @@ -3419,6 +3420,10 @@ describe('getGlobalExecutionLogWithAuth()', () => {
executionUuidCardinality: { doc_count: 5, executionUuidCardinality: { value: 5 } },
},
},
hits: {
total: { value: 5, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
};
describe('authorization', () => {
test('ensures user is authorised to access logs', async () => {
Expand Down Expand Up @@ -3474,6 +3479,10 @@ describe('getGlobalExecutionKpiWithAuth()', () => {
},
},
},
hits: {
total: { value: 5, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
};
describe('authorization', () => {
test('ensures user is authorised to access kpi', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { estypes } from '@elastic/elasticsearch';
import { fromKueryExpression } from '@kbn/es-query';
import {
getExecutionLogAggregation,
Expand Down Expand Up @@ -485,7 +486,15 @@ describe('getExecutionLogAggregation', () => {

describe('formatExecutionLogResult', () => {
test('should return empty results if aggregations are undefined', () => {
expect(formatExecutionLogResult({ aggregations: undefined })).toEqual({
expect(
formatExecutionLogResult({
aggregations: undefined,
hits: {
total: { value: 0, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
})
).toEqual({
total: 0,
data: [],
});
Expand All @@ -494,6 +503,10 @@ describe('formatExecutionLogResult', () => {
expect(
formatExecutionLogResult({
aggregations: { executionLogAgg: undefined as unknown as ExecutionUuidAggResult },
hits: {
total: { value: 5, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
})
).toEqual({
total: 0,
Expand Down Expand Up @@ -554,6 +567,10 @@ describe('formatExecutionLogResult', () => {
executionUuidCardinality: { doc_count: 1, executionUuidCardinality: { value: 1 } },
},
},
hits: {
total: { value: 5, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
};
expect(formatExecutionLogResult(results)).toEqual({
data: [
Expand Down Expand Up @@ -675,6 +692,10 @@ describe('formatExecutionLogResult', () => {
executionUuidCardinality: { doc_count: 2, executionUuidCardinality: { value: 2 } },
},
},
hits: {
total: { value: 10, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
};
expect(formatExecutionLogResult(results)).toEqual({
data: [
Expand Down Expand Up @@ -918,6 +939,10 @@ describe('formatExecutionKPIAggBuckets', () => {
expect(
formatExecutionKPIResult({
aggregations: undefined,
hits: {
total: { value: 0, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
})
).toEqual({ failure: 0, success: 0, unknown: 0, warning: 0 });
});
Expand Down Expand Up @@ -951,6 +976,10 @@ describe('formatExecutionKPIAggBuckets', () => {
},
},
},
hits: {
total: { value: 21, relation: 'eq' },
hits: [],
} as estypes.SearchHitsMetadata<unknown>,
};

expect(formatExecutionKPIResult(results)).toEqual({
Expand Down
Loading

0 comments on commit cec6529

Please sign in to comment.