From 2346d5a3f3cbdf3a559a680b3a93e3ea072a26ff Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Wed, 24 Nov 2021 14:09:10 +0000 Subject: [PATCH] Elasticsearch: omit query_string filter when no lucene query is provided (#42060) * Elasticsearch: omit query_string filter when no query is provided * Fix tests * optional lucene query * improve test and types --- .../elasticsearch/datasource.test.ts | 7 +- .../datasource/elasticsearch/datasource.ts | 43 +++---- .../datasource/elasticsearch/query_builder.ts | 36 +++--- .../elasticsearch/specs/query_builder.test.ts | 109 +++++++++++------- .../plugins/datasource/elasticsearch/types.ts | 8 ++ 5 files changed, 122 insertions(+), 81 deletions(-) diff --git a/public/app/plugins/datasource/elasticsearch/datasource.test.ts b/public/app/plugins/datasource/elasticsearch/datasource.test.ts index 7b2e28f3b49..d56366a7364 100644 --- a/public/app/plugins/datasource/elasticsearch/datasource.test.ts +++ b/public/app/plugins/datasource/elasticsearch/datasource.test.ts @@ -69,8 +69,8 @@ function getTestContext({ fetchMock.mockImplementation(mockImplementation ?? defaultMock); const templateSrv: any = { - replace: jest.fn((text) => { - if (text.startsWith('$')) { + replace: jest.fn((text?: string) => { + if (text?.startsWith('$')) { return `resolvedVariable`; } else { return text; @@ -884,7 +884,7 @@ describe('ElasticDatasource', function (this: any) { expect((interpolatedQuery.bucketAggs![0] as Filters).settings!.filters![0].query).toBe('resolvedVariable'); }); - it('should correctly handle empty query strings', () => { + it('should correctly handle empty query strings in filters bucket aggregation', () => { const { ds } = getTestContext(); const query: ElasticsearchQuery = { refId: 'A', @@ -895,7 +895,6 @@ describe('ElasticDatasource', function (this: any) { const interpolatedQuery = ds.interpolateVariablesInQueries([query], {})[0]; - expect(interpolatedQuery.query).toBe('*'); expect((interpolatedQuery.bucketAggs![0] as Filters).settings!.filters![0].query).toBe('*'); }); }); diff --git a/public/app/plugins/datasource/elasticsearch/datasource.ts b/public/app/plugins/datasource/elasticsearch/datasource.ts index f1aaeb2f90e..5eba8ec645a 100644 --- a/public/app/plugins/datasource/elasticsearch/datasource.ts +++ b/public/app/plugins/datasource/elasticsearch/datasource.ts @@ -31,7 +31,7 @@ import { IndexPattern } from './index_pattern'; import { ElasticQueryBuilder } from './query_builder'; import { defaultBucketAgg, hasMetricOfType } from './query_def'; import { getTemplateSrv, TemplateSrv } from 'app/features/templating/template_srv'; -import { DataLinkConfig, ElasticsearchOptions, ElasticsearchQuery } from './types'; +import { DataLinkConfig, ElasticsearchOptions, ElasticsearchQuery, TermsQuery } from './types'; import { RowContextOptions } from '@grafana/ui/src/components/Logs/LogRowContextProvider'; import { metricAggregationConfig } from './components/QueryEditor/MetricAggregationsEditor/utils'; import { @@ -220,7 +220,7 @@ export class ElasticDatasource const annotation = options.annotation; const timeField = annotation.timeField || '@timestamp'; const timeEndField = annotation.timeEndField || null; - const queryString = annotation.query || '*'; + const queryString = annotation.query; const tagsField = annotation.tagsField || 'tags'; const textField = annotation.textField || null; @@ -243,8 +243,8 @@ export class ElasticDatasource dateRanges.push({ range: rangeEnd }); } - const queryInterpolated = this.templateSrv.replace(queryString, {}, 'lucene'); - const query = { + const queryInterpolated = this.interpolateLuceneQuery(queryString); + const query: any = { bool: { filter: [ { @@ -253,15 +253,17 @@ export class ElasticDatasource minimum_should_match: 1, }, }, - { - query_string: { - query: queryInterpolated, - }, - }, ], }, }; + if (queryInterpolated) { + query.bool.filter.push({ + query_string: { + query: queryInterpolated, + }, + }); + } const data: any = { query, size: 10000, @@ -361,9 +363,8 @@ export class ElasticDatasource ); } - private interpolateLuceneQuery(queryString: string, scopedVars: ScopedVars) { - // Elasticsearch queryString should always be '*' if empty string - return this.templateSrv.replace(queryString, scopedVars, 'lucene') || '*'; + private interpolateLuceneQuery(queryString: string, scopedVars?: ScopedVars) { + return this.templateSrv.replace(queryString, scopedVars, 'lucene'); } interpolateVariablesInQueries(queries: ElasticsearchQuery[], scopedVars: ScopedVars): ElasticsearchQuery[] { @@ -377,7 +378,7 @@ export class ElasticDatasource ...bucketAgg.settings, filters: bucketAgg.settings?.filters?.map((filter) => ({ ...filter, - query: this.interpolateLuceneQuery(filter.query || '', scopedVars), + query: this.interpolateLuceneQuery(filter.query, scopedVars) || '*', })), }, }; @@ -646,14 +647,14 @@ export class ElasticDatasource target.metrics = []; // Setting this for metrics queries that are typed as logs - queryObj = this.queryBuilder.getLogsQuery(target, limit, adhocFilters, target.query); + queryObj = this.queryBuilder.getLogsQuery(target, limit, adhocFilters); } else { logLimits.push(); if (target.alias) { - target.alias = this.templateSrv.replace(target.alias, options.scopedVars, 'lucene'); + target.alias = this.interpolateLuceneQuery(target.alias, options.scopedVars); } - queryObj = this.queryBuilder.build(target, adhocFilters, target.query); + queryObj = this.queryBuilder.build(target, adhocFilters); } const esQuery = JSON.stringify(queryObj); @@ -795,7 +796,7 @@ export class ElasticDatasource ); } - getTerms(queryDef: any, range = getDefaultTimeRange()): Observable { + getTerms(queryDef: TermsQuery, range = getDefaultTimeRange()): Observable { const searchType = gte(this.esVersion, '5.0.0') ? 'query_then_fetch' : 'count'; const header = this.getQueryHeader(searchType, range.from, range.to); let esQuery = JSON.stringify(this.queryBuilder.getTermsQuery(queryDef)); @@ -842,13 +843,13 @@ export class ElasticDatasource const parsedQuery = JSON.parse(query); if (query) { if (parsedQuery.find === 'fields') { - parsedQuery.type = this.templateSrv.replace(parsedQuery.type, {}, 'lucene'); + parsedQuery.type = this.interpolateLuceneQuery(parsedQuery.type); return lastValueFrom(this.getFields(parsedQuery.type, range)); } if (parsedQuery.find === 'terms') { - parsedQuery.field = this.templateSrv.replace(parsedQuery.field, {}, 'lucene'); - parsedQuery.query = this.templateSrv.replace(parsedQuery.query || '*', {}, 'lucene'); + parsedQuery.field = this.interpolateLuceneQuery(parsedQuery.field); + parsedQuery.query = this.interpolateLuceneQuery(parsedQuery.query); return lastValueFrom(this.getTerms(parsedQuery, range)); } } @@ -861,7 +862,7 @@ export class ElasticDatasource } getTagValues(options: any) { - return lastValueFrom(this.getTerms({ field: options.key, query: '*' })); + return lastValueFrom(this.getTerms({ field: options.key })); } targetContainsTemplate(target: any) { diff --git a/public/app/plugins/datasource/elasticsearch/query_builder.ts b/public/app/plugins/datasource/elasticsearch/query_builder.ts index 300f8ce1fab..0f21c96b6f6 100644 --- a/public/app/plugins/datasource/elasticsearch/query_builder.ts +++ b/public/app/plugins/datasource/elasticsearch/query_builder.ts @@ -16,7 +16,7 @@ import { MetricAggregationWithInlineScript, } from './components/QueryEditor/MetricAggregationsEditor/aggregations'; import { defaultBucketAgg, defaultMetricAgg, findMetricById, highlightTags } from './query_def'; -import { ElasticsearchQuery } from './types'; +import { ElasticsearchQuery, TermsQuery } from './types'; import { convertOrderByToMetricId, getScriptValue } from './utils'; export class ElasticQueryBuilder { @@ -213,7 +213,7 @@ export class ElasticQueryBuilder { } } - build(target: ElasticsearchQuery, adhocFilters?: any, queryString?: string) { + build(target: ElasticsearchQuery, adhocFilters?: any) { // make sure query has defaults; target.metrics = target.metrics || [defaultMetricAgg()]; target.bucketAggs = target.bucketAggs || [defaultBucketAgg()]; @@ -221,23 +221,27 @@ export class ElasticQueryBuilder { let metric: MetricAggregation; let i, j, pv, nestedAggs; - const query = { + const query: any = { size: 0, query: { bool: { - filter: [ - { range: this.getRangeFilter() }, - { - query_string: { - analyze_wildcard: true, - query: queryString, - }, - }, - ], + filter: [{ range: this.getRangeFilter() }], }, }, }; + if (target.query && target.query !== '') { + query.query.bool.filter = [ + ...query.query.bool.filter, + { + query_string: { + analyze_wildcard: true, + query: target.query, + }, + }, + ]; + } + this.addAdhocFilters(query, adhocFilters); // If target doesn't have bucketAggs and type is not raw_document, it is invalid query. @@ -433,7 +437,7 @@ export class ElasticQueryBuilder { return parsedValue; } - getTermsQuery(queryDef: any) { + getTermsQuery(queryDef: TermsQuery) { const query: any = { size: 0, query: { @@ -493,7 +497,7 @@ export class ElasticQueryBuilder { return query; } - getLogsQuery(target: ElasticsearchQuery, limit: number, adhocFilters?: any, querystring?: string) { + getLogsQuery(target: ElasticsearchQuery, limit: number, adhocFilters?: any) { let query: any = { size: 0, query: { @@ -509,7 +513,7 @@ export class ElasticQueryBuilder { query.query.bool.filter.push({ query_string: { analyze_wildcard: true, - query: querystring, + query: target.query, }, }); } @@ -518,7 +522,7 @@ export class ElasticQueryBuilder { return { ...query, - aggs: this.build(target, null, querystring).aggs, + aggs: this.build(target, null).aggs, highlight: { fields: { '*': {}, diff --git a/public/app/plugins/datasource/elasticsearch/specs/query_builder.test.ts b/public/app/plugins/datasource/elasticsearch/specs/query_builder.test.ts index 718fc4e9c3c..1ed2b6d3540 100644 --- a/public/app/plugins/datasource/elasticsearch/specs/query_builder.test.ts +++ b/public/app/plugins/datasource/elasticsearch/specs/query_builder.test.ts @@ -65,8 +65,7 @@ describe('ElasticQueryBuilder', () => { metrics: [{ type: 'avg', field: '@value', id: '1' }], bucketAggs: [{ type: 'date_histogram', field: '@timestamp', id: '2' }], }, - 100, - '1000' + 100 ); const aggs = query.aggs['2'].aggs; @@ -91,7 +90,7 @@ describe('ElasticQueryBuilder', () => { ], }; - const query = builder.build(target, 100, '1000'); + const query = builder.build(target, 100); const firstLevel = query.aggs['2']; if (gte(builder.esVersion, '6.0.0')) { @@ -119,8 +118,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['2']; @@ -148,8 +146,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); expect(query.aggs['2'].terms.order._count).toEqual('asc'); @@ -171,8 +168,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['2']; @@ -197,8 +193,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['2']; @@ -223,8 +218,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['2']; @@ -246,8 +240,7 @@ describe('ElasticQueryBuilder', () => { { type: 'date_histogram', field: '@timestamp', id: '3' }, ], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['2']; @@ -273,8 +266,7 @@ describe('ElasticQueryBuilder', () => { ], bucketAggs: [{ type: 'date_histogram', field: '@timestamp', id: '3' }], }, - 100, - '1000' + 100 ); const firstLevel = query.aggs['3']; @@ -331,12 +323,6 @@ describe('ElasticQueryBuilder', () => { }, }, }, - { - query_string: { - analyze_wildcard: true, - query: undefined, - }, - }, ], }, }, @@ -662,10 +648,10 @@ describe('ElasticQueryBuilder', () => { expect(query.query.bool.must[0].match_phrase['key1'].query).toBe('value1'); expect(query.query.bool.must[1].match_phrase['key2'].query).toBe('value2'); expect(query.query.bool.must_not[0].match_phrase['key2'].query).toBe('value2'); - expect(query.query.bool.filter[2].range['key3'].lt).toBe('value3'); - expect(query.query.bool.filter[3].range['key4'].gt).toBe('value4'); - expect(query.query.bool.filter[4].regexp['key5']).toBe('value5'); - expect(query.query.bool.filter[5].bool.must_not.regexp['key6']).toBe('value6'); + expect(query.query.bool.filter[1].range['key3'].lt).toBe('value3'); + expect(query.query.bool.filter[2].range['key4'].gt).toBe('value4'); + expect(query.query.bool.filter[3].regexp['key5']).toBe('value5'); + expect(query.query.bool.filter[4].bool.must_not.regexp['key6']).toBe('value6'); }); describe('getTermsQuery', () => { @@ -709,11 +695,49 @@ describe('ElasticQueryBuilder', () => { expect(query.aggs['1'].terms.order._key).toBeUndefined(); expect(query.aggs['1'].terms.order._count).toBe('asc'); }); + + describe('lucene query', () => { + it('should add query_string filter when query is not empty', () => { + const luceneQuery = 'foo'; + const query = builder.getTermsQuery({ orderBy: 'doc_count', order: 'asc', query: luceneQuery }); + + expect(query.query.bool.filter).toContainEqual({ + query_string: { analyze_wildcard: true, query: luceneQuery }, + }); + }); + + it('should not add query_string filter when query is empty', () => { + const query = builder.getTermsQuery({ orderBy: 'doc_count', order: 'asc' }); + + expect( + query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string')) + ).toBeFalsy(); + }); + }); + }); + + describe('lucene query', () => { + it('should add query_string filter when query is not empty', () => { + const luceneQuery = 'foo'; + const query = builder.build({ refId: 'A', query: luceneQuery }); + + expect(query.query.bool.filter).toContainEqual({ + query_string: { analyze_wildcard: true, query: luceneQuery }, + }); + }); + + it('should not add query_string filter when query is empty', () => { + const query = builder.build({ refId: 'A' }); + + expect( + query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string')) + ).toBeFalsy(); + }); }); describe('getLogsQuery', () => { it('should return query with defaults', () => { - const query = builder.getLogsQuery({ refId: 'A' }, 500, null, '*'); + const query = builder.getLogsQuery({ refId: 'A' }, 500, null); expect(query.size).toEqual(500); @@ -751,18 +775,23 @@ describe('ElasticQueryBuilder', () => { expect(query.aggs).toMatchObject(expectedAggs); }); - it('with querystring', () => { - const query = builder.getLogsQuery({ refId: 'A', query: 'foo' }, 500, null, 'foo'); + describe('lucene query', () => { + it('should add query_string filter when query is not empty', () => { + const luceneQuery = 'foo'; + const query = builder.getLogsQuery({ refId: 'A', query: luceneQuery }, 500, null); - const expectedQuery = { - bool: { - filter: [ - { range: { '@timestamp': { gte: '$timeFrom', lte: '$timeTo', format: 'epoch_millis' } } }, - { query_string: { analyze_wildcard: true, query: 'foo' } }, - ], - }, - }; - expect(query.query).toEqual(expectedQuery); + expect(query.query.bool.filter).toContainEqual({ + query_string: { analyze_wildcard: true, query: luceneQuery }, + }); + }); + + it('should not add query_string filter when query is empty', () => { + const query = builder.getLogsQuery({ refId: 'A' }, 500, null); + + expect( + query.query.bool.filter.find((filter: any) => Object.keys(filter).includes('query_string')) + ).toBeFalsy(); + }); }); it('with adhoc filters', () => { @@ -775,7 +804,7 @@ describe('ElasticQueryBuilder', () => { { key: 'key5', operator: '=~', value: 'value5' }, { key: 'key6', operator: '!~', value: 'value6' }, ]; - const query = builder.getLogsQuery({ refId: 'A' }, 500, adhocFilters, '*'); + const query = builder.getLogsQuery({ refId: 'A' }, 500, adhocFilters); expect(query.query.bool.must[0].match_phrase['key1'].query).toBe('value1'); expect(query.query.bool.must_not[0].match_phrase['key2'].query).toBe('value2'); diff --git a/public/app/plugins/datasource/elasticsearch/types.ts b/public/app/plugins/datasource/elasticsearch/types.ts index 067fcfcd2d5..8b0cd31af18 100644 --- a/public/app/plugins/datasource/elasticsearch/types.ts +++ b/public/app/plugins/datasource/elasticsearch/types.ts @@ -72,6 +72,14 @@ export interface ElasticsearchQuery extends DataQuery { timeField?: string; } +export interface TermsQuery { + query?: string; + size?: number; + field?: string; + order?: 'asc' | 'desc'; + orderBy?: string; +} + export type DataLinkConfig = { field: string; url: string;