From bfda412f549b7ab422b21d36d7cde58a6bb630fb Mon Sep 17 00:00:00 2001 From: Raj Zalavadia Date: Tue, 6 Oct 2026 06:00:14 +0530 Subject: [PATCH] [ENHANCEMENT] Prometheus: explain why $__rate_interval cannot be used as Min Step Using $__rate_interval as the Min Step of a Prometheus query made the panel fail with "Invalid duration string '$__rate_interval'", which names neither the field nor the reason. $__interval, $__interval_ms and $__rate_interval are computed from the step, whose lower bound is the Min Step, so they cannot set it. The query now fails with: '$__rate_interval' cannot be used as Min Step, because its value depends on the Min Step. Leave Min Step empty to use the scrape interval of the datasource, or set a duration such as 30s. This is checked after the dashboard variables are resolved, so a variable whose value is one of them is caught too. The message then also names the Min Step, e.g. "Min Step '$res' resolves to '$__rate_interval', which cannot be used as Min Step, ...". Other invalid values now name the field and show the resolved value, e.g. "Invalid Min Step '$res' (resolved to 'abc'): expected a duration such as 30s or 5m". The accepted values do not change, and an empty Min Step (or a variable with an empty value) still falls back to the scrape interval of the datasource. The parsing moves to an internal helper, getMinStepSeconds, which resolves the TODO about validating the resolved value. The built-in variables section of the Prometheus docs now says that these variables cannot be used as Min Step. Closes perses/perses#2025 Signed-off-by: Raj Zalavadia --- docs/prometheus/README.md | 2 + .../get-time-series-data.ts | 18 +-- .../min-step.test.ts | 105 ++++++++++++++++++ .../prometheus-time-series-query/min-step.ts | 54 +++++++++ .../plugin.test.ts | 69 +++++++++++- 5 files changed, 232 insertions(+), 16 deletions(-) create mode 100644 prometheus/src/plugins/prometheus-time-series-query/min-step.test.ts create mode 100644 prometheus/src/plugins/prometheus-time-series-query/min-step.ts diff --git a/docs/prometheus/README.md b/docs/prometheus/README.md index e93d514fb..3ea8a965d 100644 --- a/docs/prometheus/README.md +++ b/docs/prometheus/README.md @@ -65,6 +65,8 @@ The Prometheus package provide several built-in variables that can be used withi - **`__interval_ms`**: Same as `$__interval` but in milliseconds. - **`$__rate_interval`**: Use this one rather than `$__interval` as the range parameter of promQL functions like `rate()`, `increase()`, etc. With such function it is advised to choose a range that is at least 4x the scrape interval (this is to allow for various races, and to be resilient to a failed scrape). `$__rate_interval` provides that, as it is defined as `max($__interval + Min Step, 4 * Min Step)`, where the Min Step value should represent the scrape interval of the metrics. +These variables are computed from the step of the query, whose lower bound is the Min Step, so they cannot be used to set the Min Step itself. + ### Variable Syntax The syntax to use is the [standard variable syntax of Perses](https://perses.dev/perses/docs/concepts/variable/#using-variables). diff --git a/prometheus/src/plugins/prometheus-time-series-query/get-time-series-data.ts b/prometheus/src/plugins/prometheus-time-series-query/get-time-series-data.ts index 053ce91d0..36deff990 100644 --- a/prometheus/src/plugins/prometheus-time-series-query/get-time-series-data.ts +++ b/prometheus/src/plugins/prometheus-time-series-query/get-time-series-data.ts @@ -13,14 +13,7 @@ import type { TimeSeriesQueryPlugin } from '@perses-dev/plugin-system'; import { datasourceSelectValueToSelector, replaceVariables } from '@perses-dev/plugin-system'; -import type { - DatasourceSpec, - DurationString, - Notice, - TimeSeries, - TimeSeriesData, - TimeSeriesExemplars, -} from '@perses-dev/spec'; +import type { DatasourceSpec, Notice, TimeSeries, TimeSeriesData, TimeSeriesExemplars } from '@perses-dev/spec'; import { parseDurationString } from '@perses-dev/spec'; import { fromUnixTime, milliseconds } from 'date-fns'; @@ -35,7 +28,6 @@ import type { import { parseValueTuple, parseSampleValue, - getDurationStringSeconds, getPrometheusTimeRange, getRangeStep, DEFAULT_PROM, @@ -45,6 +37,7 @@ import { getFormattedPrometheusSeriesName } from '../../utils'; import { interpolateDatasourceProxyParams } from '../interpolation'; import type { PrometheusDatasourceSpec } from '../types'; import { DEFAULT_SCRAPE_INTERVAL } from '../types'; +import { getMinStepSeconds } from './min-step'; import { replacePromBuiltinVariables } from './replace-prom-builtin-variables'; import type { PrometheusTimeSeriesQuerySpec } from './time-series-query-model'; @@ -78,12 +71,7 @@ export const getTimeSeriesData: TimeSeriesQueryPlugin [ + spec.name, + { value: spec.value(), loading: false }, + ]), +); + +describe('getMinStepSeconds', () => { + it('should return undefined when no Min Step is set', () => { + expect(getMinStepSeconds(undefined, {})).toBeUndefined(); + expect(getMinStepSeconds('', {})).toBeUndefined(); + }); + + it.each([ + { minStep: '30s', expected: 30 }, + { minStep: '1m30s', expected: 90 }, + { minStep: '1500ms', expected: 1 }, + { minStep: '0s', expected: 0 }, + ])('should convert $minStep to seconds', ({ minStep, expected }) => { + expect(getMinStepSeconds(minStep, PROMETHEUS_BUILTIN_VARIABLES)).toBe(expected); + }); + + it('should resolve the variables used in the Min Step', () => { + expect(getMinStepSeconds('$resolution', { resolution: { value: '5m', loading: false } })).toBe(300); + }); + + it('should return undefined when a variable used as Min Step has an empty value', () => { + expect(getMinStepSeconds('$resolution', { resolution: { value: '', loading: false } })).toBeUndefined(); + }); + + it.each([ + { minStep: '$__rate_interval', variable: '$__rate_interval' }, + { minStep: '${__rate_interval}', variable: '$__rate_interval' }, + { minStep: '$__interval', variable: '$__interval' }, + { minStep: '$__interval_ms', variable: '$__interval_ms' }, + ])('should explain that $minStep cannot be used as Min Step', ({ minStep, variable }) => { + const expected = `'${variable}' cannot be used as Min Step`; + expect(() => getMinStepSeconds(minStep, PROMETHEUS_BUILTIN_VARIABLES)).toThrow(expected); + expect(() => getMinStepSeconds(minStep, {})).toThrow(expected); + }); + + it('should explain that no built-in variable of the Prometheus datasource can be used as Min Step', () => { + // They are all computed from the step (see replacePromBuiltinVariables). + // This fails if one is added to the datasource but not to STEP_BUILTIN_VARIABLES. + const names = Object.keys(PROMETHEUS_BUILTIN_VARIABLES); + expect(names).toContain('__rate_interval'); + for (const name of names) { + expect(() => getMinStepSeconds(`$${name}`, PROMETHEUS_BUILTIN_VARIABLES)).toThrow( + `'$${name}' cannot be used as Min Step`, + ); + } + }); + + it('should tell how to fix a Min Step that uses $__rate_interval', () => { + expect(() => getMinStepSeconds('$__rate_interval', PROMETHEUS_BUILTIN_VARIABLES)).toThrow( + new Error( + "'$__rate_interval' cannot be used as Min Step, because its value depends on the Min Step. Leave Min Step empty to use the scrape interval of the datasource, or set a duration such as 30s.", + ), + ); + }); + + it('should name the Min Step when a variable used as Min Step resolves to $__rate_interval', () => { + const variableState: VariableStateMap = { + ...PROMETHEUS_BUILTIN_VARIABLES, + resolution: { value: '$__rate_interval', loading: false }, + }; + expect(() => getMinStepSeconds('$resolution', variableState)).toThrow( + new Error( + "Min Step '$resolution' resolves to '$__rate_interval', which cannot be used as Min Step, because its value depends on the Min Step. Leave Min Step empty to use the scrape interval of the datasource, or set a duration such as 30s.", + ), + ); + }); + + it('should name the Min Step when it is not a duration', () => { + expect(() => getMinStepSeconds('30 s', {})).toThrow( + new Error("Invalid Min Step '30 s': expected a duration such as 30s or 5m"), + ); + }); + + it('should show the resolved value when a variable used as Min Step is not a duration', () => { + expect(() => getMinStepSeconds('$resolution', { resolution: { value: 'abc', loading: false } })).toThrow( + new Error("Invalid Min Step '$resolution' (resolved to 'abc'): expected a duration such as 30s or 5m"), + ); + }); +}); diff --git a/prometheus/src/plugins/prometheus-time-series-query/min-step.ts b/prometheus/src/plugins/prometheus-time-series-query/min-step.ts new file mode 100644 index 000000000..e42251b86 --- /dev/null +++ b/prometheus/src/plugins/prometheus-time-series-query/min-step.ts @@ -0,0 +1,54 @@ +// Copyright The Perses Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import type { VariableStateMap } from '@perses-dev/components'; +import { parseVariables, replaceVariables } from '@perses-dev/plugin-system'; +import { isDurationString } from '@perses-dev/spec'; + +import { getDurationStringSeconds } from '../../model'; + +// The built-in variables replaced by replacePromBuiltinVariables: their values are computed from the step, whose +// lower bound is the Min Step, so they cannot be used to set the Min Step itself. +const STEP_BUILTIN_VARIABLES = new Set(['__interval', '__interval_ms', '__rate_interval']); + +/** + * Resolves the variables used in the Min Step of a query and converts it to seconds, rounding down. + * Returns undefined when no Min Step is set, so that the caller can fall back to the scrape interval of the datasource. + * Throws an error that explains the problem when the Min Step is not a valid duration. + */ +export function getMinStepSeconds(minStep: string | undefined, variableState: VariableStateMap): number | undefined { + if (!minStep) return undefined; + + const value = replaceVariables(minStep, variableState); + + // Checked on the resolved value, so that a dashboard variable whose value is one of them is caught too + const stepVariable = parseVariables(value).find((name) => STEP_BUILTIN_VARIABLES.has(name)); + if (stepVariable !== undefined) { + // When it comes from the value of a dashboard variable, also show what the Min Step contains + const usedDirectly = parseVariables(minStep).some((name) => STEP_BUILTIN_VARIABLES.has(name)); + const subject = usedDirectly ? `'$${stepVariable}'` : `Min Step '${minStep}' resolves to '$${stepVariable}', which`; + throw new Error( + `${subject} cannot be used as Min Step, because its value depends on the Min Step. Leave Min Step empty to use the scrape interval of the datasource, or set a duration such as 30s.`, + ); + } + + // A variable with an empty value means that no Min Step is set + if (value === '') return undefined; + + if (!isDurationString(value)) { + const resolved = value === minStep ? '' : ` (resolved to '${value}')`; + throw new Error(`Invalid Min Step '${minStep}'${resolved}: expected a duration such as 30s or 5m`); + } + + return getDurationStringSeconds(value); +} diff --git a/prometheus/src/plugins/prometheus-time-series-query/plugin.test.ts b/prometheus/src/plugins/prometheus-time-series-query/plugin.test.ts index 7ed52508e..415322e7b 100644 --- a/prometheus/src/plugins/prometheus-time-series-query/plugin.test.ts +++ b/prometheus/src/plugins/prometheus-time-series-query/plugin.test.ts @@ -16,7 +16,7 @@ vi.mock('echarts/core'); import type { TimeSeriesQueryContext } from '@perses-dev/plugin-system'; -import type { DatasourceSpec } from '@perses-dev/spec'; +import type { DatasourceSpec, DurationString } from '@perses-dev/spec'; import type { Mock } from 'vitest'; import type { RangeQueryResponse, InstantQueryResponse, QueryExemplarsResponse } from '../../model'; @@ -318,4 +318,71 @@ describe('PrometheusTimeSeriesQuery', () => { expect(promStubClient.instantQuery).toHaveBeenCalledTimes(1); expect(promStubClient.rangeQuery).not.toHaveBeenCalled(); }); + + it('should explain that $__rate_interval cannot be used as Min Step, for range and instant queries', async () => { + const ctx = createStubContext(); + // In a dashboard, the built-in variables of the Prometheus datasource resolve to themselves: + // they are only replaced in the PromQL query, with values computed from the step. + ctx.variableState = { __rate_interval: { value: '$__rate_interval', loading: false } }; + (promStubClient.rangeQuery as Mock).mockClear(); + (promStubClient.instantQuery as Mock).mockClear(); + const minStep = '$__rate_interval' as DurationString; + + await expect(PrometheusTimeSeriesQuery.getTimeSeriesData({ query: 'up', minStep }, ctx)).rejects.toThrow( + "'$__rate_interval' cannot be used as Min Step", + ); + await expect( + PrometheusTimeSeriesQuery.getTimeSeriesData({ query: 'up', minStep, instant: true }, ctx), + ).rejects.toThrow("'$__rate_interval' cannot be used as Min Step"); + expect(promStubClient.rangeQuery).not.toHaveBeenCalled(); + expect(promStubClient.instantQuery).not.toHaveBeenCalled(); + }); + + it('should resolve the variables used in the Min Step', async () => { + const ctx = createStubContext(); + // On a 6h range, the step is the Min Step: it is above the safe step (3s), and no step is suggested + ctx.timeRange = { start: new Date('2023-01-01T00:00:00Z'), end: new Date('2023-01-01T06:00:00Z') }; + ctx.variableState = { resolution: { value: '5m', loading: false } }; + (promStubClient.rangeQuery as Mock).mockClear(); + + await PrometheusTimeSeriesQuery.getTimeSeriesData({ query: 'up', minStep: '$resolution' as DurationString }, ctx); + + expect(promStubClient.rangeQuery).toHaveBeenCalledTimes(1); + const [params] = (promStubClient.rangeQuery as Mock).mock.calls[0] as [{ step: number }]; + expect(params.step).toBe(300); + }); + + it('should use the scrape interval of the datasource when a variable used as Min Step has an empty value', async () => { + const ctx = createStubContext(); + ctx.timeRange = { start: new Date('2023-01-01T00:00:00Z'), end: new Date('2023-01-01T06:00:00Z') }; + ctx.variableState = { resolution: { value: '', loading: false } }; + // A scrape interval that differs from the default (1m), so that the test fails if the default is used instead + getDatasource.mockImplementationOnce((): DatasourceSpec => { + return { + default: false, + plugin: { kind: 'PrometheusDatasource', spec: { ...datasource, scrapeInterval: '2m' } }, + }; + }); + (promStubClient.rangeQuery as Mock).mockClear(); + + await PrometheusTimeSeriesQuery.getTimeSeriesData({ query: 'up', minStep: '$resolution' as DurationString }, ctx); + + expect(promStubClient.rangeQuery).toHaveBeenCalledTimes(1); + const [params] = (promStubClient.rangeQuery as Mock).mock.calls[0] as [{ step: number }]; + expect(params.step).toBe(120); + }); + + it('should not use the scrape interval of the datasource when the Min Step is 0s', async () => { + const ctx = createStubContext(); + ctx.timeRange = { start: new Date('2023-01-01T00:00:00Z'), end: new Date('2023-01-01T06:00:00Z') }; + // Below the scrape interval of the datasource (1m): a Min Step of 0s sets no lower bound + ctx.suggestedStepMs = 15 * 1000; + (promStubClient.rangeQuery as Mock).mockClear(); + + await PrometheusTimeSeriesQuery.getTimeSeriesData({ query: 'up', minStep: '0s' }, ctx); + + expect(promStubClient.rangeQuery).toHaveBeenCalledTimes(1); + const [params] = (promStubClient.rangeQuery as Mock).mock.calls[0] as [{ step: number }]; + expect(params.step).toBe(15); + }); });