[v11.4.x] Alerting: AlertingQueryRunner should skip descendant nodes of invalid queries (#97830)

Alerting: AlertingQueryRunner should skip descendant nodes of invalid queries (#97528)

(cherry picked from commit 9c396b74f9)

e2e/plugin-e2e/plugin-e2e-api-tests/as-admin-user/alerting.spec.ts

@@ -1,19 +1,6 @@
 import * as e2e from '@grafana/e2e-selectors';
 import { expect, test } from '@grafana/plugin-e2e';
 
-test('should evaluate to false if entire request returns 500', async ({ page, alertRuleEditPage, selectors }) => {
-  await alertRuleEditPage.alertRuleNameField.fill('Test Alert Rule');
-
-  // remove the default query
-  const queryA = alertRuleEditPage.getAlertRuleQueryRow('A');
-  await alertRuleEditPage
-    .getByGrafanaSelector(selectors.components.QueryEditorRow.actionButton('Remove query'), {
-      root: queryA.locator,
-    })
-    .click();
-  await expect(alertRuleEditPage.evaluate()).not.toBeOK();
-});
-
 test('should evaluate to false if entire request returns 200 but partial query result is invalid', async ({
   page,
   alertRuleEditPage,

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

@@ -2,11 +2,12 @@ import { Graph } from 'app/core/utils/dag';
 import { AlertQuery } from 'app/types/unified-alerting-dto';
 
 import {
-  _getOriginsOfRefId,
-  parseRefsFromMathExpression,
   _createDagFromQueries,
+  _getDescendants,
+  _getOriginsOfRefId,
   fingerprintGraph,
   fingerPrintQueries,
+  parseRefsFromMathExpression,
 } from './dag';
 
 describe('working with dag', () => {
@@ -84,6 +85,20 @@ describe('getOriginsOfRefId', () => {
   });
 });
 
+describe('getDescendants', () => {
+  test('with multiple generations', () => {
+    const graph = new Graph();
+    graph.createNodes(['A', 'B', 'C', 'D', 'E', 'F', 'G']);
+    graph.link('A', 'B');
+    graph.link('B', 'G');
+    graph.link('A', 'C');
+    graph.link('C', 'D');
+    graph.link('E', 'F');
+
+    expect(_getDescendants('A', graph)).toEqual(['B', 'G', 'C', 'D']);
+  });
+});
+
 describe('parseRefsFromMathExpression', () => {
   const cases: Array<[string, string[]]> = [
     ['$A', ['A']],

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

@@ -59,6 +59,7 @@ export function parseRefsFromMathExpression(input: string): string[] {
 }
 
 export const getOriginOfRefId = memoize(_getOriginsOfRefId, (refId, graph) => refId + fingerprintGraph(graph));
+export const getDescendants = memoize(_getDescendants, (refId, graph) => refId + fingerprintGraph(graph));
 
 export function _getOriginsOfRefId(refId: string, graph: Graph): string[] {
   const node = graph.getNode(refId);
@@ -66,25 +67,47 @@ export function _getOriginsOfRefId(refId: string, graph: Graph): string[] {
   const origins: Node[] = [];
 
   // recurse through "node > inputEdges > inputNode"
-  function findChildNode(node: Node) {
+  function findParentNode(node: Node) {
     const inputEdges = node.inputEdges;
 
     if (inputEdges.length > 0) {
       inputEdges.forEach((edge) => {
         if (edge.inputNode) {
-          findChildNode(edge.inputNode);
+          findParentNode(edge.inputNode);
         }
       });
     } else {
-      origins?.push(node);
+      origins.push(node);
     }
   }
 
-  findChildNode(node);
+  findParentNode(node);
 
   return origins.map((origin) => origin.name);
 }
 
+// get all children (and children's children etc) from a given node
+export function _getDescendants(refId: string, graph: Graph): string[] {
+  const node = graph.getNode(refId);
+  const descendants: Node[] = [];
+
+  // recurse through "node > outputEdges > outputNode"
+  function findChildNode(node: Node) {
+    const outputEdges = node.outputEdges;
+
+    outputEdges.forEach((edge) => {
+      if (edge.outputNode) {
+        descendants.push(edge.outputNode);
+        findChildNode(edge.outputNode);
+      }
+    });
+  }
+
+  findChildNode(node);
+
+  return descendants.map((descendant) => descendant.name);
+}
+
 // create a unique fingerprint of the DAG
 export function fingerprintGraph(graph: Graph) {
   return Object.keys(graph.nodes)

public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts

@@ -216,10 +216,10 @@ describe('AlertingQueryRunner', () => {
     });
   });
 
-  it('should skip hidden queries', async () => {
+  it('should skip hidden queries and descendant nodes', async () => {
     const results = createFetchResponse<AlertingQueryResponse>({
       results: {
-        B: { frames: [createDataFrameJSON([1, 2, 3])] },
+        C: { frames: [createDataFrameJSON([1, 2, 3])] },
       },
     });
 
@@ -239,7 +239,17 @@ describe('AlertingQueryRunner', () => {
             hide: true,
           },
         }),
-        createQuery('B'),
+        createQuery('B', {
+          model: {
+            expression: 'A', // depends on A
+            refId: 'B',
+          },
+        }),
+        createQuery('C', {
+          model: {
+            refId: 'C',
+          },
+        }),
       ],
       'B'
     );
@@ -248,7 +258,8 @@ describe('AlertingQueryRunner', () => {
       const [loading, _data] = values;
 
       expect(loading.A).toBeUndefined();
-      expect(loading.B.state).toEqual(LoadingState.Done);
+      expect(loading.B).toBeUndefined();
+      expect(loading.C.state).toEqual(LoadingState.Done);
     });
   });
 });

public/app/features/alerting/unified/state/AlertingQueryRunner.ts

@@ -21,6 +21,7 @@ import { cancelNetworkRequestsOnUnsubscribe } from 'app/features/query/state/pro
 import { setStructureRevision } from 'app/features/query/state/processing/revision';
 import { AlertQuery } from 'app/types/unified-alerting-dto';
 
+import { createDagFromQueries, getDescendants } from '../components/rule-editor/dag';
 import { getTimeRangeForExpression } from '../utils/timeRange';
 
 export interface AlertingQueryResult {
@@ -51,29 +52,7 @@ export class AlertingQueryRunner {
 
   async run(queries: AlertQuery[], condition: string) {
     const empty = initialState(queries, LoadingState.Done);
-    const queriesToExclude: string[] = [];
+    const queriesToRun = await this.prepareQueries(queries);
-
-    // do not execute if one more of the queries are not runnable,
-    // for example not completely configured
-    for (const query of queries) {
-      const refId = query.model.refId;
-
-      if (isExpressionQuery(query.model)) {
-        continue;
-      }
-
-      const dataSourceInstance = await this.dataSourceSrv.get(query.datasourceUid);
-      const skipRunningQuery =
-        dataSourceInstance instanceof DataSourceWithBackend &&
-        dataSourceInstance.filterQuery &&
-        !dataSourceInstance.filterQuery(query.model);
-
-      if (skipRunningQuery) {
-        queriesToExclude.push(refId);
-      }
-    }
-
-    const queriesToRun = reject(queries, (q) => queriesToExclude.includes(q.model.refId));
 
     if (queriesToRun.length === 0) {
       return this.subject.next(empty);
@@ -98,6 +77,38 @@ export class AlertingQueryRunner {
     });
   }
 
+  // this function will omit any invalid queries and all of its descendants from the list of queries
+  // to do this we will convert the list of queries into a DAG and walk the invalid node's output edges recursively
+  async prepareQueries(queries: AlertQuery[]) {
+    const queriesToExclude: string[] = [];
+
+    // convert our list of queries to a graph
+    const queriesGraph = createDagFromQueries(queries);
+
+    // find all invalid nodes and omit those and their child nodes from the final queries array
+    // ⚠️ also make sure all dependent nodes are omitted, otherwise we will be evaluating a broken graph with missing references
+    for (const query of queries) {
+      const refId = query.model.refId;
+
+      if (isExpressionQuery(query.model)) {
+        continue;
+      }
+
+      const dataSourceInstance = await this.dataSourceSrv.get(query.datasourceUid);
+      const skipRunningQuery =
+        dataSourceInstance instanceof DataSourceWithBackend &&
+        dataSourceInstance.filterQuery &&
+        !dataSourceInstance.filterQuery(query.model);
+
+      if (skipRunningQuery) {
+        const descendants = getDescendants(refId, queriesGraph);
+        queriesToExclude.push(refId, ...descendants);
+      }
+    }
+
+    return reject(queries, (q) => queriesToExclude.includes(q.model.refId));
+  }
+
   cancel() {
     if (!this.subscription) {
       return;