Alerting: Fix resample expression and relativeTimeRange updates (#91689)

* Use dag to find origin nodes when updating resample queries Co-authored-by: Gilles De Mey <gilles.demey@grafana.com> * lint * fix test and types * short-circuit function * Prevent cycles in DAG * Handle dag cycle error * Catch the cyclic link error in dashboard variables --------- Co-authored-by: Gilles De Mey <gilles.demey@grafana.com> Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>
2025-08-02 14:52:16 +08:00 · 2024-08-23 15:22:33 +02:00
parent b4e7329543
commit 494376c5a9
9 changed files with 120 additions and 81 deletions
--- a/public/app/core/utils/dag.test.ts
+++ b/public/app/core/utils/dag.test.ts
@ -116,5 +116,11 @@ describe('Directed acyclic graph', () => {
        dag.link('A', 'non-existing');
      }).toThrowError("cannot link output node named non-existing since it doesn't exist in graph");
    });
+
+    it('when linking would create a cycle should throw error', () => {
+      expect(() => dag.link('C', 'C')).toThrow('cannot link C to C since it would create a cycle');
+      expect(() => dag.link('A', 'B')).toThrow('cannot link A to B since it would create a cycle');
+      expect(() => dag.link('A', 'E')).toThrow('cannot link A to E since it would create a cycle');
+    });
  });
 });
--- a/public/app/core/utils/dag.ts
+++ b/public/app/core/utils/dag.ts
@ -183,6 +183,9 @@ export class Graph {
    const edges: Edge[] = [];
    inputNodes.forEach((input) => {
      outputNodes.forEach((output) => {
+        if (this.willCreateCycle(input, output)) {
+          throw Error(`cannot link ${input.name} to ${output.name} since it would create a cycle`);
+        }
        edges.push(this.createEdge().link(input, output));
      });
    });
@ -217,6 +220,34 @@ export class Graph {
    return descendants;
  }

+  private willCreateCycle(input: Node, output: Node): boolean {
+    if (input === output) {
+      return true;
+    }
+
+    // Perform a DFS to check if the input node is reachable from the output node
+    const visited = new Set<Node>();
+    const stack = [output];
+
+    while (stack.length) {
+      const current = stack.pop()!;
+      if (current === input) {
+        return true;
+      }
+
+      visited.add(current);
+
+      for (const edge of current.outputEdges) {
+        const next = edge.outputNode;
+        if (next && !visited.has(next)) {
+          stack.push(next);
+        }
+      }
+    }
+
+    return false;
+  }
+
  createEdge(): Edge {
    return new Edge();
  }
--- a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx
+++ b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx
@ -16,7 +16,7 @@ import {
 import { DataQuery } from '@grafana/schema';
 import { GraphThresholdsStyleMode, Icon, InlineField, Input, Tooltip, useStyles2, Stack } from '@grafana/ui';
 import { QueryEditorRow } from 'app/features/query/components/QueryEditorRow';
-import { AlertQuery } from 'app/types/unified-alerting-dto';
+import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto';

 import { msToSingleUnitDuration } from '../../utils/time';
 import { ExpressionStatusIndicator } from '../expressions/ExpressionStatusIndicator';
@ -109,7 +109,7 @@ export const QueryWrapper = ({
  }

  // TODO add a warning label here too when the data looks like time series data and is used as an alert condition
-  function HeaderExtras({ query, error, index }: { query: AlertQuery; error?: Error; index: number }) {
+  function HeaderExtras({ query, error, index }: { query: AlertQuery<AlertDataQuery>; error?: Error; index: number }) {
    const queryOptions: AlertQueryOptions = {
      maxDataPoints: query.model.maxDataPoints,
      minInterval: query.model.intervalMs ? msToSingleUnitDuration(query.model.intervalMs) : undefined,
@ -144,7 +144,7 @@ export const QueryWrapper = ({
  return (
    <Stack direction="column" gap={0.5}>
      <div className={styles.wrapper}>
-        <QueryEditorRow<DataQuery>
+        <QueryEditorRow<AlertDataQuery>
          alerting
          collapsable={false}
          dataSource={dsSettings}
--- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx
+++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx
@ -233,19 +233,22 @@ describe('Query and expressions reducer', () => {
    });
  });

-  it('Should update time range for all expressions that have this data source when dispatching updateExpressionTimeRange', () => {
-    const expressionQuery: AlertQuery = {
+  it('Should update time range for all resample expressions that have this data source when dispatching updateExpressionTimeRange', () => {
+    const expressionQuery: AlertQuery<ExpressionQuery> = {
      refId: 'B',
      queryType: 'expression',
      datasourceUid: '__expr__',
      model: {
+        datasource: {
+          type: '__expr__',
+          uid: '__expr__',
+        },
        queryType: 'query',
-        datasource: '__expr__',
        refId: 'B',
        expression: 'A',
-        type: ExpressionQueryType.classic,
+        type: ExpressionQueryType.resample,
        window: '10s',
-      } as ExpressionQuery,
+      },
    };
    const customTimeRange: RelativeTimeRange = { from: 900, to: 1000 };

@ -262,7 +265,7 @@ describe('Query and expressions reducer', () => {
    };

    const newState = queriesAndExpressionsReducer(initialState, updateExpressionTimeRange());
-    expect(newState).toStrictEqual({
+    expect(newState).toStrictEqual<{ queries: AlertQuery[] }>({
      queries: [
        {
          refId: 'A',
@ -274,19 +277,19 @@ describe('Query and expressions reducer', () => {
        {
          datasourceUid: '__expr__',
          model: {
-            datasource: '__expr__',
            expression: 'A',
+            datasource: {
+              type: '__expr__',
+              uid: '__expr__',
+            },
            queryType: 'query',
            refId: 'B',
-            type: ExpressionQueryType.classic,
+            type: ExpressionQueryType.resample,
            window: '10s',
          },
          queryType: 'expression',
          refId: 'B',
-          relativeTimeRange: {
-            from: 900,
-            to: 1000,
-          },
+          relativeTimeRange: customTimeRange,
        },
      ],
    });
--- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts
+++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts
@ -1,4 +1,4 @@
-import { createAction, createReducer } from '@reduxjs/toolkit';
+import { createAction, createReducer, original } from '@reduxjs/toolkit';

 import {
  DataQuery,
@ -8,27 +8,34 @@ import {
  rangeUtil,
  RelativeTimeRange,
 } from '@grafana/data';
-import { findDataSourceFromExpressionRecursive } from 'app/features/alerting/unified/utils/dataSourceFromExpression';
 import { dataSource as expressionDatasource } from 'app/features/expressions/ExpressionDatasource';
 import { isExpressionQuery } from 'app/features/expressions/guards';
 import { ExpressionDatasourceUID, ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types';
 import { defaultCondition } from 'app/features/expressions/utils/expressionTypes';
 import { AlertQuery } from 'app/types/unified-alerting-dto';

+import { logError } from '../../../Analytics';
 import { getDefaultOrFirstCompatibleDataSource } from '../../../utils/datasource';
+import { createDagFromQueries, getOriginOfRefId } from '../dag';
 import { queriesWithUpdatedReferences, refIdExists } from '../util';

 export interface QueriesAndExpressionsState {
  queries: AlertQuery[];
 }

-const findDataSourceFromExpression = (
-  queries: AlertQuery[],
-  expression: string | undefined
-): AlertQuery | null | undefined => {
-  const firstReference = queries.find((alertQuery) => alertQuery.refId === expression);
-  const dataSource = firstReference && findDataSourceFromExpressionRecursive(queries, firstReference);
-  return dataSource;
+const findDataSourceFromExpression = (queries: AlertQuery[], refId: string): AlertQuery | undefined => {
+  const dag = createDagFromQueries(queries);
+  const dataSource = getOriginOfRefId(refId, dag)[0];
+  if (!dataSource) {
+    return;
+  }
+
+  const originQuery = queries.find((query) => query.refId === dataSource);
+  if (originQuery && 'relativeTimeRange' in originQuery) {
+    return originQuery;
+  }
+
+  return;
 };

 const initialState: QueriesAndExpressionsState = {
@ -137,33 +144,51 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder
      state.queries = [...state.queries, ...payload];
    })
    .addCase(updateExpression, (state, { payload }) => {
-      state.queries = state.queries.map((query) => {
-        const dataSourceAlertQuery = findDataSourceFromExpression(state.queries, payload.expression);
+      const queryToUpdate = state.queries.find((query) => query.refId === payload.refId);
+      if (!queryToUpdate) {
+        return;
+      }

-        const relativeTimeRange = dataSourceAlertQuery
-          ? dataSourceAlertQuery.relativeTimeRange
-          : getDefaultRelativeTimeRange();
+      queryToUpdate.model = payload;

-        if (query.refId === payload.refId) {
-          query.model = payload;
-          if (payload.type === ExpressionQueryType.resample) {
-            query.relativeTimeRange = relativeTimeRange;
+      // the resample expression needs to also know what the relative time range is to work with, this means we have to copy it from the source node (data source query)
+      if (payload.type === ExpressionQueryType.resample && payload.expression) {
+        // findDataSourceFromExpression uses memoization and it doesn't always work with proxies when the proxy has been revoked
+        const originalQueries = original(state)?.queries ?? [];
+
+        let relativeTimeRange = getDefaultRelativeTimeRange();
+        try {
+          const dataSourceAlertQuery = findDataSourceFromExpression(originalQueries, payload.expression);
+          if (dataSourceAlertQuery?.relativeTimeRange) {
+            relativeTimeRange = dataSourceAlertQuery.relativeTimeRange;
+          }
+        } catch (error) {
+          if (error instanceof Error) {
+            logError(error);
+          } else {
+            logError(new Error('Error while trying to find data source from expression'));
          }
        }
-        return query;
-      });
+
+        queryToUpdate.relativeTimeRange = relativeTimeRange;
+      }
    })
    .addCase(updateExpressionTimeRange, (state) => {
-      const newState = state.queries.map((query) => {
-        // It's an expression , let's update the relativeTimeRange with its dataSource relativeTimeRange
-        if (query.datasourceUid === ExpressionDatasourceUID) {
-          const dataSource = findDataSourceFromExpression(state.queries, query.model.expression);
+      state.queries.forEach((query) => {
+        // Resample expression needs to get the relativeTimeRange with its dataSource relativeTimeRange
+        if (
+          isExpressionQuery(query.model) &&
+          query.model.type === ExpressionQueryType.resample &&
+          query.model.expression
+        ) {
+          // findDataSourceFromExpression uses memoization and doesn't work with proxies
+          const originalQueries = original(state)?.queries ?? [];
+
+          const dataSource = findDataSourceFromExpression(originalQueries, query.model.expression);
          const relativeTimeRange = dataSource ? dataSource.relativeTimeRange : getDefaultRelativeTimeRange();
          query.relativeTimeRange = relativeTimeRange;
        }
-        return query;
      });
-      state.queries = newState;
    })
    .addCase(updateExpressionRefId, (state, { payload }) => {
      const { newRefId, oldRefId } = payload;
--- a/public/app/features/alerting/unified/components/rule-editor/util.test.ts
+++ b/public/app/features/alerting/unified/components/rule-editor/util.test.ts
@ -422,7 +422,7 @@ describe('findRenamedReferences', () => {
      { refId: 'MATH', model: { datasource: '-100' } },
      { refId: 'B' },
      { refId: 'C' },
-    ] as AlertQuery[];
+    ] as Array<AlertQuery<ExpressionQuery>>;

    // @ts-expect-error
    const updated = [
@ -430,7 +430,7 @@ describe('findRenamedReferences', () => {
      { refId: 'REDUCE', model: { datasource: '-100' } },
      { refId: 'B' },
      { refId: 'C' },
-    ] as AlertQuery[];
+    ] as Array<AlertQuery<ExpressionQuery>>;

    expect(findRenamedDataQueryReferences(previous, updated)).toEqual(['A', 'FOO']);
  });
--- a/public/app/features/alerting/unified/utils/dataSourceFromExpression.ts
+++ b/public/app/features/alerting/unified/utils/dataSourceFromExpression.ts
@ -1,34 +0,0 @@
-import { ExpressionDatasourceUID } from 'app/features/expressions/types';
-import { AlertQuery } from 'app/types/unified-alerting-dto';
-
-export const hasCyclicalReferences = (queries: AlertQuery[]) => {
-  try {
-    JSON.stringify(queries);
-    return false;
-  } catch (e) {
-    return true;
-  }
-};
-
-export const findDataSourceFromExpressionRecursive = (
-  queries: AlertQuery[],
-  alertQuery: AlertQuery
-): AlertQuery | null | undefined => {
-  //Check if this is not cyclical structre
-  if (hasCyclicalReferences(queries)) {
-    return null;
-  }
-  // We have the data source in this dataQuery
-  if (alertQuery.datasourceUid !== ExpressionDatasourceUID) {
-    return alertQuery;
-  }
-  // alertQuery it's an expression, we have to traverse all the tree up to the data source
-  else {
-    const alertQueryReferenced = queries.find((alertQuery_) => alertQuery_.refId === alertQuery.model.expression);
-    if (alertQueryReferenced) {
-      return findDataSourceFromExpressionRecursive(queries, alertQueryReferenced);
-    } else {
-      return null;
-    }
-  }
-};
--- a/public/app/features/variables/state/actions.ts
+++ b/public/app/features/variables/state/actions.ts
@ -20,7 +20,7 @@ import {
  VariableRefresh,
  VariableWithOptions,
 } from '@grafana/data';
-import { config, locationService } from '@grafana/runtime';
+import { config, locationService, logWarning } from '@grafana/runtime';
 import { notifyApp } from 'app/core/actions';
 import { contextSrv } from 'app/core/services/context_srv';
 import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv';
@ -579,7 +579,14 @@ export const createGraph = (variables: TypedVariableModel[]) => {
      }

      if (variableAdapters.get(v1.type).dependsOn(v1, v2)) {
-        g.link(v1.name, v2.name);
+        try {
+          // link might fail if it would create a circular dependency
+          g.link(v1.name, v2.name);
+        } catch (error) {
+          // Catch the exception and return partially linked graph. The caller will handle the case of partial linking and display errors
+          const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+          logWarning('Error linking variables', { error: errorMessage });
+        }
      }
    });
  });
--- a/public/app/types/unified-alerting-dto.ts
+++ b/public/app/types/unified-alerting-dto.ts
@ -1,6 +1,7 @@
 // Prometheus API DTOs, possibly to be autogenerated from openapi spec in the near future

 import { DataQuery, RelativeTimeRange } from '@grafana/data';
+import { ExpressionQuery } from 'app/features/expressions/types';

 import { AlertGroupTotals } from './unified-alerting';

@ -202,12 +203,12 @@ export interface AlertDataQuery extends DataQuery {
  expression?: string;
 }

-export interface AlertQuery {
+export interface AlertQuery<T = AlertDataQuery | ExpressionQuery> {
  refId: string;
  queryType: string;
  relativeTimeRange?: RelativeTimeRange;
  datasourceUid: string;
-  model: AlertDataQuery;
+  model: T;
 }

 export interface GrafanaNotificationSettings {