Alerting: Fix incorrect timing meta information for policy (#73675)

public/app/features/alerting/unified/NotificationPolicies.tsx

@@ -17,11 +17,7 @@ import { useGetContactPointsState } from './api/receiversApi';
 import { AlertmanagerPageWrapper } from './components/AlertingPageWrapper';
 import { GrafanaAlertmanagerDeliveryWarning } from './components/GrafanaAlertmanagerDeliveryWarning';
 import { MuteTimingsTable } from './components/mute-timings/MuteTimingsTable';
-import {
+import { findRoutesMatchingPredicate, NotificationPoliciesFilter } from './components/notification-policies/Filters';
-  computeInheritedTree,
-  findRoutesMatchingPredicate,
-  NotificationPoliciesFilter,
-} from './components/notification-policies/Filters';
 import {
   useAddPolicyModal,
   useEditPolicyModal,
@@ -37,6 +33,7 @@ import { useRouteGroupsMatcher } from './useRouteGroupsMatcher';
 import { addUniqueIdentifierToRoute } from './utils/amroutes';
 import { isVanillaPrometheusAlertManagerDataSource } from './utils/datasource';
 import { normalizeMatchers } from './utils/matchers';
+import { computeInheritedTree } from './utils/notification-policies';
 import { initialAsyncRequestState } from './utils/redux';
 import { addRouteToParentRoute, mergePartialAmRouteWithRouteTree, omitRouteFromRouteTree } from './utils/routeTree';
 

public/app/features/alerting/unified/components/notification-policies/Filters.tsx

@@ -5,11 +5,10 @@ import React, { useCallback, useEffect, useRef } from 'react';
 import { SelectableValue } from '@grafana/data';
 import { Stack } from '@grafana/experimental';
 import { Button, Field, Icon, Input, Label as LabelElement, Select, Tooltip, useStyles2 } from '@grafana/ui';
-import { ObjectMatcher, Receiver, Route, RouteWithID } from 'app/plugins/datasource/alertmanager/types';
+import { ObjectMatcher, Receiver, RouteWithID } from 'app/plugins/datasource/alertmanager/types';
 
 import { useURLSearchParams } from '../../hooks/useURLSearchParams';
 import { matcherToObjectMatcher, parseMatchers } from '../../utils/alertmanager';
-import { getInheritedProperties } from '../../utils/notification-policies';
 
 interface NotificationPoliciesFilterProps {
   receivers: Receiver[];
@@ -129,23 +128,6 @@ export function findRoutesMatchingPredicate(routeTree: RouteWithID, predicateFn:
   return matches;
 }
 
-/**
- * This function will compute the full tree with inherited properties – this is mostly used for search and filtering
- */
-export function computeInheritedTree<T extends Route>(parent: T): T {
-  return {
-    ...parent,
-    routes: parent.routes?.map((child) => {
-      const inheritedProperties = getInheritedProperties(parent, child);
-
-      return computeInheritedTree({
-        ...child,
-        ...inheritedProperties,
-      });
-    }),
-  };
-}
-
 const toOption = (receiver: Receiver) => ({
   label: receiver.name,
   value: receiver.name,

public/app/features/alerting/unified/components/notification-policies/Policy.tsx

@@ -279,8 +279,11 @@ const Policy: FC<PolicyComponentProps> = ({
                     <MuteTimings timings={muteTimings} alertManagerSourceName={alertManagerSourceName} />
                   </MetaText>
                 )}
-                {timingOptions && Object.values(timingOptions).some(Boolean) && (
+                {timingOptions && (
-                  <TimingOptionsMeta timingOptions={timingOptions} />
+                  // for the default policy we will also merge the default timings, that way a user can observe what the timing options would be
+                  <TimingOptionsMeta
+                    timingOptions={isDefaultPolicy ? { ...timingOptions, ...TIMING_OPTIONS_DEFAULTS } : timingOptions}
+                  />
                 )}
                 {hasInheritedProperties && (
                   <>
@@ -441,28 +444,39 @@ const MuteTimings: FC<{ timings: string[]; alertManagerSourceName: string }> = (
 };
 
 const TimingOptionsMeta: FC<{ timingOptions: TimingOptions }> = ({ timingOptions }) => {
-  const groupWait = timingOptions.group_wait ?? TIMING_OPTIONS_DEFAULTS.group_wait;
+  const groupWait = timingOptions.group_wait;
-  const groupInterval = timingOptions.group_interval ?? TIMING_OPTIONS_DEFAULTS.group_interval;
+  const groupInterval = timingOptions.group_interval;
+
+  // we don't have any timing options to show – we're inheriting everything from the parent
+  // and those show up in a separate "inherited properties" component
+  if (!groupWait && !groupInterval) {
+    return null;
+  }
 
   return (
     <MetaText icon="hourglass" data-testid="timing-options">
       <span>Wait</span>
-      <Tooltip
+      {groupWait && (
-        placement="top"
+        <Tooltip
-        content="How long to initially wait to send a notification for a group of alert instances."
+          placement="top"
-      >
+          content="How long to initially wait to send a notification for a group of alert instances."
-        <span>
+        >
-          <Strong>{groupWait}</Strong> <span>to group instances</span>,
+          <span>
-        </span>
+            <Strong>{groupWait}</Strong> <span>to group instances</span>
-      </Tooltip>
+            {groupWait && groupInterval && ','}
-      <Tooltip
+          </span>
-        placement="top"
+        </Tooltip>
-        content="How long to wait before sending a notification about new alerts that are added to a group of alerts for which an initial notification has already been sent."
+      )}
-      >
+      {groupInterval && (
-        <span>
+        <Tooltip
-          <Strong>{groupInterval}</Strong> <span>before sending updates</span>
+          placement="top"
-        </span>
+          content="How long to wait before sending a notification about new alerts that are added to a group of alerts for which an initial notification has already been sent."
-      </Tooltip>
+        >
+          <span>
+            <Strong>{groupInterval}</Strong> <span>before sending updates</span>
+          </span>
+        </Tooltip>
+      )}
     </MetaText>
   );
 };

public/app/features/alerting/unified/components/rule-editor/notificaton-preview/useAlertmanagerNotificationRoutingPreview.ts

@@ -6,8 +6,7 @@ import { Labels } from '../../../../../../types/unified-alerting-dto';
 import { useAlertmanagerConfig } from '../../../hooks/useAlertmanagerConfig';
 import { useRouteGroupsMatcher } from '../../../useRouteGroupsMatcher';
 import { addUniqueIdentifierToRoute } from '../../../utils/amroutes';
-import { AlertInstanceMatch, normalizeRoute } from '../../../utils/notification-policies';
+import { AlertInstanceMatch, computeInheritedTree, normalizeRoute } from '../../../utils/notification-policies';
-import { computeInheritedTree } from '../../notification-policies/Filters';
 
 import { getRoutesByIdMap, RouteWithPath } from './route';
 

public/app/features/alerting/unified/utils/notification-policies.test.ts

@@ -1,6 +1,11 @@
 import { MatcherOperator, Route, RouteWithID } from 'app/plugins/datasource/alertmanager/types';
 
-import { findMatchingRoutes, normalizeRoute, getInheritedProperties } from './notification-policies';
+import {
+  findMatchingRoutes,
+  normalizeRoute,
+  getInheritedProperties,
+  computeInheritedTree,
+} from './notification-policies';
 
 import 'core-js/stable/structured-clone';
 
@@ -257,6 +262,84 @@ describe('getInheritedProperties()', () => {
       expect(childInherited).toHaveProperty('receiver', 'PARENT');
     });
   });
+
+  describe('timing options', () => {
+    it('should inherit timing options', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_wait: '1m',
+        group_interval: '2m',
+      };
+
+      const child: Route = {
+        repeat_interval: '999s',
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).toHaveProperty('group_wait', '1m');
+      expect(childInherited).toHaveProperty('group_interval', '2m');
+    });
+  });
+});
+
+describe('computeInheritedTree', () => {
+  it('should merge properties from parent', () => {
+    const parent: Route = {
+      receiver: 'PARENT',
+      group_wait: '1m',
+      group_interval: '2m',
+      repeat_interval: '3m',
+      routes: [
+        {
+          repeat_interval: '999s',
+        },
+      ],
+    };
+
+    const treeRoot = computeInheritedTree(parent);
+    expect(treeRoot).toHaveProperty('group_wait', '1m');
+    expect(treeRoot).toHaveProperty('group_interval', '2m');
+    expect(treeRoot).toHaveProperty('repeat_interval', '3m');
+
+    expect(treeRoot).toHaveProperty('routes.0.group_wait', '1m');
+    expect(treeRoot).toHaveProperty('routes.0.group_interval', '2m');
+    expect(treeRoot).toHaveProperty('routes.0.repeat_interval', '999s');
+  });
+
+  it('should not regress #73573', () => {
+    const parent: Route = {
+      routes: [
+        {
+          group_wait: '1m',
+          group_interval: '2m',
+          repeat_interval: '3m',
+          routes: [
+            {
+              group_wait: '10m',
+              group_interval: '20m',
+              repeat_interval: '30m',
+            },
+            {
+              repeat_interval: '999m',
+            },
+          ],
+        },
+      ],
+    };
+
+    const treeRoot = computeInheritedTree(parent);
+    expect(treeRoot).toHaveProperty('routes.0.group_wait', '1m');
+    expect(treeRoot).toHaveProperty('routes.0.group_interval', '2m');
+    expect(treeRoot).toHaveProperty('routes.0.repeat_interval', '3m');
+
+    expect(treeRoot).toHaveProperty('routes.0.routes.0.group_wait', '10m');
+    expect(treeRoot).toHaveProperty('routes.0.routes.0.group_interval', '20m');
+    expect(treeRoot).toHaveProperty('routes.0.routes.0.repeat_interval', '30m');
+
+    expect(treeRoot).toHaveProperty('routes.0.routes.1.group_wait', '1m');
+    expect(treeRoot).toHaveProperty('routes.0.routes.1.group_interval', '2m');
+    expect(treeRoot).toHaveProperty('routes.0.routes.1.repeat_interval', '999m');
+  });
 });
 
 describe('normalizeRoute', () => {

public/app/features/alerting/unified/utils/notification-policies.ts

@@ -232,4 +232,21 @@ function getInheritedProperties(
   return inherited;
 }
 
+/**
+ * This function will compute the full tree with inherited properties – this is mostly used for search and filtering
+ */
+export function computeInheritedTree<T extends Route>(parent: T): T {
+  return {
+    ...parent,
+    routes: parent.routes?.map((child) => {
+      const inheritedProperties = getInheritedProperties(parent, child);
+
+      return computeInheritedTree({
+        ...child,
+        ...inheritedProperties,
+      });
+    }),
+  };
+}
+
 export { findMatchingAlertGroups, findMatchingRoutes, getInheritedProperties };