diff --git a/e2e/plugin-e2e/plugin-e2e-api-tests/as-admin-user/alerting.spec.ts b/e2e/plugin-e2e/plugin-e2e-api-tests/as-admin-user/alerting.spec.ts index 531cb85e091..ac18e6458da 100644 --- a/e2e/plugin-e2e/plugin-e2e-api-tests/as-admin-user/alerting.spec.ts +++ b/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, diff --git a/public/app/features/alerting/unified/components/rule-editor/dag.test.ts b/public/app/features/alerting/unified/components/rule-editor/dag.test.ts index 2b173741720..a3cba5bc652 100644 --- a/public/app/features/alerting/unified/components/rule-editor/dag.test.ts +++ b/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']], diff --git a/public/app/features/alerting/unified/components/rule-editor/dag.ts b/public/app/features/alerting/unified/components/rule-editor/dag.ts index 2971dd66a1e..cd4583fe6e2 100644 --- a/public/app/features/alerting/unified/components/rule-editor/dag.ts +++ b/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,23 +67,45 @@ 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); } } + 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 origins.map((origin) => origin.name); + return descendants.map((descendant) => descendant.name); } // create a unique fingerprint of the DAG diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts index a4dc6e5f576..e9935f7bc91 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts +++ b/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({ 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); }); }); }); diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts index c92feb7c4a3..fc7088f4508 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts +++ b/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[] = []; - - // 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)); + const queriesToRun = await this.prepareQueries(queries); 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;