diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/promql.ts b/packages/grafana-prometheus/src/components/monaco-query-field/promql.ts index 4a850789c72..d144b130208 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/promql.ts +++ b/packages/grafana-prometheus/src/components/monaco-query-field/promql.ts @@ -74,6 +74,7 @@ const functions = [ 'absent', 'ceil', 'changes', + 'clamp', 'clamp_max', 'clamp_min', 'day_of_month', diff --git a/packages/grafana-prometheus/src/locales/en-US/grafana-prometheus.json b/packages/grafana-prometheus/src/locales/en-US/grafana-prometheus.json index 34fb4482a0d..58c601d9514 100644 --- a/packages/grafana-prometheus/src/locales/en-US/grafana-prometheus.json +++ b/packages/grafana-prometheus/src/locales/en-US/grafana-prometheus.json @@ -486,11 +486,6 @@ "message-no-metrics-found": "There are no metrics found in the data source.", "name": "Name", "type": "Type" - }, - "update-function-args": { - "text": { - "query-parsing-is-ambiguous": "Query parsing is ambiguous." - } } } } diff --git a/packages/grafana-prometheus/src/querybuilder/parsing.test.ts b/packages/grafana-prometheus/src/querybuilder/parsing.test.ts index 53db5b12b95..b3da120e8f0 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsing.test.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsing.test.ts @@ -200,49 +200,13 @@ 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', () => { + it('does not throw error when visual query contains binary ops in function argument', () => { 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, - }, - ], + errors: [], }); }); - - 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"}') @@ -967,6 +931,29 @@ describe('buildVisualQueryFromString', () => { }) ); }); + + it('parses query with functions and binary operations', () => { + expect( + buildVisualQueryFromString( + 'clamp(sum(rate(loki_distributor_bytes_received_total{cluster="loki", tenant="kubernetes"}[5m])) / 1024 / 55, 5, 30)' + ) + ).toEqual( + noErrors({ + metric: 'loki_distributor_bytes_received_total', + labels: [ + { label: 'cluster', op: '=', value: 'loki' }, + { label: 'tenant', op: '=', value: 'kubernetes' }, + ], + operations: [ + { id: 'rate', params: ['5m'] }, + { id: 'sum', params: [] }, + { id: '__divide_by', params: [1024] }, + { id: '__divide_by', params: [55] }, + { id: 'clamp', params: [5, 30] }, + ], + }) + ); + }); }); function noErrors(query: PromVisualQuery) { diff --git a/packages/grafana-prometheus/src/querybuilder/parsing.ts b/packages/grafana-prometheus/src/querybuilder/parsing.ts index 9a517b53bd2..3b7c372a545 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsing.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsing.ts @@ -34,6 +34,7 @@ import { getAllByType, getLeftMostChild, getString, + isFunctionOrAggregation, makeBinOp, makeError, replaceBuiltInVariable, @@ -330,24 +331,6 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont let child = node.firstChild; while (child) { - let binaryExpressionWithinFunctionArgs: SyntaxNode | null; - if (child.type.id === BinaryExpr) { - binaryExpressionWithinFunctionArgs = child; - } else { - binaryExpressionWithinFunctionArgs = child.getChild(BinaryExpr); - } - - if (binaryExpressionWithinFunctionArgs) { - context.errors.push({ - text: t( - 'grafana-prometheus.querybuilder.update-function-args.text.query-parsing-is-ambiguous', - 'Query parsing is ambiguous.' - ), - from: binaryExpressionWithinFunctionArgs.from, - to: binaryExpressionWithinFunctionArgs.to, - }); - } - updateFunctionArgs(expr, child, context, op); child = child.nextSibling; } @@ -392,7 +375,7 @@ function updateFunctionArgs(expr: string, node: SyntaxNode | null, context: Cont * @param node * @param context */ -function handleBinary(expr: string, node: SyntaxNode, context: Context) { +function handleBinary(expr: string, node: SyntaxNode, context: Context, idx = 0) { const visQuery = context.query; const left = node.firstChild!; const op = getString(expr, left.nextSibling); @@ -407,6 +390,16 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { const rightBinary = right.type.id === BinaryExpr; + // binary operations that are part of a function argument do not get processed and added to the query until the end, this index helps keep track + // of where to add the operation in the list rather than just appending it to the end. If the binary operation is just part of a nested binary exp, + // we append at the end + const parent = node.parent; + const child = node.firstChild; + const shouldOffsetTail = + parent && !parent.type.isTop && (isFunctionOrAggregation(parent) || (child && isFunctionOrAggregation(child))); + if (shouldOffsetTail) { + idx += 1; + } if (leftNumber) { // TODO: this should be already handled in case parent is binary expression as it has to be added to parent // if query starts with a number that isn't handled now. @@ -416,14 +409,18 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { handleExpression(expr, left, context); } + // in the case we have an expression like func(...) / 2 or func(...) + 5, the binary expression will be at the top of the tree + // in which case, the idx will be 0. In this case it means that the binary operation must be added to the end of the array, and + + const newIdx = idx === 0 ? visQuery.operations.length : -idx; if (rightNumber) { - visQuery.operations.push(makeBinOp(opDef, expr, right, !!binModifier?.isBool)); + visQuery.operations.splice(newIdx, 0, makeBinOp(opDef, expr, right, !!binModifier?.isBool)); } else if (rightBinary) { // Due to the way binary ops are parsed we can get a binary operation on the right that starts with a number which // is a factor for a current binary operation. So we have to add it as an operation now. const leftMostChild = getLeftMostChild(right); if (leftMostChild?.type.id === NumberDurationLiteral) { - visQuery.operations.push(makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool)); + visQuery.operations.splice(newIdx, 0, makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool)); } // If we added the first number literal as operation here we still can continue and handle the rest as the first diff --git a/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts b/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts index c96a7877e8f..fa58edd28bc 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts @@ -4,6 +4,7 @@ import { parser } from '@prometheus-io/lezer-promql'; import { getLeftMostChild, getString, + isFunctionOrAggregation, replaceBuiltInVariable, replaceVariables, returnBuiltInVariable, @@ -123,4 +124,17 @@ describe('builtInTimeVariables', () => { expect(actual2).toBe(testCase.expr); }); }); + + describe('isFunctionOrAggregation', () => { + it('should identify function and aggregation nodes', () => { + const tree = parser.parse('clamp(sum(foo[5m]))'); + const root = tree.topNode; + const functionNode = root.firstChild!.lastChild; // clamp + const aggregationNode = root.firstChild!.lastChild!.firstChild; // sum + + expect(isFunctionOrAggregation(functionNode!)).toBe(true); + expect(isFunctionOrAggregation(aggregationNode!)).toBe(true); + expect(isFunctionOrAggregation(root)).toBe(false); + }); + }); }); diff --git a/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts b/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts index 3017bc33526..50d3a2ce725 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts @@ -1,5 +1,6 @@ // Core Grafana history https://github.com/grafana/grafana/blob/v11.0.0-preview/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.ts import { SyntaxNode, TreeCursor } from '@lezer/common'; +import { AggregateExpr, FunctionCallBody } from '@prometheus-io/lezer-promql'; import { QueryBuilderOperation, QueryBuilderOperationParamValue } from './shared/types'; @@ -194,3 +195,7 @@ export function replaceBuiltInVariable(expr: string): string { export function returnBuiltInVariable(expr: string): string { return expr.replace(builtInReplacementRegex, (match) => replacementToVariable[match]); } + +export function isFunctionOrAggregation(node: SyntaxNode): boolean { + return node.type.id === AggregateExpr || node.type.id === FunctionCallBody; +}