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

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'
     );
   });
 });

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;