From 91c78ce7843af4452083a768e38e578939abea15 Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Wed, 12 Oct 2022 09:08:34 +0200 Subject: [PATCH] Alerting: Alert list performance improvements (#56247) (#56742) Co-authored-by: Gilles De Mey (cherry picked from commit 5ddf7b85dfad52e16b43f7b32e555cefd8d2eb9b) --- .pa11yci-pr.conf.js | 2 +- .../features/alerting/unified/RuleList.tsx | 54 ++++++++------ .../unified/components/rules/CloudRules.tsx | 15 ++-- .../unified/components/rules/GrafanaRules.tsx | 19 +++-- .../components/rules/RuleListErrors.tsx | 24 +++++-- .../unified/components/rules/RulesTable.tsx | 8 +-- .../hooks/useCombinedRuleNamespaces.ts | 70 ++++++++++++------- .../alerting/unified/state/actions.ts | 17 ++--- 8 files changed, 130 insertions(+), 79 deletions(-) diff --git a/.pa11yci-pr.conf.js b/.pa11yci-pr.conf.js index a8beb923bc5..b44ca977eb8 100644 --- a/.pa11yci-pr.conf.js +++ b/.pa11yci-pr.conf.js @@ -101,7 +101,7 @@ var config = { rootElement: '.main-view', // the unified alerting promotion alert's content contrast is too low // see https://github.com/grafana/grafana/pull/41829 - threshold: 4, + threshold: 5, }, { url: '${HOST}/datasources', diff --git a/public/app/features/alerting/unified/RuleList.tsx b/public/app/features/alerting/unified/RuleList.tsx index fc3ee1c17e4..c243c988148 100644 --- a/public/app/features/alerting/unified/RuleList.tsx +++ b/public/app/features/alerting/unified/RuleList.tsx @@ -1,12 +1,15 @@ import { css } from '@emotion/css'; import React, { useEffect, useMemo, useState } from 'react'; import { useLocation } from 'react-router-dom'; +import { useAsyncFn, useInterval } from 'react-use'; import { GrafanaTheme2, urlUtil } from '@grafana/data'; import { Button, LinkButton, useStyles2, withErrorBoundary } from '@grafana/ui'; import { useQueryParams } from 'app/core/hooks/useQueryParams'; import { useDispatch } from 'app/types'; +import { CombinedRuleNamespace } from '../../../types/unified-alerting'; + import { AlertingPageWrapper } from './components/AlertingPageWrapper'; import { NoRulesSplash } from './components/rules/NoRulesCTA'; import { RuleListErrors } from './components/rules/RuleListErrors'; @@ -48,40 +51,45 @@ const RuleList = withErrorBoundary( const ViewComponent = VIEWS[view]; - // fetch rules, then poll every RULE_LIST_POLL_INTERVAL_MS - useEffect(() => { - dispatch(fetchAllPromAndRulerRulesAction()); - const interval = setInterval(() => dispatch(fetchAllPromAndRulerRulesAction()), RULE_LIST_POLL_INTERVAL_MS); - return () => { - clearInterval(interval); - }; - }, [dispatch]); - const promRuleRequests = useUnifiedAlertingSelector((state) => state.promRules); const rulerRuleRequests = useUnifiedAlertingSelector((state) => state.rulerRules); - const dispatched = rulesDataSourceNames.some( - (name) => promRuleRequests[name]?.dispatched || rulerRuleRequests[name]?.dispatched - ); const loading = rulesDataSourceNames.some( (name) => promRuleRequests[name]?.loading || rulerRuleRequests[name]?.loading ); - const haveResults = rulesDataSourceNames.some( - (name) => - (promRuleRequests[name]?.result?.length && !promRuleRequests[name]?.error) || - (Object.keys(rulerRuleRequests[name]?.result || {}).length && !rulerRuleRequests[name]?.error) + + const promRequests = Object.entries(promRuleRequests); + const allPromLoaded = promRequests.every( + ([_, state]) => state.dispatched && (state?.result !== undefined || state?.error !== undefined) ); + const allPromEmpty = promRequests.every(([_, state]) => state.dispatched && state?.result?.length === 0); - const showNewAlertSplash = dispatched && !loading && !haveResults; + // Trigger data refresh only when the RULE_LIST_POLL_INTERVAL_MS elapsed since the previous load FINISHED + const [_, fetchRules] = useAsyncFn(async () => { + if (!loading) { + await dispatch(fetchAllPromAndRulerRulesAction()); + } + }, [loading]); - const combinedNamespaces = useCombinedRuleNamespaces(); + // fetch rules, then poll every RULE_LIST_POLL_INTERVAL_MS + useEffect(() => { + dispatch(fetchAllPromAndRulerRulesAction()); + }, [dispatch]); + useInterval(fetchRules, RULE_LIST_POLL_INTERVAL_MS); + + // Show splash only when we loaded all of the data sources and none of them has alerts + const hasNoAlertRulesCreatedYet = allPromLoaded && allPromEmpty && promRequests.length > 0; + + const combinedNamespaces: CombinedRuleNamespace[] = useCombinedRuleNamespaces(); const filteredNamespaces = useFilteredRules(combinedNamespaces); return ( - + // We don't want to show the Loading... indicator for the whole page. + // We show separate indicators for Grafana-managed and Cloud rules + - {!showNewAlertSplash && ( + + {!hasNoAlertRulesCreatedYet && ( <> -
@@ -108,8 +116,8 @@ const RuleList = withErrorBoundary(
)} - {showNewAlertSplash && } - {haveResults && } + {hasNoAlertRulesCreatedYet && } + {!hasNoAlertRulesCreatedYet && } ); }, diff --git a/public/app/features/alerting/unified/components/rules/CloudRules.tsx b/public/app/features/alerting/unified/components/rules/CloudRules.tsx index 19a4ab98734..e4428669e41 100644 --- a/public/app/features/alerting/unified/components/rules/CloudRules.tsx +++ b/public/app/features/alerting/unified/components/rules/CloudRules.tsx @@ -3,7 +3,7 @@ import pluralize from 'pluralize'; import React, { FC, useMemo } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; -import { LoadingPlaceholder, Pagination, useStyles2 } from '@grafana/ui'; +import { LoadingPlaceholder, Pagination, Spinner, useStyles2 } from '@grafana/ui'; import { CombinedRuleNamespace } from 'app/types/unified-alerting'; import { DEFAULT_PER_PAGE_PAGINATION } from '../../../../../core/constants'; @@ -25,18 +25,20 @@ export const CloudRules: FC = ({ namespaces, expandAll }) => { const styles = useStyles2(getStyles); const dsConfigs = useUnifiedAlertingSelector((state) => state.dataSources); - const rules = useUnifiedAlertingSelector((state) => state.promRules); + const promRules = useUnifiedAlertingSelector((state) => state.promRules); const rulesDataSources = useMemo(getRulesDataSources, []); const groupsWithNamespaces = useCombinedGroupNamespace(namespaces); const dataSourcesLoading = useMemo( () => rulesDataSources.filter( - (ds) => isAsyncRequestStatePending(rules[ds.name]) || isAsyncRequestStatePending(dsConfigs[ds.name]) + (ds) => isAsyncRequestStatePending(promRules[ds.name]) || isAsyncRequestStatePending(dsConfigs[ds.name]) ), - [rules, dsConfigs, rulesDataSources] + [promRules, dsConfigs, rulesDataSources] ); + const hasSomeResults = rulesDataSources.some((ds) => promRules[ds.name]?.result?.length ?? 0 > 0); + const hasDataSourcesConfigured = rulesDataSources.length > 0; const hasDataSourcesLoading = dataSourcesLoading.length > 0; const hasNamespaces = namespaces.length > 0; @@ -75,6 +77,7 @@ export const CloudRules: FC = ({ namespaces, expandAll }) => { {!hasDataSourcesConfigured &&

There are no Prometheus or Loki data sources configured.

} {hasDataSourcesConfigured && !hasDataSourcesLoading && !hasNamespaces &&

No rules found.

} + {!hasSomeResults && hasDataSourcesLoading && } ({ wrapper: css` margin-bottom: ${theme.spacing(4)}; `, + spinner: css` + text-align: center; + padding: ${theme.spacing(2)}; + `, pagination: getPaginationStyles(theme), }); diff --git a/public/app/features/alerting/unified/components/rules/GrafanaRules.tsx b/public/app/features/alerting/unified/components/rules/GrafanaRules.tsx index 6f0d0265960..54d9e176ed9 100644 --- a/public/app/features/alerting/unified/components/rules/GrafanaRules.tsx +++ b/public/app/features/alerting/unified/components/rules/GrafanaRules.tsx @@ -2,7 +2,7 @@ import { css } from '@emotion/css'; import React, { FC } from 'react'; import { GrafanaTheme2 } from '@grafana/data'; -import { LoadingPlaceholder, Pagination, useStyles2 } from '@grafana/ui'; +import { LoadingPlaceholder, Pagination, Spinner, useStyles2 } from '@grafana/ui'; import { useQueryParams } from 'app/core/hooks/useQueryParams'; import { CombinedRuleNamespace } from 'app/types/unified-alerting'; @@ -26,9 +26,13 @@ export const GrafanaRules: FC = ({ namespaces, expandAll }) => { const styles = useStyles2(getStyles); const [queryParams] = useQueryParams(); - const { loading } = useUnifiedAlertingSelector( - (state) => state.promRules[GRAFANA_RULES_SOURCE_NAME] || initialAsyncRequestState - ); + const { prom, ruler } = useUnifiedAlertingSelector((state) => ({ + prom: state.promRules[GRAFANA_RULES_SOURCE_NAME] || initialAsyncRequestState, + ruler: state.rulerRules[GRAFANA_RULES_SOURCE_NAME] || initialAsyncRequestState, + })); + + const loading = prom.loading || ruler.loading; + const hasResult = !!prom.result || !!ruler.result; const wantsGroupedView = queryParams['view'] === 'grouped'; const namespacesFormat = wantsGroupedView ? namespaces : flattenGrafanaManagedRules(namespaces); @@ -57,7 +61,8 @@ export const GrafanaRules: FC = ({ namespaces, expandAll }) => { viewMode={wantsGroupedView ? 'grouped' : 'list'} /> ))} - {namespacesFormat?.length === 0 &&

No rules found.

} + {hasResult && namespacesFormat?.length === 0 &&

No rules found.

} + {!hasResult && loading && } ({ wrapper: css` margin-bottom: ${theme.spacing(4)}; `, + spinner: css` + text-align: center; + padding: ${theme.spacing(2)}; + `, pagination: getPaginationStyles(theme), }); diff --git a/public/app/features/alerting/unified/components/rules/RuleListErrors.tsx b/public/app/features/alerting/unified/components/rules/RuleListErrors.tsx index 6899ca489cc..c595014ac96 100644 --- a/public/app/features/alerting/unified/components/rules/RuleListErrors.tsx +++ b/public/app/features/alerting/unified/components/rules/RuleListErrors.tsx @@ -53,7 +53,10 @@ export function RuleListErrors(): ReactElement { result.push( <> Failed to load the data source configuration for{' '} - {dataSource.name}: {error.message || 'Unknown error.'} + + {dataSource.name} + + : {error.message || 'Unknown error.'} ); }); @@ -61,8 +64,11 @@ export function RuleListErrors(): ReactElement { promRequestErrors.forEach(({ dataSource, error }) => result.push( <> - Failed to load rules state from {dataSource.name}:{' '} - {error.message || 'Unknown error.'} + Failed to load rules state from{' '} + + {dataSource.name} + + : {error.message || 'Unknown error.'} ) ); @@ -70,14 +76,17 @@ export function RuleListErrors(): ReactElement { rulerRequestErrors.forEach(({ dataSource, error }) => result.push( <> - Failed to load rules config from {dataSource.name}:{' '} - {error.message || 'Unknown error.'} + Failed to load rules config from{' '} + + {dataSource.name} + + : {error.message || 'Unknown error.'} ) ); return result; - }, [dataSourceConfigRequests, promRuleRequests, rulerRuleRequests]); + }, [dataSourceConfigRequests, promRuleRequests, rulerRuleRequests, styles.dsLink]); return ( <> @@ -141,4 +150,7 @@ const getStyles = (theme: GrafanaTheme2) => ({ display: flex; justify-content: flex-end; `, + dsLink: css` + font-weight: ${theme.typography.fontWeightBold}; + `, }); diff --git a/public/app/features/alerting/unified/components/rules/RulesTable.tsx b/public/app/features/alerting/unified/components/rules/RulesTable.tsx index 050f7c519dd..a6771e9f47c 100644 --- a/public/app/features/alerting/unified/components/rules/RulesTable.tsx +++ b/public/app/features/alerting/unified/components/rules/RulesTable.tsx @@ -45,15 +45,9 @@ export const RulesTable: FC = ({ const wrapperClass = cx(styles.wrapper, className, { [styles.wrapperMargin]: showGuidelines }); const items = useMemo((): RuleTableItemProps[] => { - const seenKeys: string[] = []; return rules.map((rule, ruleIdx) => { - let key = JSON.stringify([rule.promRule?.type, rule.labels, rule.query, rule.name, rule.annotations]); - if (seenKeys.includes(key)) { - key += `-${ruleIdx}`; - } - seenKeys.push(key); return { - id: key, + id: `${rule.namespace.name}-${rule.group.name}-${rule.name}-${ruleIdx}`, data: rule, }; }); diff --git a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts index de6ec286687..6692bd2ba04 100644 --- a/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts +++ b/public/app/features/alerting/unified/hooks/useCombinedRuleNamespaces.ts @@ -1,3 +1,4 @@ +import { isEqual } from 'lodash'; import { useMemo, useRef } from 'react'; import { @@ -129,18 +130,29 @@ function addRulerGroupsToCombinedNamespace(namespace: CombinedRuleNamespace, gro } function addPromGroupsToCombinedNamespace(namespace: CombinedRuleNamespace, groups: RuleGroup[]): void { + const existingGroupsByName = new Map(); + namespace.groups.forEach((group) => existingGroupsByName.set(group.name, group)); + groups.forEach((group) => { - let combinedGroup = namespace.groups.find((g) => g.name === group.name); + let combinedGroup = existingGroupsByName.get(group.name); if (!combinedGroup) { combinedGroup = { name: group.name, rules: [], }; namespace.groups.push(combinedGroup); + existingGroupsByName.set(group.name, combinedGroup); } + const combinedRulesByName = new Map(); + combinedGroup!.rules.forEach((r) => { + // Prometheus rules do not have to be unique by name + const existingRule = combinedRulesByName.get(r.name); + existingRule ? existingRule.push(r) : combinedRulesByName.set(r.name, [r]); + }); + (group.rules ?? []).forEach((rule) => { - const existingRule = getExistingRuleInGroup(rule, combinedGroup!, namespace.rulesSource); + const existingRule = getExistingRuleInGroup(rule, combinedRulesByName, namespace.rulesSource); if (existingRule) { existingRule.promRule = rule; } else { @@ -201,39 +213,47 @@ function rulerRuleToCombinedRule( // find existing rule in group that matches the given prom rule function getExistingRuleInGroup( rule: Rule, - group: CombinedRuleGroup, + existingCombinedRulesMap: Map, rulesSource: RulesSource ): CombinedRule | undefined { + // Using Map of name-based rules is important performance optimization for the code below + // Otherwise we would perform find method multiple times on (possibly) thousands of rules + + const nameMatchingRules = existingCombinedRulesMap.get(rule.name); + if (!nameMatchingRules) { + return undefined; + } + if (isGrafanaRulesSource(rulesSource)) { // assume grafana groups have only the one rule. check name anyway because paranoid - return group!.rules.find((existingRule) => existingRule.name === rule.name); + return nameMatchingRules[0]; } - return ( - // try finding a rule that matches name, labels, annotations and query - group!.rules.find( - (existingRule) => !existingRule.promRule && isCombinedRuleEqualToPromRule(existingRule, rule, true) - ) ?? - // if that fails, try finding a rule that only matches name, labels and annotations. - // loki & prom can sometimes modify the query so it doesnt match, eg `2 > 1` becomes `1` - group!.rules.find( - (existingRule) => !existingRule.promRule && isCombinedRuleEqualToPromRule(existingRule, rule, false) - ) + + // try finding a rule that matches name, labels, annotations and query + const strictlyMatchingRule = nameMatchingRules.find( + (combinedRule) => !combinedRule.promRule && isCombinedRuleEqualToPromRule(combinedRule, rule, true) ); + if (strictlyMatchingRule) { + return strictlyMatchingRule; + } + + // if that fails, try finding a rule that only matches name, labels and annotations. + // loki & prom can sometimes modify the query so it doesnt match, eg `2 > 1` becomes `1` + const looselyMatchingRule = nameMatchingRules.find( + (combinedRule) => !combinedRule.promRule && isCombinedRuleEqualToPromRule(combinedRule, rule, false) + ); + if (looselyMatchingRule) { + return looselyMatchingRule; + } + + return undefined; } function isCombinedRuleEqualToPromRule(combinedRule: CombinedRule, rule: Rule, checkQuery = true): boolean { if (combinedRule.name === rule.name) { - return ( - JSON.stringify([ - checkQuery ? hashQuery(combinedRule.query) : '', - combinedRule.labels, - combinedRule.annotations, - ]) === - JSON.stringify([ - checkQuery ? hashQuery(rule.query) : '', - rule.labels || {}, - isAlertingRule(rule) ? rule.annotations || {} : {}, - ]) + return isEqual( + [checkQuery ? hashQuery(combinedRule.query) : '', combinedRule.labels, combinedRule.annotations], + [checkQuery ? hashQuery(rule.query) : '', rule.labels || {}, isAlertingRule(rule) ? rule.annotations || {} : {}] ); } return false; diff --git a/public/app/features/alerting/unified/state/actions.ts b/public/app/features/alerting/unified/state/actions.ts index 3043cfede1e..b8a842c78cd 100644 --- a/public/app/features/alerting/unified/state/actions.ts +++ b/public/app/features/alerting/unified/state/actions.ts @@ -289,9 +289,9 @@ export const fetchRulesSourceBuildInfoAction = createAsyncThunk( } ); -export function fetchAllPromAndRulerRulesAction(force = false): ThunkResult { +export function fetchAllPromAndRulerRulesAction(force = false): ThunkResult> { return async (dispatch, getStore) => { - return Promise.all( + await Promise.allSettled( getAllRulesSourceNames().map(async (rulesSourceName) => { await dispatch(fetchRulesSourceBuildInfoAction({ rulesSourceName })); @@ -302,12 +302,13 @@ export function fetchAllPromAndRulerRulesAction(force = false): ThunkResult