From c8f87f441312fce27dd5f403256411f2404133d1 Mon Sep 17 00:00:00 2001 From: Sonia Aguilar <33540275+soniaAguilarPeiron@users.noreply.github.com> Date: Thu, 3 Nov 2022 12:50:32 +0100 Subject: [PATCH] Alerting: Improving group modal with validation on evaluation interval (#57830) * Show rules list for the group with the For duration, and add validation for keeping all rules in the same group with a valid For * Sort rules by For duration * Add number evaluations column in alert list * Add Error badge in column #evaluations in case of invalid For * Add test for getNumberEvaluationsToStartAlerting method * Move re-usable new InfoIcon component into a separate file in unified components folder * Add edge case for getNumberEvaluationsToStartAlerting method, and change some namings --- .../alerting/unified/RuleList.test.tsx | 34 +- .../alerting/unified/components/InfoIcon.tsx | 11 + .../rule-editor/GrafanaEvaluationBehavior.tsx | 2 +- .../components/rules/EditRuleGroupModal.tsx | 314 +++++++++++++++++- ...etNumberEvaluationsToStartAlerting.test.ts | 17 + .../alerting/unified/state/actions.ts | 48 ++- 6 files changed, 398 insertions(+), 28 deletions(-) create mode 100644 public/app/features/alerting/unified/components/InfoIcon.tsx create mode 100644 public/app/features/alerting/unified/components/rules/getNumberEvaluationsToStartAlerting.test.ts diff --git a/public/app/features/alerting/unified/RuleList.test.tsx b/public/app/features/alerting/unified/RuleList.test.tsx index 51b1ac818c30..2f5c36da86ed 100644 --- a/public/app/features/alerting/unified/RuleList.test.tsx +++ b/public/app/features/alerting/unified/RuleList.test.tsx @@ -4,11 +4,12 @@ import userEvent from '@testing-library/user-event'; import React from 'react'; import { Provider } from 'react-redux'; import { Router } from 'react-router-dom'; -import { byLabelText, byRole, byTestId, byText } from 'testing-library-selector'; +import { byRole, byTestId, byText } from 'testing-library-selector'; import { locationService, setDataSourceSrv, logInfo } from '@grafana/runtime'; import { contextSrv } from 'app/core/services/context_srv'; import * as ruleActionButtons from 'app/features/alerting/unified/components/rules/RuleActionsButtons'; +import * as actions from 'app/features/alerting/unified/state/actions'; import { configureStore } from 'app/store/configureStore'; import { AccessControlAction } from 'app/types'; import { PromAlertingRuleState, PromApplication } from 'app/types/unified-alerting-dto'; @@ -57,9 +58,11 @@ jest.mock('@grafana/runtime', () => { }); jest.spyOn(config, 'getAllDataSources'); +jest.spyOn(actions, 'rulesInSameGroupHaveInvalidFor').mockReturnValue([]); const mocks = { getAllDataSourcesMock: jest.mocked(config.getAllDataSources), + rulesInSameGroupHaveInvalidForMock: jest.mocked(actions.rulesInSameGroupHaveInvalidFor), api: { discoverFeatures: jest.mocked(discoverFeatures), @@ -121,9 +124,11 @@ const ui = { newRuleButton: byRole('link', { name: 'New alert rule' }), editGroupModal: { - namespaceInput: byLabelText('Namespace'), - ruleGroupInput: byLabelText('Rule group'), - intervalInput: byLabelText('Rule group evaluation interval'), + namespaceInput: byRole('textbox', { hidden: true, name: /namespace/i }), + ruleGroupInput: byRole('textbox', { name: 'Evaluation group', exact: true }), + intervalInput: byRole('textbox', { + name: /Rule group evaluation interval Evaluation interval should be smaller or equal to 'For' values for existing rules in this group./i, + }), saveButton: byRole('button', { name: /Save changes/ }), }, }; @@ -131,6 +136,7 @@ const ui = { describe('RuleList', () => { beforeEach(() => { contextSrv.isEditor = true; + mocks.rulesInSameGroupHaveInvalidForMock.mockReturnValue([]); }); afterEach(() => { @@ -553,9 +559,12 @@ describe('RuleList', () => { // open edit dialog await userEvent.click(ui.editCloudGroupIcon.get(groups[0])); - - expect(ui.editGroupModal.namespaceInput.get()).toHaveValue('namespace1'); - expect(ui.editGroupModal.ruleGroupInput.get()).toHaveValue('group1'); + await expect(screen.getByRole('textbox', { hidden: true, name: /namespace/i })).toHaveDisplayValue( + 'namespace1' + ); + await expect(screen.getByRole('textbox', { name: 'Evaluation group', exact: true })).toHaveDisplayValue( + 'group1' + ); await fn(); }); } @@ -603,9 +612,14 @@ describe('RuleList', () => { testCase('rename just the lotex group', async () => { // make changes to form - await userEvent.clear(ui.editGroupModal.ruleGroupInput.get()); - await userEvent.type(ui.editGroupModal.ruleGroupInput.get(), 'super group'); - await userEvent.type(ui.editGroupModal.intervalInput.get(), '5m'); + await userEvent.clear(screen.getByRole('textbox', { name: 'Evaluation group', exact: true })); + await userEvent.type(screen.getByRole('textbox', { name: 'Evaluation group', exact: true }), 'super group'); + await userEvent.type( + screen.getByRole('textbox', { + name: /rule group evaluation interval evaluation interval should be smaller or equal to 'for' values for existing rules in this group\./i, + }), + '5m' + ); // submit, check that appropriate calls were made await userEvent.click(ui.editGroupModal.saveButton.get()); diff --git a/public/app/features/alerting/unified/components/InfoIcon.tsx b/public/app/features/alerting/unified/components/InfoIcon.tsx new file mode 100644 index 000000000000..e4653b441ca7 --- /dev/null +++ b/public/app/features/alerting/unified/components/InfoIcon.tsx @@ -0,0 +1,11 @@ +import React from 'react'; + +import { Icon, Tooltip } from '@grafana/ui'; + +export function InfoIcon({ text }: { text: string }) { + return ( + {text}}> + + + ); +} diff --git a/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx b/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx index 031afb342104..5e8e590db6b8 100644 --- a/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx @@ -14,7 +14,7 @@ import { EvaluationIntervalLimitExceeded } from '../InvalidIntervalWarning'; import { GrafanaAlertStatePicker } from './GrafanaAlertStatePicker'; import { RuleEditorSection } from './RuleEditorSection'; -const MIN_TIME_RANGE_STEP_S = 10; // 10 seconds +export const MIN_TIME_RANGE_STEP_S = 10; // 10 seconds export const forValidationOptions = (evaluateEvery: string): RegisterOptions => ({ required: { diff --git a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx index deaf7b3cecf6..2dafb3caf120 100644 --- a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx +++ b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx @@ -1,18 +1,190 @@ import { css } from '@emotion/css'; import React, { useEffect, useMemo } from 'react'; +import { FormProvider, RegisterOptions, useForm, useFormContext } from 'react-hook-form'; -import { Modal, Button, Form, Field, Input, useStyles2 } from '@grafana/ui'; +import { GrafanaTheme2 } from '@grafana/data'; +import { Stack } from '@grafana/experimental'; +import { Modal, Button, Field, Input, useStyles2, Label, Badge } from '@grafana/ui'; +import { useAppNotification } from 'app/core/copy/appNotification'; import { useCleanup } from 'app/core/hooks/useCleanup'; import { useDispatch } from 'app/types'; import { CombinedRuleGroup, CombinedRuleNamespace } from 'app/types/unified-alerting'; +import { RulerRulesConfigDTO, RulerRuleGroupDTO, RulerRuleDTO } from 'app/types/unified-alerting-dto'; import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelector'; -import { updateLotexNamespaceAndGroupAction } from '../../state/actions'; +import { rulesInSameGroupHaveInvalidFor, updateLotexNamespaceAndGroupAction } from '../../state/actions'; import { checkEvaluationIntervalGlobalLimit } from '../../utils/config'; import { getRulesSourceName } from '../../utils/datasource'; import { initialAsyncRequestState } from '../../utils/redux'; +import { isAlertingRulerRule, isGrafanaRulerRule } from '../../utils/rules'; +import { parsePrometheusDuration } from '../../utils/time'; +import { DynamicTable, DynamicTableColumnProps, DynamicTableItemProps } from '../DynamicTable'; +import { InfoIcon } from '../InfoIcon'; import { EvaluationIntervalLimitExceeded } from '../InvalidIntervalWarning'; -import { evaluateEveryValidationOptions } from '../rule-editor/GrafanaEvaluationBehavior'; +import { MIN_TIME_RANGE_STEP_S } from '../rule-editor/GrafanaEvaluationBehavior'; + +const MINUTE = '1m'; +interface AlertInfo { + alertName: string; + forDuration: string; + evaluationsToFire: number; +} +function ForError({ message }: { message: string }) { + return ; +} + +export const getNumberEvaluationsToStartAlerting = (forDuration: string, currentEvaluation: string) => { + const evalNumberMs = safeParseDurationstr(currentEvaluation); + const forNumber = safeParseDurationstr(forDuration); + if (forNumber === 0 && evalNumberMs !== 0) { + return 1; + } + if (evalNumberMs === 0) { + return 0; + } else { + const evaluationsBeforeCeil = forNumber / evalNumberMs; + return evaluationsBeforeCeil < 1 ? 0 : Math.ceil(forNumber / evalNumberMs) + 1; + } +}; + +export const getAlertInfo = (alert: RulerRuleDTO, currentEvaluation: string): AlertInfo => { + const emptyAlert: AlertInfo = { + alertName: '', + forDuration: '0s', + evaluationsToFire: 0, + }; + if (isGrafanaRulerRule(alert)) { + return { + alertName: alert.grafana_alert.title, + forDuration: alert.for, + evaluationsToFire: getNumberEvaluationsToStartAlerting(alert.for, currentEvaluation), + }; + } + if (isAlertingRulerRule(alert)) { + return { + alertName: alert.alert, + forDuration: alert.for ?? '1m', + evaluationsToFire: getNumberEvaluationsToStartAlerting(alert.for ?? '1m', currentEvaluation), + }; + } + return emptyAlert; +}; +export const isValidEvaluation = (evaluation: string) => { + try { + const duration = parsePrometheusDuration(evaluation); + + if (duration < MIN_TIME_RANGE_STEP_S * 1000) { + return false; + } + + if (duration % (MIN_TIME_RANGE_STEP_S * 1000) !== 0) { + return false; + } + + return true; + } catch (error) { + return false; + } +}; + +export const getGroupFromRuler = ( + rulerRules: RulerRulesConfigDTO | null | undefined, + groupName: string, + folderName: string +) => { + const folderObj: Array> = rulerRules ? rulerRules[folderName] : []; + return folderObj?.find((rulerRuleGroup) => rulerRuleGroup.name === groupName); +}; + +export const getIntervalForGroup = ( + rulerRules: RulerRulesConfigDTO | null | undefined, + groupName: string, + folderName: string +) => { + const group = getGroupFromRuler(rulerRules, groupName, folderName); + const interval = group?.interval ?? MINUTE; + return interval; +}; + +export const safeParseDurationstr = (duration: string): number => { + try { + return parsePrometheusDuration(duration); + } catch (e) { + return 0; + } +}; + +type AlertsWithForTableColumnProps = DynamicTableColumnProps; +type AlertsWithForTableProps = DynamicTableItemProps; + +export const RulesForGroupTable = ({ + rulerRules, + groupName, + folderName, +}: { + rulerRules: RulerRulesConfigDTO | null | undefined; + groupName: string; + folderName: string; +}) => { + const styles = useStyles2(getStyles); + const group = getGroupFromRuler(rulerRules, groupName, folderName); + const rules: RulerRuleDTO[] = group?.rules ?? []; + + const { watch } = useFormContext(); + const currentInterval = watch('groupInterval'); + + const rows: AlertsWithForTableProps[] = rules + .slice() + .map((rule: RulerRuleDTO, index) => ({ + id: index, + data: getAlertInfo(rule, currentInterval), + })) + .sort( + (alert1, alert2) => safeParseDurationstr(alert1.data.forDuration) - safeParseDurationstr(alert2.data.forDuration) + ); + + const columns: AlertsWithForTableColumnProps[] = useMemo(() => { + return [ + { + id: 'alertName', + label: 'Alert', + renderCell: ({ data: { alertName } }) => { + return <>{alertName}; + }, + size: 0.6, + }, + { + id: 'for', + label: 'For', + renderCell: ({ data: { forDuration } }) => { + return <>{forDuration}; + }, + size: 0.2, + }, + { + id: 'numberEvaluations', + label: '#Evaluations', + renderCell: ({ data: { evaluationsToFire: numberEvaluations } }) => { + if (!isValidEvaluation(currentInterval)) { + return ; + } + if (numberEvaluations === 0) { + return ; + } else { + return <>{numberEvaluations}; + } + }, + size: 0.2, + }, + ]; + }, [currentInterval]); + + return ( +
+ +
+ ); +}; interface ModalProps { namespace: CombinedRuleNamespace; @@ -32,6 +204,7 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement { const dispatch = useDispatch(); const { loading, error, dispatched } = useUnifiedAlertingSelector((state) => state.updateLotexNamespaceAndGroup) ?? initialAsyncRequestState; + const notifyApp = useAppNotification(); const defaultValues = useMemo( (): FormValues => ({ @@ -64,18 +237,77 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement { ); }; + const formAPI = useForm({ + mode: 'onBlur', + defaultValues, + shouldFocusError: true, + }); + const { + handleSubmit, + register, + watch, + formState: { isDirty, errors }, + } = formAPI; + + const onInvalid = () => { + notifyApp.error('There are errors in the form. Correct the errors and retry.'); + }; + + const rulerRuleRequests = useUnifiedAlertingSelector((state) => state.rulerRules); + const groupfoldersForSource = rulerRuleRequests[getRulesSourceName(namespace.rulesSource)]; + + const evaluateEveryValidationOptions: RegisterOptions = { + required: { + value: true, + message: 'Required.', + }, + validate: (value: string) => { + try { + const duration = parsePrometheusDuration(value); + + if (duration < MIN_TIME_RANGE_STEP_S * 1000) { + return `Cannot be less than ${MIN_TIME_RANGE_STEP_S} seconds.`; + } + + if (duration % (MIN_TIME_RANGE_STEP_S * 1000) !== 0) { + return `Must be a multiple of ${MIN_TIME_RANGE_STEP_S} seconds.`; + } + if ( + rulesInSameGroupHaveInvalidFor(groupfoldersForSource.result, group.name, namespace.name, value).length === 0 + ) { + return true; + } else { + return `Invalid evaluation interval. Evaluation interval should be smaller or equal to 'For' values for existing rules in this group.`; + } + } catch (error) { + return error instanceof Error ? error.message : 'Failed to parse duration'; + } + }, + }; + return ( -
- {({ register, errors, formState: { isDirty }, watch }) => ( + + e.preventDefault()} key={JSON.stringify(defaultValues)}> <> - + + + NameSpace + + + + } + invalid={!!errors.namespaceName} + error={errors.namespaceName?.message} + > - + + + Evaluation group + + + + } + invalid={!!errors.groupName} + error={errors.groupName?.message} + > + + Rule group evaluation interval + + + + } invalid={!!errors.groupInterval} error={errors.groupInterval?.message} > @@ -102,9 +355,23 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement { {...register('groupInterval', evaluateEveryValidationOptions)} /> + {checkEvaluationIntervalGlobalLimit(watch('groupInterval')).exceedsLimit && ( )} + {rulerRuleRequests && ( + <> +
List of rules that belong to this group
+
+ #Evaluations column represents the number of evaluations needed before alert starts firing. +
+ + + )} - - )} - + +
); } -const getStyles = () => ({ +const getStyles = (theme: GrafanaTheme2) => ({ modal: css` max-width: 560px; `, + formInput: css` + width: 275px; + & + & { + margin-left: ${theme.spacing(3)}; + } + `, + tableWrapper: css` + margin-top: ${theme.spacing(2)}; + margin-bottom: ${theme.spacing(2)}; + height: 225px; + overflow: auto; + `, + evalRequiredLabel: css` + font-size: ${theme.typography.bodySmall.fontSize}; + `, }); diff --git a/public/app/features/alerting/unified/components/rules/getNumberEvaluationsToStartAlerting.test.ts b/public/app/features/alerting/unified/components/rules/getNumberEvaluationsToStartAlerting.test.ts new file mode 100644 index 000000000000..f056e741a907 --- /dev/null +++ b/public/app/features/alerting/unified/components/rules/getNumberEvaluationsToStartAlerting.test.ts @@ -0,0 +1,17 @@ +import { getNumberEvaluationsToStartAlerting } from './EditRuleGroupModal'; +describe('getNumberEvaluationsToStartAlerting method', () => { + it('should return 0 in case of invalid data', () => { + expect(getNumberEvaluationsToStartAlerting('sd', 'ksdh')).toBe(0); + expect(getNumberEvaluationsToStartAlerting('0s', '1dfa0m')).toBe(0); + }); + it('should return 1 in case of zero For and valid interval', () => { + expect(getNumberEvaluationsToStartAlerting('0s', '10m')).toBe(1); + }); + it('should return correct number in case of valid data', () => { + expect(getNumberEvaluationsToStartAlerting('1m', '10m')).toBe(0); + expect(getNumberEvaluationsToStartAlerting('10m', '10m')).toBe(2); + expect(getNumberEvaluationsToStartAlerting('18m', '10m')).toBe(3); + expect(getNumberEvaluationsToStartAlerting('1h41m', '10m')).toBe(12); + expect(getNumberEvaluationsToStartAlerting('101m', '10m')).toBe(12); + }); +}); diff --git a/public/app/features/alerting/unified/state/actions.ts b/public/app/features/alerting/unified/state/actions.ts index 45f2224546e2..b3c34b512c72 100644 --- a/public/app/features/alerting/unified/state/actions.ts +++ b/public/app/features/alerting/unified/state/actions.ts @@ -1,4 +1,4 @@ -import { createAsyncThunk } from '@reduxjs/toolkit'; +import { createAsyncThunk, AsyncThunk } from '@reduxjs/toolkit'; import { isEmpty } from 'lodash'; import { locationService } from '@grafana/runtime'; @@ -60,6 +60,7 @@ import { FetchRulerRulesFilter, setRulerRuleGroup, } from '../api/ruler'; +import { getAlertInfo, safeParseDurationstr, getGroupFromRuler } from '../components/rules/EditRuleGroupModal'; import { RuleFormType, RuleFormValues } from '../types/rule-form'; import { addDefaultsToAlertmanagerConfig, removeMuteTimingFromRoute } from '../utils/alertmanager'; import { @@ -752,8 +753,28 @@ interface UpdateNamespaceAndGroupOptions { groupInterval?: string; } +export const rulesInSameGroupHaveInvalidFor = ( + rulerRules: RulerRulesConfigDTO | null | undefined, + groupName: string, + folderName: string, + everyDuration: string +) => { + const group = getGroupFromRuler(rulerRules, groupName, folderName); + + const rulesSameGroup: RulerRuleDTO[] = group?.rules ?? []; + + return rulesSameGroup.filter((rule: RulerRuleDTO) => { + const { forDuration } = getAlertInfo(rule, everyDuration); + return safeParseDurationstr(forDuration) < safeParseDurationstr(everyDuration); + }); +}; + // allows renaming namespace, renaming group and changing group interval, all in one go -export const updateLotexNamespaceAndGroupAction = createAsyncThunk( +export const updateLotexNamespaceAndGroupAction: AsyncThunk< + void, + UpdateNamespaceAndGroupOptions, + { state: StoreState } +> = createAsyncThunk( 'unifiedalerting/updateLotexNamespaceAndGroup', async (options: UpdateNamespaceAndGroupOptions, thunkAPI): Promise => { return withAppEvents( @@ -790,8 +811,29 @@ export const updateLotexNamespaceAndGroupAction = createAsyncThunk( ) { throw new Error('Nothing changed.'); } - + // validation for new groupInterval + if (groupInterval !== existingGroup.interval) { + const storeState = thunkAPI.getState(); + const groupfoldersForSource = storeState?.unifiedAlerting.rulerRules[rulesSourceName]; + const notValidRules = rulesInSameGroupHaveInvalidFor( + groupfoldersForSource?.result, + groupName, + namespaceName, + groupInterval ?? '1m' + ); + if (notValidRules.length > 0) { + throw new Error( + `These alerts belonging to this group will have an invalid 'For' value: ${notValidRules + .map((rule) => { + const { alertName } = getAlertInfo(rule, groupInterval ?? ''); + return alertName; + }) + .join(',')}` + ); + } + } // if renaming namespace - make new copies of all groups, then delete old namespace + if (newNamespaceName !== namespaceName) { for (const group of rulesResult[namespaceName]) { await setRulerRuleGroup(