Alerting: Fix useAlertingQueryRunner re-rendering loop (#100206)

* Fix AlertingQueryRunner infinite re-rendering loop

* Update tests

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<RecordingRuleEditorProps> = ({
     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;

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 && (
-            <Stack direction="column" gap={1}>
-              {queries.map((query) => {
-                return (
-                  <QueryPreview
-                    key={query.refId}
-                    rule={rule}
-                    refId={query.refId}
-                    model={query.model}
-                    dataSource={Object.values(config.datasources).find((ds) => ds.uid === query.datasourceUid)}
-                    queryData={data[query.refId]}
-                    relativeTimeRange={query.relativeTimeRange}
-                  />
-                );
-              })}
-            </Stack>
-          )}
+          {!isGrafanaRulerRule(rule.rulerRule) &&
+            !isFederatedRule &&
+            queryPreviewData &&
+            Object.keys(queryPreviewData).length > 0 && (
+              <Stack direction="column" gap={1}>
+                {queries.map((query) => {
+                  return (
+                    <QueryPreview
+                      key={query.refId}
+                      rule={rule}
+                      refId={query.refId}
+                      model={query.model}
+                      dataSource={Object.values(config.datasources).find((ds) => ds.uid === query.datasourceUid)}
+                      queryData={queryPreviewData[query.refId]}
+                      relativeTimeRange={query.relativeTimeRange}
+                    />
+                  );
+                })}
+              </Stack>
+            )}
           {!isFederatedRule && !allDataSourcesAvailable && (
             <Alert title="Query not available" severity="warning">
               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<string, PanelData>): boolean {
-  if (!data) {
-    return true;
-  }
-
-  return !!Object.values(data).find((d) => d.state === LoadingState.Loading);
-}
-
 export { QueryResults };

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 () => {

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({