Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alerting: Adds support for timezones in mute timings #68813

Merged
merged 13 commits into from
May 25, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ muteTimes:
- times:
- start_time: '06:00'
end_time: '23:59'
location: 'UTC'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this needs some comment about what timezone syntax is acceptable?

weekdays: ['monday:wednesday', 'saturday', 'sunday']
months: ['1:3', 'may:august', 'december']
years: ['2020:2022', '2030']
Expand Down
17 changes: 9 additions & 8 deletions public/app/features/alerting/unified/MuteTimings.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { render, waitFor, fireEvent } from '@testing-library/react';
import { render, waitFor, fireEvent, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import React from 'react';
import { TestProvider } from 'test/helpers/TestProvider';
Expand Down Expand Up @@ -109,7 +109,7 @@ describe('Mute timings', () => {

it('creates a new mute timing', async () => {
disableRBAC();
await renderMuteTimings();
renderMuteTimings();

await waitFor(() => expect(mocks.api.fetchAlertManagerConfig).toHaveBeenCalled());
expect(ui.nameField.get()).toBeInTheDocument();
Expand Down Expand Up @@ -149,8 +149,8 @@ describe('Mute timings', () => {
});
});

it('prepoluates the form when editing a mute timing', async () => {
await renderMuteTimings(
it('prepopulates the form when editing a mute timing', async () => {
renderMuteTimings(
'/alerting/routes/mute-timing/edit' + `?muteName=${encodeURIComponent(muteTimeInterval.name)}`
);

Expand All @@ -161,12 +161,13 @@ describe('Mute timings', () => {

await userEvent.clear(ui.startsAt.getAll()?.[0]);
await userEvent.clear(ui.endsAt.getAll()?.[0]);
await userEvent.clear(ui.weekdays.get());
await userEvent.clear(ui.days.get());
await userEvent.clear(ui.months.get());
await userEvent.clear(ui.years.get());

await userEvent.type(ui.weekdays.get(), 'monday');
// await userEvent.type(ui.weekdays.get(), 'monday');
const monday = within(ui.weekdays.get()).getByText('Mon');
await userEvent.click(monday);
await userEvent.type(ui.days.get(), '-7:-1');
await userEvent.type(ui.months.get(), '3, 6, 9, 12');
await userEvent.type(ui.years.get(), '2021:2024');
Expand Down Expand Up @@ -215,7 +216,7 @@ describe('Mute timings', () => {
});

it('form is invalid with duplicate mute timing name', async () => {
await renderMuteTimings();
renderMuteTimings();

await waitFor(() => expect(mocks.api.fetchAlertManagerConfig).toHaveBeenCalled());
await waitFor(() => expect(ui.nameField.get()).toBeInTheDocument());
Expand All @@ -231,7 +232,7 @@ describe('Mute timings', () => {
});

it('replaces mute timings in routes when the mute timing name is changed', async () => {
await renderMuteTimings(
renderMuteTimings(
'/alerting/routes/mute-timing/edit' + `?muteName=${encodeURIComponent(muteTimeInterval.name)}`
);

Expand Down
18 changes: 12 additions & 6 deletions public/app/features/alerting/unified/MuteTimings.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { useCallback, useEffect } from 'react';
import { Route, Redirect, Switch } from 'react-router-dom';

import { Alert, LoadingPlaceholder } from '@grafana/ui';
import { Alert } from '@grafana/ui';
import { useQueryParams } from 'app/core/hooks/useQueryParams';
import { MuteTimeInterval } from 'app/plugins/datasource/alertmanager/types';
import { useDispatch } from 'app/types';
Expand Down Expand Up @@ -56,24 +56,30 @@ const MuteTimings = () => {

return (
<>
{loading && <LoadingPlaceholder text="Loading mute timing" />}
{error && !loading && (
{error && !loading && !result && (
<Alert severity="error" title={`Error loading Alertmanager config for ${alertManagerSourceName}`}>
{error.message || 'Unknown error.'}
</Alert>
)}
{result && !error && (
{!error && result && (
<Switch>
<Route exact path="/alerting/routes/mute-timing/new">
<MuteTimingForm />
<MuteTimingForm loading={loading} />
</Route>
<Route exact path="/alerting/routes/mute-timing/edit">
{() => {
if (queryParams['muteName']) {
const muteTiming = getMuteTimingByName(String(queryParams['muteName']));
const provenance = muteTiming?.provenance;

return <MuteTimingForm muteTiming={muteTiming} showError={!muteTiming} provenance={provenance} />;
return (
<MuteTimingForm
loading={loading}
muteTiming={muteTiming}
showError={!muteTiming && !loading}
provenance={provenance}
/>
);
}
return <Redirect to="/alerting/routes" />;
}}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { css } from '@emotion/css';
import React, { useMemo } from 'react';
import React, { useState } from 'react';
import { FormProvider, useForm } from 'react-hook-form';

import { GrafanaTheme2, NavModelItem } from '@grafana/data';
import { Alert, Button, Field, FieldSet, Input, LinkButton, useStyles2 } from '@grafana/ui';
import { Alert, Button, Field, FieldSet, Input, LinkButton, LoadingPlaceholder, useStyles2 } from '@grafana/ui';
import {
AlertmanagerConfig,
AlertManagerCortexConfig,
Expand All @@ -30,47 +30,49 @@ interface Props {
muteTiming?: MuteTimeInterval;
showError?: boolean;
provenance?: string;
loading?: boolean;
}

const useDefaultValues = (muteTiming?: MuteTimeInterval): MuteTimingFields => {
return useMemo(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff here looks a bit weird but removed the unnecessary useMemo

const defaultValues = {
name: '',
time_intervals: [defaultTimeInterval],
};
const defaultValues = {
name: '',
time_intervals: [defaultTimeInterval],
};

if (!muteTiming) {
return defaultValues;
}

const intervals = muteTiming.time_intervals.map((interval) => ({
times: interval.times ?? defaultTimeInterval.times,
weekdays: interval?.weekdays?.join(', ') ?? defaultTimeInterval.weekdays,
days_of_month: interval?.days_of_month?.join(', ') ?? defaultTimeInterval.days_of_month,
months: interval?.months?.join(', ') ?? defaultTimeInterval.months,
years: interval?.years?.join(', ') ?? defaultTimeInterval.years,
}));

return {
name: muteTiming.name,
time_intervals: intervals,
};
}, [muteTiming]);
if (!muteTiming) {
return defaultValues;
}

const intervals = muteTiming.time_intervals.map((interval) => ({
times: interval.times ?? defaultTimeInterval.times,
weekdays: interval.weekdays?.join(', ') ?? defaultTimeInterval.weekdays,
days_of_month: interval.days_of_month?.join(', ') ?? defaultTimeInterval.days_of_month,
months: interval.months?.join(', ') ?? defaultTimeInterval.months,
years: interval.years?.join(', ') ?? defaultTimeInterval.years,
location: interval.location ?? defaultTimeInterval.location,
}));

return {
name: muteTiming.name,
time_intervals: intervals,
};
};

const defaultPageNav: Partial<NavModelItem> = {
icon: 'sitemap',
};

const MuteTimingForm = ({ muteTiming, showError, provenance }: Props) => {
const MuteTimingForm = ({ muteTiming, showError, loading, provenance }: Props) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loading is now being passed in to the form because for some reason the form contains the AlertingPageWrapper. We were showing multiple loaders before and one of them was even rendered entirely outside of the application.

const dispatch = useDispatch();
const alertManagers = useAlertManagersByPermission('notification');
const [alertManagerSourceName, setAlertManagerSourceName] = useAlertManagerSourceName(alertManagers);
const styles = useStyles2(getStyles);

const [updating, setUpdating] = useState(false);

const defaultAmCortexConfig = { alertmanager_config: {}, template_files: {} };
const amConfigs = useUnifiedAlertingSelector((state) => state.amConfigs);
const { result = defaultAmCortexConfig, loading } =
const { result = defaultAmCortexConfig } =
(alertManagerSourceName && amConfigs[alertManagerSourceName]) || initialAsyncRequestState;

const config: AlertmanagerConfig = result?.alertmanager_config ?? {};
Expand All @@ -96,15 +98,22 @@ const MuteTimingForm = ({ muteTiming, showError, provenance }: Props) => {
},
};

dispatch(
const saveAction = dispatch(
updateAlertManagerConfigAction({
newConfig,
oldConfig: result,
alertManagerSourceName: alertManagerSourceName!,
successMessage: 'Mute timing saved',
redirectPath: '/alerting/routes/',
redirectSearch: 'tab=mute_timings',
})
);

setUpdating(true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're disabling the form when the AM is still updating with changes – previously we immediately redirected and hoped it would have saved the changes.


saveAction.unwrap().finally(() => {
setUpdating(false);
});
};

return (
Expand All @@ -123,11 +132,12 @@ const MuteTimingForm = ({ muteTiming, showError, provenance }: Props) => {
dataSources={alertManagers}
/>
{provenance && <ProvisioningAlert resource={ProvisionedResource.MuteTiming} />}
{result && !loading && (
{loading && <LoadingPlaceholder text="Loading mute timing" />}
{showError && <Alert title="No matching mute timing found" />}
{result && !loading && !showError && (
<FormProvider {...formApi}>
<form onSubmit={formApi.handleSubmit(onSubmit)} data-testid="mute-timing-form">
{showError && <Alert title="No matching mute timing found" />}
<FieldSet label={'Create mute timing'} disabled={Boolean(provenance)}>
<FieldSet label={'Create mute timing'} disabled={Boolean(provenance) || updating}>
<Field
required
label="Name"
Expand All @@ -143,21 +153,23 @@ const MuteTimingForm = ({ muteTiming, showError, provenance }: Props) => {
const existingMuteTiming = config?.mute_time_intervals?.find(({ name }) => value === name);
return existingMuteTiming ? `Mute timing already exists for "${value}"` : true;
}
return value.length > 0 || 'Name is required';
return;
},
})}
className={styles.input}
data-testid={'mute-timing-name'}
/>
</Field>
<MuteTimingTimeInterval />
<Button type="submit" className={styles.submitButton}>
<Button type="submit" className={styles.submitButton} disabled={updating}>
Save mute timing
</Button>
<LinkButton
type="button"
variant="secondary"
href={makeAMLink('/alerting/routes/', alertManagerSourceName)}
fill="outline"
href={makeAMLink('/alerting/routes/', alertManagerSourceName, { tab: 'mute_timings' })}
disabled={updating}
>
Cancel
</LinkButton>
Expand Down
Loading