From ee0ee70aa16096326d0a8abd3ac76c77deedfc1f Mon Sep 17 00:00:00 2001 From: Matias Chomicki Date: Fri, 2 Jun 2023 12:50:23 +0200 Subject: [PATCH] Loki: Exclude queries using DISTINCT from query splitting (#69377) * Query utils: add function to identify distinct queries * Chore: return false when the expected node is found * Query splitting: exclude distinct queries from splitting * Group queries excluded from splitting --- .../datasource/loki/querySplitting.test.ts | 18 +++++++++++++-- .../plugins/datasource/loki/querySplitting.ts | 12 ++++++---- .../datasource/loki/queryUtils.test.ts | 13 +++++++++++ .../app/plugins/datasource/loki/queryUtils.ts | 22 ++++++++++++++++--- 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/public/app/plugins/datasource/loki/querySplitting.test.ts b/public/app/plugins/datasource/loki/querySplitting.test.ts index 3a441f61f35..4484ef2f481 100644 --- a/public/app/plugins/datasource/loki/querySplitting.test.ts +++ b/public/app/plugins/datasource/loki/querySplitting.test.ts @@ -182,6 +182,19 @@ describe('runSplitQuery()', () => { expect(datasource.runQuery).toHaveBeenCalledTimes(1); }); }); + test('Groups queries using distinct', async () => { + const request = getQueryOptions({ + targets: [ + { expr: '{a="b"} | distinct field', refId: 'A' }, + { expr: 'count_over_time({c="d"} | distinct something [1m])', refId: 'B' }, + ], + range, + }); + await expect(runSplitQuery(datasource, request)).toEmitValuesWith(() => { + // Queries using distinct are omitted from splitting + expect(datasource.runQuery).toHaveBeenCalledTimes(1); + }); + }); test('Respects maxLines of logs queries', async () => { const { logFrameA } = getMockFrames(); const request = getQueryOptions({ @@ -199,17 +212,18 @@ describe('runSplitQuery()', () => { expect(datasource.runQuery).toHaveBeenCalledTimes(4); }); }); - test('Groups multiple queries into logs, queries, and instant', async () => { + test('Groups multiple queries into logs, queries, instant, and distinct', async () => { const request = getQueryOptions({ targets: [ { expr: 'count_over_time({a="b"}[1m])', refId: 'A', queryType: LokiQueryType.Instant }, { expr: '{c="d"}', refId: 'B' }, { expr: 'count_over_time({c="d"}[1m])', refId: 'C' }, + { expr: 'count_over_time({c="d"} | distinct id [1m])', refId: 'D' }, ], range, }); await expect(runSplitQuery(datasource, request)).toEmitValuesWith(() => { - // 3 days, 3 chunks, 3x Logs + 3x Metric + 1x Instant, 7 requests. + // 3 days, 3 chunks, 3x Logs + 3x Metric + (1x Instant | Distinct), 7 requests. expect(datasource.runQuery).toHaveBeenCalledTimes(7); }); }); diff --git a/public/app/plugins/datasource/loki/querySplitting.ts b/public/app/plugins/datasource/loki/querySplitting.ts index 1fe8c23cd95..b64bcbc32ac 100644 --- a/public/app/plugins/datasource/loki/querySplitting.ts +++ b/public/app/plugins/datasource/loki/querySplitting.ts @@ -15,7 +15,7 @@ import { LoadingState } from '@grafana/schema'; import { LokiDatasource } from './datasource'; import { splitTimeRange as splitLogsTimeRange } from './logsTimeSplitting'; import { splitTimeRange as splitMetricTimeRange } from './metricTimeSplitting'; -import { isLogsQuery } from './queryUtils'; +import { isLogsQuery, isQueryWithDistinct } from './queryUtils'; import { combineResponses } from './responseUtils'; import { trackGroupedQueries } from './tracking'; import { LokiGroupedRequest, LokiQuery, LokiQueryType } from './types'; @@ -171,9 +171,13 @@ function getNextRequestPointers(requests: LokiGroupedRequest[], requestGroup: nu }; } +function querySupporstSplitting(query: LokiQuery) { + return query.queryType !== LokiQueryType.Instant && !isQueryWithDistinct(query.expr); +} + export function runSplitQuery(datasource: LokiDatasource, request: DataQueryRequest) { const queries = request.targets.filter((query) => !query.hide); - const [instantQueries, normalQueries] = partition(queries, (query) => query.queryType === LokiQueryType.Instant); + const [nonSplittingQueries, normalQueries] = partition(queries, (query) => !querySupporstSplitting(query)); const [logQueries, metricQueries] = partition(normalQueries, (query) => isLogsQuery(query.expr)); request.queryGroupId = uuidv4(); @@ -218,9 +222,9 @@ export function runSplitQuery(datasource: LokiDatasource, request: DataQueryRequ } } - if (instantQueries.length) { + if (nonSplittingQueries.length) { requests.push({ - request: { ...request, targets: instantQueries }, + request: { ...request, targets: nonSplittingQueries }, partition: [request.range], }); } diff --git a/public/app/plugins/datasource/loki/queryUtils.test.ts b/public/app/plugins/datasource/loki/queryUtils.test.ts index f79ce8fd6e0..c1d2559bb42 100644 --- a/public/app/plugins/datasource/loki/queryUtils.test.ts +++ b/public/app/plugins/datasource/loki/queryUtils.test.ts @@ -9,6 +9,7 @@ import { getParserFromQuery, obfuscate, requestSupportsSplitting, + isQueryWithDistinct, } from './queryUtils'; import { LokiQuery, LokiQueryType } from './types'; @@ -281,6 +282,18 @@ describe('isQueryWithLabelFormat', () => { }); }); +describe('isQueryWithDistinct', () => { + it('identifies queries using distinct', () => { + expect(isQueryWithDistinct('{job="grafana"} | distinct id')).toBe(true); + expect(isQueryWithDistinct('count_over_time({job="grafana"} | distinct id [1m])')).toBe(true); + }); + + it('does not return false positives', () => { + expect(isQueryWithDistinct('{label="distinct"} | logfmt')).toBe(false); + expect(isQueryWithDistinct('count_over_time({job="distinct"} | json [1m])')).toBe(false); + }); +}); + describe('getParserFromQuery', () => { it('returns no parser', () => { expect(getParserFromQuery('{job="grafana"}')).toBeUndefined(); diff --git a/public/app/plugins/datasource/loki/queryUtils.ts b/public/app/plugins/datasource/loki/queryUtils.ts index 1fd2a91d6ec..3547891145f 100644 --- a/public/app/plugins/datasource/loki/queryUtils.ts +++ b/public/app/plugins/datasource/loki/queryUtils.ts @@ -17,6 +17,7 @@ import { MetricExpr, Matcher, Identifier, + Distinct, } from '@grafana/lezer-logql'; import { DataQuery } from '@grafana/schema'; @@ -218,6 +219,7 @@ export function isQueryWithLabelFormat(query: string): boolean { enter: ({ type }): false | void => { if (type.id === LabelFormatExpr) { queryWithLabelFormat = true; + return false; } }, }); @@ -260,10 +262,10 @@ export function isQueryWithLabelFilter(query: string): boolean { let hasLabelFilter = false; tree.iterate({ - enter: ({ type, node }): false | void => { + enter: ({ type }): false | void => { if (type.id === LabelFilter) { hasLabelFilter = true; - return; + return false; } }, }); @@ -279,7 +281,7 @@ export function isQueryWithLineFilter(query: string): boolean { enter: ({ type }): false | void => { if (type.id === LineFilter) { queryWithLineFilter = true; - return; + return false; } }, }); @@ -287,6 +289,20 @@ export function isQueryWithLineFilter(query: string): boolean { return queryWithLineFilter; } +export function isQueryWithDistinct(query: string): boolean { + let hasDistinct = false; + const tree = parser.parse(query); + tree.iterate({ + enter: ({ type }): false | void => { + if (type.id === Distinct) { + hasDistinct = true; + return false; + } + }, + }); + return hasDistinct; +} + export function getStreamSelectorsFromQuery(query: string): string[] { const labelMatcherPositions = getStreamSelectorPositions(query);