From af392c5886062467af75f8e0b176bff6318edb2e Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Thu, 14 Sep 2023 15:05:28 +0200 Subject: [PATCH] Alerting: Always show expression warnings and errors (#74839) --- .../components/expressions/Expression.tsx | 14 ++--- .../ExpressionStatusIndicator.test.tsx | 51 +++++++++++++++++++ ...ator.tsx => ExpressionStatusIndicator.tsx} | 37 +++++++++----- .../rule-editor/ExpressionsEditor.tsx | 2 +- .../components/rule-editor/QueryRows.tsx | 10 +++- .../components/rule-editor/QueryWrapper.tsx | 16 +++--- .../QueryAndExpressionsStep.tsx | 8 +-- .../unified/components/rule-editor/util.ts | 10 ++-- 8 files changed, 105 insertions(+), 43 deletions(-) create mode 100644 public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.test.tsx rename public/app/features/alerting/unified/components/expressions/{AlertConditionIndicator.tsx => ExpressionStatusIndicator.tsx} (50%) diff --git a/public/app/features/alerting/unified/components/expressions/Expression.tsx b/public/app/features/alerting/unified/components/expressions/Expression.tsx index 54fcf984a94..f6849d41a0a 100644 --- a/public/app/features/alerting/unified/components/expressions/Expression.tsx +++ b/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 = ({
{getExpressionLabel(queryType)}
- {/* when we have an evaluation error, we show a badge next to "set as alert condition" */} - {!alertCondition && error && ( - - )} - onSetCondition(query.refId)} - enabled={alertCondition} + onSetCondition(query.refId)} + isCondition={alertCondition} /> { + it('should render two elements when error and not condition', () => { + render(); + + 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(); + + 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(); + + 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(); + + 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(); + + 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(); + + 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(); + }); +}); diff --git a/public/app/features/alerting/unified/components/expressions/AlertConditionIndicator.tsx b/public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.tsx similarity index 50% rename from public/app/features/alerting/unified/components/expressions/AlertConditionIndicator.tsx rename to public/app/features/alerting/unified/components/expressions/ExpressionStatusIndicator.tsx index 4bc22b49905..28a668e25b0 100644 --- a/public/app/features/alerting/unified/components/expressions/AlertConditionIndicator.tsx +++ b/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 ; + } else if (error) { + elements.push(); } - if (enabled && warning) { + if (warning && isCondition) { return ; + } else if (warning) { + elements.push( + + ); } - if (enabled && !error && !warning) { - return ; - } - - if (!enabled) { - return ( - ); } - return null; + return <>{elements}; }; const getStyles = (theme: GrafanaTheme2) => { diff --git a/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx b/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx index f8c507d2334..eb67ed378ac 100644 --- a/public/app/features/alerting/unified/components/rule-editor/ExpressionsEditor.tsx +++ b/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 ( {
{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 ( diff --git a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx b/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx index f7323d738bb..45c06c5afa5 100644 --- a/public/app/features/alerting/unified/components/rule-editor/QueryWrapper.tsx +++ b/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 ( - + - - onSetCondition(query.refId)} - enabled={isAlertCondition} + onSetCondition(query.refId)} + isCondition={isAlertCondition} /> - {!isAlertCondition && error && ( - - )} ); } diff --git a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx index 63ee8791fa7..8d531b8b79a 100644 --- a/public/app/features/alerting/unified/components/rule-editor/query-and-alert-condition/QueryAndExpressionsStep.tsx +++ b/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]); diff --git a/public/app/features/alerting/unified/components/rule-editor/util.ts b/public/app/features/alerting/unified/components/rule-editor/util.ts index db18a60a49b..d0f0c31e536 100644 --- a/public/app/features/alerting/unified/components/rule-editor/util.ts +++ b/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 {