From b2ca9cbda653cb2776cb09874ad3f9836555aaf4 Mon Sep 17 00:00:00 2001 From: Ivana Huckova <30407135+ivanahuckova@users.noreply.github.com> Date: Wed, 10 Feb 2021 14:02:42 +0100 Subject: [PATCH] Prometheus: Fix issues with ad-hoc filters (#30931) * Don't add {} to non-metrics starting with quotes * Fix for using template variables * Fix adding of filter to timerange * Fix adding of filter to labels provided with group modifier * Fix adding of filters to Grafana's variables * Shorten tests * Refactor and split --- .../prometheus/add_label_to_query.test.ts | 39 ++++++++- .../prometheus/add_label_to_query.ts | 79 ++++++++++++------- 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/add_label_to_query.test.ts b/public/app/plugins/datasource/prometheus/add_label_to_query.test.ts index 7ef4b7effd7..e4fb6ab6717 100644 --- a/public/app/plugins/datasource/prometheus/add_label_to_query.test.ts +++ b/public/app/plugins/datasource/prometheus/add_label_to_query.test.ts @@ -74,8 +74,43 @@ describe('addLabelToQuery()', () => { expect(addLabelToQuery('{job="grafana"} |= "foo-bar"', 'filename', 'test.txt', undefined, true)).toBe( '{filename="test.txt",job="grafana"} |= "foo-bar"' ); - expect(addLabelToQuery('{job="grafana"} |= "foo-bar"', 'filename', 'test.txt')).toBe( - '{filename="test.txt",job="grafana"} |= "foo{filename="test.txt"}-bar"' + }); + + it('should add labels to metrics with logical operators', () => { + expect(addLabelToQuery('foo_info or bar_info', 'bar', 'baz')).toBe('foo_info{bar="baz"} or bar_info{bar="baz"}'); + expect(addLabelToQuery('foo_info and bar_info', 'bar', 'baz')).toBe('foo_info{bar="baz"} and bar_info{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({bar="baz",job="foo"}[2m])) by (value $variable)' + ); + }); + + it('should not add ad-hoc filter to range', () => { + expect(addLabelToQuery('avg(rate((my_metric{job="foo"} > 0)[3h:])) by (label)', 'bar', 'baz')).toBe( + 'avg(rate((my_metric{bar="baz",job="foo"} > 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) (my_metric{type=~"foo|bar|baz-test"}) * on(id) group_right(id, type, name) sum by (id) (my_metric) * 1000', + 'bar', + 'baz' + ) + ).toBe( + 'max by (id, name, type) (my_metric{bar="baz",type=~"foo|bar|baz-test"}) * on(id) group_right(id, type, name) sum by (id) (my_metric{bar="baz"}) * 1000' + ); + }); + it('should not add ad-hoc filter to labels in label list provided with the group modifier', () => { + expect(addLabelToQuery('rate(my_metric[${__range_s}s])', 'bar', 'baz')).toBe( + 'rate(my_metric{bar="baz"}[${__range_s}s])' + ); + }); + it('should not add ad-hoc filter to labels to math operations', () => { + expect(addLabelToQuery('count(my_metric{job!="foo"} < (5*1024*1024*1024) or vector(0)) - 1', 'bar', 'baz')).toBe( + 'count(my_metric{bar="baz",job!="foo"} < (5*1024*1024*1024) or vector(0)) - 1' ); }); }); diff --git a/public/app/plugins/datasource/prometheus/add_label_to_query.ts b/public/app/plugins/datasource/prometheus/add_label_to_query.ts index bad3e696c38..269479acc3d 100644 --- a/public/app/plugins/datasource/prometheus/add_label_to_query.ts +++ b/public/app/plugins/datasource/prometheus/add_label_to_query.ts @@ -1,10 +1,12 @@ import _ from 'lodash'; -const keywords = 'by|without|on|ignoring|group_left|group_right|bool|or|and|unless'; +const keywords = 'by|without|on|ignoring|group_left|group_right|bool'; +const logicalOperators = 'or|and|unless'; // Duplicate from mode-prometheus.js, which can't be used in tests due to global ace not being loaded. const builtInWords = [ keywords, + logicalOperators, 'count|count_values|min|max|avg|sum|stddev|stdvar|bottomk|topk|quantile', 'true|false|null|__name__|job', 'abs|absent|ceil|changes|clamp_max|clamp_min|count_scalar|day_of_month|day_of_week|days_in_month|delta|deriv', @@ -15,8 +17,12 @@ const builtInWords = [ .join('|') .split('|'); -const metricNameRegexp = /([A-Za-z:][\w:]*)\b(?![\(\]{=!",])/g; -const selectorRegexp = /{([^{]*)}/g; +// We want to extract all possible metrics and also keywords +const metricsAndKeywordsRegexp = /([A-Za-z:][\w:]*)\b(?![\]{=!",])/g; +// Safari currently doesn't support negative lookbehind. When it does, we should refactor this. +// We are creating 2 matching groups. (\$) is for the Grafana's variables such as ${__rate_s}. We want to ignore +// ${__rate_s} and not add variable to it. +const selectorRegexp = /(\$)?{([^{]*)}/g; export function addLabelToQuery( query: string, @@ -34,29 +40,12 @@ export function addLabelToQuery( // Add empty selectors to bare metric names let previousWord: string; - query = query.replace(metricNameRegexp, (match, word, offset) => { - const insideSelector = isPositionInsideChars(query, offset, '{', '}'); - // Handle "sum by (key) (metric)" - const previousWordIsKeyWord = previousWord && keywords.split('|').indexOf(previousWord) > -1; - - // check for colon as as "word boundary" symbol - const isColonBounded = word.endsWith(':'); + query = query.replace(metricsAndKeywordsRegexp, (match, word, offset) => { + const isMetric = isWordMetric(query, word, offset, previousWord, hasNoMetrics); previousWord = word; - // with Prometheus datasource, adds an empty selector to a bare metric name - // but doesn't add it with Loki datasource so there are no unnecessary labels - // e.g. when the filter contains a dash (-) character inside - if ( - !hasNoMetrics && - !insideSelector && - !isColonBounded && - !previousWordIsKeyWord && - builtInWords.indexOf(word) === -1 - ) { - return `${word}{}`; - } - return word; + return isMetric ? `${word}{}` : word; }); // Adding label to existing selectors @@ -67,11 +56,19 @@ export function addLabelToQuery( while (match) { const prefix = query.slice(lastIndex, match.index); - const selector = match[1]; - const selectorWithLabel = addLabelToSelector(selector, key, transformedValue, operator); - lastIndex = match.index + match[1].length + 2; + lastIndex = match.index + match[2].length + 2; suffix = query.slice(match.index + match[0].length); - parts.push(prefix, selectorWithLabel); + // If we matched 1st group, we know it is Grafana's variable and we don't want to add labels + if (match[1]) { + parts.push(prefix); + parts.push(match[0]); + } else { + // If we didn't match first group, we are inside selector and we want to add labels + const selector = match[2]; + const selectorWithLabel = addLabelToSelector(selector, key, transformedValue, operator); + parts.push(prefix, selectorWithLabel); + } + match = selectorRegexp.exec(query); } @@ -115,4 +112,32 @@ function isPositionInsideChars(text: string, position: number, openChar: string, return nextSelectorEnd > -1 && (nextSelectorStart === -1 || nextSelectorStart > nextSelectorEnd); } +function isWordMetric(query: string, word: string, offset: number, previousWord: string, hasNoMetrics?: boolean) { + const insideSelector = isPositionInsideChars(query, offset, '{', '}'); + // Handle "sum by (key) (metric)" + const previousWordIsKeyWord = previousWord && keywords.split('|').indexOf(previousWord) > -1; + // Check for colon as as "word boundary" symbol + const isColonBounded = word.endsWith(':'); + // Check for words that start with " which means that they are not metrics + const startsWithQuote = query[offset - 1] === '"'; + // Check for template variables + const isTemplateVariable = query[offset - 1] === '$'; + // Check for time units + const isTimeUnit = ['s', 'm', 'h', 'd', 'w'].includes(word) && Boolean(Number(query[offset - 1])); + + if ( + !hasNoMetrics && + !insideSelector && + !isColonBounded && + !previousWordIsKeyWord && + !startsWithQuote && + !isTemplateVariable && + !isTimeUnit && + builtInWords.indexOf(word) === -1 + ) { + return true; + } + return false; +} + export default addLabelToQuery;