Alerting: improve error presentation in forms (#34309)

2025-02-14 01:23:32 -06:00 · 2021-05-19 10:12:44 +03:00 · 2021-05-19 10:12:44 +03:00 · 89558f20bd
commit 89558f20bd
parent 1e8e7e34f1
9 changed files with 148 additions and 119 deletions
--- a/public/app/features/alerting/unified/AmRoutes.tsx
+++ b/public/app/features/alerting/unified/AmRoutes.tsx
@ -97,11 +97,6 @@ const AmRoutes: FC = () => {
  return (
    <AlertingPageWrapper pageId="am-routes">
      <AlertManagerPicker current={alertManagerSourceName} onChange={setAlertManagerSourceName} />
-      {savingError && !saving && (
-        <Alert severity="error" title="Error saving alert manager config">
-          {savingError.message || 'Unknown error.'}
-        </Alert>
-      )}
      {resultError && !resultLoading && (
        <Alert severity="error" title="Error loading alert manager config">
          {resultError.message || 'Unknown error.'}
--- a/public/app/features/alerting/unified/api/alertmanager.ts
+++ b/public/app/features/alerting/unified/api/alertmanager.ts
@ -71,10 +71,15 @@ export async function createOrUpdateSilence(
  alertmanagerSourceName: string,
  payload: SilenceCreatePayload
 ): Promise<Silence> {
-  const result = await getBackendSrv().post(
-    `/api/alertmanager/${getDatasourceAPIId(alertmanagerSourceName)}/api/v2/silences`,
-    payload
-  );
+  const result = await getBackendSrv()
+    .fetch<Silence>({
+      url: `/api/alertmanager/${getDatasourceAPIId(alertmanagerSourceName)}/api/v2/silences`,
+      data: payload,
+      showErrorAlert: false,
+      showSuccessAlert: false,
+      method: 'POST',
+    })
+    .toPromise();
  return result.data;
 }

--- a/public/app/features/alerting/unified/api/ruler.ts
+++ b/public/app/features/alerting/unified/api/ruler.ts
@ -51,11 +51,16 @@ export async function fetchRulerRulesGroup(
 }

 export async function deleteRulerRulesGroup(dataSourceName: string, namespace: string, groupName: string) {
-  return getBackendSrv().delete(
-    `/api/ruler/${getDatasourceAPIId(dataSourceName)}/api/v1/rules/${encodeURIComponent(
-      namespace
-    )}/${encodeURIComponent(groupName)}`
-  );
+  return getBackendSrv()
+    .fetch({
+      url: `/api/ruler/${getDatasourceAPIId(dataSourceName)}/api/v1/rules/${encodeURIComponent(
+        namespace
+      )}/${encodeURIComponent(groupName)}`,
+      method: 'DELETE',
+      showSuccessAlert: false,
+      showErrorAlert: false,
+    })
+    .toPromise();
 }

 // false in case ruler is not supported. this is weird, but we'll work on it
--- a/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx
+++ b/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx
@ -1,5 +1,5 @@
 import { css } from '@emotion/css';
-import { GrafanaTheme2 } from '@grafana/data';
+import { GrafanaTheme2, AppEvents } from '@grafana/data';
 import { Alert, Button, Field, Input, LinkButton, useStyles2 } from '@grafana/ui';
 import { useCleanup } from 'app/core/hooks/useCleanup';
 import { AlertManagerCortexConfig } from 'app/plugins/datasource/alertmanager/types';
@ -12,6 +12,7 @@ import { ChannelValues, CommonSettingsComponentType, ReceiverFormValues } from '
 import { makeAMLink } from '../../../utils/misc';
 import { ChannelSubForm } from './ChannelSubForm';
 import { DeletedSubForm } from './fields/DeletedSubform';
+import { appEvents } from 'app/core/core';

 interface Props<R extends ChannelValues> {
  config: AlertManagerCortexConfig;
@ -53,7 +54,7 @@ export function ReceiverForm<R extends ChannelValues>({

  useCleanup((state) => state.unifiedAlerting.saveAMConfig);

-  const { loading, error } = useUnifiedAlertingSelector((state) => state.saveAMConfig);
+  const { loading } = useUnifiedAlertingSelector((state) => state.saveAMConfig);

  const {
    handleSubmit,
@ -79,6 +80,10 @@ export function ReceiverForm<R extends ChannelValues>({
    });
  };

+  const onInvalid = () => {
+    appEvents.emit(AppEvents.alertError, ['There are errors in the form. Please correct them and try again!']);
+  };
+
  return (
    <FormProvider {...formAPI}>
      {!config.alertmanager_config.route && (
@ -86,13 +91,8 @@ export function ReceiverForm<R extends ChannelValues>({
          Because there is no default policy configured yet, this contact point will automatically be set as default.
        </Alert>
      )}
-      <form onSubmit={handleSubmit(submitCallback)}>
+      <form onSubmit={handleSubmit(submitCallback, onInvalid)}>
        <h4 className={styles.heading}>{initialValues ? 'Update contact point' : 'Create contact point'}</h4>
-        {error && (
-          <Alert severity="error" title="Error saving receiver">
-            {error.message || String(error)}
-          </Alert>
-        )}
        <Field label="Name" invalid={!!errors.name} error={errors.name && errors.name.message}>
          <Input
            id="name"
--- a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx
+++ b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx
@ -1,6 +1,6 @@
 import React, { FC, useMemo } from 'react';
-import { GrafanaTheme2 } from '@grafana/data';
-import { PageToolbar, Button, useStyles2, CustomScrollbar, Spinner, Alert } from '@grafana/ui';
+import { GrafanaTheme2, AppEvents } from '@grafana/data';
+import { PageToolbar, Button, useStyles2, CustomScrollbar, Spinner } from '@grafana/ui';
 import { css } from '@emotion/css';

 import { AlertTypeStep } from './AlertTypeStep';
@ -20,6 +20,8 @@ import { rulerRuleToFormValues, getDefaultFormValues, getDefaultQueries } from '
 import { Link } from 'react-router-dom';
 import { useQueryParams } from 'app/core/hooks/useQueryParams';

+import { appEvents } from 'app/core/core';
+
 type Props = {
  existing?: RuleWithLocation;
 };
@ -45,15 +47,10 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
  const formAPI = useForm<RuleFormValues>({
    mode: 'onSubmit',
    defaultValues,
+    shouldFocusError: true,
  });

-  const {
-    handleSubmit,
-    watch,
-    formState: { errors },
-  } = formAPI;
-
-  const hasErrors = !!Object.values(errors).filter((x) => !!x).length;
+  const { handleSubmit, watch } = formAPI;

  const type = watch('type');
  const dataSourceName = watch('dataSourceName');
@ -78,6 +75,10 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
    );
  };

+  const onInvalid = () => {
+    appEvents.emit(AppEvents.alertError, ['There are errors in the form. Please correct them and try again!']);
+  };
+
  return (
    <FormProvider {...formAPI}>
      <form onSubmit={(e) => e.preventDefault()} className={styles.form}>
@ -90,7 +91,7 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
          <Button
            variant="secondary"
            type="button"
-            onClick={handleSubmit((values) => submit(values, false))}
+            onClick={handleSubmit((values) => submit(values, false), onInvalid)}
            disabled={submitState.loading}
          >
            {submitState.loading && <Spinner className={styles.buttonSpinner} inline={true} />}
@ -99,7 +100,7 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
          <Button
            variant="primary"
            type="button"
-            onClick={handleSubmit((values) => submit(values, true))}
+            onClick={handleSubmit((values) => submit(values, true), onInvalid)}
            disabled={submitState.loading}
          >
            {submitState.loading && <Spinner className={styles.buttonSpinner} inline={true} />}
@ -109,17 +110,6 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
        <div className={styles.contentOuter}>
          <CustomScrollbar autoHeightMin="100%" hideHorizontalTrack={true}>
            <div className={styles.contentInner}>
-              {hasErrors && (
-                <Alert
-                  severity="error"
-                  title="There are errors in the form below. Please fix them and try saving again"
-                />
-              )}
-              {submitState.error && (
-                <Alert severity="error" title="Error saving rule">
-                  {submitState.error.message || (submitState.error as any)?.data?.message || String(submitState.error)}
-                </Alert>
-              )}
              <AlertTypeStep editingExistingRule={!!existing} />
              {showStep2 && (
                <>
--- a/public/app/features/alerting/unified/components/rules/RulesFilter.tsx
+++ b/public/app/features/alerting/unified/components/rules/RulesFilter.tsx
@ -70,6 +70,7 @@ const RulesFilter = () => {
              prefix={searchIcon}
              onChange={handleQueryStringChange}
              defaultValue={queryString}
+              placeholder="Search"
            />
          </div>
          <div className={styles.rowChild}>
--- a/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx
+++ b/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx
@ -1,6 +1,6 @@
 import { Silence, SilenceCreatePayload } from 'app/plugins/datasource/alertmanager/types';
 import React, { FC, useState } from 'react';
-import { Alert, Button, Field, FieldSet, Input, LinkButton, TextArea, useStyles } from '@grafana/ui';
+import { Button, Field, FieldSet, Input, LinkButton, TextArea, useStyles } from '@grafana/ui';
 import {
  DefaultTimeZone,
  GrafanaTheme,
@ -75,7 +75,7 @@ export const SilencesEditor: FC<Props> = ({ silence, alertManagerSourceName }) =
  const dispatch = useDispatch();
  const styles = useStyles(getStyles);

-  const { loading, error } = useUnifiedAlertingSelector((state) => state.updateSilence);
+  const { loading } = useUnifiedAlertingSelector((state) => state.updateSilence);

  useCleanup((state) => state.unifiedAlerting.updateSilence);

@ -138,11 +138,6 @@ export const SilencesEditor: FC<Props> = ({ silence, alertManagerSourceName }) =
    <FormProvider {...formAPI}>
      <form onSubmit={handleSubmit(onSubmit)}>
        <FieldSet label={`${silence ? 'Recreate silence' : 'Create silence'}`}>
-          {error && (
-            <Alert severity="error" title="Error saving silence">
-              {error.message || (error as any)?.data?.message || String(error)}
-            </Alert>
-          )}
          <div className={styles.flexRow}>
            <SilencePeriod />
            <Field
--- a/public/app/features/alerting/unified/state/actions.ts
+++ b/public/app/features/alerting/unified/state/actions.ts
@ -1,7 +1,5 @@
-import { AppEvents } from '@grafana/data';
 import { locationService } from '@grafana/runtime';
 import { createAsyncThunk } from '@reduxjs/toolkit';
-import { appEvents } from 'app/core/core';
 import {
  AlertmanagerAlert,
  AlertManagerCortexConfig,
@ -36,7 +34,7 @@ import {
 import { RuleFormType, RuleFormValues } from '../types/rule-form';
 import { getAllRulesSourceNames, GRAFANA_RULES_SOURCE_NAME, isGrafanaRulesSource } from '../utils/datasource';
 import { makeAMLink } from '../utils/misc';
-import { withSerializedError } from '../utils/redux';
+import { withAppEvents, withSerializedError } from '../utils/redux';
 import { formValuesToRulerAlertingRuleDTO, formValuesToRulerGrafanaRuleDTO } from '../utils/rule-form';
 import {
  getRuleIdentifier,
@ -169,14 +167,21 @@ export function deleteRuleAction(ruleIdentifier: RuleIdentifier): ThunkResult<vo
   * reload ruler rules
   */
  return async (dispatch) => {
-    const ruleWithLocation = await findExistingRule(ruleIdentifier);
-    if (!ruleWithLocation) {
-      throw new Error('Rule not found.');
-    }
-    await deleteRule(ruleWithLocation);
-    // refetch rules for this rules source
-    dispatch(fetchRulerRulesAction(ruleWithLocation.ruleSourceName));
-    dispatch(fetchPromRulesAction(ruleWithLocation.ruleSourceName));
+    withAppEvents(
+      (async () => {
+        const ruleWithLocation = await findExistingRule(ruleIdentifier);
+        if (!ruleWithLocation) {
+          throw new Error('Rule not found.');
+        }
+        await deleteRule(ruleWithLocation);
+        // refetch rules for this rules source
+        dispatch(fetchRulerRulesAction(ruleWithLocation.ruleSourceName));
+        dispatch(fetchPromRulesAction(ruleWithLocation.ruleSourceName));
+      })(),
+      {
+        successMessage: 'Rule deleted.',
+      }
+    );
  };
 }

@ -298,32 +303,35 @@ export const saveRuleFormAction = createAsyncThunk(
    existing?: RuleWithLocation;
    redirectOnSave?: string;
  }): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        const { type } = values;
-        // in case of cloud (cortex/loki)
-        let identifier: RuleIdentifier;
-        if (type === RuleFormType.cloud) {
-          identifier = await saveLotexRule(values, existing);
-          // in case of grafana managed
-        } else if (type === RuleFormType.grafana) {
-          identifier = await saveGrafanaRule(values, existing);
-        } else {
-          throw new Error('Unexpected rule form type');
-        }
-        if (redirectOnSave) {
-          locationService.push(redirectOnSave);
-        } else {
-          // redirect to edit page
-          const newLocation = `/alerting/${encodeURIComponent(stringifyRuleIdentifier(identifier))}/edit`;
-          if (locationService.getLocation().pathname !== newLocation) {
-            locationService.replace(newLocation);
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          const { type } = values;
+          // in case of system (cortex/loki)
+          let identifier: RuleIdentifier;
+          if (type === RuleFormType.cloud) {
+            identifier = await saveLotexRule(values, existing);
+            // in case of grafana managed
+          } else if (type === RuleFormType.grafana) {
+            identifier = await saveGrafanaRule(values, existing);
+          } else {
+            throw new Error('Unexpected rule form type');
          }
-        }
-        appEvents.emit(AppEvents.alertSuccess, [
-          existing ? `Rule "${values.name}" updated.` : `Rule "${values.name}" saved.`,
-        ]);
-      })()
+          if (redirectOnSave) {
+            locationService.push(redirectOnSave);
+          } else {
+            // redirect to edit page
+            const newLocation = `/alerting/${encodeURIComponent(stringifyRuleIdentifier(identifier))}/edit`;
+            if (locationService.getLocation().pathname !== newLocation) {
+              locationService.replace(newLocation);
+            }
+          }
+        })()
+      ),
+      {
+        successMessage: existing ? `Rule "${values.name}" updated.` : `Rule "${values.name}" saved.`,
+        errorMessage: 'Failed to save rule',
+      }
    )
 );

@ -344,25 +352,27 @@ interface UpdateAlertManagerConfigActionOptions {
 export const updateAlertManagerConfigAction = createAsyncThunk<void, UpdateAlertManagerConfigActionOptions, {}>(
  'unifiedalerting/updateAMConfig',
  ({ alertManagerSourceName, oldConfig, newConfig, successMessage, redirectPath, refetch }, thunkAPI): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        const latestConfig = await fetchAlertManagerConfig(alertManagerSourceName);
-        if (JSON.stringify(latestConfig) !== JSON.stringify(oldConfig)) {
-          throw new Error(
-            'It seems configuration has been recently updated. Please reload page and try again to make sure that recent changes are not overwritten.'
-          );
-        }
-        await updateAlertManagerConfig(alertManagerSourceName, addDefaultsToAlertmanagerConfig(newConfig));
-        if (successMessage) {
-          appEvents?.emit(AppEvents.alertSuccess, [successMessage]);
-        }
-        if (refetch) {
-          await thunkAPI.dispatch(fetchAlertManagerConfigAction(alertManagerSourceName));
-        }
-        if (redirectPath) {
-          locationService.push(makeAMLink(redirectPath, alertManagerSourceName));
-        }
-      })()
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          const latestConfig = await fetchAlertManagerConfig(alertManagerSourceName);
+          if (JSON.stringify(latestConfig) !== JSON.stringify(oldConfig)) {
+            throw new Error(
+              'It seems configuration has been recently updated. Please reload page and try again to make sure that recent changes are not overwritten.'
+            );
+          }
+          await updateAlertManagerConfig(alertManagerSourceName, addDefaultsToAlertmanagerConfig(newConfig));
+          if (refetch) {
+            await thunkAPI.dispatch(fetchAlertManagerConfigAction(alertManagerSourceName));
+          }
+          if (redirectPath) {
+            locationService.push(makeAMLink(redirectPath, alertManagerSourceName));
+          }
+        })()
+      ),
+      {
+        successMessage,
+      }
    )
 );

@ -374,7 +384,9 @@ export const fetchAmAlertsAction = createAsyncThunk(

 export const expireSilenceAction = (alertManagerSourceName: string, silenceId: string): ThunkResult<void> => {
  return async (dispatch) => {
-    await expireSilence(alertManagerSourceName, silenceId);
+    await withAppEvents(expireSilence(alertManagerSourceName, silenceId), {
+      successMessage: 'Silence expired.',
+    });
    dispatch(fetchSilencesAction(alertManagerSourceName));
    dispatch(fetchAmAlertsAction(alertManagerSourceName));
  };
@ -390,16 +402,18 @@ type UpdateSilenceActionOptions = {
 export const createOrUpdateSilenceAction = createAsyncThunk<void, UpdateSilenceActionOptions, {}>(
  'unifiedalerting/updateSilence',
  ({ alertManagerSourceName, payload, exitOnSave, successMessage }): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        await createOrUpdateSilence(alertManagerSourceName, payload);
-        if (successMessage) {
-          appEvents.emit(AppEvents.alertSuccess, [successMessage]);
-        }
-        if (exitOnSave) {
-          locationService.push('/alerting/silences');
-        }
-      })()
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          await createOrUpdateSilence(alertManagerSourceName, payload);
+          if (exitOnSave) {
+            locationService.push('/alerting/silences');
+          }
+        })()
+      ),
+      {
+        successMessage,
+      }
    )
 );

--- a/public/app/features/alerting/unified/utils/redux.ts
+++ b/public/app/features/alerting/unified/utils/redux.ts
@ -1,6 +1,8 @@
 import { AnyAction, AsyncThunk, createSlice, Draft, isAsyncThunkAction, SerializedError } from '@reduxjs/toolkit';
 import { FetchError } from '@grafana/runtime';
 import { isArray } from 'angular';
+import { appEvents } from 'app/core/core';
+import { AppEvents } from '@grafana/data';
 export interface AsyncRequestState<T> {
  result?: T;
  loading: boolean;
@ -106,14 +108,36 @@ export function withSerializedError<T>(p: Promise<T>): Promise<T> {
  });
 }

+export function withAppEvents<T>(
+  p: Promise<T>,
+  options: { successMessage?: string; errorMessage?: string }
+): Promise<T> {
+  return p
+    .then((v) => {
+      if (options.successMessage) {
+        appEvents.emit(AppEvents.alertSuccess, [options.successMessage]);
+      }
+      return v;
+    })
+    .catch((e) => {
+      const msg = messageFromError(e);
+      appEvents.emit(AppEvents.alertError, [`${options.errorMessage ?? 'Error'}: ${msg}`]);
+      throw e;
+    });
+}
+
 function isFetchError(e: unknown): e is FetchError {
  return typeof e === 'object' && e !== null && 'status' in e && 'data' in e;
 }

-function messageFromError(e: Error | FetchError): string {
+function messageFromError(e: Error | FetchError | SerializedError): string {
  if (isFetchError(e)) {
    if (e.data?.message) {
-      return e.data?.message;
+      let msg = e.data?.message;
+      if (typeof e.data?.error === 'string') {
+        msg += `; ${e.data.error}`;
+      }
+      return msg;
    } else if (isArray(e.data) && e.data.length && e.data[0]?.message) {
      return e.data
        .map((d) => d?.message)