From 5faf5e48eabc8e519441d82073eb3238cfd852ef Mon Sep 17 00:00:00 2001 From: Virginia Cepeda Date: Tue, 18 Jul 2023 10:56:02 -0300 Subject: [PATCH] Alerting: Changes to evaluation group step (#71251) * Initial changes to evaluation group step * Add separate buttons for folder and eval group creation * Implement folder creation from a modal * Reset group upon folder creation * Implement creation of evaluation group in modal * Changes to evaluation group edit modal * Fix tests * Address review comments * Fix tests * Refactor to avoid passing AsyncRequestState as prop * Refactor to avoid passing AsyncRequestState as prop --- .../unified/RuleEditorExisting.test.tsx | 11 +- .../alerting/unified/RuleList.test.tsx | 11 +- .../components/rule-editor/FolderAndGroup.tsx | 443 +++++++++++++----- .../rule-editor/GrafanaEvaluationBehavior.tsx | 140 ++---- .../rules/EditRuleGroupModal.test.tsx | 2 +- .../components/rules/EditRuleGroupModal.tsx | 113 +++-- 6 files changed, 444 insertions(+), 276 deletions(-) diff --git a/public/app/features/alerting/unified/RuleEditorExisting.test.tsx b/public/app/features/alerting/unified/RuleEditorExisting.test.tsx index 97975680a55..fa85b62d952 100644 --- a/public/app/features/alerting/unified/RuleEditorExisting.test.tsx +++ b/public/app/features/alerting/unified/RuleEditorExisting.test.tsx @@ -1,4 +1,4 @@ -import { render, waitFor, screen, within, act } from '@testing-library/react'; +import { render, waitFor, screen, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { Route } from 'react-router-dom'; @@ -6,7 +6,6 @@ import { TestProvider } from 'test/helpers/TestProvider'; import { ui } from 'test/helpers/alertingRuleEditor'; import { locationService, setDataSourceSrv } from '@grafana/runtime'; -import { ADD_NEW_FOLER_OPTION } from 'app/core/components/Select/FolderPicker'; import { contextSrv } from 'app/core/services/context_srv'; import { DashboardSearchHit } from 'app/features/search/types'; import { GrafanaAlertStateDecision } from 'app/types/unified-alerting-dto'; @@ -184,14 +183,8 @@ describe('RuleEditor grafana managed rules', () => { await userEvent.click(ui.buttons.save.get()); await waitFor(() => expect(mocks.api.setRulerRuleGroup).toHaveBeenCalled()); - //check that '+ Add new' option is in folders drop down even if we don't have values - const emptyFolderInput = await ui.inputs.folderContainer.find(); mocks.searchFolders.mockResolvedValue([] as DashboardSearchHit[]); - await act(async () => { - renderRuleEditor(uid); - }); - await userEvent.click(within(emptyFolderInput).getByRole('combobox')); - expect(screen.getByText(ADD_NEW_FOLER_OPTION)).toBeInTheDocument(); + expect(screen.getByText('New folder')).toBeInTheDocument(); expect(mocks.api.setRulerRuleGroup).toHaveBeenCalledWith( { dataSourceName: GRAFANA_RULES_SOURCE_NAME, apiVersion: 'legacy' }, diff --git a/public/app/features/alerting/unified/RuleList.test.tsx b/public/app/features/alerting/unified/RuleList.test.tsx index 3ffda3e9d2e..5b8a50c1691 100644 --- a/public/app/features/alerting/unified/RuleList.test.tsx +++ b/public/app/features/alerting/unified/RuleList.test.tsx @@ -125,9 +125,9 @@ const ui = { namespaceInput: byRole('textbox', { name: /^Namespace/ }), ruleGroupInput: byRole('textbox', { name: /Evaluation group/ }), 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, + name: /Evaluation interval How often is the rule evaluated. Applies to every rule within the group./i, }), - saveButton: byRole('button', { name: /Save evaluation interval/ }), + saveButton: byRole('button', { name: /Save/ }), }, }; @@ -626,12 +626,7 @@ describe('RuleList', () => { // make changes to form await userEvent.clear(ui.editGroupModal.ruleGroupInput.get()); await userEvent.type(ui.editGroupModal.ruleGroupInput.get(), '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' - ); + await userEvent.type(ui.editGroupModal.intervalInput.get(), '5m'); // submit, check that appropriate calls were made await userEvent.click(ui.editGroupModal.saveButton.get()); diff --git a/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx b/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx index 92f2fdcc975..64bb430b056 100644 --- a/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx @@ -1,14 +1,16 @@ import { css } from '@emotion/css'; import { debounce, take, uniqueId } from 'lodash'; -import React, { useCallback, useEffect, useMemo } from 'react'; -import { useFormContext } from 'react-hook-form'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import { FormProvider, useForm, useFormContext } from 'react-hook-form'; -import { GrafanaTheme2, SelectableValue } from '@grafana/data'; -import { Stack } from '@grafana/experimental'; -import { AsyncSelect, Field, InputControl, Label, useStyles2 } from '@grafana/ui'; -import { contextSrv } from 'app/core/core'; +import { AppEvents, GrafanaTheme2, SelectableValue } from '@grafana/data'; +import { AsyncSelect, Button, Field, Input, InputControl, Label, Modal, useStyles2 } from '@grafana/ui'; +import appEvents from 'app/core/app_events'; +import { contextSrv } from 'app/core/services/context_srv'; +import { createFolder } from 'app/features/manage-dashboards/state/actions'; import { AccessControlAction, useDispatch } from 'app/types'; import { CombinedRuleGroup } from 'app/types/unified-alerting'; +import { RulerRulesConfigDTO } from 'app/types/unified-alerting-dto'; import { useCombinedRuleNamespaces } from '../../hooks/useCombinedRuleNamespaces'; import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelector'; @@ -17,10 +19,10 @@ import { RuleFormValues } from '../../types/rule-form'; import { GRAFANA_RULES_SOURCE_NAME } from '../../utils/datasource'; import { MINUTE } from '../../utils/rule-form'; import { isGrafanaRulerRule } from '../../utils/rules'; -import { InfoIcon } from '../InfoIcon'; import { ProvisioningBadge } from '../Provisioning'; +import { evaluateEveryValidationOptions } from '../rules/EditRuleGroupModal'; -import { Folder, RuleFolderPicker } from './RuleFolderPicker'; +import { containsSlashes, Folder, RuleFolderPicker } from './RuleFolderPicker'; import { checkForPathSeparator } from './util'; export const MAX_GROUP_RESULTS = 1000; @@ -67,7 +69,7 @@ const findGroupMatchingLabel = (group: SelectableValue, query: string) = return group.label?.toLowerCase().includes(query.toLowerCase()); }; -export function FolderAndGroup() { +export function FolderAndGroup({ groupfoldersForGrafana }: { groupfoldersForGrafana?: RulerRulesConfigDTO | null }) { const { formState: { errors }, watch, @@ -82,6 +84,24 @@ export function FolderAndGroup() { const { groupOptions, loading } = useGetGroupOptionsFromFolder(folder?.title ?? ''); + const [isCreatingFolder, setIsCreatingFolder] = useState(false); + const [isCreatingEvaluationGroup, setIsCreatingEvaluationGroup] = useState(false); + + const onOpenFolderCreationModal = () => setIsCreatingFolder(true); + const onOpenEvaluationGroupCreationModal = () => setIsCreatingEvaluationGroup(true); + + const handleFolderCreation = (folder: Folder) => { + resetGroup(); + setValue('folder', folder); + setIsCreatingFolder(false); + }; + + const handleEvalGroupCreation = (groupName: string, evaluationInterval: string) => { + setValue('group', groupName); + setValue('evaluateEvery', evaluationInterval); + setIsCreatingEvaluationGroup(false); + }; + const resetGroup = useCallback(() => { setValue('group', ''); }, [setValue]); @@ -102,116 +122,327 @@ export function FolderAndGroup() { return (
- - - Folder - + { + + Folder + + } + className={styles.formInput} + error={errors.folder?.message} + invalid={!!errors.folder?.message} + data-testid="folder-picker" + > + {(!isCreatingFolder && ( + ( + { + field.onChange({ title, uid }); + resetGroup(); + }} + /> + )} + name="folder" + rules={{ + required: { value: true, message: 'Select a folder' }, + validate: { + pathSeparator: (folder: Folder) => checkForPathSeparator(folder.title), + }, + }} /> - - + )) ||
Creating new folder...
} + } - className={styles.formInput} - error={errors.folder?.message} - invalid={!!errors.folder?.message} - data-testid="folder-picker" - > - ( - { - field.onChange({ title, uid }); - resetGroup(); - }} - /> - )} - name="folder" - rules={{ - required: { value: true, message: 'Select a folder' }, - validate: { - pathSeparator: (folder: Folder) => checkForPathSeparator(folder.title), - }, - }} - /> - - - ( - { - field.onChange(group.label ?? ''); - }} - isLoading={loading} - invalid={Boolean(folder) && !group && Boolean(fieldState.error)} - loadOptions={debouncedSearch} - cacheOptions - loadingMessage={'Loading groups...'} - defaultValue={defaultGroupValue} - defaultOptions={groupOptions} - getOptionLabel={(option: SelectableValue) => ( -
- {option.label} - {/* making the assumption here that it's provisioned when it's disabled, should probably change this */} - {option.isDisabled && ( - <> - {' '} - - - )} -
- )} - placeholder={'Evaluation group name'} - allowCustomValue - formatCreateLabel={(_) => '+ Add new '} - noOptionsMessage="Start typing to create evaluation group" - /> - )} - name="group" - control={control} - rules={{ - required: { value: true, message: 'Must enter a group name' }, - validate: { - pathSeparator: (group_: string) => checkForPathSeparator(group_), - }, - }} - /> - +
+ or + +
+ {isCreatingFolder && ( + setIsCreatingFolder(false)} /> + )} +
+ +
+ + ( + { + field.onChange(group.label ?? ''); + }} + isLoading={loading} + invalid={Boolean(folder) && !group && Boolean(fieldState.error)} + loadOptions={debouncedSearch} + cacheOptions + loadingMessage={'Loading groups...'} + defaultValue={defaultGroupValue} + defaultOptions={groupOptions} + getOptionLabel={(option: SelectableValue) => ( +
+ {option.label} + {/* making the assumption here that it's provisioned when it's disabled, should probably change this */} + {option.isDisabled && ( + <> + {' '} + + + )} +
+ )} + placeholder={'Select an evaluation group...'} + /> + )} + name="group" + control={control} + rules={{ + required: { value: true, message: 'Must enter a group name' }, + validate: { + pathSeparator: (group_: string) => checkForPathSeparator(group_), + }, + }} + /> + + +
+ or + +
+ {isCreatingEvaluationGroup && ( + setIsCreatingEvaluationGroup(false)} + groupfoldersForGrafana={groupfoldersForGrafana} + /> + )} +
); } + +function FolderCreationModal({ + onClose, + onCreate, +}: { + onClose: () => void; + onCreate: (folder: Folder) => void; +}): React.ReactElement { + const styles = useStyles2(getStyles); + + const [title, setTitle] = useState(''); + const onSubmit = async () => { + const newFolder = await createFolder({ title: title }); + if (!newFolder.uid) { + appEvents.emit(AppEvents.alertError, ['Folder could not be created']); + return; + } + + const folder: Folder = { title: newFolder.title, uid: newFolder.uid }; + onCreate(folder); + appEvents.emit(AppEvents.alertSuccess, ['Folder Created', 'OK']); + }; + + const error = containsSlashes(title); + + return ( + +
Create a new folder to store your rule
+ +
+ Folder name} + error={"The folder name can't contain slashes"} + invalid={error} + > + setTitle(e.currentTarget.value)} + className={styles.formInput} + /> + + + + + + +
+ + ); +} + +function EvaluationGroupCreationModal({ + onClose, + onCreate, + groupfoldersForGrafana, +}: { + onClose: () => void; + onCreate: (group: string, evaluationInterval: string) => void; + groupfoldersForGrafana?: RulerRulesConfigDTO | null; +}): React.ReactElement { + const styles = useStyles2(getStyles); + const onSubmit = () => { + onCreate(getValues('group'), getValues('evaluateEvery')); + }; + + const { watch } = useFormContext(); + + const evaluateEveryId = 'eval-every-input'; + const [groupName, folderName] = watch(['group', 'folder.title']); + + const groupRules = + (groupfoldersForGrafana && groupfoldersForGrafana[folderName]?.find((g) => g.name === groupName)?.rules) ?? []; + + const onCancel = () => { + onClose(); + }; + + const formAPI = useForm({ + defaultValues: { group: '', evaluateEvery: '' }, + mode: 'onChange', + shouldFocusError: true, + }); + + const { register, handleSubmit, formState, getValues } = formAPI; + + return ( + +
Create a new evaluation group to use for this alert rule.
+ + +
onSubmit())}> + Evaluation group name} + error={formState.errors.group?.message} + invalid={!!formState.errors.group} + > + + + + + Evaluation interval + + } + > + + + + + + +
+ + + ); +} + const getStyles = (theme: GrafanaTheme2) => ({ container: css` + margin-top: ${theme.spacing(1)}; display: flex; - flex-direction: row; + flex-direction: column; align-items: baseline; - max-width: ${theme.breakpoints.values.sm}px; + max-width: ${theme.breakpoints.values.lg}px; justify-content: space-between; `, - formInput: css` - width: 275px; + evaluationGroupsContainer: css` + width: 100%; + display: flex; + flex-direction: row; + gap: ${theme.spacing(2)}; + `, - & + & { - margin-left: ${theme.spacing(3)}; + addButton: css` + display: flex; + direction: row; + gap: ${theme.spacing(2)}; + line-height: 2; + margin-top: 35px; + `, + formInput: css` + max-width: ${theme.breakpoints.values.sm}px; + flex-grow: 1; + + label { + width: ${theme.breakpoints.values.sm}px; } `, + + modal: css` + width: ${theme.breakpoints.values.sm}px; + `, + + modalTitle: css` + color: ${theme.colors.text.secondary}; + margin-bottom: ${theme.spacing(2)}; + `, }); 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 0cf8f23b4ac..006fb4fbb36 100644 --- a/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx @@ -4,8 +4,7 @@ import { RegisterOptions, useFormContext } from 'react-hook-form'; import { GrafanaTheme2, SelectableValue } from '@grafana/data'; import { Stack } from '@grafana/experimental'; -import { Button, Field, Icon, InlineLabel, Input, InputControl, Switch, Tooltip, useStyles2 } from '@grafana/ui'; -import { RulerRulesConfigDTO } from 'app/types/unified-alerting-dto'; +import { Field, Icon, IconButton, Input, InputControl, Label, Switch, Tooltip, useStyles2 } from '@grafana/ui'; import { CombinedRuleGroup, CombinedRuleNamespace } from '../../../../../types/unified-alerting'; import { logInfo, LogMessages } from '../../Analytics'; @@ -13,13 +12,13 @@ import { useCombinedRuleNamespaces } from '../../hooks/useCombinedRuleNamespaces import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelector'; import { RuleFormValues } from '../../types/rule-form'; import { GRAFANA_RULES_SOURCE_NAME } from '../../utils/datasource'; -import { MINUTE } from '../../utils/rule-form'; import { parsePrometheusDuration } from '../../utils/time'; import { CollapseToggle } from '../CollapseToggle'; -import { EditCloudGroupModal, evaluateEveryValidationOptions } from '../rules/EditRuleGroupModal'; +import { EditCloudGroupModal } from '../rules/EditRuleGroupModal'; import { FolderAndGroup, useGetGroupOptionsFromFolder } from './FolderAndGroup'; import { GrafanaAlertStatePicker } from './GrafanaAlertStatePicker'; +import { NeedHelpInfo } from './NeedHelpInfo'; import { RuleEditorSection } from './RuleEditorSection'; export const MIN_TIME_RANGE_STEP_S = 10; // 10 seconds @@ -69,50 +68,6 @@ const useIsNewGroup = (folder: string, group: string) => { return !groupIsInGroupOptions(group); }; -export const EvaluateEveryNewGroup = ({ rules }: { rules: RulerRulesConfigDTO | null | undefined }) => { - const { - watch, - register, - formState: { errors }, - } = useFormContext(); - const styles = useStyles2(getStyles); - const evaluateEveryId = 'eval-every-input'; - const [groupName, folderName] = watch(['group', 'folder.title']); - - const groupRules = (rules && rules[folderName]?.find((g) => g.name === groupName)?.rules) ?? []; - - return ( - -
- - - Evaluate every - - - - - -
- - ); -}; - function FolderGroupAndEvaluationInterval({ evaluateEvery, setEvaluateEvery, @@ -121,7 +76,7 @@ function FolderGroupAndEvaluationInterval({ setEvaluateEvery: (value: string) => void; }) { const styles = useStyles2(getStyles); - const { watch, setValue } = useFormContext(); + const { watch, setValue, getValues } = useFormContext(); const [isEditingGroup, setIsEditingGroup] = useState(false); const [groupName, folderName] = watch(['group', 'folder.title']); @@ -138,9 +93,6 @@ function FolderGroupAndEvaluationInterval({ useEffect(() => { if (!isNewGroup && existingGroup?.interval) { setEvaluateEvery(existingGroup.interval); - } else { - setEvaluateEvery(MINUTE); - setValue('evaluateEvery', MINUTE); } }, [setEvaluateEvery, isNewGroup, setValue, existingGroup]); @@ -164,50 +116,36 @@ function FolderGroupAndEvaluationInterval({ return (
- + {folderName && isEditingGroup && ( closeEditGroupModal()} intervalEditOnly + hideFolder={true} /> )} {folderName && groupName && (
- {isNewGroup && groupName ? ( - - ) : ( - -
- {`Alert rules in the `} {groupName} group are evaluated every{' '} - {evaluateEvery}. -
- {!isNewGroup && ( -
- {`Evaluation group interval applies to every rule within a group. It overwrites intervals defined for existing alert rules.`} -
- )} - - )} + + {getValues('group') && getValues('evaluateEvery') && ( + + All rules in the selected group are evaluated every {evaluateEvery}.{' '} + {!isNewGroup && ( + + )} + + )} +
- - {!isNewGroup && ( -
- -
- )} -
)} @@ -226,14 +164,15 @@ function ForInput({ evaluateEvery }: { evaluateEvery: string }) { return ( - - for - + Pending period + + } className={styles.inlineField} error={errors.evaluateFor?.message} invalid={!!errors.evaluateFor?.message} @@ -245,6 +184,22 @@ function ForInput({ evaluateEvery }: { evaluateEvery: string }) { ); } +function getDescription() { + const textToRender = 'Define how the alert rule is evaluated.'; + const docsLink = 'https://grafana.com/docs/grafana/latest/alerting/fundamentals/alert-rules/rule-evaluation/'; + return ( + + {`${textToRender}`} + + + ); +} + export function GrafanaEvaluationBehavior({ evaluateEvery, setEvaluateEvery, @@ -263,7 +218,7 @@ export function GrafanaEvaluationBehavior({ return ( // TODO remove "and alert condition" for recording rules - + @@ -348,8 +303,7 @@ const getStyles = (theme: GrafanaTheme2) => ({ margin-right: ${theme.spacing(1)}; `, evaluationContainer: css` - background-color: ${theme.colors.background.secondary}; - padding: ${theme.spacing(2)}; + color: ${theme.colors.text.secondary}; max-width: ${theme.breakpoints.values.sm}px; font-size: ${theme.typography.size.sm}; `, diff --git a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx index 77631b55177..5d7137c0935 100644 --- a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx +++ b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx @@ -24,7 +24,7 @@ const ui = { input: { namespace: byLabelText(/^Folder|^Namespace/, { exact: true }), group: byLabelText(/Evaluation group/), - interval: byLabelText(/Rule group evaluation interval/), + interval: byLabelText(/Evaluation interval/), }, folderLink: byTitle(/Go to folder/), // without a href has the generic role table: byTestId('dynamic-table'), diff --git a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx index 87e5efbeb59..421720fc865 100644 --- a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx +++ b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx @@ -20,7 +20,6 @@ import { initialAsyncRequestState } from '../../utils/redux'; import { AlertInfo, getAlertInfo, isRecordingRulerRule } from '../../utils/rules'; import { parsePrometheusDuration, safeParseDurationstr } from '../../utils/time'; import { DynamicTable, DynamicTableColumnProps, DynamicTableItemProps } from '../DynamicTable'; -import { InfoIcon } from '../InfoIcon'; import { EvaluationIntervalLimitExceeded } from '../InvalidIntervalWarning'; import { MIN_TIME_RANGE_STEP_S } from '../rule-editor/GrafanaEvaluationBehavior'; @@ -160,6 +159,7 @@ export interface ModalProps { onClose: (saved?: boolean) => void; intervalEditOnly?: boolean; folderUrl?: string; + hideFolder?: boolean; } export function EditCloudGroupModal(props: ModalProps): React.ReactElement { @@ -234,54 +234,50 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement {
e.preventDefault()} key={JSON.stringify(defaultValues)}> <> - - {nameSpaceLabel} - - } - invalid={!!errors.namespaceName} - error={errors.namespaceName?.message} - > - - - {isGrafanaManagedGroup && props.folderUrl && ( - + {nameSpaceLabel} + + } + invalid={!!errors.namespaceName} + error={errors.namespaceName?.message} + > + + - )} - - + {isGrafanaManagedGroup && props.folderUrl && ( + + )} + + + )} - Evaluation group name - - } + label={} invalid={!!errors.groupName} error={errors.groupName?.message} > - - Rule group evaluation interval - - + Evaluation interval } invalid={!!errors.groupInterval} @@ -314,6 +307,18 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement { {checkEvaluationIntervalGlobalLimit(watch('groupInterval')).exceedsLimit && ( )} + + {!hasSomeNoRecordingRules &&
This group does not contain alert rules.
} + {hasSomeNoRecordingRules && ( + <> +
List of rules that belong to this group
+
+ #Eval column represents the number of evaluations needed before alert starts firing. +
+ + + )} +
- {!hasSomeNoRecordingRules &&
This group does not contain alert rules.
} - {hasSomeNoRecordingRules && ( - <> -
List of rules that belong to this group
-
- #Eval column represents the number of evaluations needed before alert starts firing. -
- - - )}