[ENHANCEMENT] Prometheus: explain why $__rate_interval cannot be used as Min Step

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).

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<PrometheusTimeSeriesQueryS
 
   // Min step is the lower bound of the interval between data points
   // If no value is provided for it, it should default to the scrape interval of the datasource
-  const minStep =
-    getDurationStringSeconds(
-      // resolve any variable that may have been provided
-      // TODO add a validation check to make sure the variable is a DurationString, to avoid the back & forth cast here
-      replaceVariables(spec.minStep as string, context.variableState) as DurationString,
-    ) ?? datasourceScrapeInterval;
+  const minStep = getMinStepSeconds(spec.minStep, context.variableState) ?? datasourceScrapeInterval;
 
   const timeRange = getPrometheusTimeRange(context.timeRange);
   const step = getRangeStep(timeRange, minStep, undefined, context.suggestedStepMs); // TODO: resolution

prometheus/src/plugins/prometheus-time-series-query/min-step.test.ts

@@ -0,0 +1,105 @@
+// 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 { PrometheusDatasource } from '../prometheus-datasource';
+import { getMinStepSeconds } from './min-step';
+
+// The state of the built-in variables of the Prometheus datasource in a dashboard, built as plugin-system does.
+// They resolve to themselves: they are only replaced in the PromQL query, with values computed from the step.
+const PROMETHEUS_BUILTIN_VARIABLES: VariableStateMap = Object.fromEntries(
+  (PrometheusDatasource.getBuiltinVariableDefinitions?.() ?? []).map(({ spec }) => [
+    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"),
+    );
+  });
+});

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

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<PrometheusDatasourceSpec> => {
+      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);
+  });
 });