Skip to content

Commit 494376c

Browse files
konrad147gillesdemey
konrad147
and
gillesdemey
authored
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 <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>
1 parent b4e7329 commit 494376c

File tree

9 files changed

+120
-81
lines changed

9 files changed

+120
-81
lines changed

‎public/app/core/utils/dag.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,5 +116,11 @@ describe('Directed acyclic graph', () => {
116116
dag.link('A', 'non-existing');
117117
}).toThrowError("cannot link output node named non-existing since it doesn't exist in graph");
118118
});
119+
120+
it('when linking would create a cycle should throw error', () => {
121+
expect(() => dag.link('C', 'C')).toThrow('cannot link C to C since it would create a cycle');
122+
expect(() => dag.link('A', 'B')).toThrow('cannot link A to B since it would create a cycle');
123+
expect(() => dag.link('A', 'E')).toThrow('cannot link A to E since it would create a cycle');
124+
});
119125
});
120126
});

‎public/app/core/utils/dag.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,9 @@ export class Graph {
183183
const edges: Edge[] = [];
184184
inputNodes.forEach((input) => {
185185
outputNodes.forEach((output) => {
186+
if (this.willCreateCycle(input, output)) {
187+
throw Error(`cannot link ${input.name} to ${output.name} since it would create a cycle`);
188+
}
186189
edges.push(this.createEdge().link(input, output));
187190
});
188191
});
@@ -217,6 +220,34 @@ export class Graph {
217220
return descendants;
218221
}
219222

223+
private willCreateCycle(input: Node, output: Node): boolean {
224+
if (input === output) {
225+
return true;
226+
}
227+
228+
// Perform a DFS to check if the input node is reachable from the output node
229+
const visited = new Set<Node>();
230+
const stack = [output];
231+
232+
while (stack.length) {
233+
const current = stack.pop()!;
234+
if (current === input) {
235+
return true;
236+
}
237+
238+
visited.add(current);
239+
240+
for (const edge of current.outputEdges) {
241+
const next = edge.outputNode;
242+
if (next && !visited.has(next)) {
243+
stack.push(next);
244+
}
245+
}
246+
}
247+
248+
return false;
249+
}
250+
220251
createEdge(): Edge {
221252
return new Edge();
222253
}

‎public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {
1616
import { DataQuery } from '@grafana/schema';
1717
import { GraphThresholdsStyleMode, Icon, InlineField, Input, Tooltip, useStyles2, Stack } from '@grafana/ui';
1818
import { QueryEditorRow } from 'app/features/query/components/QueryEditorRow';
19-
import { AlertQuery } from 'app/types/unified-alerting-dto';
19+
import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto';
2020

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

111111
// TODO add a warning label here too when the data looks like time series data and is used as an alert condition
112-
function HeaderExtras({ query, error, index }: { query: AlertQuery; error?: Error; index: number }) {
112+
function HeaderExtras({ query, error, index }: { query: AlertQuery<AlertDataQuery>; error?: Error; index: number }) {
113113
const queryOptions: AlertQueryOptions = {
114114
maxDataPoints: query.model.maxDataPoints,
115115
minInterval: query.model.intervalMs ? msToSingleUnitDuration(query.model.intervalMs) : undefined,
@@ -144,7 +144,7 @@ export const QueryWrapper = ({
144144
return (
145145
<Stack direction="column" gap={0.5}>
146146
<div className={styles.wrapper}>
147-
<QueryEditorRow<DataQuery>
147+
<QueryEditorRow<AlertDataQuery>
148148
alerting
149149
collapsable={false}
150150
dataSource={dsSettings}

‎public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.test.tsx

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -233,19 +233,22 @@ describe('Query and expressions reducer', () => {
233233
});
234234
});
235235

236-
it('Should update time range for all expressions that have this data source when dispatching updateExpressionTimeRange', () => {
237-
const expressionQuery: AlertQuery = {
236+
it('Should update time range for all resample expressions that have this data source when dispatching updateExpressionTimeRange', () => {
237+
const expressionQuery: AlertQuery<ExpressionQuery> = {
238238
refId: 'B',
239239
queryType: 'expression',
240240
datasourceUid: '__expr__',
241241
model: {
242+
datasource: {
243+
type: '__expr__',
244+
uid: '__expr__',
245+
},
242246
queryType: 'query',
243-
datasource: '__expr__',
244247
refId: 'B',
245248
expression: 'A',
246-
type: ExpressionQueryType.classic,
249+
type: ExpressionQueryType.resample,
247250
window: '10s',
248-
} as ExpressionQuery,
251+
},
249252
};
250253
const customTimeRange: RelativeTimeRange = { from: 900, to: 1000 };
251254

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

264267
const newState = queriesAndExpressionsReducer(initialState, updateExpressionTimeRange());
265-
expect(newState).toStrictEqual({
268+
expect(newState).toStrictEqual<{ queries: AlertQuery[] }>({
266269
queries: [
267270
{
268271
refId: 'A',
@@ -274,19 +277,19 @@ describe('Query and expressions reducer', () => {
274277
{
275278
datasourceUid: '__expr__',
276279
model: {
277-
datasource: '__expr__',
278280
expression: 'A',
281+
datasource: {
282+
type: '__expr__',
283+
uid: '__expr__',
284+
},
279285
queryType: 'query',
280286
refId: 'B',
281-
type: ExpressionQueryType.classic,
287+
type: ExpressionQueryType.resample,
282288
window: '10s',
283289
},
284290
queryType: 'expression',
285291
refId: 'B',
286-
relativeTimeRange: {
287-
from: 900,
288-
to: 1000,
289-
},
292+
relativeTimeRange: customTimeRange,
290293
},
291294
],
292295
});

‎public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/reducer.ts

Lines changed: 51 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createAction, createReducer } from '@reduxjs/toolkit';
1+
import { createAction, createReducer, original } from '@reduxjs/toolkit';
22

33
import {
44
DataQuery,
@@ -8,27 +8,34 @@ import {
88
rangeUtil,
99
RelativeTimeRange,
1010
} from '@grafana/data';
11-
import { findDataSourceFromExpressionRecursive } from 'app/features/alerting/unified/utils/dataSourceFromExpression';
1211
import { dataSource as expressionDatasource } from 'app/features/expressions/ExpressionDatasource';
1312
import { isExpressionQuery } from 'app/features/expressions/guards';
1413
import { ExpressionDatasourceUID, ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types';
1514
import { defaultCondition } from 'app/features/expressions/utils/expressionTypes';
1615
import { AlertQuery } from 'app/types/unified-alerting-dto';
1716

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

2122
export interface QueriesAndExpressionsState {
2223
queries: AlertQuery[];
2324
}
2425

25-
const findDataSourceFromExpression = (
26-
queries: AlertQuery[],
27-
expression: string | undefined
28-
): AlertQuery | null | undefined => {
29-
const firstReference = queries.find((alertQuery) => alertQuery.refId === expression);
30-
const dataSource = firstReference && findDataSourceFromExpressionRecursive(queries, firstReference);
31-
return dataSource;
26+
const findDataSourceFromExpression = (queries: AlertQuery[], refId: string): AlertQuery | undefined => {
27+
const dag = createDagFromQueries(queries);
28+
const dataSource = getOriginOfRefId(refId, dag)[0];
29+
if (!dataSource) {
30+
return;
31+
}
32+
33+
const originQuery = queries.find((query) => query.refId === dataSource);
34+
if (originQuery && 'relativeTimeRange' in originQuery) {
35+
return originQuery;
36+
}
37+
38+
return;
3239
};
3340

3441
const initialState: QueriesAndExpressionsState = {
@@ -137,33 +144,51 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder
137144
state.queries = [...state.queries, ...payload];
138145
})
139146
.addCase(updateExpression, (state, { payload }) => {
140-
state.queries = state.queries.map((query) => {
141-
const dataSourceAlertQuery = findDataSourceFromExpression(state.queries, payload.expression);
147+
const queryToUpdate = state.queries.find((query) => query.refId === payload.refId);
148+
if (!queryToUpdate) {
149+
return;
150+
}
142151

143-
const relativeTimeRange = dataSourceAlertQuery
144-
? dataSourceAlertQuery.relativeTimeRange
145-
: getDefaultRelativeTimeRange();
152+
queryToUpdate.model = payload;
146153

147-
if (query.refId === payload.refId) {
148-
query.model = payload;
149-
if (payload.type === ExpressionQueryType.resample) {
150-
query.relativeTimeRange = relativeTimeRange;
154+
// 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)
155+
if (payload.type === ExpressionQueryType.resample && payload.expression) {
156+
// findDataSourceFromExpression uses memoization and it doesn't always work with proxies when the proxy has been revoked
157+
const originalQueries = original(state)?.queries ?? [];
158+
159+
let relativeTimeRange = getDefaultRelativeTimeRange();
160+
try {
161+
const dataSourceAlertQuery = findDataSourceFromExpression(originalQueries, payload.expression);
162+
if (dataSourceAlertQuery?.relativeTimeRange) {
163+
relativeTimeRange = dataSourceAlertQuery.relativeTimeRange;
164+
}
165+
} catch (error) {
166+
if (error instanceof Error) {
167+
logError(error);
168+
} else {
169+
logError(new Error('Error while trying to find data source from expression'));
151170
}
152171
}
153-
return query;
154-
});
172+
173+
queryToUpdate.relativeTimeRange = relativeTimeRange;
174+
}
155175
})
156176
.addCase(updateExpressionTimeRange, (state) => {
157-
const newState = state.queries.map((query) => {
158-
// It's an expression , let's update the relativeTimeRange with its dataSource relativeTimeRange
159-
if (query.datasourceUid === ExpressionDatasourceUID) {
160-
const dataSource = findDataSourceFromExpression(state.queries, query.model.expression);
177+
state.queries.forEach((query) => {
178+
// Resample expression needs to get the relativeTimeRange with its dataSource relativeTimeRange
179+
if (
180+
isExpressionQuery(query.model) &&
181+
query.model.type === ExpressionQueryType.resample &&
182+
query.model.expression
183+
) {
184+
// findDataSourceFromExpression uses memoization and doesn't work with proxies
185+
const originalQueries = original(state)?.queries ?? [];
186+
187+
const dataSource = findDataSourceFromExpression(originalQueries, query.model.expression);
161188
const relativeTimeRange = dataSource ? dataSource.relativeTimeRange : getDefaultRelativeTimeRange();
162189
query.relativeTimeRange = relativeTimeRange;
163190
}
164-
return query;
165191
});
166-
state.queries = newState;
167192
})
168193
.addCase(updateExpressionRefId, (state, { payload }) => {
169194
const { newRefId, oldRefId } = payload;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -422,15 +422,15 @@ describe('findRenamedReferences', () => {
422422
{ refId: 'MATH', model: { datasource: '-100' } },
423423
{ refId: 'B' },
424424
{ refId: 'C' },
425-
] as AlertQuery[];
425+
] as Array<AlertQuery<ExpressionQuery>>;
426426

427427
// @ts-expect-error
428428
const updated = [
429429
{ refId: 'FOO' },
430430
{ refId: 'REDUCE', model: { datasource: '-100' } },
431431
{ refId: 'B' },
432432
{ refId: 'C' },
433-
] as AlertQuery[];
433+
] as Array<AlertQuery<ExpressionQuery>>;
434434

435435
expect(findRenamedDataQueryReferences(previous, updated)).toEqual(['A', 'FOO']);
436436
});

‎public/app/features/alerting/unified/utils/dataSourceFromExpression.ts

Lines changed: 0 additions & 34 deletions
This file was deleted.

‎public/app/features/variables/state/actions.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
VariableRefresh,
2121
VariableWithOptions,
2222
} from '@grafana/data';
23-
import { config, locationService } from '@grafana/runtime';
23+
import { config, locationService, logWarning } from '@grafana/runtime';
2424
import { notifyApp } from 'app/core/actions';
2525
import { contextSrv } from 'app/core/services/context_srv';
2626
import { getTimeSrv } from 'app/features/dashboard/services/TimeSrv';
@@ -579,7 +579,14 @@ export const createGraph = (variables: TypedVariableModel[]) => {
579579
}
580580

581581
if (variableAdapters.get(v1.type).dependsOn(v1, v2)) {
582-
g.link(v1.name, v2.name);
582+
try {
583+
// link might fail if it would create a circular dependency
584+
g.link(v1.name, v2.name);
585+
} catch (error) {
586+
// Catch the exception and return partially linked graph. The caller will handle the case of partial linking and display errors
587+
const errorMessage = error instanceof Error ? error.message : 'Unknown error';
588+
logWarning('Error linking variables', { error: errorMessage });
589+
}
583590
}
584591
});
585592
});

‎public/app/types/unified-alerting-dto.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Prometheus API DTOs, possibly to be autogenerated from openapi spec in the near future
22

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

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

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

205-
export interface AlertQuery {
206+
export interface AlertQuery<T = AlertDataQuery | ExpressionQuery> {
206207
refId: string;
207208
queryType: string;
208209
relativeTimeRange?: RelativeTimeRange;
209210
datasourceUid: string;
210-
model: AlertDataQuery;
211+
model: T;
211212
}
212213

213214
export interface GrafanaNotificationSettings {

0 commit comments

Comments
 (0)