From a01a9034926c231fff1cba93585e22a9ea17217f Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Wed, 23 Aug 2023 17:12:24 +0200 Subject: [PATCH] Alerting: Fix incorrect timing meta information for policy (#73675) --- .../alerting/unified/NotificationPolicies.tsx | 7 +- .../notification-policies/Filters.tsx | 20 +---- .../notification-policies/Policy.tsx | 54 +++++++----- ...eAlertmanagerNotificationRoutingPreview.ts | 3 +- .../utils/notification-policies.test.ts | 85 ++++++++++++++++++- .../unified/utils/notification-policies.ts | 17 ++++ 6 files changed, 139 insertions(+), 47 deletions(-) diff --git a/public/app/features/alerting/unified/NotificationPolicies.tsx b/public/app/features/alerting/unified/NotificationPolicies.tsx index 822463db4e3..24b624f8c67 100644 --- a/public/app/features/alerting/unified/NotificationPolicies.tsx +++ b/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 { - computeInheritedTree, - findRoutesMatchingPredicate, - NotificationPoliciesFilter, -} from './components/notification-policies/Filters'; +import { 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'; diff --git a/public/app/features/alerting/unified/components/notification-policies/Filters.tsx b/public/app/features/alerting/unified/components/notification-policies/Filters.tsx index ed07c0e1e36..5457aa8f5cd 100644 --- a/public/app/features/alerting/unified/components/notification-policies/Filters.tsx +++ b/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(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, diff --git a/public/app/features/alerting/unified/components/notification-policies/Policy.tsx b/public/app/features/alerting/unified/components/notification-policies/Policy.tsx index 4466a13f86c..374ce66ad89 100644 --- a/public/app/features/alerting/unified/components/notification-policies/Policy.tsx +++ b/public/app/features/alerting/unified/components/notification-policies/Policy.tsx @@ -279,8 +279,11 @@ const Policy: FC = ({ )} - {timingOptions && Object.values(timingOptions).some(Boolean) && ( - + {timingOptions && ( + // for the default policy we will also merge the default timings, that way a user can observe what the timing options would be + )} {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 groupInterval = timingOptions.group_interval ?? TIMING_OPTIONS_DEFAULTS.group_interval; + const groupWait = timingOptions.group_wait; + 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 ( Wait - - - {groupWait} to group instances, - - - - - {groupInterval} before sending updates - - + {groupWait && ( + + + {groupWait} to group instances + {groupWait && groupInterval && ','} + + + )} + {groupInterval && ( + + + {groupInterval} before sending updates + + + )} ); }; diff --git a/public/app/features/alerting/unified/components/rule-editor/notificaton-preview/useAlertmanagerNotificationRoutingPreview.ts b/public/app/features/alerting/unified/components/rule-editor/notificaton-preview/useAlertmanagerNotificationRoutingPreview.ts index f0ead67585c..0762ae77aa2 100644 --- a/public/app/features/alerting/unified/components/rule-editor/notificaton-preview/useAlertmanagerNotificationRoutingPreview.ts +++ b/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 { computeInheritedTree } from '../../notification-policies/Filters'; +import { AlertInstanceMatch, computeInheritedTree, normalizeRoute } from '../../../utils/notification-policies'; import { getRoutesByIdMap, RouteWithPath } from './route'; diff --git a/public/app/features/alerting/unified/utils/notification-policies.test.ts b/public/app/features/alerting/unified/utils/notification-policies.test.ts index 9f31d88aff1..2c2e3514533 100644 --- a/public/app/features/alerting/unified/utils/notification-policies.test.ts +++ b/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', () => { diff --git a/public/app/features/alerting/unified/utils/notification-policies.ts b/public/app/features/alerting/unified/utils/notification-policies.ts index a42bcda480b..42f7c8951ba 100644 --- a/public/app/features/alerting/unified/utils/notification-policies.ts +++ b/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(parent: T): T { + return { + ...parent, + routes: parent.routes?.map((child) => { + const inheritedProperties = getInheritedProperties(parent, child); + + return computeInheritedTree({ + ...child, + ...inheritedProperties, + }); + }), + }; +} + export { findMatchingAlertGroups, findMatchingRoutes, getInheritedProperties };