From 4b1fa81394e25a74142f78231565cd9070ba6e03 Mon Sep 17 00:00:00 2001 From: ismail simsek Date: Tue, 22 Apr 2025 19:57:01 +0200 Subject: [PATCH] Chore: Better builtin variable check during parsing the code (#103952) * replace and return the builtin variables * don't parse the expression twice * improve the replacement logic * better code with more tests * lint * betterer * rename the test suite --- .../src/querybuilder/parsing.ts | 48 ++---------- .../src/querybuilder/parsingUtils.test.ts | 74 ++++++++++++++++++- .../src/querybuilder/parsingUtils.ts | 57 +++++++++++++- 3 files changed, 135 insertions(+), 44 deletions(-) diff --git a/packages/grafana-prometheus/src/querybuilder/parsing.ts b/packages/grafana-prometheus/src/querybuilder/parsing.ts index 42713dfb986..ac3c45d84c6 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsing.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsing.ts @@ -34,7 +34,9 @@ import { getString, makeBinOp, makeError, + replaceBuiltInVariable, replaceVariables, + returnBuiltInVariable, } from './parsingUtils'; import { QueryBuilderLabelFilter, QueryBuilderOperation } from './shared/types'; import { PromVisualQuery, PromVisualQueryBinary } from './types'; @@ -42,12 +44,11 @@ import { PromVisualQuery, PromVisualQueryBinary } from './types'; /** * Parses a PromQL query into a visual query model. * - * It traverses the tree and uses sort of state machine to update the query model. The query model is modified - * during the traversal and sent to each handler as context. - * - * @param expr + * It traverses the tree and uses sort of state machine to update the query model. + * The query model is modified during the traversal and sent to each handler as context. */ export function buildVisualQueryFromString(expr: string): Context { + expr = replaceBuiltInVariable(expr); const replacedExpr = replaceVariables(expr); const tree = parser.parse(replacedExpr); const node = tree.topNode; @@ -80,11 +81,6 @@ export function buildVisualQueryFromString(expr: string): Context { context.errors = []; } - // We don't want parsing errors related to Grafana global variables - if (isValidPromQLMinusGrafanaGlobalVariables(expr)) { - context.errors = []; - } - return context; } @@ -100,36 +96,6 @@ interface Context { errors: ParsingError[]; } -// TODO find a better approach for grafana global variables -function isValidPromQLMinusGrafanaGlobalVariables(expr: string) { - const context: Context = { - query: { - metric: '', - labels: [], - operations: [], - }, - errors: [], - }; - - expr = expr.replace(/\$__interval/g, '1s'); - expr = expr.replace(/\$__interval_ms/g, '1000'); - expr = expr.replace(/\$__rate_interval/g, '1s'); - expr = expr.replace(/\$__range_ms/g, '1000'); - expr = expr.replace(/\$__range_s/g, '1'); - expr = expr.replace(/\$__range/g, '1s'); - - const tree = parser.parse(expr); - const node = tree.topNode; - - try { - handleExpression(expr, node, context); - } catch (err) { - return false; - } - - return context.errors.length === 0; -} - /** * Handler for default state. It will traverse the tree and call the appropriate handler for each node. The node * handled here does not necessarily need to be of type == Expr. @@ -277,7 +243,9 @@ function handleFunction(expr: string, node: SyntaxNode, context: Context) { let match = getString(expr, node).match(/\[(.+)\]/); if (match?.[1]) { interval = match[1]; - params.push(match[1]); + // We were replaced the builtin variables to prevent errors + // Here we return those back + params.push(returnBuiltInVariable(match[1])); } } diff --git a/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts b/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts index 3bef72e1fd0..b5f9a55a434 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsingUtils.test.ts @@ -1,7 +1,13 @@ // Core Grafana history https://github.com/grafana/grafana/blob/v11.0.0-preview/public/app/plugins/datasource/prometheus/querybuilder/parsingUtils.test.ts import { parser } from '@prometheus-io/lezer-promql'; -import { getLeftMostChild, getString, replaceVariables } from './parsingUtils'; +import { + getLeftMostChild, + getString, + replaceBuiltInVariable, + replaceVariables, + returnBuiltInVariable, +} from './parsingUtils'; describe('getLeftMostChild', () => { it('return left most child', () => { @@ -41,3 +47,69 @@ describe('getString', () => { expect(getString(replaced, tree.topNode)).toBe(expr); }); }); + +describe('builtInTimeVariables', () => { + const testCases = [ + { + expr: 'sum_over_time([[metric_var]]{bar="${app}"}[$__interval])', + expected: 'sum_over_time([[metric_var]]{bar="${app}"}[711_999_999])', + }, + { + expr: 'sum_over_time([[metric_var]]{bar="${app}"}[$__rate_interval])', + expected: 'sum_over_time([[metric_var]]{bar="${app}"}[7999799979997999])', + }, + { + expr: 'sum_over_time([[metric_var]]{bar="${app}"}[$__range_ms])', + expected: 'sum_over_time([[metric_var]]{bar="${app}"}[722_999_999])', + }, + { + expr: 'histogram_quantile(0.95, sum(rate(process_max_fds[$__rate_interval])) by (le)) + rate(process_max_fds[$__interval])', + expected: + 'histogram_quantile(0.95, sum(rate(process_max_fds[7999799979997999])) by (le)) + rate(process_max_fds[711_999_999])', + }, + { + expr: 'rate(http_requests_total{job="api-server"}[$__interval_ms] offset $__interval_ms)', + expected: 'rate(http_requests_total{job="api-server"}[79_999_999_999] offset 79_999_999_999)', + }, + { + expr: 'max_over_time(node_memory_usage[$__range_s])', + expected: 'max_over_time(node_memory_usage[79_299_999])', + }, + { + expr: 'avg_over_time(cpu_usage{env="prod"}[$__range])', + expected: 'avg_over_time(cpu_usage{env="prod"}[799_999])', + }, + { + expr: 'rate(requests[$__interval]) / rate(requests[$__interval] offset $__interval)', + expected: 'rate(requests[711_999_999]) / rate(requests[711_999_999] offset 711_999_999)', + }, + { + expr: 'sum(rate(http_requests_total{status=~"5.."}[$__rate_interval])) / sum(rate(http_requests_total[$__rate_interval])) or vector($__range_ms / $__interval_ms)', + expected: + 'sum(rate(http_requests_total{status=~"5.."}[7999799979997999])) / sum(rate(http_requests_total[7999799979997999])) or vector(722_999_999 / 79_999_999_999)', + }, + { + expr: 'sum(rate(http_requests_total{job="api"}[5m]))', + expected: 'sum(rate(http_requests_total{job="api"}[5m]))', + }, + { + expr: 'max_over_time(rate(cpu{instance="server-01"}[$__interval])[$__range_s:$__interval])', + expected: 'max_over_time(rate(cpu{instance="server-01"}[711_999_999])[79_299_999:711_999_999])', + }, + { + expr: 'rate(cpu[$__interval]) + rate(memory[$__interval_ms]) + rate(disk[$__rate_interval]) + rate(network[$__range]) + rate(io[$__range_s]) + rate(gpu[$__range_ms])', + expected: + 'rate(cpu[711_999_999]) + rate(memory[79_999_999_999]) + rate(disk[7999799979997999]) + rate(network[799_999]) + rate(io[79_299_999]) + rate(gpu[722_999_999])', + }, + ]; + + testCases.forEach((testCase) => { + it(testCase.expr, () => { + const actual1 = replaceBuiltInVariable(testCase.expr); + expect(actual1).toBe(testCase.expected); + + const actual2 = returnBuiltInVariable(actual1); + expect(actual2).toBe(testCase.expr); + }); + }); +}); diff --git a/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts b/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts index 2b9cb162d93..15136e51005 100644 --- a/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts +++ b/packages/grafana-prometheus/src/querybuilder/parsingUtils.ts @@ -32,9 +32,8 @@ export function makeError(expr: string, node: SyntaxNode) { const variableRegex = /\$(\w+)|\[\[([\s\S]+?)(?::(\w+))?\]\]|\${(\w+)(?:\.([^:^\}]+))?(?::([^\}]+))?}/g; /** - * As variables with $ are creating parsing errors, we first replace them with magic string that is parsable and at - * the same time we can get the variable and its format back from it. - * @param expr + * As variables with $ are creating parsing errors, we first replace them with magic string that is + * parsable and at the same time we can get the variable and its format back from it. */ export function replaceVariables(expr: string) { return expr.replace(variableRegex, (match, var1, var2, fmt2, var3, fieldPath, fmt3) => { @@ -138,3 +137,55 @@ export const regexifyLabelValuesQueryString = (query: string) => { const queryArray = query.split(' '); return queryArray.map((query) => `${query}.*`).join(''); }; + +/** + * Built-in Grafana variables used for time ranges and intervals in Prometheus queries + * Each variable has a carefully crafted numeric replacement that: + * 1. Has exactly the same string length as the original variable + * 2. Is valid in Prometheus syntax to avoid parsing errors + * 3. Preserves error position information for accurate error reporting + * 4. Uses readable number formatting with digit grouping via underscores + * https://prometheus.io/docs/prometheus/latest/querying/basics/#float-literals-and-time-durations + */ +const BUILT_IN_VARIABLES = [ + { variable: '$__interval_ms', replacement: '79_999_999_999' }, + { variable: '$__interval', replacement: '711_999_999' }, + { variable: '$__rate_interval', replacement: '7999799979997999' }, + { variable: '$__range_ms', replacement: '722_999_999' }, + { variable: '$__range_s', replacement: '79_299_999' }, + { variable: '$__range', replacement: '799_999' }, +]; + +// Derived maps for efficient lookups +const variableToReplacement = BUILT_IN_VARIABLES.reduce>((map, { variable, replacement }) => { + map[variable] = replacement; + return map; +}, {}); + +const replacementToVariable = BUILT_IN_VARIABLES.reduce>((map, { variable, replacement }) => { + map[replacement] = variable; + return map; +}, {}); + +// Pre-compiled regular expressions for efficient search/replace +const builtInVariablePattern = BUILT_IN_VARIABLES.map(({ variable }) => variable.replace(/\$/g, '\\$')).join('|'); +const builtInVariableRegex = new RegExp(builtInVariablePattern, 'g'); + +const builtInReplacementPattern = BUILT_IN_VARIABLES.map(({ replacement }) => replacement).join('|'); +const builtInReplacementRegex = new RegExp(builtInReplacementPattern, 'g'); + +/** + * Replaces Grafana built-in variables with numeric replacements + * This helps prevent these variables from causing parsing errors + */ +export function replaceBuiltInVariable(expr: string): string { + return expr.replace(builtInVariableRegex, (match) => variableToReplacement[match]); +} + +/** + * Restores the original built-in variables from their replacement format + * Reverses the transformation done by replaceBuiltInVariable + */ +export function returnBuiltInVariable(expr: string): string { + return expr.replace(builtInReplacementRegex, (match) => replacementToVariable[match]); +}