From 423a45185826f7d33a31a506302fed0668ad991c Mon Sep 17 00:00:00 2001 From: Sven Grossmann Date: Thu, 31 Aug 2023 15:24:03 +0200 Subject: [PATCH] Loki: Fix filtering with structured metadata (#73955) * check languageProvider to work with non-indexed metadata * change loki devenv to work with non-indexed metadata * trigger ci * add forced labels after parsers * add comment * Update public/app/plugins/datasource/loki/modifyQuery.ts Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> --------- Co-authored-by: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> --- devenv/docker/blocks/loki/data/data.js | 8 +- devenv/docker/blocks/loki/docker-compose.yaml | 2 +- devenv/docker/blocks/loki/loki-config.yaml | 2 +- .../datasource/loki/datasource.test.ts | 90 +++++++++++-------- .../app/plugins/datasource/loki/datasource.ts | 20 ++++- .../datasource/loki/modifyQuery.test.ts | 12 +++ .../plugins/datasource/loki/modifyQuery.ts | 28 ++++-- 7 files changed, 109 insertions(+), 53 deletions(-) diff --git a/devenv/docker/blocks/loki/data/data.js b/devenv/docker/blocks/loki/data/data.js index b08a494ef4e..f420e216f1a 100644 --- a/devenv/docker/blocks/loki/data/data.js +++ b/devenv/docker/blocks/loki/data/data.js @@ -47,12 +47,12 @@ async function sleep(duration) { }); } -async function lokiSendLogLine(timestampNs, line, tags) { +async function lokiSendLogLine(timestampNs, line, tags, nonIndexed = {}) { const data = { streams: [ { stream: tags, - values: [[timestampNs, line]], + values: [[timestampNs, line, nonIndexed]], }, ], }; @@ -187,8 +187,8 @@ async function sendNewLogs() { const nowMs = new Date().getTime(); const timestampNs = `${nowMs}${getRandomNanosecPart()}`; const item = getRandomLogItem(globalCounter) - await lokiSendLogLine(timestampNs, JSON.stringify(item), {age:'new', place:'moon', ...sharedLabels}); - await lokiSendLogLine(timestampNs, logFmtLine(item), {age:'new', place:'luna', ...sharedLabels}); + await lokiSendLogLine(timestampNs, JSON.stringify(item), {age:'new', place:'moon', ...sharedLabels}, {nonIndexed: 'value'}); + await lokiSendLogLine(timestampNs, logFmtLine(item), {age:'new', place:'luna', ...sharedLabels}, {nonIndexed: 'value'}); const sleepDuration = 200 + Math.random() * 800; // between 0.2 and 1 seconds await sleep(sleepDuration); } diff --git a/devenv/docker/blocks/loki/docker-compose.yaml b/devenv/docker/blocks/loki/docker-compose.yaml index 4495dc81057..0d904b5f46e 100644 --- a/devenv/docker/blocks/loki/docker-compose.yaml +++ b/devenv/docker/blocks/loki/docker-compose.yaml @@ -1,5 +1,5 @@ loki: - image: grafana/loki:latest + image: grafana/loki:main command: -config.file=/etc/loki/loki-config.yaml volumes: - ./docker/blocks/loki/loki-config.yaml:/etc/loki/loki-config.yaml diff --git a/devenv/docker/blocks/loki/loki-config.yaml b/devenv/docker/blocks/loki/loki-config.yaml index bd3dbe1e287..cce90ce7353 100644 --- a/devenv/docker/blocks/loki/loki-config.yaml +++ b/devenv/docker/blocks/loki/loki-config.yaml @@ -21,7 +21,7 @@ schema_config: - from: 2020-10-24 store: tsdb object_store: filesystem - schema: v11 + schema: v13 index: prefix: index_ period: 24h diff --git a/public/app/plugins/datasource/loki/datasource.test.ts b/public/app/plugins/datasource/loki/datasource.test.ts index f761ec8de3e..789ca8e914c 100644 --- a/public/app/plugins/datasource/loki/datasource.test.ts +++ b/public/app/plugins/datasource/loki/datasource.test.ts @@ -609,6 +609,7 @@ describe('LokiDatasource', () => { let ds: LokiDatasource; beforeEach(() => { ds = createLokiDatasource(templateSrvStub); + ds.languageProvider.labelKeys = ['bar', 'job']; }); describe('and query has no parser', () => { @@ -638,34 +639,44 @@ describe('LokiDatasource', () => { expect(result.refId).toEqual('A'); expect(result.expr).toEqual('rate({bar="baz", job="grafana"}[5m])'); }); - describe('and query has parser', () => { - it('then the correct label should be added for logs query', () => { - const query: LokiQuery = { refId: 'A', expr: '{bar="baz"} | logfmt' }; - const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER' }; - const result = ds.modifyQuery(query, action); - expect(result.refId).toEqual('A'); - expect(result.expr).toEqual('{bar="baz"} | logfmt | job=`grafana`'); - }); - it('then the correct label should be added for metrics query', () => { - const query: LokiQuery = { refId: 'A', expr: 'rate({bar="baz"} | logfmt [5m])' }; - const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER' }; - const result = ds.modifyQuery(query, action); + it('then the correct label should be added for non-indexed metadata as LabelFilter', () => { + const query: LokiQuery = { refId: 'A', expr: '{bar="baz"}' }; + const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER' }; + ds.languageProvider.labelKeys = ['bar']; + const result = ds.modifyQuery(query, action); - expect(result.refId).toEqual('A'); - expect(result.expr).toEqual('rate({bar="baz"} | logfmt | job=`grafana` [5m])'); - }); + expect(result.refId).toEqual('A'); + expect(result.expr).toEqual('{bar="baz"} | job=`grafana`'); + }); + }); + describe('and query has parser', () => { + it('then the correct label should be added for logs query', () => { + const query: LokiQuery = { refId: 'A', expr: '{bar="baz"} | logfmt' }; + const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER' }; + const result = ds.modifyQuery(query, action); + + expect(result.refId).toEqual('A'); + expect(result.expr).toEqual('{bar="baz"} | logfmt | job=`grafana`'); + }); + it('then the correct label should be added for metrics query', () => { + const query: LokiQuery = { refId: 'A', expr: 'rate({bar="baz"} | logfmt [5m])' }; + const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER' }; + const result = ds.modifyQuery(query, action); + + expect(result.refId).toEqual('A'); + expect(result.expr).toEqual('rate({bar="baz"} | logfmt | job=`grafana` [5m])'); }); }); }); describe('when called with ADD_FILTER_OUT', () => { + let ds: LokiDatasource; + beforeEach(() => { + ds = createLokiDatasource(templateSrvStub); + ds.languageProvider.labelKeys = ['bar', 'job']; + }); describe('and query has no parser', () => { - let ds: LokiDatasource; - beforeEach(() => { - ds = createLokiDatasource(templateSrvStub); - }); - it('then the correct label should be added for logs query', () => { const query: LokiQuery = { refId: 'A', expr: '{bar="baz"}' }; const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER_OUT' }; @@ -692,28 +703,29 @@ describe('LokiDatasource', () => { expect(result.refId).toEqual('A'); expect(result.expr).toEqual('rate({bar="baz", job!="grafana"}[5m])'); }); - describe('and query has parser', () => { - let ds: LokiDatasource; - beforeEach(() => { - ds = createLokiDatasource(templateSrvStub); - }); + }); + describe('and query has parser', () => { + let ds: LokiDatasource; + beforeEach(() => { + ds = createLokiDatasource(templateSrvStub); + ds.languageProvider.labelKeys = ['bar', 'job']; + }); - it('then the correct label should be added for logs query', () => { - const query: LokiQuery = { refId: 'A', expr: '{bar="baz"} | logfmt' }; - const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER_OUT' }; - const result = ds.modifyQuery(query, action); + it('then the correct label should be added for logs query', () => { + const query: LokiQuery = { refId: 'A', expr: '{bar="baz"} | logfmt' }; + const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER_OUT' }; + const result = ds.modifyQuery(query, action); - expect(result.refId).toEqual('A'); - expect(result.expr).toEqual('{bar="baz"} | logfmt | job!=`grafana`'); - }); - it('then the correct label should be added for metrics query', () => { - const query: LokiQuery = { refId: 'A', expr: 'rate({bar="baz"} | logfmt [5m])' }; - const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER_OUT' }; - const result = ds.modifyQuery(query, action); + expect(result.refId).toEqual('A'); + expect(result.expr).toEqual('{bar="baz"} | logfmt | job!=`grafana`'); + }); + it('then the correct label should be added for metrics query', () => { + const query: LokiQuery = { refId: 'A', expr: 'rate({bar="baz"} | logfmt [5m])' }; + const action = { options: { key: 'job', value: 'grafana' }, type: 'ADD_FILTER_OUT' }; + const result = ds.modifyQuery(query, action); - expect(result.refId).toEqual('A'); - expect(result.expr).toEqual('rate({bar="baz"} | logfmt | job!=`grafana` [5m])'); - }); + expect(result.refId).toEqual('A'); + expect(result.expr).toEqual('rate({bar="baz"} | logfmt | job!=`grafana` [5m])'); }); }); }); diff --git a/public/app/plugins/datasource/loki/datasource.ts b/public/app/plugins/datasource/loki/datasource.ts index 32f1b11df03..be559e9f252 100644 --- a/public/app/plugins/datasource/loki/datasource.ts +++ b/public/app/plugins/datasource/loki/datasource.ts @@ -659,18 +659,34 @@ export class LokiDatasource modifyQuery(query: LokiQuery, action: QueryFixAction): LokiQuery { let expression = query.expr ?? ''; + // NB: Usually the labelKeys should be fetched and cached in the datasource, + // but there might be some edge cases where this wouldn't be the case. + // However the changed would make this method `async`. + const allLabels = this.languageProvider.getLabelKeys(); switch (action.type) { case 'ADD_FILTER': { if (action.options?.key && action.options?.value) { const value = escapeLabelValueInSelector(action.options.value); - expression = addLabelToQuery(expression, action.options.key, '=', value); + expression = addLabelToQuery( + expression, + action.options.key, + '=', + value, + allLabels.includes(action.options.key) === false + ); } break; } case 'ADD_FILTER_OUT': { if (action.options?.key && action.options?.value) { const value = escapeLabelValueInSelector(action.options.value); - expression = addLabelToQuery(expression, action.options.key, '!=', value); + expression = addLabelToQuery( + expression, + action.options.key, + '!=', + value, + allLabels.includes(action.options.key) === false + ); } break; } diff --git a/public/app/plugins/datasource/loki/modifyQuery.test.ts b/public/app/plugins/datasource/loki/modifyQuery.test.ts index 6d831690fd3..38284968b53 100644 --- a/public/app/plugins/datasource/loki/modifyQuery.test.ts +++ b/public/app/plugins/datasource/loki/modifyQuery.test.ts @@ -71,6 +71,18 @@ describe('addLabelToQuery()', () => { } } ); + + it('should always add label as labelFilter if force flag is given', () => { + expect(addLabelToQuery('{foo="bar"}', 'forcedLabel', '=', 'value', true)).toEqual( + '{foo="bar"} | forcedLabel=`value`' + ); + }); + + it('should always add label as labelFilter if force flag is given with a parser', () => { + expect(addLabelToQuery('{foo="bar"} | logfmt', 'forcedLabel', '=', 'value', true)).toEqual( + '{foo="bar"} | logfmt | forcedLabel=`value`' + ); + }); }); describe('addParserToQuery', () => { diff --git a/public/app/plugins/datasource/loki/modifyQuery.ts b/public/app/plugins/datasource/loki/modifyQuery.ts index 3678a723ca8..61edcc93029 100644 --- a/public/app/plugins/datasource/loki/modifyQuery.ts +++ b/public/app/plugins/datasource/loki/modifyQuery.ts @@ -139,12 +139,19 @@ function getMatchersWithFilter(query: string, label: string, operator: string, v * This operates on substrings of the query with labels and operates just on those. This makes this * more robust and can alter even invalid queries, and preserves in general the query structure and whitespace. * - * @param query - * @param key - * @param value - * @param operator + * @param {string} query + * @param {string} key + * @param {string} operator + * @param {string} value + * @param {boolean} [forceAsLabelFilter=false] - if true, it will add a LabelFilter expression even if there is no parser in the query */ -export function addLabelToQuery(query: string, key: string, operator: string, value: string): string { +export function addLabelToQuery( + query: string, + key: string, + operator: string, + value: string, + forceAsLabelFilter = false +): string { if (!key || !value) { throw new Error('Need label to add to query.'); } @@ -167,7 +174,16 @@ export function addLabelToQuery(query: string, key: string, operator: string, va const filter = toLabelFilter(key, value, operator); // If we have non-empty stream selector and parser/label filter, we want to add a new label filter after the last one. // If some of the stream selectors don't have matchers, we want to add new matcher to the all stream selectors. - if (everyStreamSelectorHasMatcher && (labelFilterPositions.length || parserPositions.length)) { + if (forceAsLabelFilter) { + // `forceAsLabelFilter` is mostly used for structured metadata labels. Those are not + // very well distinguishable from real labels, but need to be added as label + // filters after the last stream selector, parser or label filter. This is + // just a quickfix for now and still has edge-cases where it can fail. + // TODO: improve this once we have a better API in Loki to distinguish + // between the origins of labels. + const positionToAdd = findLastPosition([...streamSelectorPositions, ...labelFilterPositions, ...parserPositions]); + return addFilterAsLabelFilter(query, [positionToAdd], filter); + } else if (everyStreamSelectorHasMatcher && (labelFilterPositions.length || parserPositions.length)) { const positionToAdd = findLastPosition([...labelFilterPositions, ...parserPositions]); return addFilterAsLabelFilter(query, [positionToAdd], filter); } else {