From 5a39f29c74672f03a10d801485f36e8502ffb897 Mon Sep 17 00:00:00 2001 From: Gareth Date: Mon, 7 Jul 2025 13:04:07 +0100 Subject: [PATCH] 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 * prettier --------- Co-authored-by: ismail simsek --- .../monaco-query-field/MonacoQueryField.tsx | 37 ++++--- .../validation.test.ts | 83 ++++++++-------- .../monaco-completion-provider/validation.ts | 96 ++++++++++++++----- 3 files changed, 139 insertions(+), 77 deletions(-) diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/MonacoQueryField.tsx b/packages/grafana-prometheus/src/components/monaco-query-field/MonacoQueryField.tsx index 1c24fadd96b..0200645d392 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/MonacoQueryField.tsx +++ b/packages/grafana-prometheus/src/components/monaco-query-field/MonacoQueryField.tsx @@ -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]); }); } }} diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.test.ts b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.test.ts index 3768457dfce..14acf601422 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.test.ts +++ b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.test.ts @@ -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 }, + ], + }); }); }); diff --git a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.ts b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.ts index 34ce38c060b..64b96aa8c57 100644 --- a/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.ts +++ b/packages/grafana-prometheus/src/components/monaco-query-field/monaco-completion-provider/validation.ts @@ -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 = { + 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; }