From 6723159b12333efab889898fe831f19a7e78c4db Mon Sep 17 00:00:00 2001 From: Konrad Lalik Date: Mon, 10 Feb 2025 10:51:54 +0100 Subject: [PATCH] Alerting: Fix useAlertingQueryRunner re-rendering loop (#100206) * Fix AlertingQueryRunner infinite re-rendering loop * Update tests --- .../rule-editor/RecordingRuleEditor.tsx | 65 ++++++++++--------- .../components/rule-viewer/tabs/Query.tsx | 65 ++++++++----------- .../unified/state/AlertingQueryRunner.test.ts | 16 ++--- .../unified/state/AlertingQueryRunner.ts | 3 +- 4 files changed, 65 insertions(+), 84 deletions(-) diff --git a/public/app/features/alerting/unified/components/rule-editor/RecordingRuleEditor.tsx b/public/app/features/alerting/unified/components/rule-editor/RecordingRuleEditor.tsx index 7e29b573b54..73431a3f64b 100644 --- a/public/app/features/alerting/unified/components/rule-editor/RecordingRuleEditor.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/RecordingRuleEditor.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import { FC, useEffect, useState } from 'react'; +import { FC, useCallback, useEffect, useState } from 'react'; import { useAsync } from 'react-use'; import { CoreApp, GrafanaTheme2, LoadingState, PanelData } from '@grafana/data'; @@ -51,39 +51,42 @@ export const RecordingRuleEditor: FC = ({ return getDataSourceSrv().get(dataSourceName); }, [dataSourceName]); - const handleChangedQuery = (changedQuery: DataQuery) => { - if (!isPromOrLokiQuery(changedQuery) || !dataSource) { - return; - } + const handleChangedQuery = useCallback( + (changedQuery: DataQuery) => { + if (!isPromOrLokiQuery(changedQuery) || !dataSource) { + return; + } - const [query] = queries; - const { uid: dataSourceId, type } = dataSource; - const isLoki = type === DataSourceType.Loki; - const expr = changedQuery.expr; + const [query] = queries; + const { uid: dataSourceId, type } = dataSource; + const isLoki = type === DataSourceType.Loki; + const expr = changedQuery.expr; - const merged = { - ...query, - ...changedQuery, - datasourceUid: dataSourceId, - expr, - model: { + const merged = { + ...query, + ...changedQuery, + datasourceUid: dataSourceId, expr, - datasource: changedQuery.datasource, - refId: changedQuery.refId, - editorMode: changedQuery.editorMode, - // Instant and range are used by Prometheus queries - instant: changedQuery.instant, - range: changedQuery.range, - // Query type is used by Loki queries - // On first render/when creating a recording rule, the query type is not set - // unless the user has changed it betwee range/instant. The cleanest way to handle this - // is to default to instant, or whatever the changed type is - queryType: isLoki ? changedQuery.queryType || LokiQueryType.Instant : changedQuery.queryType, - legendFormat: changedQuery.legendFormat, - }, - }; - onChangeQuery([merged]); - }; + model: { + expr, + datasource: changedQuery.datasource, + refId: changedQuery.refId, + editorMode: changedQuery.editorMode, + // Instant and range are used by Prometheus queries + instant: changedQuery.instant, + range: changedQuery.range, + // Query type is used by Loki queries + // On first render/when creating a recording rule, the query type is not set + // unless the user has changed it betwee range/instant. The cleanest way to handle this + // is to default to instant, or whatever the changed type is + queryType: isLoki ? changedQuery.queryType || LokiQueryType.Instant : changedQuery.queryType, + legendFormat: changedQuery.legendFormat, + }, + }; + onChangeQuery([merged]); + }, + [dataSource, queries, onChangeQuery] + ); if (loading || dataSource?.name !== dataSourceName) { return null; diff --git a/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx b/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx index 967b3f76a75..c7550d5bd21 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/tabs/Query.tsx @@ -1,25 +1,21 @@ import { useCallback, useEffect, useMemo } from 'react'; -import { useObservable } from 'react-use'; -import { LoadingState, PanelData } from '@grafana/data'; import { config } from '@grafana/runtime'; import { Alert, Stack } from '@grafana/ui'; import { CombinedRule } from 'app/types/unified-alerting'; import { GrafanaRuleQueryViewer, QueryPreview } from '../../../GrafanaRuleQueryViewer'; import { useAlertQueriesStatus } from '../../../hooks/useAlertQueriesStatus'; -import { AlertingQueryRunner } from '../../../state/AlertingQueryRunner'; import { alertRuleToQueries } from '../../../utils/query'; import { isFederatedRuleGroup, isGrafanaRulerRule } from '../../../utils/rules'; +import { useAlertQueryRunner } from '../../rule-editor/query-and-alert-condition/useAlertQueryRunner'; interface Props { rule: CombinedRule; } const QueryResults = ({ rule }: Props) => { - const runner = useMemo(() => new AlertingQueryRunner(), []); - const data = useObservable(runner.get()); - const loadingData = isLoading(data); + const { queryPreviewData, runQueries, isPreviewLoading } = useAlertQueryRunner(); const queries = useMemo(() => alertRuleToQueries(rule), [rule]); @@ -31,9 +27,9 @@ const QueryResults = ({ rule }: Props) => { if (rule && isGrafanaRulerRule(rule.rulerRule)) { condition = rule.rulerRule.grafana_alert.condition; } - runner.run(queries, condition ?? 'A'); + runQueries(queries, condition ?? 'A'); } - }, [queries, allDataSourcesAvailable, rule, runner]); + }, [queries, allDataSourcesAvailable, rule, runQueries]); useEffect(() => { if (allDataSourcesAvailable) { @@ -41,15 +37,11 @@ const QueryResults = ({ rule }: Props) => { } }, [allDataSourcesAvailable, onRunQueries]); - useEffect(() => { - return () => runner.destroy(); - }, [runner]); - const isFederatedRule = isFederatedRuleGroup(rule.group); return ( <> - {loadingData ? ( + {isPreviewLoading ? ( 'Loading...' ) : ( <> @@ -58,27 +50,30 @@ const QueryResults = ({ rule }: Props) => { rule={rule} condition={rule.rulerRule.grafana_alert.condition} queries={queries} - evalDataByQuery={data} + evalDataByQuery={queryPreviewData} /> )} - {!isGrafanaRulerRule(rule.rulerRule) && !isFederatedRule && data && Object.keys(data).length > 0 && ( - - {queries.map((query) => { - return ( - ds.uid === query.datasourceUid)} - queryData={data[query.refId]} - relativeTimeRange={query.relativeTimeRange} - /> - ); - })} - - )} + {!isGrafanaRulerRule(rule.rulerRule) && + !isFederatedRule && + queryPreviewData && + Object.keys(queryPreviewData).length > 0 && ( + + {queries.map((query) => { + return ( + ds.uid === query.datasourceUid)} + queryData={queryPreviewData[query.refId]} + relativeTimeRange={query.relativeTimeRange} + /> + ); + })} + + )} {!isFederatedRule && !allDataSourcesAvailable && ( Cannot display the query preview. Some of the data sources used in the queries are not available. @@ -90,12 +85,4 @@ const QueryResults = ({ rule }: Props) => { ); }; -function isLoading(data?: Record): boolean { - if (!data) { - return true; - } - - return !!Object.values(data).find((d) => d.state === LoadingState.Loading); -} - export { QueryResults }; diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts index a926172c907..8985847981a 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts +++ b/public/app/features/alerting/unified/state/AlertingQueryRunner.test.ts @@ -1,6 +1,6 @@ import { defaultsDeep } from 'lodash'; -import { Observable, of, throwError } from 'rxjs'; -import { delay, take } from 'rxjs/operators'; +import { Observable, TimeoutError, lastValueFrom, of, throwError } from 'rxjs'; +import { delay, take, timeout } from 'rxjs/operators'; import { createFetchResponse } from 'test/helpers/createFetchResponse'; import { @@ -200,7 +200,7 @@ describe('AlertingQueryRunner', () => { }); }); - it('should not execute if all queries fail filterQuery check', async () => { + it('should not push any values if all queries fail filterQuery check', async () => { const runner = new AlertingQueryRunner( mockBackendSrv({ fetch: () => throwError(new Error("shouldn't happen")), @@ -211,15 +211,7 @@ describe('AlertingQueryRunner', () => { const data = runner.get(); runner.run([createQuery('A'), createQuery('B')], 'B'); - await expect(data.pipe(take(1))).toEmitValuesWith((values) => { - const [data] = values; - - expect(data.A.state).toEqual(LoadingState.Done); - expect(data.A.series).toHaveLength(0); - - expect(data.B.state).toEqual(LoadingState.Done); - expect(data.B.series).toHaveLength(0); - }); + await expect(lastValueFrom(data.pipe(timeout(200)))).rejects.toThrow(TimeoutError); }); it('should skip hidden queries and descendant nodes', async () => { diff --git a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts index fcba1587acc..3e8fc2b8cf7 100644 --- a/public/app/features/alerting/unified/state/AlertingQueryRunner.ts +++ b/public/app/features/alerting/unified/state/AlertingQueryRunner.ts @@ -51,11 +51,10 @@ export class AlertingQueryRunner { } async run(queries: AlertQuery[], condition: string) { - const empty = initialState(queries, LoadingState.Done); const queriesToRun = await this.prepareQueries(queries); if (queriesToRun.length === 0) { - return this.subject.next(empty); + return; } this.subscription = runRequest(this.backendSrv, queriesToRun, condition).subscribe({