From 7928f170ce34cc74f8a9310399f04ff4f04ae7c2 Mon Sep 17 00:00:00 2001 From: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Date: Wed, 19 Oct 2022 17:24:00 +0200 Subject: [PATCH] Loki: Fix adding of adhoc filters to stream selector when query with empty stream selector (#57280) * disable double stringify * Refactor test for addLabelToQuery * Add tests (TDD for expected behaviour) * Fix adding ad hoc filters to correct place when no stream selector * Update * Update comment * Fix getAllPositionsInNodeByType Co-authored-by: Sven Grossmann --- .../datasource/loki/modifyQuery.test.ts | 198 +++++------------- .../plugins/datasource/loki/modifyQuery.ts | 36 +++- 2 files changed, 81 insertions(+), 153 deletions(-) diff --git a/public/app/plugins/datasource/loki/modifyQuery.test.ts b/public/app/plugins/datasource/loki/modifyQuery.test.ts index fc7a32a3a7c..d54f2e35a46 100644 --- a/public/app/plugins/datasource/loki/modifyQuery.test.ts +++ b/public/app/plugins/datasource/loki/modifyQuery.test.ts @@ -7,157 +7,53 @@ import { } from './modifyQuery'; describe('addLabelToQuery()', () => { - it('should add label to simple query', () => { - expect(() => { - addLabelToQuery('foo', '', '=', ''); - }).toThrow(); - expect(addLabelToQuery('{}', 'bar', '=', 'baz')).toBe('{bar="baz"}'); - expect(addLabelToQuery('{x="yy"}', 'bar', '=', 'baz')).toBe('{x="yy", bar="baz"}'); - }); - - it('should add custom operator', () => { - expect(addLabelToQuery('{}', 'bar', '!=', 'baz')).toBe('{bar!="baz"}'); - expect(addLabelToQuery('{x="yy"}', 'bar', '!=', 'baz')).toBe('{x="yy", bar!="baz"}'); - }); - - it('should not modify ranges', () => { - expect(addLabelToQuery('rate({}[1m])', 'foo', '=', 'bar')).toBe('rate({foo="bar"}[1m])'); - }); - - it('should detect in-order function use', () => { - expect(addLabelToQuery('sum by (host) (rate({} [1m]))', 'bar', '=', 'baz')).toBe( - 'sum by (host) (rate({bar="baz"}[1m]))' - ); - }); - - it('should handle selectors with punctuation', () => { - expect(addLabelToQuery('{instance="my-host.com:9100"}', 'bar', '=', 'baz')).toBe( - '{instance="my-host.com:9100", bar="baz"}' - ); - expect(addLabelToQuery('{list="a,b,c"}', 'bar', '=', 'baz')).toBe('{list="a,b,c", bar="baz"}'); - }); - - it('should work on arithmetical expressions', () => { - expect(addLabelToQuery('{} + {}', 'bar', '=', 'baz')).toBe('{bar="baz"}+ {bar="baz"}'); - expect(addLabelToQuery('avg(rate({x="y"} [$__interval]))+ sum(rate({}[5m]))', 'bar', '=', 'baz')).toBe( - 'avg(rate({x="y", bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))' - ); - expect(addLabelToQuery('{x="yy"} * {y="zz",a="bb"} * {}', 'bar', '=', 'baz')).toBe( - '{x="yy", bar="baz"} * {y="zz", a="bb", bar="baz"} * {bar="baz"}' - ); - }); - - it('should not add duplicate labels to a query', () => { - expect(addLabelToQuery(addLabelToQuery('{x="yy"}', 'bar', '!=', 'baz'), 'bar', '!=', 'baz')).toBe( - '{x="yy", bar!="baz"}' - ); - expect(addLabelToQuery(addLabelToQuery('rate({}[1m])', 'foo', '=', 'bar'), 'foo', '=', 'bar')).toBe( - 'rate({foo="bar"}[1m])' - ); - expect(addLabelToQuery(addLabelToQuery('{list="a,b,c"}', 'bar', '=', 'baz'), 'bar', '=', 'baz')).toBe( - '{list="a,b,c", bar="baz"}' - ); - expect( - addLabelToQuery( - addLabelToQuery('avg(rate({bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))', 'bar', '=', 'baz'), - 'bar', - '=', - 'baz' - ) - ).toBe('avg(rate({bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))'); - }); - - it('should not remove filters', () => { - expect(addLabelToQuery('{x="y"} |="yy"', 'bar', '=', 'baz')).toBe('{x="y", bar="baz"} |="yy"'); - expect(addLabelToQuery('{x="y"} |="yy" !~"xx"', 'bar', '=', 'baz')).toBe('{x="y", bar="baz"} |="yy" !~"xx"'); - }); - - it('should add label to query properly with Loki datasource', () => { - expect(addLabelToQuery('{job="grafana"} |= "foo-bar"', 'filename', '=', 'test.txt')).toBe( - '{job="grafana", filename="test.txt"} |= "foo-bar"' - ); - }); - - it('should add labels to metrics with logical operators', () => { - expect(addLabelToQuery('{x="y"} or {}', 'bar', '=', 'baz')).toBe('{x="y", bar="baz"} or {bar="baz"}'); - expect(addLabelToQuery('{x="y"} and {}', 'bar', '=', 'baz')).toBe('{x="y", bar="baz"} and {bar="baz"}'); - }); - - it('should not add ad-hoc filter to template variables', () => { - expect(addLabelToQuery('sum(rate({job="foo"}[2m])) by (value $variable)', 'bar', '=', 'baz')).toBe( - 'sum(rate({job="foo", bar="baz"}[2m])) by (value $variable)' - ); - }); - - it('should not add ad-hoc filter to range', () => { - expect(addLabelToQuery('avg(rate(({job="foo"} > 0)[3h:])) by (label)', 'bar', '=', 'baz')).toBe( - 'avg(rate(({job="foo", bar="baz"} > 0)[3h:])) by (label)' - ); - }); - it('should not add ad-hoc filter to labels in label list provided with the group modifier', () => { - expect( - addLabelToQuery( - 'max by (id, name, type) ({type=~"foo|bar|baz-test"}) * on(id) group_right(id, type, name) sum by (id) (rate({} [5m])) * 1000', - 'bar', - '=', - 'baz' - ) - ).toBe( - 'max by (id, name, type) ({type=~"foo|bar|baz-test", bar="baz"}) * on(id) group_right(id, type, name) sum by (id) (rate({bar="baz"}[5m])) * 1000' - ); - }); - it('should not add ad-hoc filter to labels in label list provided with the group modifier', () => { - expect(addLabelToQuery('rate({x="y"}[${__range_s}s])', 'bar', '=', 'baz')).toBe( - 'rate({x="y", bar="baz"}[${__range_s}s])' - ); - }); - it('should not add ad-hoc filter to labels to math operations', () => { - expect(addLabelToQuery('count({job!="foo"} < (5*1024*1024*1024) or vector(0)) - 1', 'bar', '=', 'baz')).toBe( - 'count({job!="foo", bar="baz"} < (5*1024*1024*1024) or vector(0)) - 1' - ); - }); - - describe('should add label as label filter is query with parser', () => { - it('should add label filter after parser', () => { - expect(addLabelToQuery('{foo="bar"} | logfmt', 'bar', '=', 'baz')).toBe('{foo="bar"} | logfmt | bar=`baz`'); - }); - it('should add label filter after last parser when multiple parsers', () => { - expect(addLabelToQuery('{foo="bar"} | logfmt | json', 'bar', '=', 'baz')).toBe( - '{foo="bar"} | logfmt | json | bar=`baz`' - ); - }); - it('should add label filter after last label filter when multiple label filters', () => { - expect(addLabelToQuery('{foo="bar"} | logfmt | x="y"', 'bar', '=', 'baz')).toBe( - '{foo="bar"} | logfmt | x="y" | bar=`baz`' - ); - }); - it('should add label filter in metric query', () => { - expect(addLabelToQuery('rate({foo="bar"} | logfmt [5m])', 'bar', '=', 'baz')).toBe( - 'rate({foo="bar"} | logfmt | bar=`baz` [5m])' - ); - }); - it('should add label filter in complex metric query', () => { - expect( - addLabelToQuery( - 'sum by(host) (rate({foo="bar"} | logfmt | x="y" | line_format "{{.status}}" [5m]))', - 'bar', - '=', - 'baz' - ) - ).toBe('sum by(host) (rate({foo="bar"} | logfmt | x="y" | bar=`baz` | line_format "{{.status}}" [5m]))'); - }); - it('should not add adhoc filter to line_format expressions', () => { - expect(addLabelToQuery('{foo="bar"} | logfmt | line_format "{{.status}}"', 'bar', '=', 'baz')).toBe( - '{foo="bar"} | logfmt | bar=`baz` | line_format "{{.status}}"' - ); - }); - - it('should not add adhoc filter to line_format expressions', () => { - expect(addLabelToQuery('{foo="bar"} | logfmt | line_format "{{status}}"', 'bar', '=', 'baz')).toBe( - '{foo="bar"} | logfmt | bar=`baz` | line_format "{{status}}"' - ); - }); - }); + it.each` + query | description | label | operator | value | expectedResult + ${'{x="y"}'} | ${'no label and value'} | ${''} | ${'='} | ${''} | ${''} + ${'{x="yy"}'} | ${'simple query'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="yy", bar="baz"}'} + ${'{x="yy"}'} | ${'simple query'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="yy", bar="baz"}'} + ${'{x="yy"}'} | ${'custom operator'} | ${'bar'} | ${'!='} | ${'baz'} | ${'{x="yy", bar!="baz"}'} + ${'rate({}[1m])'} | ${'do not modify ranges'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({bar="baz"}[1m])'} + ${'sum by (host) (rate({} [1m]))'} | ${'detect in-order function use'} | ${'bar'} | ${'='} | ${'baz'} | ${'sum by (host) (rate({bar="baz"}[1m]))'} + ${'{instance="my-host.com:9100"}'} | ${'selectors with punctuation'} | ${'bar'} | ${'='} | ${'baz'} | ${'{instance="my-host.com:9100", bar="baz"}'} + ${'{list="a,b,c"}'} | ${'selectors with punctuation'} | ${'bar'} | ${'='} | ${'baz'} | ${'{list="a,b,c", bar="baz"}'} + ${'rate({}[5m]) + rate({}[5m])'} | ${'arithmetical expressions'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({bar="baz"}[5m]) + rate({bar="baz"}[5m])'} + ${'avg(rate({x="y"} [$__interval]))+ sum(rate({}[5m]))'} | ${'arithmetical expressions'} | ${'bar'} | ${'='} | ${'baz'} | ${'avg(rate({x="y", bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))'} + ${'rate({x="yy"}[5m]) * rate({y="zz",a="bb"}[5m]) * rate({}[5m])'} | ${'arithmetical expressions'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({x="yy", bar="baz"}[5m]) * rate({y="zz", a="bb", bar="baz"}[5m]) * rate({bar="baz"}[5m])'} + ${'{x="yy", bar!="baz"}'} | ${'do not add duplicate labels'} | ${'bar'} | ${'!='} | ${'baz'} | ${'{x="yy", bar!="baz"}'} + ${'rate({bar="baz"}[1m])'} | ${'do not add duplicate labels'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({bar="baz"}[1m])'} + ${'{list="a,b,c", bar="baz"}'} | ${'do not add duplicate labels'} | ${'bar'} | ${'='} | ${'baz'} | ${'{list="a,b,c", bar="baz"}'} + ${'avg(rate({bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))'} | ${'do not add duplicate labels'} | ${'bar'} | ${'='} | ${'baz'} | ${'avg(rate({bar="baz"} [$__interval]))+ sum(rate({bar="baz"}[5m]))'} + ${'{x="y"} |="yy"'} | ${'do not remove filters'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="y", bar="baz"} |="yy"'} + ${'{x="y"} |="yy" !~"xx"'} | ${'do not remove filters'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="y", bar="baz"} |="yy" !~"xx"'} + ${'{x="y"} or {}'} | ${'metrics with logical operators'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="y", bar="baz"} or {bar="baz"}'} + ${'{x="y"} and {}'} | ${'metrics with logical operators'} | ${'bar'} | ${'='} | ${'baz'} | ${'{x="y", bar="baz"} and {bar="baz"}'} + ${'sum(rate({job="foo"}[2m])) by (value $variable)'} | ${'template variables'} | ${'bar'} | ${'='} | ${'baz'} | ${'sum(rate({job="foo", bar="baz"}[2m])) by (value $variable)'} + ${'rate({x="y"}[${__range_s}s])'} | ${'metrics query with range grafana variable'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({x="y", bar="baz"}[${__range_s}s])'} + ${'max by (id, name, type) ({type=~"foo|bar|baz-test"}) * on(id) group_right(id, type, name) sum by (id) (rate({} [5m])) * 1000'} | ${'metrics query with labels in label list with the group modifier'} | ${'bar'} | ${'='} | ${'baz'} | ${'max by (id, name, type) ({type=~"foo|bar|baz-test", bar="baz"}) * on(id) group_right(id, type, name) sum by (id) (rate({bar="baz"}[5m])) * 1000'} + ${'{foo="bar"} | logfmt'} | ${'query with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'{foo="bar"} | logfmt | bar=`baz`'} + ${'{foo="bar"} | logfmt | json'} | ${'query with multiple parsers'} | ${'bar'} | ${'='} | ${'baz'} | ${'{foo="bar"} | logfmt | json | bar=`baz`'} + ${'{foo="bar"} | logfmt | x="y"'} | ${'query with parser and label filter'} | ${'bar'} | ${'='} | ${'baz'} | ${'{foo="bar"} | logfmt | x="y" | bar=`baz`'} + ${'rate({foo="bar"} | logfmt [5m])'} | ${'metrics query with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'rate({foo="bar"} | logfmt | bar=`baz` [5m])'} + ${'sum by(host) (rate({foo="bar"} | logfmt | x="y" | line_format "{{.status}}" [5m]))'} | ${'metrics query with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'sum by(host) (rate({foo="bar"} | logfmt | x="y" | bar=`baz` | line_format "{{.status}}" [5m]))'} + ${'{foo="bar"} | logfmt | line_format "{{.status}}"'} | ${'do not add filter to line_format expressions in query with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'{foo="bar"} | logfmt | bar=`baz` | line_format "{{.status}}"'} + ${'{foo="bar"} | logfmt | line_format "{{status}}"'} | ${'do not add filter to line_format expressions in query with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'{foo="bar"} | logfmt | bar=`baz` | line_format "{{status}}"'} + ${'{}'} | ${'query without stream selector'} | ${'bar'} | ${'='} | ${'baz'} | ${'{bar="baz"}'} + ${'{} | logfmt'} | ${'query without stream selector and with parser'} | ${'bar'} | ${'='} | ${'baz'} | ${'{bar="baz"}| logfmt'} + ${'{} | x="y"'} | ${'query without stream selector and with label filter'} | ${'bar'} | ${'='} | ${'baz'} | ${'{bar="baz"}| x="y"'} + ${'{} | logfmt | x="y"'} | ${'query without stream selector and with parser and label filter'} | ${'bar'} | ${'='} | ${'baz'} | ${'{bar="baz"}| logfmt | x="y"'} + `( + 'should add label to query: $query, description: $description', + ({ query, description, label, operator, value, expectedResult }) => { + if (description === 'no label and value') { + expect(() => { + addLabelToQuery(query, label, operator, value); + }).toThrow(); + } else { + expect(addLabelToQuery(query, label, operator, value)).toEqual(expectedResult); + } + } + ); }); describe('addParserToQuery', () => { diff --git a/public/app/plugins/datasource/loki/modifyQuery.ts b/public/app/plugins/datasource/loki/modifyQuery.ts index d6e05838cf9..332bcc85ce0 100644 --- a/public/app/plugins/datasource/loki/modifyQuery.ts +++ b/public/app/plugins/datasource/loki/modifyQuery.ts @@ -1,3 +1,4 @@ +import { SyntaxNode } from '@lezer/common'; import { sortBy } from 'lodash'; import { @@ -8,6 +9,7 @@ import { LineFilters, LogExpr, LogRangeExpr, + Matcher, parser, PipelineExpr, Selector, @@ -40,6 +42,7 @@ export function addLabelToQuery(query: string, key: string, operator: string, va } const streamSelectorPositions = getStreamSelectorPositions(query); + const hasStreamSelectorMatchers = getMatcherInStreamPositions(query).length > 0; const parserPositions = getParserPositions(query); const labelFilterPositions = getLabelFilterPositions(query); if (!streamSelectorPositions.length) { @@ -47,8 +50,8 @@ export function addLabelToQuery(query: string, key: string, operator: string, va } const filter = toLabelFilter(key, value, operator); - // If we have label filters or parser, we want to add new label filter after the last one - if (labelFilterPositions.length || parserPositions.length) { + // If we have non-empty stream selector and parser/label filter, we want to add a new label filter after the last one. + if (hasStreamSelectorMatchers && (labelFilterPositions.length || parserPositions.length)) { const positionToAdd = findLastPosition([...labelFilterPositions, ...parserPositions]); return addFilterAsLabelFilter(query, [positionToAdd], filter); } else { @@ -145,6 +148,19 @@ function getStreamSelectorPositions(query: string): Position[] { return positions; } +function getMatcherInStreamPositions(query: string): Position[] { + const tree = parser.parse(query); + const positions: Position[] = []; + tree.iterate({ + enter: ({ node }): false | void => { + if (node.type.id === Selector) { + positions.push(...getAllPositionsInNodeByType(query, node, Matcher)); + } + }, + }); + return positions; +} + /** * Parse the string and get all LabelParser positions in the query. * @param query @@ -397,3 +413,19 @@ function labelExists(labels: QueryBuilderLabelFilter[], filter: QueryBuilderLabe function findLastPosition(positions: Position[]): Position { return positions.reduce((prev, current) => (prev.to > current.to ? prev : current)); } + +function getAllPositionsInNodeByType(query: string, node: SyntaxNode, type: number): Position[] { + if (node.type.id === type) { + return [{ from: node.from, to: node.to }]; + } + + const positions: Position[] = []; + let pos = 0; + let child = node.childAfter(pos); + while (child) { + positions.push(...getAllPositionsInNodeByType(query, child, type)); + pos = child.to; + child = node.childAfter(pos); + } + return positions; +}