From 7952907e4882b252561ba816caee7a2202c3527e Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Fri, 16 Jun 2023 13:14:46 +0200 Subject: [PATCH] Alerting: Allow executing "hidden" queries (#70064) --- .betterer.results | 3 +- .../PanelAlertTabContent.test.tsx.snap | 2 - .../components/rule-editor/AlertRuleForm.tsx | 59 +++++++++-- .../components/rule-editor/QueryRows.tsx | 97 ++++++++++--------- .../components/rule-editor/QueryWrapper.tsx | 16 +-- .../unified/state/AlertingQueryRunner.test.ts | 44 ++++++++- .../unified/state/AlertingQueryRunner.ts | 32 +++--- .../alerting/unified/utils/rule-form.ts | 5 +- 8 files changed, 174 insertions(+), 84 deletions(-) diff --git a/.betterer.results b/.betterer.results index 46ecf2ed512..717c08524f3 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1905,8 +1905,7 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "1"] ], "public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx:5381": [ - [0, 0, 0, "Do not use any type assertions.", "0"], - [0, 0, 0, "Do not use any type assertions.", "1"] + [0, 0, 0, "Do not use any type assertions.", "0"] ], "public/app/features/alerting/unified/components/rule-editor/AnnotationKeyInput.tsx:5381": [ [0, 0, 0, "Do not use any type assertions.", "0"] diff --git a/public/app/features/alerting/unified/__snapshots__/PanelAlertTabContent.test.tsx.snap b/public/app/features/alerting/unified/__snapshots__/PanelAlertTabContent.test.tsx.snap index 66bf801052d..c1d0f7b6949 100644 --- a/public/app/features/alerting/unified/__snapshots__/PanelAlertTabContent.test.tsx.snap +++ b/public/app/features/alerting/unified/__snapshots__/PanelAlertTabContent.test.tsx.snap @@ -67,7 +67,6 @@ exports[`PanelAlertTabContent Will render alerts belonging to panel and a button "uid": "__expr__", }, "expression": "A", - "hide": false, "reducer": "last", "refId": "B", "type": "reduce", @@ -106,7 +105,6 @@ exports[`PanelAlertTabContent Will render alerts belonging to panel and a button "uid": "__expr__", }, "expression": "B", - "hide": false, "refId": "C", "type": "threshold", }, diff --git a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx index deb450c5c7c..85fd1eb5de6 100644 --- a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx @@ -1,4 +1,5 @@ import { css } from '@emotion/css'; +import { omit } from 'lodash'; import React, { useEffect, useMemo, useState } from 'react'; import { DeepMap, FieldError, FormProvider, useForm, useFormContext, UseFormWatch } from 'react-hook-form'; import { Link, useParams } from 'react-router-dom'; @@ -12,6 +13,7 @@ import { useCleanup } from 'app/core/hooks/useCleanup'; import { useQueryParams } from 'app/core/hooks/useQueryParams'; import { useDispatch } from 'app/types'; import { RuleWithLocation } from 'app/types/unified-alerting'; +import { RulerRuleDTO } from 'app/types/unified-alerting-dto'; import { LogMessages, trackNewAlerRuleFormError } from '../../Analytics'; import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelector'; @@ -90,21 +92,21 @@ export const AlertRuleForm = ({ existing, prefill }: Props) => { const defaultValues: RuleFormValues = useMemo(() => { if (existing) { - return rulerRuleToFormValues(existing); + return formValuesFromExistingRule(existing); } if (prefill) { - return { - ...getDefaultFormValues(), - ...prefill, - }; + return formValuesFromPrefill(prefill); + } + + if (typeof queryParams['defaults'] === 'string') { + return formValuesFromQueryParams(queryParams['defaults'], ruleType); } return { ...getDefaultFormValues(), - queries: getDefaultQueries(), condition: 'C', - ...(queryParams['defaults'] ? JSON.parse(queryParams['defaults'] as string) : {}), + queries: getDefaultQueries(), type: ruleType || RuleFormType.grafana, evaluateEvery: evaluateEvery, }; @@ -281,6 +283,49 @@ const isCortexLokiOrRecordingRule = (watch: UseFormWatch) => { return (ruleType === RuleFormType.cloudAlerting || ruleType === RuleFormType.cloudRecording) && dataSourceName !== ''; }; +// the backend will always execute "hidden" queries, so we have no choice but to remove the property in the front-end +// to avoid confusion. The query editor shows them as "disabled" and that's a different semantic meaning. +// furthermore the "AlertingQueryRunner" calls `filterQuery` on each data source and those will skip running queries that are "hidden"." +// It seems like we have no choice but to act like "hidden" queries don't exist in alerting. +const ignoreHiddenQueries = (ruleDefinition: RuleFormValues): RuleFormValues => { + return { + ...ruleDefinition, + queries: ruleDefinition.queries?.map((query) => omit(query, 'model.hide')), + }; +}; + +function formValuesFromQueryParams(ruleDefinition: string, type: RuleFormType): RuleFormValues { + let ruleFromQueryParams: Partial; + + try { + ruleFromQueryParams = JSON.parse(ruleDefinition); + } catch (err) { + return { + ...getDefaultFormValues(), + queries: getDefaultQueries(), + }; + } + + return ignoreHiddenQueries({ + ...getDefaultFormValues(), + ...ruleFromQueryParams, + queries: ruleFromQueryParams.queries ?? getDefaultQueries(), + type: type || RuleFormType.grafana, + evaluateEvery: MINUTE, + }); +} + +function formValuesFromPrefill(rule: Partial): RuleFormValues { + return ignoreHiddenQueries({ + ...getDefaultFormValues(), + ...rule, + }); +} + +function formValuesFromExistingRule(rule: RuleWithLocation) { + return ignoreHiddenQueries(rulerRuleToFormValues(rule)); +} + const getStyles = (theme: GrafanaTheme2) => { return { buttonSpinner: css` diff --git a/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx b/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx index 1043482850b..bca5bdb1efe 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx @@ -3,6 +3,7 @@ import React, { PureComponent, useState } from 'react'; import { DragDropContext, Droppable, DropResult } from 'react-beautiful-dnd'; import { DataQuery, DataSourceInstanceSettings, LoadingState, PanelData, RelativeTimeRange } from '@grafana/data'; +import { Stack } from '@grafana/experimental'; import { getDataSourceSrv } from '@grafana/runtime'; import { Button, Card, Icon } from '@grafana/ui'; import { QueryOperationRow } from 'app/core/components/QueryOperationRow/QueryOperationRow'; @@ -142,59 +143,61 @@ export class QueryRows extends PureComponent { {(provided) => { return (
- {queries.map((query, index) => { - const data: PanelData = this.props.data?.[query.refId] ?? { - series: [], - state: LoadingState.NotStarted, - }; - const dsSettings = this.getDataSourceSettings(query); + + {queries.map((query, index) => { + const data: PanelData = this.props.data?.[query.refId] ?? { + series: [], + state: LoadingState.NotStarted, + }; + const dsSettings = this.getDataSourceSettings(query); - const isAlertCondition = this.props.condition === query.refId; - const error = isAlertCondition ? errorFromSeries(data.series) : undefined; + const isAlertCondition = this.props.condition === query.refId; + const error = isAlertCondition ? errorFromSeries(data.series) : undefined; + + if (!dsSettings) { + return ( + { + const defaultDataSource = getDatasourceSrv().getInstanceSettings(null); + if (defaultDataSource) { + this.onChangeDataSource(defaultDataSource, index); + } + }} + onRemoveQuery={() => { + this.onRemoveQuery(query); + }} + /> + ); + } - if (!dsSettings) { return ( - { - const defaultDataSource = getDatasourceSrv().getInstanceSettings(null); - if (defaultDataSource) { - this.onChangeDataSource(defaultDataSource, index); - } - }} - onRemoveQuery={() => { - this.onRemoveQuery(query); - }} + key={query.refId} + dsSettings={dsSettings} + data={data} + error={error} + query={query} + onChangeQuery={this.onChangeQuery} + onRemoveQuery={this.onRemoveQuery} + queries={[...queries, ...expressions]} + onChangeDataSource={this.onChangeDataSource} + onDuplicateQuery={this.props.onDuplicateQuery} + onChangeTimeRange={this.onChangeTimeRange} + onChangeQueryOptions={this.onChangeQueryOptions} + thresholds={thresholdByRefId[query.refId]?.config} + thresholdsType={thresholdByRefId[query.refId]?.mode} + onRunQueries={this.props.onRunQueries} + condition={this.props.condition} + onSetCondition={this.props.onSetCondition} /> ); - } - - return ( - - ); - })} - {provided.placeholder} + })} + {provided.placeholder} +
); }} 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 ba365456a0d..44d43b3fc4a 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx @@ -4,7 +4,6 @@ import React, { ChangeEvent, useState } from 'react'; import { CoreApp, - DataQuery, DataSourceApi, DataSourceInstanceSettings, getDefaultRelativeTimeRange, @@ -15,6 +14,7 @@ import { ThresholdsConfig, } from '@grafana/data'; import { Stack } from '@grafana/experimental'; +import { DataQuery } from '@grafana/schema'; import { GraphTresholdsStyleMode, Icon, @@ -82,6 +82,11 @@ export const QueryWrapper = ({ const [dsInstance, setDsInstance] = useState(); const defaults = dsInstance?.getDefaultQuery ? dsInstance.getDefaultQuery(CoreApp.UnifiedAlerting) : {}; + const queryWithDefaults = { + ...defaults, + ...cloneDeep(query.model), + }; + function SelectingDataSourceTooltip() { const styles = useStyles2(getStyles); return ( @@ -139,6 +144,8 @@ export const QueryWrapper = ({ ); } + const showVizualisation = data.state !== LoadingState.NotStarted; + return (
@@ -151,10 +158,7 @@ export const QueryWrapper = ({ index={index} key={query.refId} data={data} - query={{ - ...defaults, - ...cloneDeep(query.model), - }} + query={queryWithDefaults} onChange={(query) => onChangeQuery(query, index)} onRemoveQuery={onRemoveQuery} onAddQuery={() => onDuplicateQuery(cloneDeep(query))} @@ -165,7 +169,7 @@ export const QueryWrapper = ({ hideDisableQuery={true} />
- {data.state !== LoadingState.NotStarted && ( + {showVizualisation && ( { }); }); - it('should not execute if a query fails filterQuery check', async () => { + it('should not execute if all queries fail filterQuery check', async () => { const runner = new AlertingQueryRunner( mockBackendSrv({ fetch: () => throwError(new Error("shouldn't happen")), @@ -209,6 +210,39 @@ describe('AlertingQueryRunner', () => { expect(data.B.series).toHaveLength(0); }); }); + + it('should skip hidden queries', async () => { + const results = createFetchResponse({ + results: { + B: { frames: [createDataFrameJSON([1, 2, 3])] }, + }, + }); + + const runner = new AlertingQueryRunner( + mockBackendSrv({ + fetch: () => of(results), + }), + mockDataSourceSrv({ filterQuery: (model: AlertDataQuery) => model.hide !== true }) + ); + + const data = runner.get(); + runner.run([ + createQuery('A', { + model: { + refId: 'A', + hide: true, + }, + }), + createQuery('B'), + ]); + + await expect(data.pipe(take(1))).toEmitValuesWith((values) => { + const [loading, _data] = values; + + expect(loading.A).toBeUndefined(); + expect(loading.B.state).toEqual(LoadingState.Done); + }); + }); }); type MockBackendSrvConfig = { @@ -269,12 +303,12 @@ const createDataFrameJSON = (values: number[]): DataFrameJSON => { }; }; -const createQuery = (refId: string): AlertQuery => { - return { +const createQuery = (refId: string, options?: Partial): AlertQuery => { + return defaultsDeep(options, { refId, queryType: '', datasourceUid: '', model: { refId }, relativeTimeRange: getDefaultRelativeTimeRange(), - }; + }); }; diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts index 93362a7cdcd..b8aa462e620 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts +++ b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts @@ -1,3 +1,4 @@ +import { reject } from 'lodash'; import { Observable, of, OperatorFunction, ReplaySubject, Unsubscribable } from 'rxjs'; import { catchError, map, share } from 'rxjs/operators'; import { v4 as uuidv4 } from 'uuid'; @@ -44,24 +45,33 @@ export class AlertingQueryRunner { } async run(queries: AlertQuery[]) { - if (queries.length === 0) { - const empty = initialState(queries, LoadingState.Done); - return this.subject.next(empty); - } + 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) { - if (!isExpressionQuery(query.model)) { - const ds = await this.dataSourceSrv.get(query.datasourceUid); - if (ds.filterQuery && !ds.filterQuery(query.model)) { - const empty = initialState(queries, LoadingState.Done); - return this.subject.next(empty); - } + const refId = query.model.refId; + + if (isExpressionQuery(query.model)) { + continue; + } + + const dataSourceInstance = await this.dataSourceSrv.get(query.datasourceUid); + const skipRunningQuery = dataSourceInstance.filterQuery && !dataSourceInstance.filterQuery(query.model); + + if (skipRunningQuery) { + queriesToExclude.push(refId); } } - this.subscription = runRequest(this.backendSrv, queries).subscribe({ + const queriesToRun = reject(queries, (q) => queriesToExclude.includes(q.model.refId)); + + if (queriesToRun.length === 0) { + return this.subject.next(empty); + } + + this.subscription = runRequest(this.backendSrv, queriesToRun).subscribe({ next: (dataPerQuery) => { const nextResult = applyChange(dataPerQuery, (refId, data) => { const previous = this.lastResult[refId]; diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 09baa289232..7c3bf3d18c1 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -127,6 +127,7 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF if (isGrafanaRulesSource(ruleSourceName)) { if (isGrafanaRulerRule(rule)) { const ga = rule.grafana_alert; + return { ...defaultFormValues, name: ga.title, @@ -220,7 +221,6 @@ export const getDefaultQueries = (): AlertQuery[] => { relativeTimeRange, model: { refId: 'A', - hide: false, }, }, ...getDefaultExpressions('B', 'C'), @@ -240,7 +240,6 @@ export const getDefaultRecordingRulesQueries = ( relativeTimeRange, model: { refId: 'A', - hide: false, }, }, ]; @@ -252,7 +251,6 @@ const getDefaultExpressions = (...refIds: [string, string]): AlertQuery[] => { const reduceExpression: ExpressionQuery = { refId: refIds[0], - hide: false, type: ExpressionQueryType.reduce, datasource: { uid: ExpressionDatasourceUID, @@ -283,7 +281,6 @@ const getDefaultExpressions = (...refIds: [string, string]): AlertQuery[] => { const thresholdExpression: ExpressionQuery = { refId: refTwo, - hide: false, type: ExpressionQueryType.threshold, datasource: { uid: ExpressionDatasourceUID,