From 840dbb22cfcbac87ae69e6bcd9f3b9b0cd1b1d4c Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Fri, 13 Mar 2020 12:32:25 -0400 Subject: [PATCH] resolves https://github.com/elastic/kibana/issues/58905 The current index threshold alert uses a `size` limit on term aggregation, when used, but does not sort the buckets, so it's just using descending count on the grouped buckets as the sort to determine what to return. The watcher API for the index threshold notes this as "top N of", implying a sort. This PR applies sorting when the using `groupBy: top`, and the `aggType != count`. For count, ES is already sorting the way we want. The sort is calculated as a separate agg beside the date_range aggregation, which is the same metrics agg specified in the query - `aggType(aggField)`. This field is then referenced in a new `order` property in the terms agg, using 'asc' sorting for `min`, and `desc` sorting for `avg`, `max`, and `sum`. This doesn't change the shape of the output at all, just changes which term buckets will be returned, if there are more term buckets than requested with the `termSize` parameter. --- .../index_threshold/lib/time_series_query.ts | 26 +++++++++++ .../time_series_query_endpoint.ts | 46 +++++++++++++++---- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts index a4f64c0f37f41a..0382792dafb35b 100644 --- a/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts +++ b/x-pack/plugins/alerting_builtins/server/alert_types/index_threshold/lib/time_series_query.ts @@ -77,6 +77,15 @@ export async function timeSeriesQuery( }, }, }; + + // if not count add an order + if (!isCountAgg) { + const sortOrder = aggType === 'min' ? 'asc' : 'desc'; + aggParent.aggs.groupAgg.terms.order = { + sortValueAgg: sortOrder, + }; + } + aggParent = aggParent.aggs.groupAgg; } @@ -89,6 +98,16 @@ export async function timeSeriesQuery( }, }, }; + + // if not count, add a sorted value agg + if (!isCountAgg) { + aggParent.aggs.sortValueAgg = { + [aggType]: { + field: aggField, + }, + }; + } + aggParent = aggParent.aggs.dateAgg; // finally, the metric aggregation, if requested @@ -106,13 +125,20 @@ export async function timeSeriesQuery( const logPrefix = 'indexThreshold timeSeriesQuery: callCluster'; logger.debug(`${logPrefix} call: ${JSON.stringify(esQuery)}`); + // note there are some commented out console.log()'s below, which are left + // in, as they are VERY useful when debugging these queries; debug logging + // isn't as nice since it's a single long JSON line. + + // console.log('time_series_query.ts request\n', JSON.stringify(esQuery, null, 4)); try { esResult = await callCluster('search', esQuery); } catch (err) { + // console.log('time_series_query.ts error\n', JSON.stringify(err, null, 4)); logger.warn(`${logPrefix} error: ${JSON.stringify(err.message)}`); throw new Error('error running search'); } + // console.log('time_series_query.ts response\n', JSON.stringify(esResult, null, 4)); logger.debug(`${logPrefix} result: ${JSON.stringify(esResult)}`); return getResultFromEs(isCountAgg, isGroupAgg, esResult); } diff --git a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts index ee44c7f25cf61f..1aa1d3d21f00d1 100644 --- a/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts +++ b/x-pack/test/alerting_api_integration/spaces_only/tests/alerting/builtin_alert_types/index_threshold/time_series_query_endpoint.ts @@ -196,14 +196,6 @@ export default function timeSeriesQueryEndpointTests({ getService }: FtrProvider const expected = { results: [ - { - group: 'groupA', - metrics: [ - [START_DATE_MINUS_2INTERVALS, 4 / 1], - [START_DATE_MINUS_1INTERVALS, (4 + 2) / 2], - [START_DATE_MINUS_0INTERVALS, (4 + 2 + 1) / 3], - ], - }, { group: 'groupB', metrics: [ @@ -212,12 +204,50 @@ export default function timeSeriesQueryEndpointTests({ getService }: FtrProvider [START_DATE_MINUS_0INTERVALS, (5 + 3 + 2) / 3], ], }, + { + group: 'groupA', + metrics: [ + [START_DATE_MINUS_2INTERVALS, 4 / 1], + [START_DATE_MINUS_1INTERVALS, (4 + 2) / 2], + [START_DATE_MINUS_0INTERVALS, (4 + 2 + 1) / 3], + ], + }, ], }; expect(await runQueryExpect(query, 200)).eql(expected); }); + it('should return correct sorted group for average', async () => { + const query = getQueryBody({ + aggType: 'avg', + aggField: 'testedValue', + groupBy: 'top', + termField: 'group', + termSize: 1, + dateStart: START_DATE_MINUS_2INTERVALS, + dateEnd: START_DATE_MINUS_0INTERVALS, + }); + const result = await runQueryExpect(query, 200); + expect(result.results.length).to.be(1); + expect(result.results[0].group).to.be('groupB'); + }); + + it('should return correct sorted group for min', async () => { + const query = getQueryBody({ + aggType: 'min', + aggField: 'testedValue', + groupBy: 'top', + termField: 'group', + termSize: 1, + dateStart: START_DATE_MINUS_2INTERVALS, + dateEnd: START_DATE_MINUS_0INTERVALS, + }); + const result = await runQueryExpect(query, 200); + expect(result.results.length).to.be(1); + expect(result.results[0].group).to.be('groupA'); + }); + it('should return an error when passed invalid input', async () => { const query = { ...getQueryBody(), aggType: 'invalid-agg-type' }; const expected = {