From 8336b8d4c1f77fd93d6d80b9cf9846061152e1ea Mon Sep 17 00:00:00 2001 From: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com> Date: Thu, 28 Sep 2023 10:46:56 -0500 Subject: [PATCH] Prometheus: Query builder - show warning when converting ambiguous order of operations (#75256) * throw error when converting into visual query when parser detects that an aggregation expression node contains a function that contains a binary expression to detect queries that are ambiguously parsed --- .../components/PromQueryEditorSelector.tsx | 4 +- .../prometheus/querybuilder/parsing.test.ts | 66 +++++++++++++++++++ .../prometheus/querybuilder/parsing.ts | 23 +++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryEditorSelector.tsx b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryEditorSelector.tsx index e7861247fd3..63108bfea33 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryEditorSelector.tsx +++ b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryEditorSelector.tsx @@ -96,8 +96,8 @@ export const PromQueryEditorSelector = React.memo((props) => { <> { changeEditorMode(query, QueryEditorMode.Builder, onChange); diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts index c938cc0911a..08fa72b7b8a 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts @@ -71,6 +71,72 @@ describe('buildVisualQueryFromString', () => { ); }); + describe('nested binary operation errors in visual query editor', () => { + // Visual query builder does not currently have support for nested binary operations, for now we should throw an error in the UI letting users know that their query will be misinterpreted + it('throws error when visual query parse is ambiguous', () => { + expect( + buildVisualQueryFromString('topk(5, node_arp_entries / node_arp_entries{cluster="dev-eu-west-2"})') + ).toMatchObject({ + errors: [ + { + from: 8, + text: 'Query parsing is ambiguous.', + to: 68, + }, + ], + }); + }); + it('throws error when visual query parse with aggregation is ambiguous (scalar)', () => { + expect(buildVisualQueryFromString('topk(5, 1 / 2)')).toMatchObject({ + errors: [ + { + from: 8, + text: 'Query parsing is ambiguous.', + to: 13, + }, + ], + }); + }); + it('throws error when visual query parse with functionCall is ambiguous', () => { + expect( + buildVisualQueryFromString( + 'clamp_min(sum by(cluster)(rate(X{le="2.5"}[5m]))+sum by (cluster) (rate(X{le="5"}[5m])), 0.001)' + ) + ).toMatchObject({ + errors: [ + { + from: 10, + text: 'Query parsing is ambiguous.', + to: 87, + }, + ], + }); + }); + it('does not throw error when visual query parse is unambiguous', () => { + expect( + buildVisualQueryFromString('topk(5, node_arp_entries) / node_arp_entries{cluster="dev-eu-west-2"}') + ).toMatchObject({ + errors: [], + }); + }); + it('does not throw error when visual query parse is unambiguous (scalar)', () => { + // Note this topk query with scalars is not valid in prometheus, but it does not currently throw an error during parse + expect(buildVisualQueryFromString('topk(5, 1) / 2')).toMatchObject({ + errors: [], + }); + }); + it('does not throw error when visual query parse is unambiguous, function call', () => { + // Note this topk query with scalars is not valid in prometheus, but it does not currently throw an error during parse + expect( + buildVisualQueryFromString( + 'clamp_min(sum by(cluster) (rate(X{le="2.5"}[5m])), 0.001) + sum by(cluster) (rate(X{le="5"}[5m]))' + ) + ).toMatchObject({ + errors: [], + }); + }); + }); + it('parses query with rate and interval', () => { expect(buildVisualQueryFromString('rate(counters_logins{app="frontend"}[5m])')).toEqual( noErrors({ diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts index f21c768685e..79792b8afe2 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsing.ts @@ -288,6 +288,16 @@ function handleAggregation(expr: string, node: SyntaxNode, context: Context) { const body = node.getChild(FunctionCallBody); const callArgs = body!.getChild(FunctionCallArgs); + const callArgsExprChild = callArgs?.getChild(Expr); + const binaryExpressionWithinAggregationArgs = callArgsExprChild?.getChild(BinaryExpr); + + if (binaryExpressionWithinAggregationArgs) { + context.errors.push({ + text: 'Query parsing is ambiguous.', + from: binaryExpressionWithinAggregationArgs.from, + to: binaryExpressionWithinAggregationArgs.to, + }); + } const op: QueryBuilderOperation = { id: funcName, params: [] }; visQuery.operations.unshift(op); @@ -318,10 +328,23 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont // FunctionCallArgs are nested bit weirdly as mentioned so we have to go one deeper in this case. case FunctionCallArgs: { let child = node.firstChild; + while (child) { + const callArgsExprChild = child.getChild(Expr); + const binaryExpressionWithinFunctionArgs = callArgsExprChild?.getChild(BinaryExpr); + + if (binaryExpressionWithinFunctionArgs) { + context.errors.push({ + text: 'Query parsing is ambiguous.', + from: binaryExpressionWithinFunctionArgs.from, + to: binaryExpressionWithinFunctionArgs.to, + }); + } + updateFunctionArgs(expr, child, context, op); child = child.nextSibling; } + break; }