Alerting: Always show expression warnings and errors (grafana#74839)

Author: gillesdemey

public/app/features/alerting/unified/components/expressions/Expression.tsx

@@ -4,7 +4,7 @@ import React, { FC, useCallback, useState } from 'react';
 
 import { DataFrame, dateTimeFormat, GrafanaTheme2, isTimeSeriesFrames, LoadingState, PanelData } from '@grafana/data';
 import { Stack } from '@grafana/experimental';
-import { AutoSizeInput, Badge, Button, clearButtonStyles, IconButton, useStyles2 } from '@grafana/ui';
+import { AutoSizeInput, Button, clearButtonStyles, IconButton, useStyles2 } from '@grafana/ui';
 import { ClassicConditions } from 'app/features/expressions/components/ClassicConditions';
 import { Math } from 'app/features/expressions/components/Math';
 import { Reduce } from 'app/features/expressions/components/Reduce';
@@ -23,7 +23,7 @@ import { HoverCard } from '../HoverCard';
 import { Spacer } from '../Spacer';
 import { AlertStateTag } from '../rules/AlertStateTag';
 
-import { AlertConditionIndicator } from './AlertConditionIndicator';
+import { ExpressionStatusIndicator } from './ExpressionStatusIndicator';
 import { formatLabels, getSeriesLabels, getSeriesName, getSeriesValue, isEmptySeries } from './util';
 
 interface ExpressionProps {
@@ -302,15 +302,11 @@ const Header: FC<HeaderProps> = ({
           <div>{getExpressionLabel(queryType)}</div>
         </Stack>
         <Spacer />
-        {/* when we have an evaluation error, we show a badge next to "set as alert condition" */}
-        {!alertCondition && error && (
-          <Badge color="red" icon="exclamation-circle" text="Error" tooltip={error.message} />
-        )}
-        <AlertConditionIndicator
-          onSetCondition={() => onSetCondition(query.refId)}
-          enabled={alertCondition}
+        <ExpressionStatusIndicator
           error={error}
           warning={warning}
+          onSetCondition={() => onSetCondition(query.refId)}
+          isCondition={alertCondition}
         />
         <IconButton
           name="trash-alt"

public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.test.tsx

@@ -0,0 +1,51 @@
+import { screen, render } from '@testing-library/react';
+import React from 'react';
+
+import { ExpressionStatusIndicator } from './ExpressionStatusIndicator';
+
+describe('ExpressionStatusIndicator', () => {
+  it('should render two elements when error and not condition', () => {
+    render(<ExpressionStatusIndicator isCondition={false} warning={new Error('this is a warning')} />);
+
+    expect(screen.getByText('Warning')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Set as alert condition' })).toBeInTheDocument();
+  });
+
+  it('should render one element when warning and condition', () => {
+    render(<ExpressionStatusIndicator isCondition warning={new Error('this is a warning')} />);
+
+    expect(screen.getByText('Alert condition')).toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Set as alert condition' })).not.toBeInTheDocument();
+  });
+
+  it('should render two elements when error and not condition', () => {
+    render(<ExpressionStatusIndicator isCondition={false} error={new Error('this is a error')} />);
+
+    expect(screen.getByText('Error')).toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Set as alert condition' })).toBeInTheDocument();
+  });
+
+  it('should render one element when error and condition', () => {
+    render(<ExpressionStatusIndicator isCondition error={new Error('this is a error')} />);
+
+    expect(screen.getByText('Alert condition')).toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Set as alert condition' })).not.toBeInTheDocument();
+  });
+
+  it('should render one element if condition', () => {
+    render(<ExpressionStatusIndicator isCondition />);
+
+    expect(screen.queryByText('Error')).not.toBeInTheDocument();
+    expect(screen.queryByText('Warning')).not.toBeInTheDocument();
+    expect(screen.getByText('Alert condition')).toBeInTheDocument();
+  });
+
+  it('should render one element if not condition', () => {
+    render(<ExpressionStatusIndicator isCondition={false} />);
+
+    expect(screen.queryByText('Error')).not.toBeInTheDocument();
+    expect(screen.queryByText('Warning')).not.toBeInTheDocument();
+    expect(screen.queryByRole('button', { name: 'Alert condition' })).not.toBeInTheDocument();
+    expect(screen.getByRole('button', { name: 'Set as alert condition' })).toBeInTheDocument();
+  });
+});

…/expressions/AlertConditionIndicator.tsx …xpressions/ExpressionStatusIndicator.tsxpublic/app/features/alerting/unified/components/expressions/AlertConditionIndicator.tsx renamed to public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.tsx public/app/features/alerting/unified/components/expressions/AlertConditionIndicator.tsx renamed to public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.tsx

@@ -5,36 +5,47 @@ import { GrafanaTheme2 } from '@grafana/data';
 import { Badge, clearButtonStyles, useStyles2 } from '@grafana/ui';
 
 interface AlertConditionProps {
-  enabled?: boolean;
-  error?: Error;
   warning?: Error;
+  error?: Error;
+  isCondition?: boolean;
   onSetCondition?: () => void;
 }
 
-export const AlertConditionIndicator = ({ enabled = false, error, warning, onSetCondition }: AlertConditionProps) => {
+export const ExpressionStatusIndicator = ({ error, warning, isCondition, onSetCondition }: AlertConditionProps) => {
   const styles = useStyles2(getStyles);
 
-  if (enabled && error) {
+  const elements: JSX.Element[] = [];
+
+  if (error && isCondition) {
     return <Badge color="red" icon="exclamation-circle" text="Alert condition" tooltip={error.message} />;
+  } else if (error) {
+    elements.push(<Badge key="error" color="red" icon="exclamation-circle" text="Error" tooltip={error.message} />);
   }
 
-  if (enabled && warning) {
+  if (warning && isCondition) {
     return <Badge color="orange" icon="exclamation-triangle" text="Alert condition" tooltip={warning.message} />;
+  } else if (warning) {
+    elements.push(
+      <Badge key="warning" color="orange" icon="exclamation-triangle" text="Warning" tooltip={warning.message} />
+    );
   }
 
-  if (enabled && !error && !warning) {
-    return <Badge color="green" icon="check" text="Alert condition" />;
-  }
-
-  if (!enabled) {
-    return (
-      <button type="button" className={styles.actionLink} onClick={() => onSetCondition && onSetCondition()}>
+  if (isCondition) {
+    elements.unshift(<Badge key="condition" color="green" icon="check" text="Alert condition" />);
+  } else {
+    elements.unshift(
+      <button
+        key="make-condition"
+        type="button"
+        className={styles.actionLink}
+        onClick={() => onSetCondition && onSetCondition()}
+      >
         Set as alert condition
       </button>
     );
   }
 
-  return null;
+  return <>{elements}</>;
 };
 
 const getStyles = (theme: GrafanaTheme2) => {

public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx

@@ -46,7 +46,7 @@ export const ExpressionsEditor = ({
 
         const isAlertCondition = condition === query.refId;
         const error = data ? errorFromPreviewData(data) : undefined;
-        const warning = isAlertCondition && data ? warningFromSeries(data.series) : undefined;
+        const warning = data ? warningFromSeries(data.series) : undefined;
 
         return (
           <Expression

public/app/features/alerting/unified/components/rule-editor/QueryRows.tsx

@@ -18,7 +18,7 @@ import { getDatasourceSrv } from 'app/features/plugins/datasource_srv';
 import { AlertDataQuery, AlertQuery } from 'app/types/unified-alerting-dto';
 
 import { AlertQueryOptions, EmptyQueryWrapper, QueryWrapper } from './QueryWrapper';
-import { errorFromPreviewData, getThresholdsForQueries } from './util';
+import { errorFromCurrentCondition, errorFromPreviewData, getThresholdsForQueries } from './util';
 
 interface Props {
   // The query configuration
@@ -156,12 +156,18 @@ export class QueryRows extends PureComponent<Props> {
               <div ref={provided.innerRef} {...provided.droppableProps}>
                 <Stack direction="column">
                   {queries.map((query, index) => {
+                    const isCondition = this.props.condition === query.refId;
                     const data: PanelData = this.props.data?.[query.refId] ?? {
                       series: [],
                       state: LoadingState.NotStarted,
                     };
                     const dsSettings = this.getDataSourceSettings(query);
-                    const error = data ? errorFromPreviewData(data) : undefined;
+                    let error: Error | undefined = undefined;
+                    if (data && isCondition) {
+                      error = errorFromCurrentCondition(data);
+                    } else if (data) {
+                      error = errorFromPreviewData(data);
+                    }
 
                     if (!dsSettings) {
                       return (

public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx

@@ -14,12 +14,12 @@ import {
 } from '@grafana/data';
 import { Stack } from '@grafana/experimental';
 import { DataQuery } from '@grafana/schema';
-import { Badge, GraphTresholdsStyleMode, Icon, InlineField, Input, Tooltip, useStyles2 } from '@grafana/ui';
+import { GraphTresholdsStyleMode, Icon, InlineField, Input, Tooltip, useStyles2 } from '@grafana/ui';
 import { QueryEditorRow } from 'app/features/query/components/QueryEditorRow';
 import { AlertQuery } from 'app/types/unified-alerting-dto';
 
 import { msToSingleUnitDuration } from '../../utils/time';
-import { AlertConditionIndicator } from '../expressions/AlertConditionIndicator';
+import { ExpressionStatusIndicator } from '../expressions/ExpressionStatusIndicator';
 
 import { QueryOptions } from './QueryOptions';
 import { VizWrapper } from './VizWrapper';
@@ -122,7 +122,7 @@ export const QueryWrapper = ({
     const isAlertCondition = condition === query.refId;
 
     return (
-      <Stack direction="row" alignItems="baseline" gap={1}>
+      <Stack direction="row" alignItems="center" gap={1}>
         <SelectingDataSourceTooltip />
         <QueryOptions
           onChangeTimeRange={onChangeTimeRange}
@@ -131,15 +131,11 @@ export const QueryWrapper = ({
           onChangeQueryOptions={onChangeQueryOptions}
           index={index}
         />
-
-        <AlertConditionIndicator
-          onSetCondition={() => onSetCondition(query.refId)}
-          enabled={isAlertCondition}
+        <ExpressionStatusIndicator
           error={error}
+          onSetCondition={() => onSetCondition(query.refId)}
+          isCondition={isAlertCondition}
         />
-        {!isAlertCondition && error && (
-          <Badge color="red" icon="exclamation-circle" text="Error" tooltip={error.message} />
-        )}
       </Stack>
     );
   }

public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx

@@ -25,7 +25,7 @@ import { NeedHelpInfo } from '../NeedHelpInfo';
 import { QueryEditor } from '../QueryEditor';
 import { RecordingRuleEditor } from '../RecordingRuleEditor';
 import { RuleEditorSection } from '../RuleEditorSection';
-import { errorFromSeries, findRenamedDataQueryReferences, refIdExists } from '../util';
+import { errorFromCurrentCondition, errorFromPreviewData, findRenamedDataQueryReferences, refIdExists } from '../util';
 
 import { CloudDataSourceSelector } from './CloudDataSourceSelector';
 import { SmartAlertTypeDetector } from './SmartAlertTypeDetector';
@@ -107,11 +107,13 @@ export const QueryAndExpressionsStep = ({ editingExistingRule, onDataChange }: P
   useEffect(() => {
     const currentCondition = getValues('condition');
 
-    if (!currentCondition || RuleFormType.cloudRecording) {
+    if (!currentCondition || !queryPreviewData[currentCondition]) {
       return;
     }
 
-    const error = errorFromSeries(queryPreviewData[currentCondition]?.series || []);
+    const error =
+      errorFromPreviewData(queryPreviewData[currentCondition]) ??
+      errorFromCurrentCondition(queryPreviewData[currentCondition]);
     onDataChange(error?.message || '');
   }, [queryPreviewData, getValues, onDataChange]);
 

public/app/features/alerting/unified/components/rule-editor/util.ts

@@ -95,12 +95,13 @@ export function checkForPathSeparator(value: string): ValidateResult {
   return true;
 }
 
-export function errorFromSeries(series: DataFrame[]): Error | undefined {
-  if (series.length === 0) {
+// this function assumes we've already checked if the data passed in to the function is of the alert condition
+export function errorFromCurrentCondition(data: PanelData): Error | undefined {
+  if (data.series.length === 0) {
     return;
   }
 
-  const isTimeSeriesResults = isTimeSeriesFrames(series);
+  const isTimeSeriesResults = isTimeSeriesFrames(data.series);
 
   let error;
   if (isTimeSeriesResults) {
@@ -116,8 +117,7 @@ export function errorFromPreviewData(data: PanelData): Error | undefined {
     return new Error(data.errors[0].message);
   }
 
-  // if none, return errors from series
-  return errorFromSeries(data.series);
+  return;
 }
 
 export function warningFromSeries(series: DataFrame[]): Error | undefined {