Prometheus: display a warning when subquery is used (#106199)

* display warning when subquery used * lint fix * update tests to work with new return value * prettier * use object shorthand * only warn agains x:x, ignore 2x:x * update error message * prettier * fix tests * add tests * prettier * Update packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.ts Co-authored-by: ismail simsek <ismailsimsek09@gmail.com> * prettier --------- Co-authored-by: ismail simsek <ismailsimsek09@gmail.com>
2025-07-07 13:04:07 +01:00
parent 3b6784cfcf
commit 5a39f29c74
3 changed files with 139 additions and 77 deletions
@@ -257,23 +257,30 @@ const MonacoQueryField = (props: Props) => {
                return;
              }
              const query = model.getValue();
-              const errors =
-                validateQuery(
-                  query,
-                  datasource.interpolateString(query, placeHolderScopedVars),
-                  model.getLinesContent(),
-                  parser
-                ) || [];
+              const { errors, warnings } = validateQuery(
+                query,
+                datasource.interpolateString(query, placeHolderScopedVars),
+                model.getLinesContent(),
+                parser
+              );

-              const markers = errors.map(({ error, ...boundary }) => ({
-                message: `${
-                  error ? `Error parsing "${error}"` : 'Parse error'
-                }. The query appears to be incorrect and could fail to be executed.`,
-                severity: monaco.MarkerSeverity.Error,
-                ...boundary,
-              }));
+              const errorMarkers = errors.map(({ issue, ...boundary }) => {
+                return {
+                  message: `${issue ? `Error parsing "${issue}"` : 'Parse error'}. The query appears to be incorrect and could fail to be executed.`,
+                  severity: monaco.MarkerSeverity.Error,
+                  ...boundary,
+                };
+              });

-              monaco.editor.setModelMarkers(model, 'owner', markers);
+              const warningMarkers = warnings.map(({ issue, ...boundary }) => {
+                return {
+                  message: `Warning: ${issue}`,
+                  severity: monaco.MarkerSeverity.Warning,
+                  ...boundary,
+                };
+              });
+
+              monaco.editor.setModelMarkers(model, 'owner', [...errorMarkers, ...warningMarkers]);
            });
          }
        }}
@@ -1,11 +1,11 @@
 // Core Grafana history https://github.com/grafana/grafana/blob/v11.0.0-preview/public/app/plugins/datasource/prometheus/components/monaco-query-field/monaco-completion-provider/validation.test.ts
 import { parser } from '@prometheus-io/lezer-promql';

-import { validateQuery } from './validation';
+import { validateQuery, warningTypes } from './validation';

 describe('Monaco Query Validation', () => {
  test('Identifies empty queries as valid', () => {
-    expect(validateQuery('', '', [], parser)).toBeFalsy();
+    expect(validateQuery('', '', [], parser)).toEqual({ errors: [], warnings: [] });
  });

  test.each([
@@ -17,45 +17,30 @@ describe('Monaco Query Validation', () => {
    'rate(http_requests_total[5m])',
    'topk(3, sum by (app, proc) (rate(instance_cpu_time_ns[5m])))',
  ])('Identifies valid queries', (query: string) => {
-    expect(validateQuery(query, query, [], parser)).toBeFalsy();
+    expect(validateQuery(query, query, [], parser).errors).toEqual([]);
  });

  test('Identifies invalid queries', () => {
    // Missing } at the end
    let query = 'access_evaluation_duration_sum{job="grafana"';
-    expect(validateQuery(query, query, [query], parser)).toEqual([
-      {
-        endColumn: 45,
-        endLineNumber: 1,
-        error: '{job="grafana"',
-        startColumn: 31,
-        startLineNumber: 1,
-      },
-    ]);
+    expect(validateQuery(query, query, [query], parser)).toEqual({
+      errors: [{ endColumn: 45, endLineNumber: 1, issue: '{job="grafana"', startColumn: 31, startLineNumber: 1 }],
+      warnings: [],
+    });

    // Missing handler="value"
    query = 'http_requests_total{job="apiserver", handler}[5m]';
-    expect(validateQuery(query, query, [query], parser)).toEqual([
-      {
-        endColumn: 45,
-        endLineNumber: 1,
-        error: 'handler',
-        startColumn: 38,
-        startLineNumber: 1,
-      },
-    ]);
+    expect(validateQuery(query, query, [query], parser)).toEqual({
+      errors: [{ endColumn: 45, endLineNumber: 1, issue: 'handler', startColumn: 38, startLineNumber: 1 }],
+      warnings: [],
+    });

    // Missing : in [30s:5s]
    query = 'max_over_time(deriv(rate(distance_covered_total[5s])[30s5s])[10m:])';
-    expect(validateQuery(query, query, [query], parser)).toEqual([
-      {
-        endColumn: 59,
-        endLineNumber: 1,
-        error: '5s',
-        startColumn: 57,
-        startLineNumber: 1,
-      },
-    ]);
+    expect(validateQuery(query, query, [query], parser)).toEqual({
+      errors: [{ endColumn: 59, endLineNumber: 1, issue: '5s', startColumn: 57, startLineNumber: 1 }],
+      warnings: [],
+    });
  });

  test('Identifies valid multi-line queries', () => {
@@ -64,7 +49,7 @@ sum by (job) (
    rate(http_requests_total[5m])
 )`;
    const queryLines = query.split('\n');
-    expect(validateQuery(query, query, queryLines, parser)).toBeFalsy();
+    expect(validateQuery(query, query, queryLines, parser)).toEqual({ errors: [], warnings: [] });
  });

  test('Identifies invalid multi-line queries', () => {
@@ -73,14 +58,32 @@ sum by (job) (
    rate(http_requests_total[])
 )`;
    const queryLines = query.split('\n');
-    expect(validateQuery(query, query, queryLines, parser)).toEqual([
-      {
-        endColumn: 30,
-        endLineNumber: 3,
-        error: '',
-        startColumn: 30,
-        startLineNumber: 3,
-      },
-    ]);
+    expect(validateQuery(query, query, queryLines, parser)).toEqual({
+      errors: [{ endColumn: 30, endLineNumber: 3, issue: '', startColumn: 30, startLineNumber: 3 }],
+      warnings: [],
+    });
+  });
+
+  test('Warns agains subqueries with same duration and step', () => {
+    const query = 'rate(http_requests_total[5m:5m])';
+    const queryLines = query.split('\n');
+    expect(validateQuery(query, query, queryLines, parser)).toEqual({
+      errors: [],
+      warnings: [
+        { endColumn: 32, endLineNumber: 1, issue: warningTypes.SubqueryExpr, startColumn: 6, startLineNumber: 1 },
+      ],
+    });
+  });
+
+  test('Warns agains queries with multiple subqueries', () => {
+    const query = 'quantile_over_time(0.5, rate(http_requests_total[1m:1m]) [1m:1m])';
+    const queryLines = query.split('\n');
+    expect(validateQuery(query, query, queryLines, parser)).toEqual({
+      errors: [],
+      warnings: [
+        { issue: warningTypes.SubqueryExpr, startColumn: 25, endColumn: 65, startLineNumber: 1, endLineNumber: 1 },
+        { issue: warningTypes.SubqueryExpr, startColumn: 30, endColumn: 56, startLineNumber: 1, endLineNumber: 1 },
+      ],
+    });
  });
 });
@@ -5,15 +5,25 @@ import { LRParser } from '@lezer/lr';
 // Although 0 isn't explicitly provided in the @grafana/lezer-logql library as the error node ID, it does appear to be the ID of error nodes within lezer.
 export const ErrorId = 0;

-interface ParserErrorBoundary {
+export const warningTypes: Record<string, string> = {
+  SubqueryExpr:
+    'This subquery may return only one data point, preventing rate/increase/delta calculations. Use a range at least twice the step size (e.g., [2x:x]).',
+};
+
+enum NodeType {
+  SubqueryExpr = 'SubqueryExpr',
+  Duration = 'NumberDurationLiteralInDurationContext',
+}
+
+interface ParserIssueBoundary {
  startLineNumber: number;
  startColumn: number;
  endLineNumber: number;
  endColumn: number;
-  error: string;
+  issue: string;
 }

-interface ParseError {
+interface ParseIssue {
  text: string;
  node: SyntaxNode;
 }
@@ -30,9 +40,9 @@ export function validateQuery(
  interpolatedQuery: string,
  queryLines: string[],
  parser: LRParser
-): ParserErrorBoundary[] | false {
+): { errors: ParserIssueBoundary[]; warnings: ParserIssueBoundary[] } {
  if (!query) {
-    return false;
+    return { errors: [], warnings: [] };
  }

  /**
@@ -42,51 +52,93 @@ export function validateQuery(
   * have different lengths. With this, we also exclude irrelevant parser errors that are produced by
   * lezer not understanding $variables and $__variables, which usually generate 2 or 3 error SyntaxNode.
   */
-  const interpolatedErrors: ParseError[] = parseQuery(interpolatedQuery, parser);
-  if (!interpolatedErrors.length) {
-    return false;
+  const { errors: interpolatedErrors, warnings: interpolatedWarnings } = parseQuery(interpolatedQuery, parser);
+  if (!interpolatedErrors.length && !interpolatedWarnings.length) {
+    return { errors: [], warnings: [] };
  }

-  let parseErrors: ParseError[] = interpolatedErrors;
+  let parseErrors: ParseIssue[] = interpolatedErrors;
+  let parseWarnings: ParseIssue[] = interpolatedWarnings;
  if (query !== interpolatedQuery) {
-    const queryErrors: ParseError[] = parseQuery(query, parser);
+    const { errors: queryErrors, warnings: queryWarnings } = parseQuery(query, parser);
    parseErrors = interpolatedErrors.flatMap(
      (interpolatedError) =>
        queryErrors.filter((queryError) => interpolatedError.text === queryError.text) || interpolatedError
    );
+    parseWarnings = interpolatedWarnings.flatMap(
+      (interpolatedWarning) =>
+        queryWarnings.filter((queryWarning) => interpolatedWarning.node.from === queryWarning.node.from) ||
+        interpolatedWarning
+    );
  }

-  return parseErrors.map((parseError) => findErrorBoundary(query, queryLines, parseError)).filter(isErrorBoundary);
+  const errorBoundaries = parseErrors
+    .map((parseError) => findIssueBoundary(query, queryLines, parseError, 'error'))
+    .filter(isValidIssueBoundary);
+  const warningBoundaries = parseWarnings
+    .map((parseWarning) => findIssueBoundary(query, queryLines, parseWarning, 'warning'))
+    .filter(isValidIssueBoundary);
+
+  return {
+    errors: errorBoundaries,
+    warnings: warningBoundaries,
+  };
 }

 function parseQuery(query: string, parser: LRParser) {
-  const parseErrors: ParseError[] = [];
+  const parseErrors: ParseIssue[] = [];
+  const parseWarnings: ParseIssue[] = [];
+
  const tree = parser.parse(query);
  tree.iterate({
    enter: (nodeRef): false | void => {
      if (nodeRef.type.id === ErrorId) {
        const node = nodeRef.node;
-        parseErrors.push({
-          node: node,
-          text: query.substring(node.from, node.to),
-        });
+        parseErrors.push({ node: node, text: query.substring(node.from, node.to) });
+      }
+
+      if (nodeRef.type.name === NodeType.SubqueryExpr) {
+        const node = nodeRef.node;
+        const durations: string[] = [];
+
+        const children = node.getChildren(NodeType.Duration);
+        for (const child of children) {
+          durations.push(query.substring(child.from, child.to));
+        }
+
+        if (durations.length === 2 && durations[0] === durations[1]) {
+          parseWarnings.push({ node: node, text: query.substring(node.from, node.to) });
+        }
      }
    },
  });
-  return parseErrors;
+
+  return { errors: parseErrors, warnings: parseWarnings };
 }

-function findErrorBoundary(query: string, queryLines: string[], parseError: ParseError): ParserErrorBoundary | null {
+function findIssueBoundary(
+  query: string,
+  queryLines: string[],
+  parseError: ParseIssue,
+  issueType: 'error' | 'warning'
+): ParserIssueBoundary | null {
  if (queryLines.length === 1) {
    const isEmptyString = parseError.node.from === parseError.node.to;
    const errorNode = isEmptyString && parseError.node.parent ? parseError.node.parent : parseError.node;
-    const error = isEmptyString ? query.substring(errorNode.from, errorNode.to) : parseError.text;
+    let issue: string;
+
+    if (issueType === 'error') {
+      issue = isEmptyString ? query.substring(errorNode.from, errorNode.to) : parseError.text;
+    } else {
+      issue = warningTypes[parseError.node.type.name];
+    }
+
    return {
      startLineNumber: 1,
      startColumn: errorNode.from + 1,
      endLineNumber: 1,
      endColumn: errorNode.to + 1,
-      error,
+      issue,
    };
  }

@@ -105,14 +157,14 @@ function findErrorBoundary(query: string, queryLines: string[], parseError: Pars
      startColumn: parseError.node.from - startPos + 1,
      endLineNumber: line + 1,
      endColumn: parseError.node.to - startPos + 1,
-      error: parseError.text,
+      issue: issueType === 'error' ? parseError.text : warningTypes[parseError.node.type.name],
    };
  }

  return null;
 }

-function isErrorBoundary(boundary: ParserErrorBoundary | null): boundary is ParserErrorBoundary {
+function isValidIssueBoundary(boundary: ParserIssueBoundary | null): boundary is ParserIssueBoundary {
  return boundary !== null;
 }