Alerting: Adds support for timezones in mute timings #68813

gillesdemey · 2023-05-22T12:15:42Z

What is this feature?

This PR adds several improvements to mute timings:

Support for timezones / locations
Better visual selector for week days
Improvements to loading / error handling
Better validation for timeranges

Which issue(s) does this PR fix?:

Fixes #48508

Special notes for your reviewer:

Still needs to write a few tests for this one, but it's ready for a functionality and code review.

Future improvements

While working on this I'm thinking the following improvements might be interesting;

Show how long until the next mute timing
Calculate how long the mute timing lasts
Show a calendar type of visualizations for a mute timing

gillesdemey · 2023-05-22T12:29:48Z

docs/sources/alerting/set-up/provision-alerting-resources/file-provisioning/index.md

@@ -631,6 +631,7 @@ muteTimes:
      - times:
          - start_time: '06:00'
            end_time: '23:59'
+            location: 'UTC'


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

gillesdemey · 2023-05-22T12:32:41Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingForm.tsx

 }

 const useDefaultValues = (muteTiming?: MuteTimeInterval): MuteTimingFields => {
-  return useMemo(() => {


Diff here looks a bit weird but removed the unnecessary useMemo

gillesdemey · 2023-05-22T12:34:24Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingForm.tsx

 };

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

-const MuteTimingForm = ({ muteTiming, showError, provenance }: Props) => {
+const MuteTimingForm = ({ muteTiming, showError, loading, provenance }: Props) => {


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.

gillesdemey · 2023-05-22T12:35:17Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingForm.tsx

      })
    );
+
+    setUpdating(true);


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.

gillesdemey · 2023-05-22T12:36:40Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingTimeInterval.tsx

-            </div>
-          );
-        })}
+        <Stack direction="column" gap={2}>


Another weird diff; this time because I'm wrapping the intervals in a <Stack /> instead of applying bottom margins. There are some important changes in here though so don't skip past this part of the review.

gillesdemey · 2023-05-22T12:38:26Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingTimeInterval.tsx

+                    })}
+                    width={50}
+                    placeholder="Example: 1:3, may:august, december"
+                    // @ts-ignore react-hook-form doesn't handle nested field arrays well


You'll see // @ts-ignore in a few places – this basically comes down to react-hook-form being sort of clueless about the types of nested fields when using useFieldArray<>()

gillesdemey · 2023-05-22T12:39:57Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingTimeInterval.tsx

+  return DAYS_OF_THE_WEEK.slice(startIndex, endIndex + 1);
+}
+
+const DaysOfTheWeek = ({ defaultValue = '', onChange }: DaysOfTheWeekProps) => {


These changes are for parsing the Alertmanager config for weekdays in to our visual weekday selector.

Valid config is for example monday, wednesday:friday sunday which translates to monday, wednesday, thursday, friday, sunday

gillesdemey · 2023-05-22T12:40:37Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingTimeRange.tsx

-    if (!timeString) {
-      return true;
-    }
-    const [hour, minutes] = timeString.split(':').map((x) => parseInt(x, 10));


Removed all of this in favor of a regular expression I've copied from the Alertmanager source. Previous implementation contained small bugs and would allow invalid syntax.

gillesdemey · 2023-05-22T12:42:46Z

public/app/features/alerting/unified/components/mute-timings/MuteTimingsTable.tsx

@@ -186,26 +183,6 @@ function useColumns(alertManagerSourceName: string, hideActions = false, setMute
  }, [alertManagerSourceName, setMuteTimingName, showActions, permissions]);
 }

-function renderTimeIntervals(timeIntervals: TimeInterval[]) {


moved to utils

gillesdemey · 2023-05-22T12:46:57Z

public/app/features/alerting/unified/state/actions.ts

  refetch?: boolean; // refetch config on success
 }

 export const updateAlertManagerConfigAction = createAsyncThunk<void, UpdateAlertManagerConfigActionOptions, {}>(
  'unifiedalerting/updateAMConfig',
-  ({ alertManagerSourceName, oldConfig, newConfig, successMessage, redirectPath, refetch }, thunkAPI): Promise<void> =>
+  ({ alertManagerSourceName, oldConfig, newConfig, successMessage, redirectPath, redirectSearch, refetch }, thunkAPI): Promise<void> =>


Added support for redirectSearch here so we don't lose the original search params when redirecting

brendamuir · 2023-05-22T14:55:03Z

Hi there-so we'd need to ask Deyan perhaps--but as a user I prefer the text to tell me what I have to enter in a field-so more actionable really - so "Enter a time inclusive xxxx" also I think we need full stops - it looks odd to me without - but again - not sure of Grafana rules on this exactly.

konrad147 · 2023-05-25T07:52:49Z

public/app/plugins/datasource/alertmanager/types.ts

@@ -315,6 +315,8 @@ export interface TimeInterval {
  days_of_month?: string[];
  months?: string[];
  years?: string[];
+  /** IANA TZ identifier like "Europe/Brussels", also supports "Local" or "UTC" */
+  location?: string;


So, location is a feature of Alertmanager?

Yep! prometheus/alertmanager#2782

konrad147 · 2023-05-25T07:53:29Z

public/app/features/alerting/unified/utils/alertmanager.ts

  return (
    'Times: ' +
-    (times ? times?.map(({ start_time, end_time }) => `${start_time} - ${end_time} UTC`).join(' and ') : 'All')
+    (times


Can we slightly refactor this method so it's more readable? Does it return the format required by Alertmanager?

Nay, this is just our presentation of a time string so we can do whatever we want here.

I had initially wanted to refactor the way we render mute timing, making it more visual and showing both the next mute time start and duration but decided it's probably best if I don't make this PR too large.

I'll refactor this function a bit though because it's turning in to quite the monstrosity

konrad147 · 2023-05-25T07:54:10Z

public/app/features/alerting/unified/utils/misc.ts

@@ -92,8 +92,10 @@ export function recordToArray(record: Record<string, string>): Array<{ key: stri
  return Object.entries(record).map(([key, value]) => ({ key, value }));
 }

-export function makeAMLink(path: string, alertManagerName?: string, options?: Record<string, string>): string {
+type URLParamsLike = ConstructorParameters<typeof URLSearchParams>[0];


konrad147 · 2023-05-25T07:54:54Z

public/app/features/alerting/unified/components/mute-timings/util.tsx

+  // @ts-ignore typescript types here incorrect, sigh
+  const startDate = moment().startOf('day').add(startTime, timeUnit);
+  // @ts-ignore typescript types here incorrect, sigh
+  const endDate = moment().startOf('day').add(endTime, timeUnit);


Maybe date-fns provides better types?

Unfortunately I didn't find a function in date-fns that can work with the HH:mm notation – I checked that library first before settling on moment :(

konrad147 · 2023-05-25T08:00:46Z

public/app/features/alerting/unified/components/mute-timings/timezones.tsx

+import { SelectableValue } from '@grafana/data';
+import { Select, SelectCommonProps } from '@grafana/ui';
+
+const TIMEZONES = [


You can take a look if Intl.supportedValuesOf('timeZone') could give you this list

Great idea! I was looking for it in the Inl namespace but didn't find that one – thanks!

konrad147

LGTM! Great work! 🎉

jagan985 · 2023-08-16T05:28:27Z

Is this live already? if yes, which version of Grafana is this pls?

grobinson-grafana · 2023-08-16T07:00:05Z

Hi! 👋 It will be in Grafana 10.1, scheduled to be released next week!

benoittgt · 2023-11-02T20:26:59Z

@gillesdemey Thanks a lot for this.

Isn't an issue that it is still written in "The time inclusive of the starting time and exclusive of the end time in UTC" ?

Also is it possible to write a time range with a midnight overlap? To avoid having writing this.

gillesdemey added 4 commits May 15, 2023 14:49

mute timing form update

b429a2f

WIP

d5e326a

Merge branch 'main' into alerting/mute-timings-timezone

bf16393

add support for timezones (location)

9c5285f

gillesdemey added area/alerting Grafana Alerting area/frontend add to changelog no-backport Skip backport of PR add to what's new labels May 22, 2023

gillesdemey added this to the 10.1.x milestone May 22, 2023

grafanabot added the type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project label May 22, 2023

gillesdemey added 2 commits May 22, 2023 14:21

revert InlineField

ecb3438

better typings

d81aeb9

gillesdemey commented May 22, 2023

View reviewed changes

small updates after self-PR

a981705

gillesdemey commented May 22, 2023

View reviewed changes

gillesdemey added 4 commits May 22, 2023 14:53

update mute timing summary

4c83583

fix test

52f37e7

update CTA for vanilla prometheus

b028ee7

add test for renderTimeIntervals

37864b5

gillesdemey marked this pull request as ready for review May 22, 2023 13:52

gillesdemey requested a review from a team as a code owner May 22, 2023 13:52

gillesdemey requested a review from a team May 22, 2023 13:52

gillesdemey requested a review from brendamuir as a code owner May 22, 2023 13:52

konrad147 reviewed May 25, 2023

View reviewed changes

prettier

64480ba

konrad147 approved these changes May 25, 2023

View reviewed changes

Merge branch 'main' into alerting/mute-timings-timezone

b55d937

gillesdemey merged commit 721d51a into main May 25, 2023

gillesdemey deleted the alerting/mute-timings-timezone branch May 25, 2023 14:19

ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023

chenrui333 mentioned this pull request Aug 24, 2023

grafana 10.1.0 Homebrew/homebrew-core#140321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Adds support for timezones in mute timings #68813

Alerting: Adds support for timezones in mute timings #68813

gillesdemey commented May 22, 2023 •

edited

Loading

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

gillesdemey May 22, 2023

brendamuir commented May 22, 2023

konrad147 May 25, 2023

gillesdemey May 25, 2023

konrad147 May 25, 2023

gillesdemey May 25, 2023

konrad147 May 25, 2023

konrad147 May 25, 2023

gillesdemey May 25, 2023

konrad147 May 25, 2023

gillesdemey May 25, 2023

konrad147 left a comment

jagan985 commented Aug 16, 2023

grobinson-grafana commented Aug 16, 2023

benoittgt commented Nov 2, 2023 •

edited

Loading

Alerting: Adds support for timezones in mute timings #68813

Alerting: Adds support for timezones in mute timings #68813

Conversation

gillesdemey commented May 22, 2023 • edited Loading

Future improvements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendamuir commented May 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konrad147 left a comment

Choose a reason for hiding this comment

jagan985 commented Aug 16, 2023

grobinson-grafana commented Aug 16, 2023

benoittgt commented Nov 2, 2023 • edited Loading

gillesdemey commented May 22, 2023 •

edited

Loading

benoittgt commented Nov 2, 2023 •

edited

Loading