From ac981971328fa88f8618573735e46a16d5d2dcb5 Mon Sep 17 00:00:00 2001 From: Galen Kistler <109082771+gtk-grafana@users.noreply.github.com> Date: Fri, 22 Sep 2023 07:17:01 -0500 Subject: [PATCH] Loki Query Builder: binary expression and numeric literal bugs (#74950) * Following similar changes made to prometheus in #47198, reverse the order of binary operator parameter array, and fix bugs introduced by importing prometheus lezer constants into loki parser * add unit test asserting buildVisualQueryFromString does not properly parse queries with nested binary operations * fix onOrIgnoring parsing logic which was always stripping out the value of the matcher when a boolean was found --- .../querybuilder/binaryScalarOperations.ts | 7 +- .../loki/querybuilder/parsing.test.ts | 110 ++++++++++++++++++ .../datasource/loki/querybuilder/parsing.ts | 17 +-- .../prometheus/querybuilder/parsing.test.ts | 46 +++++++- 4 files changed, 168 insertions(+), 12 deletions(-) diff --git a/public/app/plugins/datasource/loki/querybuilder/binaryScalarOperations.ts b/public/app/plugins/datasource/loki/querybuilder/binaryScalarOperations.ts index a5759ff3060..dc93930bf1d 100644 --- a/public/app/plugins/datasource/loki/querybuilder/binaryScalarOperations.ts +++ b/public/app/plugins/datasource/loki/querybuilder/binaryScalarOperations.ts @@ -82,12 +82,12 @@ export const binaryScalarOperations: QueryBuilderOperationDef[] = binaryScalarDe const params: QueryBuilderOperationParamDef[] = [{ name: 'Value', type: 'number' }]; const defaultParams: any[] = [2]; if (opDef.comparison) { - params.unshift({ + params.push({ name: 'Bool', type: 'boolean', description: 'If checked comparison will return 0 or 1 for the value rather than filtering.', }); - defaultParams.unshift(false); + defaultParams.push(false); } return { @@ -107,8 +107,7 @@ function getSimpleBinaryRenderer(operator: string) { let param = model.params[0]; let bool = ''; if (model.params.length === 2) { - param = model.params[1]; - bool = model.params[0] ? ' bool' : ''; + bool = model.params[1] ? ' bool' : ''; } return `${innerExpr} ${operator}${bool} ${param}`; diff --git a/public/app/plugins/datasource/loki/querybuilder/parsing.test.ts b/public/app/plugins/datasource/loki/querybuilder/parsing.test.ts index c0bfedf7e4a..fe1a50185a0 100644 --- a/public/app/plugins/datasource/loki/querybuilder/parsing.test.ts +++ b/public/app/plugins/datasource/loki/querybuilder/parsing.test.ts @@ -11,6 +11,116 @@ describe('buildVisualQueryFromString', () => { ); }); + it('parses simple binary comparison', () => { + expect(buildVisualQueryFromString('count_over_time({app="aggregator"} [$__auto]) == 11')).toEqual({ + query: { + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + operations: [ + { + id: LokiOperationId.CountOverTime, + params: ['$__auto'], + }, + { + id: LokiOperationId.EqualTo, + // defined in getSimpleBinaryRenderer, the first argument is the bool value, and the second is the comparison operator + params: [11, false], + }, + ], + }, + errors: [], + }); + }); + + // This still fails because loki doesn't properly parse the bool operator + it('parses simple query with label-values with boolean operator', () => { + expect(buildVisualQueryFromString('count_over_time({app="aggregator"} [$__auto]) == bool 12')).toEqual({ + query: { + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + operations: [ + { + id: LokiOperationId.CountOverTime, + params: ['$__auto'], + }, + { + id: LokiOperationId.EqualTo, + // defined in getSimpleBinaryRenderer, the first argument is the bool value, and the second is the comparison operator + params: [12, true], + }, + ], + }, + errors: [], + }); + }); + + it('parses binary operation with query', () => { + expect( + // There is no capability for "bool" in the query builder for (nested) binary operation with query as of now, it will always be stripped out + buildVisualQueryFromString( + 'max by(stream) (count_over_time({app="aggregator"}[1m])) > bool ignoring(stream) avg(count_over_time({app="aggregator"}[1m]))' + ) + ).toEqual({ + query: { + binaryQueries: [ + { + // nested binary operation + operator: '>', + query: { + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + operations: [ + { + id: 'count_over_time', + params: ['1m'], + }, + { + id: 'avg', + params: [], + }, + ], + }, + vectorMatches: 'stream', + vectorMatchesType: 'ignoring', + }, + ], + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + operations: [ + { + id: 'count_over_time', + params: ['1m'], + }, + { + id: '__max_by', + params: ['stream'], + }, + ], + }, + errors: [], + }); + }); + it('parses simple query with label-values', () => { expect(buildVisualQueryFromString('{app="frontend"}')).toEqual( noErrors({ diff --git a/public/app/plugins/datasource/loki/querybuilder/parsing.ts b/public/app/plugins/datasource/loki/querybuilder/parsing.ts index 7e12bf65fda..63cb300cb03 100644 --- a/public/app/plugins/datasource/loki/querybuilder/parsing.ts +++ b/public/app/plugins/datasource/loki/querybuilder/parsing.ts @@ -1,5 +1,4 @@ import { SyntaxNode } from '@lezer/common'; -import { BinModifiers, OnOrIgnoring } from '@prometheus-io/lezer-promql'; import { And, @@ -50,6 +49,8 @@ import { VectorAggregationExpr, VectorOp, Without, + BinOpModifier, + OnOrIgnoringModifier, } from '@grafana/lezer-logql'; import { @@ -565,7 +566,7 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { const visQuery = context.query; const left = node.firstChild!; const op = getString(expr, left.nextSibling); - const binModifier = getBinaryModifier(expr, node.getChild(BinModifiers)); + const binModifier = getBinaryModifier(expr, node.getChild(BinOpModifier)); const right = node.lastChild!; @@ -591,7 +592,7 @@ function handleBinary(expr: string, node: SyntaxNode, context: Context) { // 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?.name === 'Number') { + if (leftMostChild?.type.id === NumberLezer) { visQuery.operations.push(makeBinOp(opDef, expr, leftMostChild, !!binModifier?.isBool)); } @@ -624,15 +625,17 @@ function getBinaryModifier( node: SyntaxNode | null ): | { isBool: true; isMatcher: false } - | { isBool: false; isMatcher: true; matches: string; matchType: 'ignoring' | 'on' } + | { isBool: boolean; isMatcher: true; matches: string; matchType: 'ignoring' | 'on' } | undefined { if (!node) { return undefined; } - if (node.getChild(Bool)) { + const matcher = node.getChild(OnOrIgnoringModifier); + const boolMatcher = node.getChild(Bool); + + if (!matcher && boolMatcher) { return { isBool: true, isMatcher: false }; } else { - const matcher = node.getChild(OnOrIgnoring); if (!matcher) { // Not sure what this could be, maybe should be an error. return undefined; @@ -640,7 +643,7 @@ function getBinaryModifier( const labels = getString(expr, matcher.getChild(GroupingLabels)?.getChild(GroupingLabelList)); return { isMatcher: true, - isBool: false, + isBool: !!boolMatcher, matches: labels, matchType: matcher.getChild(On) ? 'on' : 'ignoring', }; diff --git a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts index 0562d897981..c938cc0911a 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/parsing.test.ts @@ -1,5 +1,5 @@ import { buildVisualQueryFromString } from './parsing'; -import { PromVisualQuery } from './types'; +import { PromOperationId, PromVisualQuery } from './types'; describe('buildVisualQueryFromString', () => { it('creates no errors for empty query', () => { @@ -11,6 +11,50 @@ describe('buildVisualQueryFromString', () => { }) ); }); + it('parses simple binary comparison', () => { + expect(buildVisualQueryFromString('{app="aggregator"} == 11')).toEqual({ + query: { + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + metric: '', + operations: [ + { + id: PromOperationId.EqualTo, + params: [11, false], + }, + ], + }, + errors: [], + }); + }); + + // This still fails because loki doesn't properly parse the bool operator + it('parses simple query with with boolean operator', () => { + expect(buildVisualQueryFromString('{app="aggregator"} == bool 12')).toEqual({ + query: { + labels: [ + { + label: 'app', + op: '=', + value: 'aggregator', + }, + ], + metric: '', + operations: [ + { + id: PromOperationId.EqualTo, + params: [12, true], + }, + ], + }, + errors: [], + }); + }); it('parses simple query', () => { expect(buildVisualQueryFromString('counters_logins{app="frontend"}')).toEqual( noErrors({