diff --git a/.betterer.results b/.betterer.results index bba659d251f..f74ac1484c1 100644 --- a/.betterer.results +++ b/.betterer.results @@ -1870,11 +1870,12 @@ exports[`better eslint`] = { [0, 0, 0, "No untranslated strings. Wrap text with ", "4"] ], "public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx:5381": [ - [0, 0, 0, "No untranslated strings. Wrap text with ", "0"], + [0, 0, 0, "Do not use any type assertions.", "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 ", "4"], + [0, 0, 0, "No untranslated strings. Wrap text with ", "5"] ], "public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx:5381": [ [0, 0, 0, "No untranslated strings in text props. Wrap text with or use t()", "0"], 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 08c67ebf3d2..4e951c5110a 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx @@ -14,6 +14,7 @@ import { getDataSourceSrv } from '@grafana/runtime'; import { DataQuery } from '@grafana/schema'; import { Button, Card, Icon, Stack } from '@grafana/ui'; import { QueryOperationRow } from 'app/core/components/QueryOperationRow/QueryOperationRow'; +import { isExpressionQuery } from 'app/features/expressions/guards'; import { getDatasourceSrv } from 'app/features/plugins/datasource_srv'; import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; @@ -236,8 +237,10 @@ function copyModel(item: AlertQuery, settings: DataSourceInstanceSettings): Omit } function newModel(item: AlertQuery, settings: DataSourceInstanceSettings): Omit { - const isInstant = getInstantFromDataQuery(item.model, settings.type); - return { + const isExpression = isExpressionQuery(item); + const isInstant = isExpression ? false : getInstantFromDataQuery(item); + + const newQuery: Omit = { refId: item.refId, relativeTimeRange: item.relativeTimeRange, queryType: '', @@ -246,9 +249,14 @@ function newModel(item: AlertQuery, settings: DataSourceInstanceSettings): Omit< refId: item.refId, hide: false, datasource: getDataSourceRef(settings), - instant: isInstant, }, }; + + if (isInstant && !isExpressionQuery(item)) { + (newQuery as AlertQuery).model.instant = isInstant; + } + + return newQuery; } interface DatasourceNotFoundProps { 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 b32219a4fb6..463401721a0 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 @@ -23,7 +23,12 @@ import { } from '@grafana/ui'; import { Trans, t } from 'app/core/internationalization'; import { isExpressionQuery } from 'app/features/expressions/guards'; -import { ExpressionDatasourceUID, ExpressionQueryType, expressionTypes } from 'app/features/expressions/types'; +import { + ExpressionDatasourceUID, + ExpressionQuery, + ExpressionQueryType, + expressionTypes, +} from 'app/features/expressions/types'; import { AlertQuery } from 'app/types/unified-alerting-dto'; import { useRulesSourcesWithRuler } from '../../../hooks/useRuleSourcesWithRuler'; @@ -50,7 +55,7 @@ import { RuleEditorSection } from '../RuleEditorSection'; import { errorFromCurrentCondition, errorFromPreviewData, findRenamedDataQueryReferences, refIdExists } from '../util'; import { CloudDataSourceSelector } from './CloudDataSourceSelector'; -import { SimpleConditionEditor, SimpleConditionIdentifier, getSimpleConditionFromExpressions } from './SimpleCondition'; +import { SimpleConditionEditor, getSimpleConditionFromExpressions } from './SimpleCondition'; import { SmartAlertTypeDetector } from './SmartAlertTypeDetector'; import { DESCRIPTIONS } from './descriptions'; import { @@ -146,7 +151,7 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange, mod ); const simplifiedQueryStep = - isSwitchModeEnabled && isGrafanaAlertingType ? getValues('editorSettings.simplifiedQueryEditor') : false; + isSwitchModeEnabled && isGrafanaAlertingType ? editorSettings?.simplifiedQueryEditor : false; // If we switch to simple mode we need to update the simple condition with the data in the queries reducer useEffect(() => { @@ -164,15 +169,22 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange, mod // Grafana Managed rules and recording rules do return; } - // we need to be sure the condition is set once we switch to simple mode + if (simplifiedQueryStep) { - setValue('condition', SimpleConditionIdentifier.thresholdId); - runQueries(getValues('queries'), SimpleConditionIdentifier.thresholdId); + const lastExpression = expressionQueries.at(-1); + if (!lastExpression) { + return; + } + + const condition = lastExpression.refId; + // we need to be sure the condition is set once we switch to simple mode + setValue('condition', condition); + runQueries(getValues('queries'), condition); } else { runQueries(getValues('queries'), condition || (getValues('condition') ?? '')); } }, - [isCloudAlertRuleType, runQueries, getValues, simplifiedQueryStep, setValue] + [isCloudAlertRuleType, expressionQueries, simplifiedQueryStep, setValue, runQueries, getValues] ); // whenever we update the queries we have to update the form too @@ -247,7 +259,9 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange, mod // As a workaround we update form values as soon as possible to avoid stale state // This way we can access up to date queries in runQueriesPreview without waiting for re-render const previousQueries = getValues('queries'); - const expressionQueries = previousQueries.filter((query) => isExpressionQuery(query.model)); + + const expressionQueries = previousQueries.filter>(isExpressionQueryInAlert); + setValue('queries', [...updatedQueries, ...expressionQueries], { shouldValidate: false }); updateExpressionAndDatasource(updatedQueries); diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/SimpleCondition.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/SimpleCondition.tsx index acd8402694d..0fd8b0d8760 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/SimpleCondition.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/SimpleCondition.tsx @@ -17,11 +17,6 @@ import { ExpressionResult } from '../../expressions/Expression'; import { updateExpression } from './reducer'; -export const SimpleConditionIdentifier = { - queryId: 'A', - reducerId: 'B', - thresholdId: 'C', -} as const; export interface SimpleCondition { whenField?: string; evaluator: { @@ -158,10 +153,8 @@ function updateReduceExpression( expressionQueriesList: Array>, dispatch: Dispatch ) { - const reduceExpression = expressionQueriesList.find( - (query) => - query.model.type === ExpressionQueryType.reduce && query.model.refId === SimpleConditionIdentifier.reducerId - ); + // 1. make sure have have a reduce expression and that it is pointing to the data query + const reduceExpression = expressionQueriesList.find((query) => query.model.type === ExpressionQueryType.reduce); const newReduceExpression = reduceExpression ? produce(reduceExpression?.model, (draft) => { @@ -179,10 +172,7 @@ function updateThresholdFunction( expressionQueriesList: Array>, dispatch: Dispatch ) { - const thresholdExpression = expressionQueriesList.find( - (query) => - query.model.type === ExpressionQueryType.threshold && query.model.refId === SimpleConditionIdentifier.thresholdId - ); + const thresholdExpression = expressionQueriesList.find((query) => query.model.type === ExpressionQueryType.threshold); const newThresholdExpression = produce(thresholdExpression, (draft) => { if (draft && draft.model.conditions) { @@ -198,10 +188,7 @@ function updateThresholdValue( expressionQueriesList: Array>, dispatch: Dispatch ) { - const thresholdExpression = expressionQueriesList.find( - (query) => - query.model.type === ExpressionQueryType.threshold && query.model.refId === SimpleConditionIdentifier.thresholdId - ); + const thresholdExpression = expressionQueriesList.find((query) => query.model.type === ExpressionQueryType.threshold); const newThresholdExpression = produce(thresholdExpression, (draft) => { if (draft && draft.model.conditions) { @@ -212,13 +199,8 @@ function updateThresholdValue( } export function getSimpleConditionFromExpressions(expressions: Array>): SimpleCondition { - const reduceExpression = expressions.find( - (query) => query.model.type === ExpressionQueryType.reduce && query.refId === SimpleConditionIdentifier.reducerId - ); - const thresholdExpression = expressions.find( - (query) => - query.model.type === ExpressionQueryType.threshold && query.refId === SimpleConditionIdentifier.thresholdId - ); + const reduceExpression = expressions.find((query) => query.model.type === ExpressionQueryType.reduce); + const thresholdExpression = expressions.find((query) => query.model.type === ExpressionQueryType.threshold); const conditionsFromThreshold = thresholdExpression?.model.conditions ?? []; const whenField = reduceExpression?.model.reducer; const params = conditionsFromThreshold[0]?.evaluator?.params diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/areQueriesTransformableToSimpleCondition.test.ts b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/areQueriesTransformableToSimpleCondition.test.ts index 2a9294ada7d..96be5b02dc5 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/areQueriesTransformableToSimpleCondition.test.ts +++ b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/__snapshots__/areQueriesTransformableToSimpleCondition.test.ts @@ -1,35 +1,50 @@ import { produce } from 'immer'; import { EvalFunction } from 'app/features/alerting/state/alertDef'; -import { dataQuery, reduceExpression, thresholdExpression } from 'app/features/alerting/unified/mocks'; +import { + mockDataQuery, + mockDataSource, + mockReduceExpression, + mockThresholdExpression, +} from 'app/features/alerting/unified/mocks'; import { areQueriesTransformableToSimpleCondition } from 'app/features/alerting/unified/rule-editor/formProcessing'; +import { setupDataSources } from 'app/features/alerting/unified/testSetup/datasources'; +import { DataSourceType } from 'app/features/alerting/unified/utils/datasource'; import { ExpressionQuery, ReducerMode } from 'app/features/expressions/types'; import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; -const expressionQueries: Array> = [reduceExpression, thresholdExpression]; +const reduceExpression = mockReduceExpression({ expression: 'A', settings: { mode: ReducerMode.Strict } }); +const thresholdExpression = mockThresholdExpression({ expression: 'B' }); +const expressionQueries: Array> = [reduceExpression, thresholdExpression]; +const ds = mockDataSource({ type: DataSourceType.Prometheus, name: 'Mimir-cloud', uid: 'abc123' }); describe('areQueriesTransformableToSimpleCondition', () => { + beforeEach(() => { + setupDataSources(ds); + }); it('should return false if dataQueries length is not 1', () => { // zero dataQueries expect(areQueriesTransformableToSimpleCondition([], expressionQueries)).toBe(false); // more than one dataQueries - expect(areQueriesTransformableToSimpleCondition([dataQuery, dataQuery], expressionQueries)).toBe(false); + expect(areQueriesTransformableToSimpleCondition([mockDataQuery(), mockDataQuery()], expressionQueries)).toBe(false); }); + it('should return false if expressionQueries length is not 2', () => { - const dataQueries: Array> = [dataQuery]; + const dataQueries: Array> = [mockDataQuery()]; const result = areQueriesTransformableToSimpleCondition(dataQueries, []); expect(result).toBe(false); }); - it('should return false if the dataQuery refId does not match SimpleConditionIdentifier.queryId', () => { - const dataQueries: Array> = [ - { refId: 'notSimpleCondition', datasourceUid: 'abc123', queryType: '', model: { refId: 'notSimpleCondition' } }, - ]; + // notSimpleCondition + // reducer: + it('should return false if the mockDataQuery() refId does not match SimpleConditionIdentifier.queryId', () => { + const dataQueries: Array> = [mockDataQuery({ refId: 'foo' })]; const result = areQueriesTransformableToSimpleCondition(dataQueries, expressionQueries); expect(result).toBe(false); }); + it('should return false if no reduce expression is found with correct type and refId', () => { - const dataQueries: Array> = [dataQuery]; + const dataQueries: Array> = [mockDataQuery()]; const result = areQueriesTransformableToSimpleCondition(dataQueries, [ { ...reduceExpression, refId: 'hello' }, thresholdExpression, @@ -37,17 +52,25 @@ describe('areQueriesTransformableToSimpleCondition', () => { expect(result).toBe(false); }); - it('should return false if no threshold expression is found with correct type and refId', () => { - const dataQueries: Array> = [dataQuery]; + it('should return false if no threshold expression is found that points to reducer', () => { + const dataQueries: Array> = [mockDataQuery()]; const result = areQueriesTransformableToSimpleCondition(dataQueries, [ reduceExpression, - { ...thresholdExpression, refId: 'hello' }, + mockThresholdExpression({ expression: 'hello' }), + ]); + expect(result).toBe(false); + }); + + it('should return false if no threshold expression is found that points to instant data query', () => { + const dataQueries: Array> = [mockDataQuery({ instant: true })]; + const result = areQueriesTransformableToSimpleCondition(dataQueries, [ + mockThresholdExpression({ expression: 'hello' }), ]); expect(result).toBe(false); }); it('should return false if reduceExpression settings mode is not ReducerMode.Strict', () => { - const dataQueries: Array> = [dataQuery]; + const dataQueries: Array> = [mockDataQuery()]; const transformedReduceExpression = produce(reduceExpression, (draft) => { draft.model.settings = { mode: ReducerMode.DropNonNumbers }; }); @@ -60,7 +83,7 @@ describe('areQueriesTransformableToSimpleCondition', () => { }); it('should return false if thresholdExpression unloadEvaluator has a value', () => { - const dataQueries: Array> = [dataQuery]; + const dataQueries: Array> = [mockDataQuery()]; const transformedThresholdExpression = produce(thresholdExpression, (draft) => { draft.model.conditions = [ @@ -79,9 +102,17 @@ describe('areQueriesTransformableToSimpleCondition', () => { ]); expect(result).toBe(false); }); - it('should return true when all conditions are met', () => { - const dataQueries: Array> = [dataQuery]; - const result = areQueriesTransformableToSimpleCondition(dataQueries, expressionQueries); + + it('should return true when data query is connected to valid reducer and threshold', () => { + const result = areQueriesTransformableToSimpleCondition([mockDataQuery({ refId: 'A' })], expressionQueries); + expect(result).toBe(true); + }); + + it('should return true when all conditions are met for instant data query with threshold', () => { + const result = areQueriesTransformableToSimpleCondition( + [mockDataQuery({ instant: true })], + [mockThresholdExpression({ expression: 'A' })] + ); expect(result).toBe(true); }); }); 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 a316611bf55..292a6904f80 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 @@ -130,16 +130,16 @@ exports[`Query and expressions reducer should add reduce expression if there is }, "expression": "A", "reducer": "last", - "refId": "B", + "refId": "reducer", "type": "reduce", }, "queryType": "expression", - "refId": "B", + "refId": "reducer", }, { "datasourceUid": "__expr__", "model": { - "expression": "B", + "expression": "reducer", "refId": "C", "type": "threshold", }, @@ -218,6 +218,31 @@ exports[`Query and expressions reducer should remove first reducer 1`] = ` } `; +exports[`Query and expressions reducer should remove reducer even if reducer is not the first expression 1`] = ` +{ + "queries": [ + { + "datasourceUid": "abc123", + "model": { + "refId": "A", + }, + "queryType": "query", + "refId": "A", + }, + { + "datasourceUid": "__expr__", + "model": { + "expression": "A", + "refId": "C", + "type": "threshold", + }, + "queryType": "expression", + "refId": "C", + }, + ], +} +`; + exports[`Query and expressions reducer should rewire expressions 1`] = ` { "queries": [ 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 2a112e089c3..73372e12a57 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 @@ -10,7 +10,6 @@ import { import { defaultCondition } from 'app/features/expressions/utils/expressionTypes'; import { AlertQuery } from 'app/types/unified-alerting-dto'; -import { SimpleConditionIdentifier } from './SimpleCondition'; import { QueriesAndExpressionsState, addNewDataQuery, @@ -28,22 +27,23 @@ import { } from './reducer'; const reduceExpression: AlertQuery = { - refId: SimpleConditionIdentifier.reducerId, + refId: 'B', queryType: 'expression', datasourceUid: '__expr__', model: { type: ExpressionQueryType.reduce, - refId: SimpleConditionIdentifier.reducerId, + refId: 'B', settings: { mode: ReducerMode.Strict }, + expression: 'A', }, }; const thresholdExpression: AlertQuery = { - refId: SimpleConditionIdentifier.thresholdId, + refId: 'C', queryType: 'expression', datasourceUid: '__expr__', model: { type: ExpressionQueryType.threshold, - refId: SimpleConditionIdentifier.thresholdId, + refId: 'C', }, }; @@ -400,7 +400,7 @@ describe('Query and expressions reducer', () => { expect(newState).toMatchSnapshot(); }); - it('should not remove first reducer if reducer is not the first expression', () => { + it('should remove reducer even if reducer is not the first expression', () => { const initialState: QueriesAndExpressionsState = { queries: [alertQuery, thresholdExpression, reduceExpression], }; @@ -412,7 +412,7 @@ describe('Query and expressions reducer', () => { expressionQueries: [thresholdExpression, reduceExpression], }) ); - expect(newState).toEqual(initialState); + expect(newState).toMatchSnapshot(); }); it('should not remove first reducer if reducer is not the second query', () => { 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 aab9e184a56..b3499b8c520 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 @@ -8,21 +8,25 @@ import { getNextRefId, rangeUtil, } from '@grafana/data'; -import { getDataSourceSrv } from '@grafana/runtime'; import { DataQuery } from '@grafana/schema'; 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 { + defaultCondition, + isReducerExpression, + isThresholdExpression, +} from 'app/features/expressions/utils/expressionTypes'; import { AlertQuery } from 'app/types/unified-alerting-dto'; import { logError } from '../../../Analytics'; -import { DataSourceType, getDefaultOrFirstCompatibleDataSource } from '../../../utils/datasource'; +import { getDefaultOrFirstCompatibleDataSource } from '../../../utils/datasource'; import { getDefaultQueries, getInstantFromDataQuery } from '../../../utils/rule-form'; import { createDagFromQueries, getOriginOfRefId } from '../dag'; import { queriesWithUpdatedReferences, refIdExists } from '../util'; -import { SimpleConditionIdentifier } from './SimpleCondition'; +// this one will be used as the refID when we create a new reducer for the threshold expression +export const NEW_REDUCER_REF = 'reducer'; export interface QueriesAndExpressionsState { queries: AlertQuery[]; @@ -64,9 +68,10 @@ export const updateMaxDataPoints = createAction<{ refId: string; maxDataPoints: export const updateMinInterval = createAction<{ refId: string; minInterval: string }>('updateMinInterval'); export const resetToSimpleCondition = createAction('resetToSimpleCondition'); -export const optimizeReduceExpression = createAction<{ updatedQueries: AlertQuery[]; expressionQueries: AlertQuery[] }>( - 'optimizeReduceExpression' -); +export const optimizeReduceExpression = createAction<{ + updatedQueries: AlertQuery[]; + expressionQueries: Array>; +}>('optimizeReduceExpression'); export const setRecordingRulesQueries = createAction<{ recordingRuleQueries: AlertQuery[]; expression: string }>( 'setRecordingRulesQueries' ); @@ -231,6 +236,7 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder .addCase(rewireExpressions, (state, { payload }) => { state.queries = queriesWithUpdatedReferences(state.queries, payload.oldRefId, payload.newRefId); }) + // removes the reduce expression when we have a instant data query .addCase(optimizeReduceExpression, (state, { payload }) => { const { updatedQueries, expressionQueries } = payload; @@ -239,48 +245,29 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder return; } - //sometimes we dont have data source in the model yet - const getDataSourceSettingsForFirstQuery = getDataSourceSrv().getInstanceSettings( - updatedQueries[0].datasourceUid - ); - - if (!getDataSourceSettingsForFirstQuery) { - return; - } - const type = getDataSourceSettingsForFirstQuery?.type; - - const firstQueryIsPromOrLoki = type === DataSourceType.Prometheus || type === DataSourceType.Loki; - - const isInstant = getInstantFromDataQuery(updatedQueries[0].model, type); - - const shouldRemoveReducer = - firstQueryIsPromOrLoki && updatedQueries.length === 1 && isInstant && expressionQueries.length === 2; - - const onlyOneExpressionNotReducer = - expressionQueries.length === 1 && - 'type' in expressionQueries[0].model && - expressionQueries[0].model.type !== ExpressionQueryType.reduce; - - // we only add the reduce expression if we have one data query and one expression query. For other cases we don't do anything, - // and let the user add the reducer manually. - const shouldAddReduceExpression = - firstQueryIsPromOrLoki && updatedQueries.length === 1 && !isInstant && onlyOneExpressionNotReducer; + const dataQuery = updatedQueries.at(0); + const isInstantDataQuery = dataQuery ? getInstantFromDataQuery(dataQuery) : false; + const shouldRemoveReducer = isInstantDataQuery && expressionQueries.length === 2; if (shouldRemoveReducer) { const reduceExpressionIndex = state.queries.findIndex( - (query) => isExpressionQuery(query.model) && query.model.type === ExpressionQueryType.reduce + (query) => + isExpressionQuery(query.model) && + isReducerExpression(query.model) && + query.model.expression === dataQuery?.refId ); - if (reduceExpressionIndex === 1) { - // means the reduce expression is the second query - state.queries.splice(reduceExpressionIndex, 1); - state.queries[1].model.expression = SimpleConditionIdentifier.queryId; - } + state.queries.splice(reduceExpressionIndex, 1); + state.queries[1].model.expression = dataQuery?.refId; } + + const shouldAddReduceExpression = + !isInstantDataQuery && expressionQueries.length === 1 && isThresholdExpression(expressionQueries[0].model); if (shouldAddReduceExpression) { // add reducer to the second position // we only update the refid and the model to point to the reducer expression - state.queries[1].model.expression = SimpleConditionIdentifier.reducerId; + state.queries[1].model.expression = NEW_REDUCER_REF; + // insert in second position the reducer expression state.queries.splice(1, 0, { datasourceUid: ExpressionDatasourceUID, @@ -288,10 +275,10 @@ export const queriesAndExpressionsReducer = createReducer(initialState, (builder type: ExpressionQueryType.reduce, reducer: ReducerID.last, conditions: [{ ...defaultCondition, query: { params: [] } }], - expression: SimpleConditionIdentifier.queryId, - refId: SimpleConditionIdentifier.reducerId, + expression: dataQuery?.refId, + refId: NEW_REDUCER_REF, }), - refId: SimpleConditionIdentifier.reducerId, + refId: NEW_REDUCER_REF, queryType: 'expression', }); } 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 680367f38ec..13a4a6480b5 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 @@ -2,6 +2,7 @@ import { ExpressionDatasourceRef } from '@grafana/runtime/src/utils/DataSourceWi import { ClassicCondition, ExpressionQuery } from 'app/features/expressions/types'; import { AlertQuery } from 'app/types/unified-alerting-dto'; +import { NEW_REDUCER_REF } from './query-and-alert-condition/reducer'; import { containsPathSeparator, findRenamedDataQueryReferences, @@ -163,10 +164,10 @@ describe('rule-editor', () => { it('should rewire threshold expressions', () => { const queries: AlertQuery[] = [dataSource, reduceExpression, thresholdExpression]; - const rewiredQueries = queriesWithUpdatedReferences(queries, 'B', 'REDUCER'); + const rewiredQueries = queriesWithUpdatedReferences(queries, 'B', NEW_REDUCER_REF); const queryModel = rewiredQueries[2].model as ExpressionQuery; - expect(queryModel.expression).toBe('REDUCER'); + expect(queryModel.expression).toBe(NEW_REDUCER_REF); }); it('should rewire multiple expressions', () => { diff --git a/public/app/features/alerting/unified/mocks.ts b/public/app/features/alerting/unified/mocks.ts index 8e2701e0e04..4e0cd678109 100644 --- a/public/app/features/alerting/unified/mocks.ts +++ b/public/app/features/alerting/unified/mocks.ts @@ -56,7 +56,6 @@ import { import { DashboardSearchItem, DashboardSearchItemType } from '../../search/types'; -import { SimpleConditionIdentifier } from './components/rule-editor/query-and-alert-condition/SimpleCondition'; import { GRAFANA_RULES_SOURCE_NAME } from './utils/datasource'; import { parsePromQLStyleMatcherLooseSafe } from './utils/matchers'; @@ -772,30 +771,33 @@ export function mockDashboardDto( }; } -export const dataQuery: AlertQuery = { - refId: SimpleConditionIdentifier.queryId, +export const mockDataQuery = (partial: Partial = {}): AlertQuery => ({ + refId: partial?.refId ?? 'A', datasourceUid: 'abc123', queryType: '', - model: { refId: SimpleConditionIdentifier.queryId }, -}; + model: { refId: 'A', ...partial }, +}); -export const reduceExpression: AlertQuery = { - refId: SimpleConditionIdentifier.reducerId, +export const mockReduceExpression = (partial: Partial = {}): AlertQuery => ({ + refId: 'B', queryType: 'expression', datasourceUid: '__expr__', model: { type: ExpressionQueryType.reduce, - refId: SimpleConditionIdentifier.reducerId, + refId: 'B', settings: { mode: ReducerMode.Strict }, reducer: ReducerID.last, + ...partial, }, -}; -export const thresholdExpression: AlertQuery = { - refId: SimpleConditionIdentifier.thresholdId, +}); + +export const mockThresholdExpression = (partial: Partial = {}): AlertQuery => ({ + refId: 'C', queryType: 'expression', datasourceUid: '__expr__', model: { type: ExpressionQueryType.threshold, - refId: SimpleConditionIdentifier.thresholdId, + refId: 'C', + ...partial, }, -}; +}); diff --git a/public/app/features/alerting/unified/rule-editor/formDefaults.test.ts b/public/app/features/alerting/unified/rule-editor/formDefaults.test.ts index 800b95a6193..9dd60a76b8d 100644 --- a/public/app/features/alerting/unified/rule-editor/formDefaults.test.ts +++ b/public/app/features/alerting/unified/rule-editor/formDefaults.test.ts @@ -1,6 +1,6 @@ import { config } from '@grafana/runtime'; -import { mockAlertQuery, mockDataSource, reduceExpression, thresholdExpression } from '../mocks'; +import { mockAlertQuery, mockDataSource, mockReduceExpression, mockThresholdExpression } from '../mocks'; import { testWithFeatureToggles } from '../test/test-utils'; import { RuleFormType } from '../types/rule-form'; import { Annotation } from '../utils/constants'; @@ -73,7 +73,11 @@ describe('formValuesFromQueryParams', () => { it('should enable simplified query editor if queries are transformable to simple condition', () => { const result = formValuesFromQueryParams( JSON.stringify({ - queries: [mockAlertQuery(), reduceExpression, thresholdExpression], + queries: [ + mockAlertQuery(), + mockReduceExpression({ expression: 'A' }), + mockThresholdExpression({ expression: 'B' }), + ], }), RuleFormType.grafana ); @@ -85,7 +89,7 @@ describe('formValuesFromQueryParams', () => { it('should disable simplified query editor if queries are not transformable to simple condition', () => { const result = formValuesFromQueryParams( JSON.stringify({ - queries: [mockAlertQuery(), mockAlertQuery(), thresholdExpression], + queries: [mockAlertQuery(), mockAlertQuery(), mockThresholdExpression({ expression: 'B' })], }), RuleFormType.grafana ); diff --git a/public/app/features/alerting/unified/rule-editor/formProcessing.ts b/public/app/features/alerting/unified/rule-editor/formProcessing.ts index c9fa1951417..d9f79892ce5 100644 --- a/public/app/features/alerting/unified/rule-editor/formProcessing.ts +++ b/public/app/features/alerting/unified/rule-editor/formProcessing.ts @@ -1,14 +1,15 @@ -import { omit } from 'lodash'; +import { isEmpty, omit } from 'lodash'; import { config } from '@grafana/runtime'; import { isExpressionQuery } from 'app/features/expressions/guards'; -import { ExpressionQuery, ExpressionQueryType, ReducerMode } from 'app/features/expressions/types'; +import { ExpressionQuery, ExpressionQueryType } from 'app/features/expressions/types'; +import { isStrictReducer } from 'app/features/expressions/utils/expressionTypes'; import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto'; -import { SimpleConditionIdentifier } from '../components/rule-editor/query-and-alert-condition/SimpleCondition'; import { KVObject, RuleFormValues } from '../types/rule-form'; import { defaultAnnotations } from '../utils/constants'; import { DataSourceType } from '../utils/datasource'; +import { getInstantFromDataQuery } from '../utils/rule-form'; export function setQueryEditorSettings(values: RuleFormValues): RuleFormValues { const isQuerySwitchModeEnabled = config.featureToggles.alertingQueryAndExpressionsStepMode ?? false; @@ -64,47 +65,63 @@ export function setInstantOrRange(values: RuleFormValues): RuleFormValues { }; } +/** + * A alert rule is "transformable" to a simple condition editor if + * 1. we have a single data query + * 2. we have _either_ + * 2.1 a reduce expression (pointing to the data query) _and_ a threshold expression pointing to the reducer + * 2.2 a threshold expression pointing to a (instant) data query + * ⚠️ do not assert on refIds or indexes of the queries + */ export function areQueriesTransformableToSimpleCondition( - dataQueries: Array>, + dataQueries: Array>, expressionQueries: Array> ) { + // 1. check if we only have a _single_ data query if (dataQueries.length !== 1) { return false; } - const singleReduceExpressionInInstantQuery = - 'instant' in dataQueries[0].model && dataQueries[0].model.instant && expressionQueries.length === 1; - if (expressionQueries.length !== 2 && !singleReduceExpressionInInstantQuery) { + // short-circuit when we have more than 2 expressions, we don't know what to do with that + if (expressionQueries.length > 2) { return false; } - const query = dataQueries[0]; + const dataQuery = dataQueries.at(0); - if (query.refId !== SimpleConditionIdentifier.queryId) { - return false; + // find the reduce or threshold expressions + const reduceExpression = expressionQueries.find((query) => query.model.type === ExpressionQueryType.reduce); + const thresholdExpression = expressionQueries.find((query) => query.model.type === ExpressionQueryType.threshold); + + // reducer should be set to "strict" mode + const reducerIsStrict = reduceExpression ? isStrictReducer(reduceExpression.model) : false; + // threshold expression shouldn't have an unload evaluator (custom recovery threshold) + const thresholdExpressionIsClean = + thresholdExpression?.model.conditions?.every((condition) => { + return isEmpty(condition.unloadEvaluator); + }) ?? true; + + const validReducerExpression = reduceExpression && reducerIsStrict; + const validThresholdExpression = thresholdExpression && thresholdExpressionIsClean; + + const thresholdPointingToReducer = thresholdExpression?.model.expression === reduceExpression?.refId; + const reducerPointingToDataQuery = reduceExpression?.model.expression === dataQuery?.refId; + + // 2.1 check for a reduce + threshold expression and their targets + if (validReducerExpression && reducerPointingToDataQuery && validThresholdExpression && thresholdPointingToReducer) { + return true; } - const reduceExpressionIndex = expressionQueries.findIndex( - (query) => query.model.type === ExpressionQueryType.reduce && query.refId === SimpleConditionIdentifier.reducerId - ); - const reduceExpression = expressionQueries.at(reduceExpressionIndex); - const reduceOk = - reduceExpression && - reduceExpressionIndex === 0 && - (reduceExpression.model.settings?.mode === ReducerMode.Strict || - reduceExpression.model.settings?.mode === undefined); + // 2.2 check for a single threshold expression pointing to an "instant" data query + const isInstantDataQuery = dataQuery ? getInstantFromDataQuery(dataQuery) : false; + const hasSingleThresholdExpression = expressionQueries.length === 1 && thresholdExpression; + const thresholdPointingToDataQuery = thresholdExpression?.model.expression === dataQuery?.refId; - const thresholdExpressionIndex = expressionQueries.findIndex( - (query) => - query.model.type === ExpressionQueryType.threshold && query.refId === SimpleConditionIdentifier.thresholdId - ); - const thresholdExpression = expressionQueries.at(thresholdExpressionIndex); - const conditions = thresholdExpression?.model.conditions ?? []; - const thresholdIndexOk = singleReduceExpressionInInstantQuery - ? thresholdExpressionIndex === 0 - : thresholdExpressionIndex === 1; - const thresholdOk = thresholdExpression && thresholdIndexOk && conditions[0]?.unloadEvaluator === undefined; - return (Boolean(reduceOk) || Boolean(singleReduceExpressionInInstantQuery)) && Boolean(thresholdOk); + if (isInstantDataQuery && hasSingleThresholdExpression && validThresholdExpression && thresholdPointingToDataQuery) { + return true; + } + + return false; } export function isExpressionQueryInAlert( 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 e8939c0c336..5c13f97c954 100644 --- a/public/app/features/alerting/unified/utils/rule-form.test.ts +++ b/public/app/features/alerting/unified/utils/rule-form.test.ts @@ -1,10 +1,18 @@ import { PromQuery } from '@grafana/prometheus'; -import { GrafanaAlertStateDecision, GrafanaRuleDefinition, RulerAlertingRuleDTO } from 'app/types/unified-alerting-dto'; +import { + AlertDataQuery, + AlertQuery, + GrafanaAlertStateDecision, + GrafanaRuleDefinition, + RulerAlertingRuleDTO, +} from 'app/types/unified-alerting-dto'; +import { mockDataSource } from '../mocks'; import { getDefaultFormValues } from '../rule-editor/formDefaults'; +import { setupDataSources } from '../testSetup/datasources'; import { AlertManagerManualRouting, RuleFormType, RuleFormValues } from '../types/rule-form'; -import { GRAFANA_RULES_SOURCE_NAME } from './datasource'; +import { DataSourceType, GRAFANA_RULES_SOURCE_NAME } from './datasource'; import { alertingRulerRuleToRuleForm, cleanAnnotations, @@ -12,6 +20,7 @@ import { formValuesToRulerGrafanaRuleDTO, formValuesToRulerRuleDTO, getContactPointsFromDTO, + getInstantFromDataQuery, getNotificationSettingsForDTO, } from './rule-form'; @@ -254,3 +263,71 @@ describe('cleanLabels', () => { expect(output).toStrictEqual([{ key: 'key', value: '' }]); }); }); + +describe('getInstantFromDataQuery', () => { + const query: AlertQuery = { + refId: 'Q', + datasourceUid: 'abc123', + queryType: '', + relativeTimeRange: { + from: 600, + to: 0, + }, + model: { + refId: 'Q', + }, + }; + + it('should return undefined if datasource UID is undefined', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Prometheus, name: 'Mimir-cloud', uid: 'mimir-1' })); + const result = getInstantFromDataQuery({ ...query }); + expect(result).toBeUndefined(); + }); + + it('should return undefined if datasource type is not Prometheus or Loki', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Alertmanager, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ ...query, datasourceUid: 'aa' }); + expect(result).toBeUndefined(); + }); + + it('should return true if datasource is Prometheus and instant is not defined', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Prometheus, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ ...query, datasourceUid: 'aa' }); + + expect(result).toBe(true); + }); + + it('should return the value of instant if datasource is Prometheus and instant is defined', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Prometheus, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ ...query, datasourceUid: 'aa', model: { refId: 'f', instant: false } }); + expect(result).toBe(false); + }); + + it('should return true if datasource is Loki and queryType is not defined', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Loki, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ ...query, datasourceUid: 'aa' }); + expect(result).toBe(true); + }); + + it('should return true if datasource is Loki and queryType is instant', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Loki, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ + ...query, + datasourceUid: 'aa', + model: { refId: 'f', queryType: 'instant' }, + }); + + expect(result).toBe(true); + }); + + it('should return false if datasource is Loki and queryType is not instant', () => { + setupDataSources(mockDataSource({ type: DataSourceType.Loki, name: 'aa', uid: 'aa-1' })); + const result = getInstantFromDataQuery({ + ...query, + datasourceUid: 'aa', + model: { refId: 'f', queryType: 'range' }, + }); + + expect(result).toBe(false); + }); +}); diff --git a/public/app/features/alerting/unified/utils/rule-form.ts b/public/app/features/alerting/unified/utils/rule-form.ts index 194b7bec552..7845c5e228f 100644 --- a/public/app/features/alerting/unified/utils/rule-form.ts +++ b/public/app/features/alerting/unified/utils/rule-form.ts @@ -803,19 +803,22 @@ export function isPromOrLokiQuery(model: AlertDataQuery): model is PromOrLokiQue return 'expr' in model; } -export function getInstantFromDataQuery(model: AlertDataQuery, type: string): boolean | undefined { - // if the datasource is not prometheus or loki, instant is defined in the model or defaults to undefined - if (type !== DataSourceType.Prometheus && type !== DataSourceType.Loki) { - if ('instant' in model) { - return model.instant; - } else { - if ('queryType' in model) { - return model.queryType === 'instant'; - } else { - return undefined; - } - } +export function getInstantFromDataQuery(query: AlertQuery): boolean | undefined { + const dataSourceUID = query.datasourceUid ?? query.model.datasource?.uid; + if (!dataSourceUID) { + return undefined; } + + // find the datasource type from the UID + const type = getDataSourceSrv().getInstanceSettings(dataSourceUID)?.type; + + // if the datasource is not prometheus or loki, return "undefined" + if (type !== DataSourceType.Prometheus && type !== DataSourceType.Loki) { + return undefined; + } + + const { model } = query; + // if the datasource is prometheus or loki, instant is defined in the model, or defaults to true const isInstantForPrometheus = 'instant' in model && model.instant !== undefined ? model.instant : true; const isInstantForLoki = 'queryType' in model && model.queryType !== undefined ? model.queryType === 'instant' : true; diff --git a/public/app/features/expressions/utils/expressionTypes.ts b/public/app/features/expressions/utils/expressionTypes.ts index 514dfdcb907..dd648512b85 100644 --- a/public/app/features/expressions/utils/expressionTypes.ts +++ b/public/app/features/expressions/utils/expressionTypes.ts @@ -2,7 +2,7 @@ import { ReducerID } from '@grafana/data'; import { EvalFunction } from '../../alerting/state/alertDef'; import { isReducerType } from '../guards'; -import { ClassicCondition, ExpressionQuery, ExpressionQueryType, ReducerType } from '../types'; +import { ClassicCondition, ExpressionQuery, ExpressionQueryType, ReducerMode, ReducerType } from '../types'; export const getDefaults = (query: ExpressionQuery) => { switch (query.type) { @@ -69,3 +69,20 @@ export function getReducerType(value: string): ReducerType | undefined { } return undefined; } + +export function isStrictReducer(expressionModel: ExpressionQuery): boolean { + if (!isReducerExpression(expressionModel)) { + return false; + } + + const mode = expressionModel.settings?.mode; + return mode === ReducerMode.Strict || mode === undefined; +} + +export function isReducerExpression(expressionModel: ExpressionQuery) { + return expressionModel.type === ExpressionQueryType.reduce; +} + +export function isThresholdExpression(expressionModel: ExpressionQuery) { + return expressionModel.type === ExpressionQueryType.threshold; +}