diff --git a/public/app/features/alerting/unified/components/expressions/Expression.tsx b/public/app/features/alerting/unified/components/expressions/Expression.tsx index 79f805d52ad..0e5d5fd4b79 100644 --- a/public/app/features/alerting/unified/components/expressions/Expression.tsx +++ b/public/app/features/alerting/unified/components/expressions/Expression.tsx @@ -48,7 +48,6 @@ interface ExpressionProps { onSetCondition: (refId: string) => void; onUpdateRefId: (oldRefId: string, newRefId: string) => void; onRemoveExpression: (refId: string) => void; - onUpdateExpressionType: (refId: string, type: ExpressionQueryType) => void; onChangeQuery: (query: ExpressionQuery) => void; } @@ -62,7 +61,6 @@ export const Expression: FC = ({ onSetCondition, onUpdateRefId, onRemoveExpression, - onUpdateExpressionType, // this method is not used? maybe we should remove it onChangeQuery, }) => { const styles = useStyles2(getStyles); diff --git a/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx b/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx index 87952c4b19f..bf1a9985ec2 100644 --- a/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx @@ -4,7 +4,7 @@ import { useMemo } from 'react'; import { GrafanaTheme2, PanelData } from '@grafana/data'; import { useStyles2 } from '@grafana/ui'; import { isExpressionQuery } from 'app/features/expressions/guards'; -import { ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types'; +import { ExpressionQuery } from 'app/features/expressions/types'; import { AlertQuery } from 'app/types/unified-alerting-dto'; import { Expression } from '../expressions/Expression'; @@ -18,7 +18,6 @@ interface Props { queries: AlertQuery[]; onRemoveExpression: (refId: string) => void; onUpdateRefId: (oldRefId: string, newRefId: string) => void; - onUpdateExpressionType: (refId: string, type: ExpressionQueryType) => void; onUpdateQueryExpression: (query: ExpressionQuery) => void; } @@ -29,12 +28,15 @@ export const ExpressionsEditor = ({ panelData, onUpdateRefId, onRemoveExpression, - onUpdateExpressionType, onUpdateQueryExpression, }: Props) => { const expressionQueries = useMemo(() => { return queries.reduce((acc: ExpressionQuery[], query) => { - return isExpressionQuery(query.model) ? acc.concat(query.model) : acc; + if (isExpressionQuery(query.model)) { + acc.push(query.model); + } + + return acc; }, []); }, [queries]); const styles = useStyles2(getStyles); @@ -64,7 +66,6 @@ export const ExpressionsEditor = ({ onSetCondition={onSetCondition} onRemoveExpression={onRemoveExpression} onUpdateRefId={onUpdateRefId} - onUpdateExpressionType={onUpdateExpressionType} onChangeQuery={onUpdateQueryExpression} /> ); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx index 844443a4dff..d19ac4bb619 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx @@ -73,7 +73,6 @@ import { updateExpression, updateExpressionRefId, updateExpressionTimeRange, - updateExpressionType, } from './reducer'; import { useAdvancedMode } from './useAdvancedMode'; import { useAlertQueryRunner } from './useAlertQueryRunner'; @@ -591,9 +590,6 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange, mod dispatch(removeExpression(refId)); }} onUpdateRefId={onUpdateRefId} - onUpdateExpressionType={(refId, type) => { - dispatch(updateExpressionType({ refId, type })); - }} onUpdateQueryExpression={(model) => { dispatch(updateExpression(model)); }} diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap index 292a6904f80..33f3b0ea4e8 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/reducer.test.tsx.snap @@ -442,55 +442,3 @@ exports[`Query and expressions reducer should update an expression refId and rew ], } `; - -exports[`Query and expressions reducer should update expression type 1`] = ` -{ - "queries": [ - { - "datasourceUid": "abc123", - "model": { - "refId": "A", - }, - "queryType": "query", - "refId": "A", - }, - { - "datasourceUid": "__expr__", - "model": { - "conditions": [ - { - "evaluator": { - "params": [ - 0, - 0, - ], - "type": "gt", - }, - "operator": { - "type": "and", - }, - "query": { - "params": [], - }, - "reducer": { - "params": [], - "type": "avg", - }, - "type": "query", - }, - ], - "datasource": { - "name": "Expression", - "type": "__expr__", - "uid": "__expr__", - }, - "expression": "", - "refId": "B", - "type": "reduce", - }, - "queryType": "", - "refId": "B", - }, - ], -} -`; 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 31858408509..a4a214b8348 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 @@ -22,7 +22,6 @@ import { updateExpression, updateExpressionRefId, updateExpressionTimeRange, - updateExpressionType, } from './reducer'; const reduceExpression: AlertQuery = { @@ -388,22 +387,6 @@ describe('Query and expressions reducer', () => { expect(newState).toMatchSnapshot(); }); - - it('should update expression type', () => { - const initialState: QueriesAndExpressionsState = { - queries: [alertQuery, expressionQuery], - }; - - const newState = queriesAndExpressionsReducer( - initialState, - updateExpressionType({ - refId: 'B', - type: ExpressionQueryType.reduce, - }) - ); - - expect(newState).toMatchSnapshot(); - }); it('should remove first reducer', () => { const initialState: QueriesAndExpressionsState = { queries: [alertQuery, reduceExpression, thresholdExpression], 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 372e028bbcc..1514533cce0 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 @@ -283,23 +283,6 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder queryType: 'expression', }); } - }) - .addCase(updateExpressionType, (state, action) => { - state.queries = state.queries.map((query) => { - return query.refId === action.payload.refId - ? { - ...query, - model: { - ...expressionDatasource.newQuery({ - type: action.payload.type, - conditions: [{ ...defaultCondition, query: { params: [] } }], - expression: '', - }), - refId: action.payload.refId, - }, - } - : query; - }); }); }); diff --git a/public/app/features/alerting/unified/utils/rule-form.test.ts b/public/app/features/alerting/unified/utils/rule-form.test.ts index 2e9b7a5730b..a5410e6ebd6 100644 --- a/public/app/features/alerting/unified/utils/rule-form.test.ts +++ b/public/app/features/alerting/unified/utils/rule-form.test.ts @@ -7,6 +7,7 @@ import { GrafanaAlertStateDecision, GrafanaRuleDefinition, RulerAlertingRuleDTO, + RulerGrafanaRuleDTO, } from 'app/types/unified-alerting-dto'; import { EvalFunction } from '../../state/alertDef'; @@ -20,6 +21,7 @@ import { alertingRulerRuleToRuleForm, cleanAnnotations, cleanLabels, + fixMissingRefIdsInExpressionModel, formValuesToRulerGrafanaRuleDTO, formValuesToRulerRuleDTO, getContactPointsFromDTO, @@ -520,3 +522,70 @@ describe('getDefaultExpressions', () => { expect(thresholdModel.expression).toBe('X'); }); }); + +describe('fixMissingRefIdsInExpressionModel', () => { + it('should return non-Grafana managed rules unchanged', () => { + const cloudAlertingRule: RulerAlertingRuleDTO = { + alert: 'CloudAlert', + expr: 'up == 0', + for: '5m', + labels: { severity: 'critical' }, + annotations: { summary: 'Instance down' }, + }; + + const result = fixMissingRefIdsInExpressionModel(cloudAlertingRule); + + expect(result).toEqual(cloudAlertingRule); + expect(result).toBe(cloudAlertingRule); // should be the exact same reference + }); + + it('should copy refId from query to model when model.refId is missing in Grafana managed rules', () => { + const ruleWithMissingRefId: RulerGrafanaRuleDTO = { + grafana_alert: { + uid: 'test-uid', + title: 'Test Alert', + namespace_uid: 'namespace-uid', + rule_group: 'test-group', + condition: 'B', + no_data_state: GrafanaAlertStateDecision.NoData, + exec_err_state: GrafanaAlertStateDecision.Alerting, + is_paused: false, + data: [ + { + refId: 'A', + datasourceUid: 'datasource-uid', + queryType: '', + relativeTimeRange: { from: 600, to: 0 }, + // @ts-ignore + model: { + // refId is missing here + datasource: { + type: 'grafana-testdata-datasource', + uid: 'PD8C576611E62080A', + }, + }, + }, + { + refId: 'B', + datasourceUid: ExpressionDatasourceUID, + queryType: '', + // @ts-ignore + model: { + // refId is missing here + type: ExpressionQueryType.reduce, + expression: 'A', + }, + }, + ], + }, + for: '5m', + labels: {}, + annotations: {}, + }; + + const result = fixMissingRefIdsInExpressionModel(ruleWithMissingRefId); + + expect(result.grafana_alert.data[0].model.refId).toBe('A'); + expect(result.grafana_alert.data[1].model.refId).toBe('B'); + }); +}); diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 077934e21e1..0e83a276851 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -1,3 +1,5 @@ +import { produce } from 'immer'; + import { DataSourceInstanceSettings, IntervalValues, @@ -278,14 +280,16 @@ function getEditorSettingsFromDTO(ga: GrafanaRuleDefinition) { export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleFormValues { const { ruleSourceName, namespace, group, rule } = ruleWithLocation; - const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(rule); + const normalizedRule = fixMissingRefIdsInExpressionModel(rule); + + const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(normalizedRule); const defaultFormValues = getDefaultFormValues(isGrafanaRecordingRule ? RuleFormType.grafanaRecording : undefined); if (isGrafanaRulesSource(ruleSourceName)) { // GRAFANA-MANAGED RULES if (isGrafanaRecordingRule) { // grafana recording rule - const ga = rule.grafana_alert; + const ga = normalizedRule.grafana_alert; return { ...defaultFormValues, name: ga.title, @@ -294,16 +298,16 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF evaluateEvery: group.interval || defaultFormValues.evaluateEvery, queries: ga.data, condition: ga.condition, - annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)), - labels: listifyLabelsOrAnnotations(rule.labels, true), + annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)), + labels: listifyLabelsOrAnnotations(normalizedRule.labels, true), folder: { title: namespace, uid: ga.namespace_uid }, isPaused: ga.is_paused, metric: ga.record?.metric, targetDatasourceUid: ga.record?.target_datasource_uid || defaultFormValues.targetDatasourceUid, }; - } else if (rulerRuleType.grafana.rule(rule)) { + } else if (rulerRuleType.grafana.rule(normalizedRule)) { // grafana alerting rule - const ga = rule.grafana_alert; + const ga = normalizedRule.grafana_alert; const routingSettings: AlertManagerManualRouting | undefined = getContactPointsFromDTO(ga); if (ga.no_data_state !== undefined && ga.exec_err_state !== undefined) { return { @@ -312,14 +316,14 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF type: RuleFormType.grafana, group: group.name, evaluateEvery: group.interval || defaultFormValues.evaluateEvery, - evaluateFor: rule.for || '0', - keepFiringFor: rule.keep_firing_for || '0', + evaluateFor: normalizedRule.for || '0', + keepFiringFor: normalizedRule.keep_firing_for || '0', noDataState: ga.no_data_state, execErrState: ga.exec_err_state, queries: ga.data, condition: ga.condition, - annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)), - labels: listifyLabelsOrAnnotations(rule.labels, true), + annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)), + labels: listifyLabelsOrAnnotations(normalizedRule.labels, true), folder: { title: namespace, uid: ga.namespace_uid }, isPaused: ga.is_paused, @@ -338,7 +342,7 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF } } else { // DATASOURCE-MANAGED RULES - if (rulerRuleType.dataSource.alertingRule(rule)) { + if (rulerRuleType.dataSource.alertingRule(normalizedRule)) { const datasourceUid = getDataSourceSrv().getInstanceSettings(ruleSourceName)?.uid ?? ''; const defaultQuery = { @@ -346,27 +350,27 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF datasourceUid, queryType: '', relativeTimeRange: getDefaultRelativeTimeRange(), - expr: rule.expr, + expr: normalizedRule.expr, model: { refId: 'A', hide: false, - expr: rule.expr, + expr: normalizedRule.expr, }, }; - const alertingRuleValues = alertingRulerRuleToRuleForm(rule); + const alertingRuleValues = alertingRulerRuleToRuleForm(normalizedRule); return { ...defaultFormValues, ...alertingRuleValues, queries: [defaultQuery], - annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(rule.annotations, false)), + annotations: normalizeDefaultAnnotations(listifyLabelsOrAnnotations(normalizedRule.annotations, false)), type: RuleFormType.cloudAlerting, dataSourceName: ruleSourceName, namespace, group: group.name, }; - } else if (rulerRuleType.dataSource.recordingRule(rule)) { + } else if (rulerRuleType.dataSource.recordingRule(normalizedRule)) { const datasourceUid = getDataSourceSrv().getInstanceSettings(ruleSourceName)?.uid ?? ''; const defaultQuery = { @@ -374,15 +378,15 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF datasourceUid, queryType: '', relativeTimeRange: getDefaultRelativeTimeRange(), - expr: rule.expr, + expr: normalizedRule.expr, model: { refId: 'A', hide: false, - expr: rule.expr, + expr: normalizedRule.expr, }, }; - const recordingRuleValues = recordingRulerRuleToRuleForm(rule); + const recordingRuleValues = recordingRulerRuleToRuleForm(normalizedRule); return { ...defaultFormValues, @@ -399,6 +403,23 @@ export function rulerRuleToFormValues(ruleWithLocation: RuleWithLocation): RuleF } } +/** + * This function isn't supposed to be needed, but we've noticed some customers are creating rules via Provisioning or + * other interfaces where they aren't including the RefId in the "model" of the expression so copy the refId from the query definition. + */ +export function fixMissingRefIdsInExpressionModel(rule: T): T { + // non-Grafana managed rules don't use expression nodes so we return the rule as-is + if (!rulerRuleType.grafana.rule(rule)) { + return rule; + } + + return produce(rule, (draft) => { + draft.grafana_alert.data.forEach((query) => { + query.model.refId = query.model.refId ?? query.refId; + }); + }); +} + export function grafanaRuleDtoToFormValues(rule: RulerGrafanaRuleDTO, namespace: string): RuleFormValues { const isGrafanaRecordingRule = rulerRuleType.grafana.recordingRule(rule); const defaultFormValues = getDefaultFormValues(isGrafanaRecordingRule ? RuleFormType.grafanaRecording : undefined);