From 0d121bab290867c9c43342be4cc3104beddc0567 Mon Sep 17 00:00:00 2001 From: Matias Chomicki Date: Fri, 28 Jul 2023 14:49:02 +0200 Subject: [PATCH] Elasticsearch: Implement modify query using a Lucene parser (#71954) * Lucene: add dependency * ModifyQuery: use Lucene parser to detect key:values in queries * ModifyQuery: use Lucene parser to remove filters * Remove test code * Modify query: switch to recursive implementation * Modify query: implement remove filter * Update query normalizing function * FlagElasticToggleableFilters: remove feature flag * Remove unused feature flag from test * Elasticsearch: escape quotes in filter values --- .../feature-toggles/index.md | 1 - package.json | 2 + .../src/types/featureToggles.gen.ts | 1 - pkg/services/featuremgmt/registry.go | 7 - pkg/services/featuremgmt/toggles_gen.csv | 1 - pkg/services/featuremgmt/toggles_gen.go | 4 - .../elasticsearch/datasource.test.ts | 10 +- .../datasource/elasticsearch/datasource.ts | 9 +- .../elasticsearch/modifyQuery.test.ts | 31 ++- .../datasource/elasticsearch/modifyQuery.ts | 184 ++++++++++++++---- yarn.lock | 16 ++ 11 files changed, 215 insertions(+), 51 deletions(-) diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index c07d9f8c0b7..f9427d75fd4 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -117,7 +117,6 @@ Experimental features might be changed or removed without prior notice. | `recordedQueriesMulti` | Enables writing multiple items from a single query within Recorded Queries | | `pluginsDynamicAngularDetectionPatterns` | Enables fetching Angular detection patterns for plugins from GCOM and fallback to hardcoded ones | | `alertingLokiRangeToInstant` | Rewrites eligible loki range queries to instant queries | -| `elasticToggleableFilters` | Enable support to toggle filters off from the query through the Logs Details component | | `vizAndWidgetSplit` | Split panels between vizualizations and widgets | | `prometheusIncrementalQueryInstrumentation` | Adds RudderStack events to incremental queries | | `logsExploreTableVisualisation` | A table visualisation for logs in Explore | diff --git a/package.json b/package.json index b9efa587f92..e0fd3ac3286 100644 --- a/package.json +++ b/package.json @@ -139,6 +139,7 @@ "@types/jsurl": "^1.2.28", "@types/lodash": "4.14.191", "@types/logfmt": "^1.2.3", + "@types/lucene": "^2", "@types/marked": "5.0.1", "@types/mousetrap": "1.6.11", "@types/node": "18.16.16", @@ -351,6 +352,7 @@ "logfmt": "^1.3.2", "lru-cache": "10.0.0", "lru-memoize": "^1.1.0", + "lucene": "^2.1.1", "marked": "5.1.1", "marked-mangle": "1.1.0", "memoize-one": "6.0.0", diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index f28214c2e4c..2975b7530d2 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -102,7 +102,6 @@ export interface FeatureToggles { recordedQueriesMulti?: boolean; pluginsDynamicAngularDetectionPatterns?: boolean; alertingLokiRangeToInstant?: boolean; - elasticToggleableFilters?: boolean; vizAndWidgetSplit?: boolean; prometheusIncrementalQueryInstrumentation?: boolean; logsExploreTableVisualisation?: boolean; diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 1c113caf336..74b658eaf44 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -576,13 +576,6 @@ var ( FrontendOnly: false, Owner: grafanaAlertingSquad, }, - { - Name: "elasticToggleableFilters", - Description: "Enable support to toggle filters off from the query through the Logs Details component", - Stage: FeatureStageExperimental, - FrontendOnly: true, - Owner: grafanaObservabilityLogsSquad, - }, { Name: "vizAndWidgetSplit", Description: "Split panels between vizualizations and widgets", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 6e380fb330b..9755a44cfc9 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -83,7 +83,6 @@ exploreScrollableLogsContainer,experimental,@grafana/observability-logs,false,fa recordedQueriesMulti,experimental,@grafana/observability-metrics,false,false,false,false pluginsDynamicAngularDetectionPatterns,experimental,@grafana/plugins-platform-backend,false,false,false,false alertingLokiRangeToInstant,experimental,@grafana/alerting-squad,false,false,false,false -elasticToggleableFilters,experimental,@grafana/observability-logs,false,false,false,true vizAndWidgetSplit,experimental,@grafana/dashboards-squad,false,false,false,true prometheusIncrementalQueryInstrumentation,experimental,@grafana/observability-metrics,false,false,false,true logsExploreTableVisualisation,experimental,@grafana/observability-logs,false,false,false,true diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index d7137fdf903..b5e6f274243 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -343,10 +343,6 @@ const ( // Rewrites eligible loki range queries to instant queries FlagAlertingLokiRangeToInstant = "alertingLokiRangeToInstant" - // FlagElasticToggleableFilters - // Enable support to toggle filters off from the query through the Logs Details component - FlagElasticToggleableFilters = "elasticToggleableFilters" - // FlagVizAndWidgetSplit // Split panels between vizualizations and widgets FlagVizAndWidgetSplit = "vizAndWidgetSplit" diff --git a/public/app/plugins/datasource/elasticsearch/datasource.test.ts b/public/app/plugins/datasource/elasticsearch/datasource.test.ts index 88dbb47639a..3fbbe32a06c 100644 --- a/public/app/plugins/datasource/elasticsearch/datasource.test.ts +++ b/public/app/plugins/datasource/elasticsearch/datasource.test.ts @@ -1237,7 +1237,6 @@ describe('toggleQueryFilter', () => { let ds: ElasticDatasource; beforeEach(() => { ds = getTestContext().ds; - config.featureToggles.elasticToggleableFilters = true; }); describe('with empty query', () => { let query: ElasticsearchQuery; @@ -1362,6 +1361,15 @@ describe('addAdhocFilters', () => { const query = ds.addAdHocFilters(''); expect(query).toBe('field\\:name:"field:value"'); }); + + it('should escape characters in filter values', () => { + jest + .mocked(templateSrvMock.getAdhocFilters) + .mockReturnValue([{ key: 'field:name', operator: '=', value: 'field "value"', condition: '' }]); + + const query = ds.addAdHocFilters(''); + expect(query).toBe('field\\:name:"field \\"value\\""'); + }); }); describe('with multiple ad hoc filters', () => { diff --git a/public/app/plugins/datasource/elasticsearch/datasource.ts b/public/app/plugins/datasource/elasticsearch/datasource.ts index 9ee27a2019b..3f49d181e77 100644 --- a/public/app/plugins/datasource/elasticsearch/datasource.ts +++ b/public/app/plugins/datasource/elasticsearch/datasource.ts @@ -56,7 +56,13 @@ import { } from './components/QueryEditor/MetricAggregationsEditor/aggregations'; import { metricAggregationConfig } from './components/QueryEditor/MetricAggregationsEditor/utils'; import { isMetricAggregationWithMeta } from './guards'; -import { addFilterToQuery, escapeFilter, queryHasFilter, removeFilterFromQuery } from './modifyQuery'; +import { + addFilterToQuery, + escapeFilter, + escapeFilterValue, + queryHasFilter, + removeFilterFromQuery, +} from './modifyQuery'; import { trackAnnotationQuery, trackQuery } from './tracking'; import { Logs, @@ -959,6 +965,7 @@ export class ElasticDatasource * colons, which needs to be escaped. */ key = escapeFilter(key); + value = escapeFilterValue(value); switch (operator) { case '=': return `${key}:"${value}"`; diff --git a/public/app/plugins/datasource/elasticsearch/modifyQuery.test.ts b/public/app/plugins/datasource/elasticsearch/modifyQuery.test.ts index e64ec36a954..7e211da673d 100644 --- a/public/app/plugins/datasource/elasticsearch/modifyQuery.test.ts +++ b/public/app/plugins/datasource/elasticsearch/modifyQuery.test.ts @@ -7,6 +7,11 @@ describe('queryHasFilter', () => { expect(queryHasFilter('label : "value"', 'label', 'value')).toBe(true); expect(queryHasFilter('label:value', 'label', 'value')).toBe(true); expect(queryHasFilter('this:"that" AND label:value', 'label', 'value')).toBe(true); + expect(queryHasFilter('this:"that" OR (test:test AND label:value)', 'label', 'value')).toBe(true); + expect(queryHasFilter('this:"that" OR (test:test AND label:value)', 'test', 'test')).toBe(true); + expect(queryHasFilter('(this:"that" OR test:test) AND label:value', 'this', 'that')).toBe(true); + expect(queryHasFilter('(this:"that" OR test:test) AND label:value', 'test', 'test')).toBe(true); + expect(queryHasFilter('(this:"that" OR test :test) AND label:value', 'test', 'test')).toBe(true); expect( queryHasFilter( 'message:"Jun 20 17:19:47 Xtorm syslogd[348]: ASL Sender Statistics"', @@ -34,6 +39,10 @@ describe('queryHasFilter', () => { expect(queryHasFilter('label\\:name:"value"', 'label:name', 'value')).toBe(true); expect(queryHasFilter('-label\\:name:"value"', 'label:name', 'value', '-')).toBe(true); }); + it('should support filters containing quotes', () => { + expect(queryHasFilter('label\\:name:"some \\"value\\""', 'label:name', 'some "value"')).toBe(true); + expect(queryHasFilter('-label\\:name:"some \\"value\\""', 'label:name', 'some "value"', '-')).toBe(true); + }); }); describe('addFilterToQuery', () => { @@ -52,6 +61,9 @@ describe('addFilterToQuery', () => { it('should support filters with colons', () => { expect(addFilterToQuery('', 'label:name', 'value')).toBe('label\\:name:"value"'); }); + it('should support filters with quotes', () => { + expect(addFilterToQuery('', 'label:name', 'the "value"')).toBe('label\\:name:"the \\"value\\""'); + }); }); describe('removeFilterFromQuery', () => { @@ -62,8 +74,20 @@ describe('removeFilterFromQuery', () => { expect(removeFilterFromQuery('label:"value" AND label2:"value2"', 'label', 'value')).toBe('label2:"value2"'); expect(removeFilterFromQuery('label:value AND label2:"value2"', 'label', 'value')).toBe('label2:"value2"'); expect(removeFilterFromQuery('label : "value" OR label2:"value2"', 'label', 'value')).toBe('label2:"value2"'); - expect(removeFilterFromQuery('test="test" OR label:"value" AND label2:"value2"', 'label', 'value')).toBe( - 'test="test" AND label2:"value2"' + expect(removeFilterFromQuery('test:"test" OR label:"value" AND label2:"value2"', 'label', 'value')).toBe( + 'test:"test" OR label2:"value2"' + ); + expect(removeFilterFromQuery('test:"test" OR (label:"value" AND label2:"value2")', 'label', 'value')).toBe( + 'test:"test" OR label2:"value2"' + ); + expect(removeFilterFromQuery('(test:"test" OR label:"value") AND label2:"value2"', 'label', 'value')).toBe( + '(test:"test") AND label2:"value2"' + ); + expect(removeFilterFromQuery('(test:"test" OR label:"value") AND label2:"value2"', 'test', 'test')).toBe( + 'label:"value" AND label2:"value2"' + ); + expect(removeFilterFromQuery('test:"test" OR (label:"value" AND label2:"value2")', 'label2', 'value2')).toBe( + 'test:"test" OR (label:"value")' ); }); it('should not remove the wrong filter', () => { @@ -80,4 +104,7 @@ describe('removeFilterFromQuery', () => { it('should support filters with colons', () => { expect(removeFilterFromQuery('label\\:name:"value"', 'label:name', 'value')).toBe(''); }); + it('should support filters with quotes', () => { + expect(removeFilterFromQuery('label\\:name:"the \\"value\\""', 'label:name', 'the "value"')).toBe(''); + }); }); diff --git a/public/app/plugins/datasource/elasticsearch/modifyQuery.ts b/public/app/plugins/datasource/elasticsearch/modifyQuery.ts index 0d843525278..97c271ab05d 100644 --- a/public/app/plugins/datasource/elasticsearch/modifyQuery.ts +++ b/public/app/plugins/datasource/elasticsearch/modifyQuery.ts @@ -1,4 +1,5 @@ -import { escapeRegex } from '@grafana/data'; +import { isEqual } from 'lodash'; +import lucene, { AST, BinaryAST, LeftOnlyAST, NodeTerm } from 'lucene'; type ModifierType = '' | '-'; @@ -6,18 +7,50 @@ type ModifierType = '' | '-'; * Checks for the presence of a given label:"value" filter in the query. */ export function queryHasFilter(query: string, key: string, value: string, modifier: ModifierType = ''): boolean { - key = escapeFilter(key); - const regex = getFilterRegex(key, value); - const matches = query.matchAll(regex); - for (const match of matches) { - if (modifier === '-' && match[0].startsWith(modifier)) { - return true; - } - if (modifier === '' && !match[0].startsWith('-')) { - return true; - } + return findFilterNode(query, key, value, modifier) !== null; +} + +/** + * Given a query, find the NodeTerm that matches the given field and value. + */ +export function findFilterNode( + query: string, + key: string, + value: string, + modifier: ModifierType = '' +): NodeTerm | null { + const field = `${modifier}${lucene.term.escape(key)}`; + value = lucene.phrase.escape(value); + let ast: AST | null = parseQuery(query); + if (!ast) { + return null; } - return false; + + return findNodeInTree(ast, field, value); +} + +function findNodeInTree(ast: AST, field: string, value: string): NodeTerm | null { + // {} + if (Object.keys(ast).length === 0) { + return null; + } + // { left: {}, right: {} } or { left: {} } + if (isAST(ast.left)) { + return findNodeInTree(ast.left, field, value); + } + if (isNodeTerm(ast.left) && ast.left.field === field && ast.left.term === value) { + return ast.left; + } + if (isLeftOnlyAST(ast)) { + return null; + } + if (isNodeTerm(ast.right) && ast.right.field === field && ast.right.term === value) { + return ast.right; + } + if (isBinaryAST(ast.right)) { + return findNodeInTree(ast.right, field, value); + } + return null; } /** @@ -28,41 +61,126 @@ export function addFilterToQuery(query: string, key: string, value: string, modi return query; } - key = escapeFilter(key); + key = lucene.term.escape(key); + value = lucene.phrase.escape(value); const filter = `${modifier}${key}:"${value}"`; return query === '' ? filter : `${query} AND ${filter}`; } -function getFilterRegex(key: string, value: string) { - return new RegExp(`[-]{0,1}\\s*${escapeRegex(key)}\\s*:\\s*["']{0,1}${escapeRegex(value)}["']{0,1}`, 'ig'); -} - /** * Removes a label:"value" expression from the query. */ export function removeFilterFromQuery(query: string, key: string, value: string, modifier: ModifierType = ''): string { - key = escapeFilter(key); - const regex = getFilterRegex(key, value); - const matches = query.matchAll(regex); - const opRegex = new RegExp(`\\s+(?:AND|OR)\\s*$|^\\s*(?:AND|OR)\\s+`, 'ig'); - for (const match of matches) { - if (modifier === '-' && match[0].startsWith(modifier)) { - query = query.replace(regex, '').replace(opRegex, ''); - } - if (modifier === '' && !match[0].startsWith('-')) { - query = query.replace(regex, '').replace(opRegex, ''); - } + const node = findFilterNode(query, key, value, modifier); + const ast = parseQuery(query); + if (!node || !ast) { + return query; } - query = query.replace(/AND\s+OR/gi, 'OR'); - query = query.replace(/OR\s+AND/gi, 'AND'); - return query; + + return lucene.toString(removeNodeFromTree(ast, node)); +} + +function removeNodeFromTree(ast: AST, node: NodeTerm): AST { + // {} + if (Object.keys(ast).length === 0) { + return ast; + } + // { left: {}, right: {} } or { left: {} } + if (isAST(ast.left)) { + ast.left = removeNodeFromTree(ast.left, node); + return ast; + } + if (isNodeTerm(ast.left) && isEqual(ast.left, node)) { + Object.assign( + ast, + { + left: undefined, + operator: undefined, + right: undefined, + }, + 'right' in ast ? ast.right : {} + ); + return ast; + } + if (isLeftOnlyAST(ast)) { + return ast; + } + if (isNodeTerm(ast.right) && isEqual(ast.right, node)) { + Object.assign(ast, { + right: undefined, + operator: undefined, + }); + return ast; + } + if (isBinaryAST(ast.right)) { + ast.right = removeNodeFromTree(ast.right, node); + return ast; + } + return ast; } /** - * Filters can possibly contain colons, which are used as a separator in the query. + * Filters can possibly reserved characters such as colons which are part of the Lucene syntax. * Use this function to escape filter keys. */ export function escapeFilter(value: string) { - return value.replace(/:/g, '\\:'); + return lucene.term.escape(value); +} + +/** + * Values can possibly reserved special characters such as quotes. + * Use this function to escape filter values. + */ +export function escapeFilterValue(value: string) { + return lucene.phrase.escape(value); +} + +/** + * Normalizes the query by removing whitespace around colons, which breaks parsing. + */ +function normalizeQuery(query: string) { + return query.replace(/(\w+)\s(:)/gi, '$1$2'); +} + +function isLeftOnlyAST(ast: unknown): ast is LeftOnlyAST { + if (!ast) { + return false; + } + if ('left' in ast && !('right' in ast)) { + return true; + } + return false; +} + +function isBinaryAST(ast: unknown): ast is BinaryAST { + if (!ast) { + return false; + } + if ('left' in ast && 'right' in ast) { + return true; + } + return false; +} + +function isAST(ast: unknown): ast is AST { + return isLeftOnlyAST(ast) || isBinaryAST(ast); +} + +function isNodeTerm(ast: unknown): ast is NodeTerm { + if (!ast) { + return false; + } + if ('term' in ast) { + return true; + } + return false; +} + +function parseQuery(query: string) { + try { + return lucene.parse(normalizeQuery(query)); + } catch (e) { + return null; + } } diff --git a/yarn.lock b/yarn.lock index 240e00d66ea..92f83e14f98 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10199,6 +10199,13 @@ __metadata: languageName: node linkType: hard +"@types/lucene@npm:^2": + version: 2.1.4 + resolution: "@types/lucene@npm:2.1.4" + checksum: 418057a390752b36745428887ef527121740d54137a2b2da9f10388d2e9d1fe13d1d04b9b2605101bdd99a38ae357d1d5b08f6302f2eca7cead4e28f30ec964d + languageName: node + linkType: hard + "@types/marked@npm:5.0.1": version: 5.0.1 resolution: "@types/marked@npm:5.0.1" @@ -19340,6 +19347,7 @@ __metadata: "@types/jsurl": ^1.2.28 "@types/lodash": 4.14.191 "@types/logfmt": ^1.2.3 + "@types/lucene": ^2 "@types/marked": 5.0.1 "@types/mousetrap": 1.6.11 "@types/node": 18.16.16 @@ -19468,6 +19476,7 @@ __metadata: logfmt: ^1.3.2 lru-cache: 10.0.0 lru-memoize: ^1.1.0 + lucene: ^2.1.1 marked: 5.1.1 marked-mangle: 1.1.0 memoize-one: 6.0.0 @@ -23257,6 +23266,13 @@ __metadata: languageName: node linkType: hard +"lucene@npm:^2.1.1": + version: 2.1.1 + resolution: "lucene@npm:2.1.1" + checksum: 29bbbddfc0b31b3b5f24b2ee19bc134bb5ca9db0f948b7506df96215e2c24631ed7031cc31a0dddc71024c640ce6eb05c92722bf57bf0e6a4f387ca796cc8df7 + languageName: node + linkType: hard + "lz-string@npm:^1.5.0": version: 1.5.0 resolution: "lz-string@npm:1.5.0"