From 80387bdf8250d0e1891ca01c8e4e1c57cbdcdc59 Mon Sep 17 00:00:00 2001 From: Virginia Cepeda Date: Fri, 23 Jun 2023 14:15:24 -0300 Subject: [PATCH] Alerting: Display correct results when using different filters on alerting panels (#70482) * Trigger separate rules request for each alerting panel in a dashboard * Add RTK method to fetch prom rules * Use RTKQuery to get prom rules in UnifiedAlertList * Fix lint * Mock promRules call * Address PR comments * Fix tests --- .../alerting/unified/api/alertRuleApi.ts | 53 ++++++++- .../alerting/unified/api/prometheus.ts | 40 ++++--- .../hooks/useCombinedRuleNamespaces.ts | 14 ++- .../alerting/unified/mocks/alertRuleApi.ts | 8 +- .../panel/alertlist/UnifiedAlertList.tsx | 17 ++- .../panel/alertlist/UnifiedalertList.test.tsx | 108 ++++++++++++------ 6 files changed, 182 insertions(+), 58 deletions(-) diff --git a/public/app/features/alerting/unified/api/alertRuleApi.ts b/public/app/features/alerting/unified/api/alertRuleApi.ts index 836848e876c9..0caa567431b4 100644 --- a/public/app/features/alerting/unified/api/alertRuleApi.ts +++ b/public/app/features/alerting/unified/api/alertRuleApi.ts @@ -1,10 +1,26 @@ import { RelativeTimeRange } from '@grafana/data'; -import { AlertQuery, Annotations, GrafanaAlertStateDecision, Labels } from 'app/types/unified-alerting-dto'; +import { Matcher } from 'app/plugins/datasource/alertmanager/types'; +import { RuleIdentifier, RuleNamespace } from 'app/types/unified-alerting'; +import { + AlertQuery, + Annotations, + GrafanaAlertStateDecision, + Labels, + PromRulesResponse, +} from 'app/types/unified-alerting-dto'; import { Folder } from '../components/rule-editor/RuleFolderPicker'; +import { GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource'; import { arrayKeyValuesToObject } from '../utils/labels'; +import { isCloudRuleIdentifier, isPrometheusRuleIdentifier } from '../utils/rules'; import { alertingApi } from './alertingApi'; +import { + FetchPromRulesFilter, + groupRulesByFileName, + paramsWithMatcherAndState, + prepareRulesFilterQueryParams, +} from './prometheus'; export type ResponseLabels = { labels: AlertInstances[]; @@ -17,6 +33,8 @@ export interface Datasource { } export const PREVIEW_URL = '/api/v1/rule/test/grafana'; +export const PROM_RULES_URL = 'api/prometheus/grafana/api/v1/rules'; + export interface Data { refId: string; relativeTimeRange: RelativeTimeRange; @@ -76,5 +94,38 @@ export const alertRuleApi = alertingApi.injectEndpoints({ method: 'POST', }), }), + + prometheusRulesByNamespace: build.query< + RuleNamespace[], + { + limitAlerts?: number; + identifier?: RuleIdentifier; + filter?: FetchPromRulesFilter; + state?: string[]; + matcher?: Matcher[]; + } + >({ + query: ({ limitAlerts, identifier, filter, state, matcher }) => { + const searchParams = new URLSearchParams(); + + // if we're fetching for Grafana managed rules, we should add a limit to the number of alert instances + // we do this because the response is large otherwise and we don't show all of them in the UI anyway. + if (limitAlerts) { + searchParams.set('limit_alerts', String(limitAlerts)); + } + + if (identifier && (isPrometheusRuleIdentifier(identifier) || isCloudRuleIdentifier(identifier))) { + searchParams.set('file', identifier.namespace); + searchParams.set('rule_group', identifier.groupName); + } + + const params = prepareRulesFilterQueryParams(searchParams, filter); + + return { url: PROM_RULES_URL, params: paramsWithMatcherAndState(params, state, matcher) }; + }, + transformResponse: (response: PromRulesResponse): RuleNamespace[] => { + return groupRulesByFileName(response.data.groups, GRAFANA_RULES_SOURCE_NAME); + }, + }), }), }); diff --git a/public/app/features/alerting/unified/api/prometheus.ts b/public/app/features/alerting/unified/api/prometheus.ts index 2f7b8d05dbe8..75cd01f68c24 100644 --- a/public/app/features/alerting/unified/api/prometheus.ts +++ b/public/app/features/alerting/unified/api/prometheus.ts @@ -3,7 +3,7 @@ import { lastValueFrom } from 'rxjs'; import { getBackendSrv } from '@grafana/runtime'; import { Matcher } from 'app/plugins/datasource/alertmanager/types'; import { RuleIdentifier, RuleNamespace } from 'app/types/unified-alerting'; -import { PromRulesResponse } from 'app/types/unified-alerting-dto'; +import { PromRuleGroupDTO, PromRulesResponse } from 'app/types/unified-alerting-dto'; import { getDatasourceAPIUid, GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource'; import { isCloudRuleIdentifier, isPrometheusRuleIdentifier } from '../utils/rules'; @@ -83,6 +83,26 @@ export function paramsWithMatcherAndState( return paramsResult; } +export const groupRulesByFileName = (groups: PromRuleGroupDTO[], dataSourceName: string) => { + const nsMap: { [key: string]: RuleNamespace } = {}; + groups.forEach((group) => { + group.rules.forEach((rule) => { + rule.query = rule.query || ''; + }); + if (!nsMap[group.file]) { + nsMap[group.file] = { + dataSourceName, + name: group.file, + groups: [group], + }; + } else { + nsMap[group.file].groups.push(group); + } + }); + + return Object.values(nsMap); +}; + export async function fetchRules( dataSourceName: string, filter?: FetchPromRulesFilter, @@ -116,21 +136,5 @@ export async function fetchRules( throw e; }); - const nsMap: { [key: string]: RuleNamespace } = {}; - response.data.data.groups.forEach((group) => { - group.rules.forEach((rule) => { - rule.query = rule.query || ''; - }); - if (!nsMap[group.file]) { - nsMap[group.file] = { - dataSourceName, - name: group.file, - groups: [group], - }; - } else { - nsMap[group.file].groups.push(group); - } - }); - - return Object.values(nsMap); + return groupRulesByFileName(response.data.data.groups, dataSourceName); } diff --git a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts index 9a903261b265..f0663ffdf14c 100644 --- a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts +++ b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts @@ -24,6 +24,7 @@ import { import { getAllRulesSources, getRulesSourceByName, + GRAFANA_RULES_SOURCE_NAME, isCloudRulesSource, isGrafanaRulesSource, } from '../utils/datasource'; @@ -45,7 +46,10 @@ interface CacheValue { // this little monster combines prometheus rules and ruler rules to produce a unified data structure // can limit to a single rules source -export function useCombinedRuleNamespaces(rulesSourceName?: string): CombinedRuleNamespace[] { +export function useCombinedRuleNamespaces( + rulesSourceName?: string, + grafanaPromRuleNamespaces?: RuleNamespace[] +): CombinedRuleNamespace[] { const promRulesResponses = useUnifiedAlertingSelector((state) => state.promRules); const rulerRulesResponses = useUnifiedAlertingSelector((state) => state.rulerRules); @@ -67,9 +71,13 @@ export function useCombinedRuleNamespaces(rulesSourceName?: string): CombinedRul return rulesSources .map((rulesSource): CombinedRuleNamespace[] => { const rulesSourceName = isCloudRulesSource(rulesSource) ? rulesSource.name : rulesSource; - const promRules = promRulesResponses[rulesSourceName]?.result; const rulerRules = rulerRulesResponses[rulesSourceName]?.result; + let promRules = promRulesResponses[rulesSourceName]?.result; + if (rulesSourceName === GRAFANA_RULES_SOURCE_NAME && grafanaPromRuleNamespaces) { + promRules = grafanaPromRuleNamespaces; + } + const cached = cache.current[rulesSourceName]; if (cached && cached.promRules === promRules && cached.rulerRules === rulerRules) { return cached.result; @@ -104,7 +112,7 @@ export function useCombinedRuleNamespaces(rulesSourceName?: string): CombinedRul return result; }) .flat(); - }, [promRulesResponses, rulerRulesResponses, rulesSources]); + }, [promRulesResponses, rulerRulesResponses, rulesSources, grafanaPromRuleNamespaces]); } // merge all groups in case of grafana managed, essentially treating namespaces (folders) as groups diff --git a/public/app/features/alerting/unified/mocks/alertRuleApi.ts b/public/app/features/alerting/unified/mocks/alertRuleApi.ts index 8cab914111ec..7eb598b37fa4 100644 --- a/public/app/features/alerting/unified/mocks/alertRuleApi.ts +++ b/public/app/features/alerting/unified/mocks/alertRuleApi.ts @@ -1,8 +1,14 @@ import { rest } from 'msw'; import { SetupServer } from 'msw/node'; -import { PreviewResponse, PREVIEW_URL } from '../api/alertRuleApi'; +import { PromRulesResponse } from 'app/types/unified-alerting-dto'; + +import { PreviewResponse, PREVIEW_URL, PROM_RULES_URL } from '../api/alertRuleApi'; export function mockPreviewApiResponse(server: SetupServer, result: PreviewResponse) { server.use(rest.post(PREVIEW_URL, (req, res, ctx) => res(ctx.json(result)))); } + +export function mockPromRulesApiResponse(server: SetupServer, result: PromRulesResponse) { + server.use(rest.get(PROM_RULES_URL, (req, res, ctx) => res(ctx.json(result)))); +} diff --git a/public/app/plugins/panel/alertlist/UnifiedAlertList.tsx b/public/app/plugins/panel/alertlist/UnifiedAlertList.tsx index da19f2c8352e..93486f5be17f 100644 --- a/public/app/plugins/panel/alertlist/UnifiedAlertList.tsx +++ b/public/app/plugins/panel/alertlist/UnifiedAlertList.tsx @@ -18,6 +18,7 @@ import { import { config } from 'app/core/config'; import { contextSrv } from 'app/core/services/context_srv'; import alertDef from 'app/features/alerting/state/alertDef'; +import { alertRuleApi } from 'app/features/alerting/unified/api/alertRuleApi'; import { INSTANCES_DISPLAY_LIMIT } from 'app/features/alerting/unified/components/rules/RuleDetails'; import { useCombinedRuleNamespaces } from 'app/features/alerting/unified/hooks/useCombinedRuleNamespaces'; import { useUnifiedAlertingSelector } from 'app/features/alerting/unified/hooks/useUnifiedAlertingSelector'; @@ -60,6 +61,8 @@ export function UnifiedAlertList(props: PanelProps) { const rulesDataSourceNames = useMemo(getAllRulesSourceNames, []); const [limitInstances, toggleLimit] = useToggle(true); + const { usePrometheusRulesByNamespaceQuery } = alertRuleApi; + // backwards compat for "Inactive" state filter useEffect(() => { if (props.options.stateFilter.inactive === true) { @@ -153,9 +156,19 @@ export function UnifiedAlertList(props: PanelProps) { const promRulesRequests = useUnifiedAlertingSelector((state) => state.promRules); const rulerRulesRequests = useUnifiedAlertingSelector((state) => state.rulerRules); - const combinedRules = useCombinedRuleNamespaces(); const somePromRulesDispatched = rulesDataSourceNames.some((name) => promRulesRequests[name]?.dispatched); + + //For grafana managed rules, get the result using RTK Query to avoid the need of using the redux store + //See https://github.com/grafana/grafana/pull/70482 + const { currentData: promRules = [], isLoading: grafanaRulesLoading } = usePrometheusRulesByNamespaceQuery({ + limitAlerts: limitInstances ? INSTANCES_DISPLAY_LIMIT : undefined, + matcher: matcherList, + state: stateList, + }); + + const combinedRules = useCombinedRuleNamespaces(undefined, promRules); + const someRulerRulesDispatched = rulesDataSourceNames.some((name) => rulerRulesRequests[name]?.dispatched); const dispatched = somePromRulesDispatched || someRulerRulesDispatched; @@ -183,7 +196,7 @@ export function UnifiedAlertList(props: PanelProps) { return (
- {dispatched && loading && !haveResults && } + {(grafanaRulesLoading || (dispatched && loading && !haveResults)) && } {noAlertsMessage &&
{noAlertsMessage}
}
{props.options.viewMode === ViewMode.Stat && haveResults && ( diff --git a/public/app/plugins/panel/alertlist/UnifiedalertList.test.tsx b/public/app/plugins/panel/alertlist/UnifiedalertList.test.tsx index 1ff3972f11c7..b0c9cb8242cc 100644 --- a/public/app/plugins/panel/alertlist/UnifiedalertList.test.tsx +++ b/public/app/plugins/panel/alertlist/UnifiedalertList.test.tsx @@ -2,11 +2,17 @@ import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { Provider } from 'react-redux'; +import { act } from 'react-test-renderer'; import { byRole, byText } from 'testing-library-selector'; import { FieldConfigSource, getDefaultTimeRange, LoadingState, PanelProps } from '@grafana/data'; import { TimeRangeUpdatedEvent } from '@grafana/runtime'; +import { setupMswServer } from 'app/features/alerting/unified/mockApi'; +import { mockPromRulesApiResponse } from 'app/features/alerting/unified/mocks/alertRuleApi'; +import { mockRulerRulesApiResponse } from 'app/features/alerting/unified/mocks/rulerApi'; +import { Annotation } from 'app/features/alerting/unified/utils/constants'; import { DashboardSrv, setDashboardSrv } from 'app/features/dashboard/services/DashboardSrv'; +import { PromRuleGroupDTO, PromRulesResponse, RulerGrafanaRuleDTO } from 'app/types/unified-alerting-dto'; import { contextSrv } from '../../../core/services/context_srv'; import { @@ -14,6 +20,7 @@ import { mockPromAlertingRule, mockPromRuleGroup, mockPromRuleNamespace, + mockRulerGrafanaRule, mockUnifiedAlertingStore, } from '../../../features/alerting/unified/mocks'; import { GRAFANA_RULES_SOURCE_NAME } from '../../../features/alerting/unified/utils/datasource'; @@ -22,8 +29,55 @@ import { UnifiedAlertList } from './UnifiedAlertList'; import { GroupMode, SortOrder, UnifiedAlertListOptions, ViewMode } from './types'; import * as utils from './util'; +const grafanaRuleMock = { + promRules: { + grafana: { + loading: false, + dispatched: true, + result: [ + mockPromRuleNamespace({ + name: 'ns1', + groups: [ + mockPromRuleGroup({ + name: 'group1', + rules: [ + mockPromAlertingRule({ + name: 'rule1', + alerts: [mockPromAlert({ labels: { severity: 'critical' } })], + totals: { alerting: 1 }, + totalsFiltered: { alerting: 1 }, + }), + ], + }), + ], + }), + ], + }, + }, +}; + jest.mock('app/features/alerting/unified/api/alertmanager'); +const fakeResponse: PromRulesResponse = { + data: { groups: grafanaRuleMock.promRules.grafana.result[0].groups as PromRuleGroupDTO[] }, + status: 'success', +}; + +const server = setupMswServer(); + +mockPromRulesApiResponse(server, fakeResponse); +const originRule: RulerGrafanaRuleDTO = mockRulerGrafanaRule( + { + for: '1m', + labels: { severity: 'critical', region: 'nasa' }, + annotations: { [Annotation.summary]: 'This is a very important alert rule' }, + }, + { uid: 'grafana-rule-1', title: 'First Grafana Rule', data: [] } +); +mockRulerRulesApiResponse(server, 'grafana', { + 'folder-one': [{ name: 'group1', interval: '20s', rules: [originRule] }], +}); + const defaultOptions: UnifiedAlertListOptions = { maxItems: 2, sortOrder: SortOrder.AlphaAsc, @@ -73,32 +127,7 @@ const dashboard = { }; const renderPanel = (options: Partial = defaultOptions) => { - const store = mockUnifiedAlertingStore({ - promRules: { - grafana: { - loading: false, - dispatched: true, - result: [ - mockPromRuleNamespace({ - name: 'ns1', - groups: [ - mockPromRuleGroup({ - name: 'group1', - rules: [ - mockPromAlertingRule({ - name: 'rule1', - alerts: [mockPromAlert({ labels: { severity: 'critical' } })], - totals: { alerting: 1 }, - totalsFiltered: { alerting: 1 }, - }), - ], - }), - ], - }), - ], - }, - }, - }); + const store = mockUnifiedAlertingStore(grafanaRuleMock); const dashSrv: unknown = { getCurrent: () => dashboard }; setDashboardSrv(dashSrv as DashboardSrv); @@ -115,12 +144,19 @@ const renderPanel = (options: Partial = defaultOptions) describe('UnifiedAlertList', () => { it('subscribes to the dashboard refresh interval', async () => { jest.spyOn(defaultProps, 'replaceVariables').mockReturnValue('severity=critical'); - await renderPanel(); + + await act(async () => { + renderPanel(); + }); + expect(dashboard.events.subscribe).toHaveBeenCalledTimes(1); expect(dashboard.events.subscribe.mock.calls[0][0]).toEqual(TimeRangeUpdatedEvent); }); it('should replace option variables before filtering', async () => { + await waitFor(() => { + expect(screen.queryByText('Loading...')).not.toBeInTheDocument(); + }); jest.spyOn(contextSrv, 'hasPermission').mockReturnValue(true); const filterAlertsSpy = jest.spyOn(utils, 'filterAlerts'); @@ -128,12 +164,18 @@ describe('UnifiedAlertList', () => { const user = userEvent.setup(); - await renderPanel({ - alertInstanceLabelFilter: '$label', - dashboardAlerts: false, - alertName: '', - datasource: GRAFANA_RULES_SOURCE_NAME, - folder: undefined, + await act(async () => { + renderPanel({ + alertInstanceLabelFilter: '$label', + dashboardAlerts: false, + alertName: '', + datasource: GRAFANA_RULES_SOURCE_NAME, + folder: undefined, + }); + }); + + await waitFor(() => { + expect(screen.queryByText('Loading...')).not.toBeInTheDocument(); }); expect(byText('rule1').get()).toBeInTheDocument();