Skip to content

Commit

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

Fixes #171409 

This fixes weird behavior when attempting to filter more than 1000 rule
execution logs.

The execution log aggregator had two problems:

- It had a max bucket limit of 1000, while the KPI agg had a max bucket
limit of 10000, leading to inconsistent results
- For pagination, it reported the total number of logs by using a
cardinality aggregator, which wasn't subject to any bucket limit at all

The endpoint responds with a `total` (the cardinality) and an array of
`data` (a paginated slice of the aggregated logs). The way the data
table UI works is that it takes the `total` value, caps it at 1000, and
then generates that many blank rows to fill with whatever's in the
`data` array.

So as seen in the issue, we could easily run into a situation where:

1. User enters a filter query that last appeared more than 1000 logs ago
2. The cardinality agg accurately reports the number of logs that should
be fetched, and generates enough blank rows for these logs
3. The actual execution log agg hits the 1000 bucket limit, and returns
an empty `data` array

This PR fixes this by using a bucket sum aggregation to determine the
data table's `total` instead of a cardinality aggregation.

It also ups the bucket limit from 1000 to 10000, to match the bucket
limit from the summary KPI endpoint. This prevents a situation where:

1. User enters a filter query that last appeared in <1000 logs, but more
than 1000 logs ago
2. Summary KPI widget, with a bucket limit of 10000, accurately displays
a count of the <1000 logs this query matches
3. The data table is empty because the execution log bucket limit tops
out at 1000

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Xavier Mouligneau <xavier.mouligneau@elastic.co>
  • Loading branch information
3 people committed Jan 9, 2024
1 parent a100cbb commit 9d32f88
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 9d32f88

Please sign in to comment.