From c829535f1407741ea032272ed042db18b28a6f43 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Thu, 13 Jan 2022 10:48:13 +0100 Subject: [PATCH] Alerting: Fix the silence url's matcher parameters (#43898) * Split silence matchers parameter into a separate entry for each label * Unify the silence link creation * Remove duplicated matchers when parsing to/from query params * Add tests for matchers * Add a comment with a duplication removal explanation * Improve label duplication comment * Remove redundant code * Use uniqBy to simplify the code. Rename matchers parameter * Fix Silence test data --- .../alerting/unified/Silences.test.tsx | 7 ++-- .../components/alert-groups/AlertDetails.tsx | 8 ++--- .../rules/RuleDetailsActionButtons.tsx | 4 +-- .../components/silences/SilencesEditor.tsx | 27 +++++++------- .../unified/hooks/useURLSearchParams.ts | 8 +++++ .../alerting/unified/utils/matchers.test.ts | 36 +++++++++++++++++++ .../alerting/unified/utils/matchers.ts | 32 +++++++++-------- .../features/alerting/unified/utils/misc.ts | 27 +++++++++----- .../plugins/panel/alertGroups/AlertGroup.tsx | 7 ++-- 9 files changed, 104 insertions(+), 52 deletions(-) create mode 100644 public/app/features/alerting/unified/hooks/useURLSearchParams.ts create mode 100644 public/app/features/alerting/unified/utils/matchers.test.ts diff --git a/public/app/features/alerting/unified/Silences.test.tsx b/public/app/features/alerting/unified/Silences.test.tsx index 3b468dd0ed7..c87a92581a5 100644 --- a/public/app/features/alerting/unified/Silences.test.tsx +++ b/public/app/features/alerting/unified/Silences.test.tsx @@ -174,9 +174,10 @@ describe('Silence edit', () => { it( 'prefills the matchers field with matchers params', async () => { - renderSilences( - `${baseUrlPath}?matchers=${encodeURIComponent('foo=bar,bar=~ba.+,hello!=world,cluster!~us-central.*')}` - ); + const matchersParams = ['foo=bar', 'bar=~ba.+', 'hello!=world', 'cluster!~us-central.*']; + const matchersQueryString = matchersParams.map((matcher) => `matcher=${encodeURIComponent(matcher)}`).join('&'); + + renderSilences(`${baseUrlPath}?${matchersQueryString}`); await waitFor(() => expect(ui.editor.durationField.query()).not.toBeNull()); const matchers = ui.editor.matchersField.queryAll(); diff --git a/public/app/features/alerting/unified/components/alert-groups/AlertDetails.tsx b/public/app/features/alerting/unified/components/alert-groups/AlertDetails.tsx index 8b16461f7d0..77e83600c5b 100644 --- a/public/app/features/alerting/unified/components/alert-groups/AlertDetails.tsx +++ b/public/app/features/alerting/unified/components/alert-groups/AlertDetails.tsx @@ -2,11 +2,9 @@ import { css } from '@emotion/css'; import { GrafanaTheme2 } from '@grafana/data'; import { LinkButton, useStyles2 } from '@grafana/ui'; import { AlertmanagerAlert, AlertState } from 'app/plugins/datasource/alertmanager/types'; - import React, { FC } from 'react'; -import { makeAMLink } from '../../utils/misc'; +import { makeAMLink, makeLabelBasedSilenceLink } from '../../utils/misc'; import { AnnotationDetailsField } from '../AnnotationDetailsField'; -import { getMatcherQueryParams } from '../../utils/matchers'; interface AmNotificationsAlertDetailsProps { alertManagerSourceName: string; @@ -33,9 +31,7 @@ export const AlertDetails: FC = ({ alert, aler )} {alert.status.state === AlertState.Active && ( = ({ rule, rulesSource }) => { key="silence" icon="bell-slash" target="__blank" - href={makeSilenceLink(alertmanagerSourceName, rule)} + href={makeRuleBasedSilenceLink(alertmanagerSourceName, rule)} > Silence diff --git a/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx b/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx index a2bb9c45aa5..e3d7b46499e 100644 --- a/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx +++ b/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx @@ -8,7 +8,6 @@ import { addDurationToDate, dateTime, isValidDate, - UrlQueryMap, GrafanaTheme2, } from '@grafana/data'; import { useDebounce } from 'react-use'; @@ -24,35 +23,34 @@ import { css, cx } from '@emotion/css'; import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelector'; import { makeAMLink } from '../../utils/misc'; import { useCleanup } from 'app/core/hooks/useCleanup'; -import { useQueryParams } from 'app/core/hooks/useQueryParams'; import { parseQueryParamMatchers } from '../../utils/matchers'; import { matcherToMatcherField, matcherFieldToMatcher } from '../../utils/alertmanager'; +import { useURLSearchParams } from '../../hooks/useURLSearchParams'; interface Props { silence?: Silence; alertManagerSourceName: string; } -const defaultsFromQuery = (queryParams: UrlQueryMap): Partial => { +const defaultsFromQuery = (searchParams: URLSearchParams): Partial => { const defaults: Partial = {}; - const { matchers, comment } = queryParams; + const comment = searchParams.get('comment'); + const matchers = searchParams.getAll('matcher'); - if (typeof matchers === 'string') { - const formMatchers = parseQueryParamMatchers(matchers); - if (formMatchers.length) { - defaults.matchers = formMatchers.map(matcherToMatcherField); - } + const formMatchers = parseQueryParamMatchers(matchers); + if (formMatchers.length) { + defaults.matchers = formMatchers.map(matcherToMatcherField); } - if (typeof comment === 'string') { + if (comment) { defaults.comment = comment; } return defaults; }; -const getDefaultFormValues = (queryParams: UrlQueryMap, silence?: Silence): SilenceFormFields => { +const getDefaultFormValues = (searchParams: URLSearchParams, silence?: Silence): SilenceFormFields => { const now = new Date(); if (silence) { const isExpired = Date.parse(silence.endsAt) < Date.now(); @@ -89,14 +87,15 @@ const getDefaultFormValues = (queryParams: UrlQueryMap, silence?: Silence): Sile matcherName: '', matcherValue: '', timeZone: DefaultTimeZone, - ...defaultsFromQuery(queryParams), + ...defaultsFromQuery(searchParams), }; } }; export const SilencesEditor: FC = ({ silence, alertManagerSourceName }) => { - const [queryParams] = useQueryParams(); - const defaultValues = useMemo(() => getDefaultFormValues(queryParams, silence), [silence, queryParams]); + const [urlSearchParams] = useURLSearchParams(); + + const defaultValues = useMemo(() => getDefaultFormValues(urlSearchParams, silence), [silence, urlSearchParams]); const formAPI = useForm({ defaultValues }); const dispatch = useDispatch(); const styles = useStyles2(getStyles); diff --git a/public/app/features/alerting/unified/hooks/useURLSearchParams.ts b/public/app/features/alerting/unified/hooks/useURLSearchParams.ts new file mode 100644 index 00000000000..4a9ecf08225 --- /dev/null +++ b/public/app/features/alerting/unified/hooks/useURLSearchParams.ts @@ -0,0 +1,8 @@ +import { useMemo } from 'react'; +import { useLocation } from 'react-router-dom'; + +export function useURLSearchParams(): [URLSearchParams] { + const { search } = useLocation(); + const queryParams = useMemo(() => new URLSearchParams(search), [search]); + return [queryParams]; +} diff --git a/public/app/features/alerting/unified/utils/matchers.test.ts b/public/app/features/alerting/unified/utils/matchers.test.ts new file mode 100644 index 00000000000..11cfe2f4d0f --- /dev/null +++ b/public/app/features/alerting/unified/utils/matchers.test.ts @@ -0,0 +1,36 @@ +import { getMatcherQueryParams, parseQueryParamMatchers } from './matchers'; + +describe('Unified Alerting matchers', () => { + describe('getMatcherQueryParams tests', () => { + it('Should create an entry for each label', () => { + const params = getMatcherQueryParams({ foo: 'bar', alertname: 'TestData - No data', rule_uid: 'YNZBpGJnk' }); + + const matcherParams = params.getAll('matcher'); + + expect(matcherParams).toHaveLength(3); + expect(matcherParams).toContain('foo=bar'); + expect(matcherParams).toContain('alertname=TestData - No data'); + expect(matcherParams).toContain('rule_uid=YNZBpGJnk'); + }); + }); + + describe('parseQueryParamMatchers tests', () => { + it('Should create a matcher for each unique label-expression pair', () => { + const matchers = parseQueryParamMatchers(['alertname=TestData 1', 'rule_uid=YNZBpGJnk']); + + expect(matchers).toHaveLength(2); + expect(matchers[0].name).toBe('alertname'); + expect(matchers[0].value).toBe('TestData 1'); + expect(matchers[1].name).toBe('rule_uid'); + expect(matchers[1].value).toBe('YNZBpGJnk'); + }); + + it('Should create one matcher, using the first occurence when duplicated labels exists', () => { + const matchers = parseQueryParamMatchers(['alertname=TestData 1', 'alertname=TestData 2']); + + expect(matchers).toHaveLength(1); + expect(matchers[0].name).toBe('alertname'); + expect(matchers[0].value).toBe('TestData 1'); + }); + }); +}); diff --git a/public/app/features/alerting/unified/utils/matchers.ts b/public/app/features/alerting/unified/utils/matchers.ts index fee8c2d53ef..ebef3ca3ed5 100644 --- a/public/app/features/alerting/unified/utils/matchers.ts +++ b/public/app/features/alerting/unified/utils/matchers.ts @@ -1,22 +1,26 @@ import { Matcher } from 'app/plugins/datasource/alertmanager/types'; import { Labels } from '@grafana/data'; import { parseMatcher } from './alertmanager'; +import { uniqBy } from 'lodash'; -// parses comma separated matchers like "foo=bar,baz=~bad*" into SilenceMatcher[] -export function parseQueryParamMatchers(paramValue: string): Matcher[] { - return paramValue - .split(',') - .filter((x) => !!x.trim()) - .map((x) => parseMatcher(x.trim())); +// Parses a list of entries like like "['foo=bar', 'baz=~bad*']" into SilenceMatcher[] +export function parseQueryParamMatchers(matcherPairs: string[]): Matcher[] { + const parsedMatchers = matcherPairs.filter((x) => !!x.trim()).map((x) => parseMatcher(x.trim())); + + // Due to migration, old alert rules might have a duplicated alertname label + // To handle that case want to filter out duplicates and make sure there are only unique labels + return uniqBy(parsedMatchers, (matcher) => matcher.name); } export const getMatcherQueryParams = (labels: Labels) => { - return `matchers=${encodeURIComponent( - Object.entries(labels) - .filter(([labelKey]) => !(labelKey.startsWith('__') && labelKey.endsWith('__'))) - .map(([labelKey, labelValue]) => { - return `${labelKey}=${labelValue}`; - }) - .join(',') - )}`; + const validMatcherLabels = Object.entries(labels).filter( + ([labelKey]) => !(labelKey.startsWith('__') && labelKey.endsWith('__')) + ); + + const matcherUrlParams = new URLSearchParams(); + validMatcherLabels.forEach(([labelKey, labelValue]) => + matcherUrlParams.append('matcher', `${labelKey}=${labelValue}`) + ); + + return matcherUrlParams; }; diff --git a/public/app/features/alerting/unified/utils/misc.ts b/public/app/features/alerting/unified/utils/misc.ts index 33741bad6e6..51615cf0e84 100644 --- a/public/app/features/alerting/unified/utils/misc.ts +++ b/public/app/features/alerting/unified/utils/misc.ts @@ -1,4 +1,4 @@ -import { urlUtil, UrlQueryMap } from '@grafana/data'; +import { urlUtil, UrlQueryMap, Labels } from '@grafana/data'; import { config } from '@grafana/runtime'; import { Alert, CombinedRule, FilterState, RulesSource, SilenceFilterState } from 'app/types/unified-alerting'; import { ALERTMANAGER_NAME_QUERY_KEY } from './constants'; @@ -8,6 +8,7 @@ import { SortOrder } from 'app/plugins/panel/alertlist/types'; import { alertInstanceKey } from 'app/features/alerting/unified/utils/rules'; import { sortBy } from 'lodash'; import { GrafanaAlertState, PromAlertingRuleState } from 'app/types/unified-alerting-dto'; +import { getMatcherQueryParams } from './matchers'; export function createViewLink(ruleSource: RulesSource, rule: CombinedRule, returnTo: string): string { const sourceName = getRulesSourceName(ruleSource); @@ -65,13 +66,23 @@ export function makeAMLink(path: string, alertManagerName?: string, options?: Re return `${path}?${search.toString()}`; } -export function makeSilenceLink(alertmanagerSourceName: string, rule: CombinedRule) { - return ( - `${config.appSubUrl}/alerting/silence/new?alertmanager=${alertmanagerSourceName}` + - `&matchers=alertname=${rule.name},${Object.entries(rule.labels) - .map(([key, value]) => encodeURIComponent(`${key}=${value}`)) - .join(',')}` - ); +export function makeRuleBasedSilenceLink(alertManagerSourceName: string, rule: CombinedRule) { + const labels: Labels = { + alertname: rule.name, + ...rule.labels, + }; + + return makeLabelBasedSilenceLink(alertManagerSourceName, labels); +} + +export function makeLabelBasedSilenceLink(alertManagerSourceName: string, labels: Labels) { + const silenceUrlParams = new URLSearchParams(); + silenceUrlParams.append('alertmanager', alertManagerSourceName); + + const matcherParams = getMatcherQueryParams(labels); + matcherParams.forEach((value, key) => silenceUrlParams.append(key, value)); + + return `${config.appSubUrl}/alerting/silence/new?${silenceUrlParams.toString()}`; } // keep retrying fn if it's error passes shouldRetry(error) and timeout has not elapsed yet diff --git a/public/app/plugins/panel/alertGroups/AlertGroup.tsx b/public/app/plugins/panel/alertGroups/AlertGroup.tsx index caf2727f287..04f94548cf0 100644 --- a/public/app/plugins/panel/alertGroups/AlertGroup.tsx +++ b/public/app/plugins/panel/alertGroups/AlertGroup.tsx @@ -8,8 +8,7 @@ import { AlertLabels } from 'app/features/alerting/unified/components/AlertLabel import { AlertGroupHeader } from 'app/features/alerting/unified/components/alert-groups/AlertGroupHeader'; import { CollapseToggle } from 'app/features/alerting/unified/components/CollapseToggle'; import { getNotificationsTextColors } from 'app/features/alerting/unified/styles/notifications'; -import { makeAMLink } from 'app/features/alerting/unified/utils/misc'; -import { getMatcherQueryParams } from 'app/features/alerting/unified/utils/matchers'; +import { makeAMLink, makeLabelBasedSilenceLink } from 'app/features/alerting/unified/utils/misc'; type Props = { alertManagerSourceName: string; @@ -68,9 +67,7 @@ export const AlertGroup = ({ alertManagerSourceName, group, expandAll }: Props) )} {alert.status.state === AlertState.Active && (