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

* Use dag to find origin nodes when updating resample queries

Co-authored-by: Gilles De Mey <[email protected]>

* 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 <[email protected]>
Co-authored-by: Gilles De Mey <[email protected]>

Author: konrad147

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

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

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}

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,
         },
       ],
     });

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;

public/app/features/alerting/unified/components/rule-editor/util.test.ts

@@ -422,15 +422,15 @@ describe('findRenamedReferences', () => {
       { refId: 'MATH', model: { datasource: '-100' } },
       { refId: 'B' },
       { refId: 'C' },
-    ] as AlertQuery[];
+    ] as Array<AlertQuery<ExpressionQuery>>;
 
     // @ts-expect-error
     const updated = [
       { refId: 'FOO' },
       { refId: 'REDUCE', model: { datasource: '-100' } },
       { refId: 'B' },
       { refId: 'C' },
-    ] as AlertQuery[];
+    ] as Array<AlertQuery<ExpressionQuery>>;
 
     expect(findRenamedDataQueryReferences(previous, updated)).toEqual(['A', 'FOO']);
   });

public/app/features/alerting/unified/utils/dataSourceFromExpression.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 });
+        }
       }
     });
   });

public/app/features/variables/state/actions.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 {