diff --git a/.betterer.results b/.betterer.results index 334e3e5dd2a..de9d9f96881 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1545,7 +1545,7 @@ exports[`better eslint`] = { "public/app/features/alerting/unified/GrafanaRuleQueryViewer.tsx:5381": [ [0, 0, 0, "\'@grafana/data/src/datetime/rangeutil\' import is restricted from being used by a pattern. Import from the public export instead.", "0"], [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "1"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "2"], + [0, 0, 0, "No untranslated strings in text props in text props. Wrap text with or use t() or use t()", "2"], [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], [0, 0, 0, "No untranslated strings. Wrap text with ", "4"], [0, 0, 0, "No untranslated strings. Wrap text with ", "5"], @@ -1558,7 +1558,8 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "12"], [0, 0, 0, "No untranslated strings. Wrap text with ", "13"], [0, 0, 0, "No untranslated strings. Wrap text with ", "14"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "15"] + [0, 0, 0, "No untranslated strings. Wrap text with ", "15"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "16"] ], "public/app/features/alerting/unified/NotificationPoliciesPage.tsx:5381": [ [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], @@ -2339,6 +2340,9 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "10"], [0, 0, 0, "No untranslated strings. Wrap text with ", "11"] ], + "public/app/features/alerting/unified/components/rule-editor/dag.test.ts:5381": [ + [0, 0, 0, "\'@grafana/runtime/src/utils/DataSourceWithBackend\' import is restricted from being used by a pattern. Import from the public export instead.", "0"] + ], "public/app/features/alerting/unified/components/rule-editor/labels/LabelsButtons.tsx:5381": [ [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] @@ -2463,9 +2467,18 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], [0, 0, 0, "No untranslated strings. Wrap text with ", "4"] ], - "public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx:5381": [ - [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], - [0, 0, 0, "No untranslated strings. Wrap text with ", "1"] + "public/app/features/alerting/unified/components/rule-viewer/tabs/Details.tsx:5381": [ + [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "1"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "2"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "3"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "4"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "5"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "6"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "7"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "8"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "9"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "10"] ], "public/app/features/alerting/unified/components/rule-viewer/tabs/Routing.tsx:5381": [ [0, 0, 0, "No untranslated strings. Wrap text with ", "0"] @@ -2919,6 +2932,9 @@ exports[`better eslint`] = { "public/app/features/alerting/unified/rule-list/components/RuleListIcon.tsx:5381": [ [0, 0, 0, "\'@grafana/ui/src/components/Text/Text\' import is restricted from being used by a pattern. Import from the public export instead.", "0"] ], + "public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts:5381": [ + [0, 0, 0, "\'@grafana/runtime/src/utils/DataSourceWithBackend\' import is restricted from being used by a pattern. Import from the public export instead.", "0"] + ], "public/app/features/alerting/unified/state/actions.ts:5381": [ [0, 0, 0, "\'@grafana/runtime/src/utils/logging\' import is restricted from being used by a pattern. Import from the public export instead.", "0"] ], diff --git a/public/app/features/alerting/unified/GrafanaRuleQueryViewer.test.tsx b/public/app/features/alerting/unified/GrafanaRuleQueryViewer.test.tsx index fafaeaab9c1..484616e3910 100644 --- a/public/app/features/alerting/unified/GrafanaRuleQueryViewer.test.tsx +++ b/public/app/features/alerting/unified/GrafanaRuleQueryViewer.test.tsx @@ -1,7 +1,6 @@ import { render, screen, waitFor } from 'test/test-utils'; -import { DataSourceRef } from '@grafana/schema'; -import { AlertQuery } from 'app/types/unified-alerting-dto'; +import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; import { GrafanaRuleQueryViewer } from './GrafanaRuleQueryViewer'; import { mockCombinedRule } from './mocks'; @@ -10,71 +9,79 @@ describe('GrafanaRuleQueryViewer', () => { it('renders without crashing', async () => { const rule = mockCombinedRule(); - const getDataSourceQuery = (refId: string) => { - const query: AlertQuery = { - refId: refId, - datasourceUid: 'abc123', - queryType: '', - relativeTimeRange: { - from: 600, - to: 0, - }, - model: { - refId: 'A', - }, - }; - return query; - }; - const queries = [ - getDataSourceQuery('A'), - getDataSourceQuery('B'), - getDataSourceQuery('C'), - getDataSourceQuery('D'), - getDataSourceQuery('E'), - ]; - - const getExpression = (refId: string, dsRef: DataSourceRef) => { - const expr = { - refId: refId, - datasourceUid: '__expr__', - queryType: '', - model: { - refId: refId, - type: 'classic_conditions', - datasource: dsRef, - conditions: [ - { - type: 'query', - evaluator: { - params: [3], - type: 'gt', - }, - operator: { - type: 'and', - }, - query: { - params: ['A'], - }, - reducer: { - params: [], - type: 'last', - }, - }, - ], - }, - }; - return expr; - }; - - const expressions = [ - getExpression('A', { type: '' }), - getExpression('B', { type: '' }), - getExpression('C', { type: '' }), - getExpression('D', { type: '' }), - ]; + const expressions = [getExpression('F'), getExpression('G'), getExpression('H'), getExpression('I')]; render(); await waitFor(() => expect(screen.getByTestId('queries-container')).toHaveStyle('flex-wrap: wrap')); expect(screen.getByTestId('expressions-container')).toHaveStyle('flex-wrap: wrap'); }); + + it('should catch cyclical references', async () => { + const rule = mockCombinedRule(); + + const queries = [ + getExpression('A'), // this always points to A + ]; + + jest.spyOn(console, 'error').mockImplementation((message) => { + expect(message).toMatch(/Failed to parse thresholds/i); + }); + render(); + }); }); + +function getDataSourceQuery(sourceRefId: string) { + const query: AlertQuery = { + refId: sourceRefId, + datasourceUid: 'abc123', + queryType: '', + relativeTimeRange: { + from: 600, + to: 0, + }, + model: { + refId: sourceRefId, + }, + }; + return query; +} +const queries = [ + getDataSourceQuery('A'), + getDataSourceQuery('B'), + getDataSourceQuery('C'), + getDataSourceQuery('D'), + getDataSourceQuery('E'), +]; + +function getExpression(refId: string) { + const expr = { + refId: refId, + datasourceUid: '__expr__', + queryType: '', + model: { + refId: refId, + type: 'classic_conditions', + datasource: { type: '' }, + conditions: [ + { + type: 'query', + evaluator: { + params: [3], + type: 'gt', + }, + operator: { + type: 'and', + }, + query: { + params: ['A'], + }, + reducer: { + params: [], + type: 'last', + }, + }, + ], + }, + }; + return expr; +} diff --git a/public/app/features/alerting/unified/GrafanaRuleQueryViewer.tsx b/public/app/features/alerting/unified/GrafanaRuleQueryViewer.tsx index 702eaf01fed..1839ea4c8eb 100644 --- a/public/app/features/alerting/unified/GrafanaRuleQueryViewer.tsx +++ b/public/app/features/alerting/unified/GrafanaRuleQueryViewer.tsx @@ -1,12 +1,12 @@ import { css, cx } from '@emotion/css'; -import { keyBy, startCase } from 'lodash'; +import { keyBy, startCase, uniqueId } from 'lodash'; import * as React from 'react'; import { DataSourceInstanceSettings, DataSourceRef, GrafanaTheme2, PanelData, urlUtil } from '@grafana/data'; import { secondsToHms } from '@grafana/data/src/datetime/rangeutil'; import { config } from '@grafana/runtime'; import { Preview } from '@grafana/sql/src/components/visual-query-builder/Preview'; -import { Badge, ErrorBoundaryAlert, LinkButton, Stack, Text, useStyles2 } from '@grafana/ui'; +import { Alert, Badge, ErrorBoundaryAlert, LinkButton, Stack, Text, useStyles2 } from '@grafana/ui'; import { CombinedRule } from 'app/types/unified-alerting'; import { AlertDataQuery, AlertQuery } from '../../../types/unified-alerting-dto'; @@ -188,9 +188,6 @@ const getQueryPreviewStyles = (theme: GrafanaTheme2) => ({ contentBox: css({ flex: '1 0 100%', }), - visualization: css({ - padding: theme.spacing(1), - }), dataSource: css({ border: `1px solid ${theme.colors.border.weak}`, borderRadius: theme.shape.radius.default, @@ -208,6 +205,8 @@ interface ExpressionPreviewProps extends Pick { } function ExpressionPreview({ refId, model, evalData, isAlertCondition }: ExpressionPreviewProps) { + const styles = useStyles2(getQueryBoxStyles); + function renderPreview() { switch (model.type) { case ExpressionQueryType.math: @@ -243,7 +242,14 @@ function ExpressionPreview({ refId, model, evalData, isAlertCondition }: Express ]} isAlertCondition={isAlertCondition} > - {renderPreview()} +
+ {evalData?.errors?.map((error) => ( + + {error.message} + + ))} + {renderPreview()} +
{evalData && } @@ -310,6 +316,9 @@ const getQueryBoxStyles = (theme: GrafanaTheme2) => ({ border: `1px solid ${theme.colors.border.weak}`, borderRadius: theme.shape.radius.default, }), + previewWrapper: css({ + padding: theme.spacing(1), + }), }); function ClassicConditionViewer({ model }: { model: ExpressionQuery }) { @@ -345,7 +354,6 @@ function ClassicConditionViewer({ model }: { model: ExpressionQuery }) { const getClassicConditionViewerStyles = (theme: GrafanaTheme2) => ({ container: css({ - padding: theme.spacing(1), display: 'grid', gridTemplateColumns: 'repeat(6, max-content)', gap: theme.spacing(0, 1), @@ -378,7 +386,6 @@ function ReduceConditionViewer({ model }: { model: ExpressionQuery }) { const getReduceConditionViewerStyles = (theme: GrafanaTheme2) => ({ container: css({ - padding: theme.spacing(1), display: 'grid', gap: theme.spacing(0.5), gridTemplateRows: '1fr 1fr', @@ -417,7 +424,6 @@ function ResampleExpressionViewer({ model }: { model: ExpressionQuery }) { const getResampleExpressionViewerStyles = (theme: GrafanaTheme2) => ({ container: css({ - padding: theme.spacing(1), display: 'grid', gap: theme.spacing(0.5), gridTemplateColumns: 'repeat(4, 1fr)', @@ -486,7 +492,6 @@ const getExpressionViewerStyles = (theme: GrafanaTheme2) => { maxWidth: '100%', }), container: css({ - padding: theme.spacing(1), display: 'flex', gap: theme.spacing(0.5), }), diff --git a/public/app/features/alerting/unified/components/rule-editor/__snapshots__/dag.test.ts.snap b/public/app/features/alerting/unified/components/rule-editor/__snapshots__/dag.test.ts.snap new file mode 100644 index 00000000000..ec13355c5d3 --- /dev/null +++ b/public/app/features/alerting/unified/components/rule-editor/__snapshots__/dag.test.ts.snap @@ -0,0 +1,11 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`working with dag should throw on references to self 1`] = ` +[ + { + "error": [Error: cannot link A to A since it would create a cycle], + "source": "A", + "target": "A", + }, +] +`; 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 fdb5b22cea4..052db896825 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 @@ -1,11 +1,16 @@ +import { ExpressionDatasourceRef } from '@grafana/runtime/src/utils/DataSourceWithBackend'; import { Graph } from 'app/core/utils/dag'; +import { EvalFunction } from 'app/features/alerting/state/alertDef'; +import { ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types'; import { AlertQuery } from 'app/types/unified-alerting-dto'; import { + DAGError, _getDescendants, _getOriginsOfRefId, createDagFromQueries, fingerprintGraph, + getTargets, parseRefsFromMathExpression, } from './dag'; @@ -93,6 +98,31 @@ describe('working with dag', () => { dag.getNode('A'); }).not.toThrow(); }); + + it('should throw on references to self', () => { + const queries: Array> = [ + { + refId: 'A', + model: { refId: 'A', expression: '$A', datasource: ExpressionDatasourceRef, type: ExpressionQueryType.math }, + queryType: '', + datasourceUid: '__expr__', + }, + ]; + + expect(() => createDagFromQueries(queries)).toThrowError(/failed to create DAG from queries/i); + + // now assert we get the correct error diagnostics + try { + createDagFromQueries(queries); + } catch (error) { + if (!(error instanceof Error)) { + throw error; + } + + expect(error instanceof DAGError).toBe(true); + expect(error!.cause).toMatchSnapshot(); + } + }); }); describe('getOriginsOfRefId', () => { @@ -157,3 +187,60 @@ describe('fingerprints', () => { expect(fingerprintGraph(graph)).toMatchInlineSnapshot(`"A:B: B:C:A, D C::B D:B:"`); }); }); + +describe('getTargets', () => { + it('should correct get targets from Math expression', () => { + const expression: ExpressionQuery = { + refId: 'C', + type: ExpressionQueryType.math, + datasource: ExpressionDatasourceRef, + expression: '$A + $B', + }; + + expect(getTargets(expression)).toEqual(['A', 'B']); + }); + + it('should be able to find the targets of a classic condition', () => { + const expression: ExpressionQuery = { + refId: 'C', + type: ExpressionQueryType.classic, + datasource: ExpressionDatasourceRef, + expression: '', + conditions: [ + { + evaluator: { + params: [0, 0], + type: EvalFunction.IsAbove, + }, + operator: { type: 'and' }, + query: { params: ['A'] }, + reducer: { params: [], type: 'avg' }, + type: 'query', + }, + { + evaluator: { + params: [0, 0], + type: EvalFunction.IsAbove, + }, + operator: { type: 'and' }, + query: { params: ['B'] }, + reducer: { params: [], type: 'avg' }, + type: 'query', + }, + ], + }; + + expect(getTargets(expression)).toEqual(['A', 'B']); + }); + + it('should work for any other expression type', () => { + const expression: ExpressionQuery = { + refId: 'C', + type: ExpressionQueryType.reduce, + datasource: ExpressionDatasourceRef, + expression: 'A', + }; + + expect(getTargets(expression)).toEqual(['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 8ca3c0c1969..6b6d7a50b6e 100644 --- a/public/app/features/alerting/unified/components/rule-editor/dag.ts +++ b/public/app/features/alerting/unified/components/rule-editor/dag.ts @@ -1,4 +1,4 @@ -import { compact, memoize, uniq } from 'lodash'; +import { compact, memoize, reject, uniq } from 'lodash'; import { Edge, Graph, Node } from 'app/core/utils/dag'; import { isExpressionQuery } from 'app/features/expressions/guards'; @@ -12,6 +12,9 @@ import { AlertQuery } from 'app/types/unified-alerting-dto'; export function createDagFromQueries(queries: AlertQuery[]): Graph { const graph = new Graph(); + // collect link errors in here so we can throw a single error with all nodes that failed to link + const linkErrors: LinkError[] = []; + const nodes = queries.map((query) => query.refId); graph.createNodes(nodes); @@ -25,18 +28,66 @@ export function createDagFromQueries(queries: AlertQuery[]): Graph { const targets = getTargets(query.model); targets.forEach((target) => { - const isSelf = source === target; - - if (source && target && !isSelf) { - graph.link(target, source); + if (source && target) { + try { + graph.link(target, source); + } catch (error) { + linkErrors.push({ source, target, error }); + } } }); }); + if (linkErrors.length > 0) { + throw new DAGError('failed to create DAG from queries', { cause: linkErrors }); + } + return graph; } -function getTargets(model: ExpressionQuery) { +/** + * This function attempts to create a "clean" DAG where only the nodes that successfully link are left + * ⚠️ This is a recursive function and very expensive for larger DAGs or large amount of queries + */ +export function createDAGFromQueriesSafe( + queries: AlertQuery[], + collectedLinkErrors: LinkError[] = [] +): [Graph, LinkError[]] { + try { + return [createDagFromQueries(queries), collectedLinkErrors]; + } catch (error) { + if (error instanceof DAGError) { + const linkErrors = error.cause; + collectedLinkErrors.push(...linkErrors); + + const updatedQueries = reject(queries, (query) => + linkErrors.some((linkError) => linkError.source === query.refId) + ); + + return createDAGFromQueriesSafe(updatedQueries, collectedLinkErrors); + } + } + + return [new Graph(), collectedLinkErrors]; +} + +export interface LinkError { + source: string; + target: string; + error: unknown; +} + +/** DAGError subclass, this is just a regular error but with LinkError[] as the cause */ +export class DAGError extends Error { + constructor(message: string, options: { cause: LinkError[] }) { + super(message, options); + this.cause = options?.cause ?? []; + } + + cause: LinkError[]; +} + +export function getTargets(model: ExpressionQuery) { const isMathExpression = model.type === ExpressionQueryType.math; const isClassicCondition = model.type === ExpressionQueryType.classic; diff --git a/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx b/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx index c7550d5bd21..7d954c53303 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useMemo } from 'react'; import { config } from '@grafana/runtime'; import { Alert, Stack } from '@grafana/ui'; +import { Trans, t } from 'app/core/internationalization'; import { CombinedRule } from 'app/types/unified-alerting'; import { GrafanaRuleQueryViewer, QueryPreview } from '../../../GrafanaRuleQueryViewer'; @@ -39,47 +40,47 @@ const QueryResults = ({ rule }: Props) => { const isFederatedRule = isFederatedRuleGroup(rule.group); + if (isPreviewLoading) { + return Loading...; + } + return ( <> - {isPreviewLoading ? ( - 'Loading...' - ) : ( - <> - {isGrafanaRulerRule(rule.rulerRule) && !isFederatedRule && ( - - )} + {isGrafanaRulerRule(rule.rulerRule) && !isFederatedRule && ( + + )} - {!isGrafanaRulerRule(rule.rulerRule) && - !isFederatedRule && - queryPreviewData && - Object.keys(queryPreviewData).length > 0 && ( - - {queries.map((query) => { - return ( - ds.uid === query.datasourceUid)} - queryData={queryPreviewData[query.refId]} - relativeTimeRange={query.relativeTimeRange} - /> - ); - })} - - )} - {!isFederatedRule && !allDataSourcesAvailable && ( - - Cannot display the query preview. Some of the data sources used in the queries are not available. - - )} - + {!isGrafanaRulerRule(rule.rulerRule) && + !isFederatedRule && + queryPreviewData && + Object.keys(queryPreviewData).length > 0 && ( + + {queries.map((query) => { + return ( + ds.uid === query.datasourceUid)} + queryData={queryPreviewData[query.refId]} + relativeTimeRange={query.relativeTimeRange} + /> + ); + })} + + )} + {!isFederatedRule && !allDataSourcesAvailable && ( + + + Cannot display the query preview. Some of the data sources used in the queries are not available. + + )} ); diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts index 8985847981a..9f64cbdb08e 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts +++ b/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts @@ -14,6 +14,7 @@ import { rangeUtil, } from '@grafana/data'; import { DataSourceSrv, DataSourceWithBackend, FetchResponse } from '@grafana/runtime'; +import { ExpressionDatasourceRef } from '@grafana/runtime/src/utils/DataSourceWithBackend'; import { DataQuery } from '@grafana/schema'; import { BackendSrv } from 'app/core/services/backend_srv'; import { @@ -22,6 +23,7 @@ import { } from 'app/features/alerting/unified/components/settings/__mocks__/server'; import { setupMswServer } from 'app/features/alerting/unified/mockApi'; import { setupDataSources } from 'app/features/alerting/unified/testSetup/datasources'; +import { ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types'; import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; import { AlertingQueryResponse, AlertingQueryRunner } from './AlertingQueryRunner'; @@ -239,8 +241,8 @@ describe('AlertingQueryRunner', () => { }), createQuery('B', { model: { - expression: 'A', // depends on A refId: 'B', + hide: false, }, }), createQuery('C', { @@ -309,6 +311,66 @@ const expectDataFrameWithValues = ({ time, values }: { time: number[]; values: n }; }; +describe('prepareQueries', () => { + it('should skip node that fail to link', async () => { + const queries = [ + createQuery('A', { + model: { + refId: 'A', + hide: true, // this node will be omitted + }, + }), + createQuery('B', { + model: { + refId: 'B', + hide: false, // this node will _not_ be omitted + }, + }), + createExpression('C', { + model: { + refId: 'C', + type: ExpressionQueryType.math, + expression: '$A', // this node will be omitted because it is a descendant of A (omitted) + }, + }), + createExpression('D', { + model: { + refId: 'D', + type: ExpressionQueryType.math, + expression: '$ZZZ', // this node will be omitted, ref does not exist + }, + }), + createExpression('E', { + model: { + refId: 'E', + type: ExpressionQueryType.math, + expression: '$B', // this node will be omitted, ref does not exist + }, + }), + createExpression('F', { + model: { + refId: 'F', + type: ExpressionQueryType.math, + expression: '$D', // this node will be omitted, because D is broken too + }, + }), + ]; + + const runner = new AlertingQueryRunner( + mockBackendSrv({ + fetch: () => of(), + }), + mockDataSourceSrv({ filterQuery: (model: AlertDataQuery) => model.hide !== true }) + ); + + const queriesToRun = await runner.prepareQueries(queries); + + expect(queriesToRun).toHaveLength(2); + expect(queriesToRun[0]).toStrictEqual(queries[1]); + expect(queriesToRun[1]).toStrictEqual(queries[4]); + }); +}); + const createDataFrameJSON = (values: number[]): DataFrameJSON => { const startTime = 1620051602238; const timeValues = values.map((_, index) => startTime + (index + 1) * 10000); @@ -326,7 +388,7 @@ const createDataFrameJSON = (values: number[]): DataFrameJSON => { }; }; -const createQuery = (refId: string, options?: Partial): AlertQuery => { +const createQuery = (refId: string, options?: Partial>): AlertQuery => { return defaultsDeep(options, { refId, queryType: '', @@ -335,3 +397,16 @@ const createQuery = (refId: string, options?: Partial): AlertQuery = relativeTimeRange: getDefaultRelativeTimeRange(), }); }; + +const createExpression = ( + refId: string, + options?: Partial> +): AlertQuery => { + return defaultsDeep(options, { + refId, + queryType: '', + datasourceUid: EXTERNAL_VANILLA_ALERTMANAGER_UID, + model: { refId, datasource: ExpressionDatasourceRef }, + relativeTimeRange: getDefaultRelativeTimeRange(), + }); +}; diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts index 3e8fc2b8cf7..0da68b13ca6 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts +++ b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts @@ -15,13 +15,14 @@ import { withLoadingIndicator, } from '@grafana/data'; import { DataSourceWithBackend, FetchResponse, getDataSourceSrv, toDataQueryError } from '@grafana/runtime'; +import { t } from 'app/core/internationalization'; import { BackendSrv, getBackendSrv } from 'app/core/services/backend_srv'; import { isExpressionQuery } from 'app/features/expressions/guards'; import { cancelNetworkRequestsOnUnsubscribe } from 'app/features/query/state/processing/canceler'; 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 { LinkError, createDAGFromQueriesSafe, getDescendants } from '../components/rule-editor/dag'; import { getTimeRangeForExpression } from '../utils/timeRange'; export interface AlertingQueryResult { @@ -53,11 +54,17 @@ export class AlertingQueryRunner { async run(queries: AlertQuery[], condition: string) { const queriesToRun = await this.prepareQueries(queries); + // if we don't have any queries to run we just bail if (queriesToRun.length === 0) { return; } - this.subscription = runRequest(this.backendSrv, queriesToRun, condition).subscribe({ + // if the condition isn't part of the queries to run, try to run the alert rule without it. + // It indicates that the "condition" node points to a non-existent node. We still want to be able to evaluate the other nodes. + const isConditionAvailable = queriesToRun.some((query) => query.refId === condition); + const ruleCondition = isConditionAvailable ? condition : ''; + + this.subscription = runRequest(this.backendSrv, queriesToRun, ruleCondition).subscribe({ next: (dataPerQuery) => { const nextResult = applyChange(dataPerQuery, (refId, data) => { const previous = this.lastResult[refId]; @@ -65,6 +72,12 @@ export class AlertingQueryRunner { return setStructureRevision(preProcessed, previous); }); + // add link errors to the panelData and mark them as errors + const [_, linkErrors] = createDAGFromQueriesSafe(queries); + linkErrors.forEach((linkError) => { + nextResult[linkError.source] = createLinkErrorPanelData(linkError); + }); + this.lastResult = nextResult; this.subject.next(this.lastResult); }, @@ -78,17 +91,14 @@ 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[]) { + async prepareQueries(queries: AlertQuery[]): Promise { 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 + // find all invalid nodes and omit those for (const query of queries) { const refId = query.model.refId; + // expression queries cannot be excluded / filtered out if (isExpressionQuery(query.model)) { continue; } @@ -100,12 +110,28 @@ export class AlertingQueryRunner { !dataSourceInstance.filterQuery(query.model); if (skipRunningQuery) { - const descendants = getDescendants(refId, queriesGraph); - queriesToExclude.push(refId, ...descendants); + queriesToExclude.push(refId); } } - return reject(queries, (q) => queriesToExclude.includes(q.model.refId)); + // exclude nodes that failed to link and their child nodes from the final queries array by trying to parse the graph + // ⚠️ also make sure all dependent nodes are omitted, otherwise we will be evaluating a broken graph with missing references + const [cleanGraph] = createDAGFromQueriesSafe(queries); + const cleanNodes = Object.keys(cleanGraph.nodes); + + // find descendant nodes of data queries that have been excluded + queriesToExclude.forEach((refId) => { + const descendants = getDescendants(refId, cleanGraph); + queriesToExclude.push(...descendants); + }); + + // also exclude all nodes that aren't in cleanGraph, this means they point to other broken nodes + const nodesNotInGraph = queries.filter((query) => !cleanNodes.includes(query.refId)); + nodesNotInGraph.forEach((node) => { + queriesToExclude.push(node.refId); + }); + + return reject(queries, (query) => queriesToExclude.includes(query.refId)); } cancel() { @@ -219,7 +245,10 @@ const mapToPanelData = ( const mapErrorToPanelData = (lastResult: Record, error: Error): Record => { const queryError = toDataQueryError(error); - return applyChange(lastResult, (refId, data) => { + return applyChange(lastResult, (_refId, data) => { + if (data.state === LoadingState.Error) { + return data; + } return { ...data, state: LoadingState.Error, @@ -240,3 +269,29 @@ const applyChange = ( return nextResult; }; + +const createLinkErrorPanelData = (error: LinkError): PanelData => ({ + series: [], + state: LoadingState.Error, + errors: [ + { + message: createLinkErrorMessage(error), + }, + ], + timeRange: getDefaultTimeRange(), +}); + +function createLinkErrorMessage(error: LinkError): string { + const isSelfReference = error.source === error.target; + + return isSelfReference + ? t('alerting.dag.self-reference', "You can't link an expression to itself") + : t( + 'alerting.dag.missing-reference', + `Expression "{{source}}" failed to run because "{{target}}" is missing or also failed.`, + { + source: error.source, + target: error.target, + } + ); +} diff --git a/public/locales/en-US/grafana.json b/public/locales/en-US/grafana.json index 10b77debdf9..1416699e4d5 100644 --- a/public/locales/en-US/grafana.json +++ b/public/locales/en-US/grafana.json @@ -334,6 +334,10 @@ "label": "Contact point" }, "copy-to-clipboard": "Copy \"{{label}}\" to clipboard", + "dag": { + "missing-reference": "Expression \"{{source}}\" failed to run because \"{{target}}\" is missing or also failed.", + "self-reference": "You can't link an expression to itself" + }, "export": { "subtitle": { "formats": "Select the format and download the file or copy the contents to clipboard", @@ -566,6 +570,14 @@ "paused": "Paused", "recording-rule": "Recording rule" }, + "rule-view": { + "query": { + "datasources-na": { + "description": "Cannot display the query preview. Some of the data sources used in the queries are not available.", + "title": "Query not available" + } + } + }, "rule-viewer": { "prometheus-consistency-check": { "alert-message": "Alert rule has been updated. Changes may take up to a minute to appear on the Alert rules list view.", diff --git a/public/locales/pseudo-LOCALE/grafana.json b/public/locales/pseudo-LOCALE/grafana.json index f39af58738b..39128a2cf53 100644 --- a/public/locales/pseudo-LOCALE/grafana.json +++ b/public/locales/pseudo-LOCALE/grafana.json @@ -334,6 +334,10 @@ "label": "Cőʼnŧäčŧ pőįʼnŧ" }, "copy-to-clipboard": "Cőpy \"{{label}}\" ŧő čľįpþőäřđ", + "dag": { + "missing-reference": "Ēχpřęşşįőʼn \"{{source}}\" ƒäįľęđ ŧő řūʼn þęčäūşę \"{{target}}\" įş mįşşįʼnģ őř äľşő ƒäįľęđ.", + "self-reference": "Ÿőū čäʼn'ŧ ľįʼnĸ äʼn ęχpřęşşįőʼn ŧő įŧşęľƒ" + }, "export": { "subtitle": { "formats": "Ŝęľęčŧ ŧĥę ƒőřmäŧ äʼnđ đőŵʼnľőäđ ŧĥę ƒįľę őř čőpy ŧĥę čőʼnŧęʼnŧş ŧő čľįpþőäřđ", @@ -566,6 +570,14 @@ "paused": "Päūşęđ", "recording-rule": "Ŗęčőřđįʼnģ řūľę" }, + "rule-view": { + "query": { + "datasources-na": { + "description": "Cäʼnʼnőŧ đįşpľäy ŧĥę qūęřy přęvįęŵ. Ŝőmę őƒ ŧĥę đäŧä şőūřčęş ūşęđ įʼn ŧĥę qūęřįęş äřę ʼnőŧ äväįľäþľę.", + "title": "Qūęřy ʼnőŧ äväįľäþľę" + } + } + }, "rule-viewer": { "prometheus-consistency-check": { "alert-message": "Åľęřŧ řūľę ĥäş þęęʼn ūpđäŧęđ. Cĥäʼnģęş mäy ŧäĸę ūp ŧő ä mįʼnūŧę ŧő äppęäř őʼn ŧĥę Åľęřŧ řūľęş ľįşŧ vįęŵ.",