From 494376c5a91a726074e4870da01e7fa29e203364 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Fri, 23 Aug 2024 15:22:33 +0200 Subject: [PATCH] Alerting: Fix resample expression and relativeTimeRange updates (#91689) * Use dag to find origin nodes when updating resample queries Co-authored-by: Gilles De Mey * 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 Co-authored-by: Gilles De Mey --- public/app/core/utils/dag.test.ts | 6 ++ public/app/core/utils/dag.ts | 31 ++++++++ .../components/rule-editor/QueryWrapper.tsx | 6 +- .../reducer.test.tsx | 27 ++++--- .../query-and-alert-condition/reducer.ts | 77 ++++++++++++------- .../components/rule-editor/util.test.ts | 4 +- .../unified/utils/dataSourceFromExpression.ts | 34 -------- .../app/features/variables/state/actions.ts | 11 ++- public/app/types/unified-alerting-dto.ts | 5 +- 9 files changed, 120 insertions(+), 81 deletions(-) delete mode 100644 public/app/features/alerting/unified/utils/dataSourceFromExpression.ts diff --git a/public/app/core/utils/dag.test.ts b/public/app/core/utils/dag.test.ts index 22e02e3a6e0..ee3feaa27e7 100644 --- 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'); + }); }); }); diff --git a/public/app/core/utils/dag.ts b/public/app/core/utils/dag.ts index 3997442ffa1..c4e84f520ac 100644 --- 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(); + 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(); } diff --git a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx index ca9ddb606d9..928e111d7b4 100644 --- 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; 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 (
- + alerting collapsable={false} dataSource={dsSettings} diff --git 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 index 8a4a72c2173..ba5f3dcfd7a 100644 --- 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 = { 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, }, ], }); diff --git 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 index ab483e93843..44ebf2f1f18 100644 --- 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; diff --git a/public/app/features/alerting/unified/components/rule-editor/util.test.ts b/public/app/features/alerting/unified/components/rule-editor/util.test.ts index b5ffc31189c..680367f38ec 100644 --- 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>; // @ts-expect-error const updated = [ @@ -430,7 +430,7 @@ describe('findRenamedReferences', () => { { refId: 'REDUCE', model: { datasource: '-100' } }, { refId: 'B' }, { refId: 'C' }, - ] as AlertQuery[]; + ] as Array>; expect(findRenamedDataQueryReferences(previous, updated)).toEqual(['A', 'FOO']); }); diff --git a/public/app/features/alerting/unified/utils/dataSourceFromExpression.ts b/public/app/features/alerting/unified/utils/dataSourceFromExpression.ts deleted file mode 100644 index eec3f1182f5..00000000000 --- a/public/app/features/alerting/unified/utils/dataSourceFromExpression.ts +++ /dev/null @@ -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; - } - } -}; diff --git a/public/app/features/variables/state/actions.ts b/public/app/features/variables/state/actions.ts index e7ea07df89e..2e1ea5ba255 100644 --- 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 }); + } } }); }); diff --git a/public/app/types/unified-alerting-dto.ts b/public/app/types/unified-alerting-dto.ts index 663a128b726..dd69743f700 100644 --- 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 { refId: string; queryType: string; relativeTimeRange?: RelativeTimeRange; datasourceUid: string; - model: AlertDataQuery; + model: T; } export interface GrafanaNotificationSettings {