diff --git a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts index e84a1651df..e85a7ded08 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -4705,6 +4705,355 @@ describe('checkAlerts', () => { expect(alertHistories[1].state).toBe('OK'); }); + describe('dashboard variables', () => { + const NOW = new Date('2023-11-16T22:12:00.000Z'); + // Inside the last alert window, 22:05 - 22:10. + const EVENT_AT = new Date(NOW.getTime() - ms('5m')); + + const seedLogs = (serviceNames: string[]) => + bulkInsertLogs( + serviceNames.map((ServiceName, i) => ({ + ServiceName, + Timestamp: EVENT_AT, + SeverityText: 'error', + Body: `variable alert test event ${i}`, + })), + ); + + /** A dashboard filter on `ServiceName`, exposed as `$svc` by default. */ + const serviceFilter = (overrides: Record = {}) => ({ + id: 'service-filter', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'unused-by-the-alert-task', + whereLanguage: 'sql', + isVariableEnabled: true, + variableName: 'svc', + ...overrides, + }); + + const tileAlertConfig = ( + webhookId: string, + dashboardId: string, + tileId: string, + ) => ({ + source: AlertSource.TILE as const, + channel: { type: 'webhook' as const, webhookId }, + interval: '5m' as const, + thresholdType: AlertThresholdType.ABOVE, + threshold: 1, + dashboardId, + tileId, + }); + + const lastValue = async (alertId: string) => { + const histories = await AlertHistory.find({ alert: alertId }).sort({ + createdAt: 1, + }); + expect(histories.length).toBe(1); + return histories[0].lastValues[0]?.count; + }; + + it('expands a Lucene reference to its empty state, and does not broadcast the filter', async () => { + const { + team, + webhook, + connection, + source, + teamWebhooksById, + clickhouseClient, + } = await setupSavedSearchAlertTest(); + + await seedLogs(['api', 'web']); + + const dashboard = await new Dashboard({ + name: 'Variables Dashboard', + team: team._id, + // The filter's own `where` would exclude every row if it were + // broadcast onto the tile — alerts only take the variable. + filters: [ + serviceFilter({ + source: source.id, + where: "ServiceName = 'nothing-matches-this'", + }), + ], + tiles: [ + { + id: 'lucene-var', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + name: 'Logs Count', + select: [ + { + aggFn: 'count', + aggCondition: '', + valueExpression: '', + aggConditionLanguage: 'lucene', + }, + ], + where: 'ServiceName:$svc', + whereLanguage: 'lucene', + displayType: 'line', + granularity: 'auto', + source: source.id, + groupBy: '', + }, + }, + ], + }).save(); + + const tile = dashboard.tiles?.find((t: any) => t.id === 'lucene-var'); + if (!tile) throw new Error('tile not found'); + + const details = await createAlertDetails( + team, + source, + tileAlertConfig(webhook._id.toString(), dashboard.id, 'lucene-var'), + { taskType: AlertTaskType.TILE, tile, dashboard }, + ); + + await processAlertAtTime( + NOW, + details, + clickhouseClient, + connection, + alertProvider, + teamWebhooksById, + ); + + // The empty Lucene selection renders as `("")`, which drops out of the + // predicate — so both rows are counted. Left unsubstituted, the literal + // `$svc` would match nothing and the alert would stay OK. + expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT'); + expect(await lastValue(details.alert.id)).toBe(2); + expect( + (await Alert.findById(details.alert.id))!.executionErrors ?? [], + ).toHaveLength(0); + }); + + it('expands $__conditionalAll in a SQL where to its no-op form', async () => { + const { + team, + webhook, + connection, + source, + teamWebhooksById, + clickhouseClient, + } = await setupSavedSearchAlertTest(); + + await seedLogs(['api', 'web']); + + const dashboard = await new Dashboard({ + name: 'Variables Dashboard', + team: team._id, + filters: [serviceFilter({ source: source.id })], + tiles: [ + { + id: 'macro-var', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + name: 'Logs Count', + select: [ + { + aggFn: 'count', + aggCondition: '', + valueExpression: '', + aggConditionLanguage: 'lucene', + }, + ], + where: "$__conditionalAll(ServiceName = 'api', svc)", + whereLanguage: 'sql', + displayType: 'line', + granularity: 'auto', + source: source.id, + groupBy: '', + }, + }, + ], + }).save(); + + const tile = dashboard.tiles?.find((t: any) => t.id === 'macro-var'); + if (!tile) throw new Error('tile not found'); + + const details = await createAlertDetails( + team, + source, + tileAlertConfig(webhook._id.toString(), dashboard.id, 'macro-var'), + { taskType: AlertTaskType.TILE, tile, dashboard }, + ); + + await processAlertAtTime( + NOW, + details, + clickhouseClient, + connection, + alertProvider, + teamWebhooksById, + ); + + // Nothing is selected, so the guarded condition drops out and both rows + // are counted. Left unexpanded, `$__conditionalAll(...)` is not valid + // SQL and the query would fail outright. + expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT'); + expect(await lastValue(details.alert.id)).toBe(2); + }); + + it('leaves references the dashboard does not declare exactly as written', async () => { + const { + team, + webhook, + connection, + source, + teamWebhooksById, + clickhouseClient, + } = await setupSavedSearchAlertTest(); + + await seedLogs(['$abc', '$hidden', 'api']); + + const dashboard = await new Dashboard({ + name: 'Variables Dashboard', + team: team._id, + filters: [ + serviceFilter({ source: source.id }), + // Collects a value but does not expose it as a variable, so + // `$hidden` names nothing. + serviceFilter({ + id: 'hidden-filter', + name: 'Hidden', + variableName: 'hidden', + isVariableEnabled: false, + source: source.id, + }), + ], + tiles: [ + { + id: 'unknown-var', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + name: 'Logs Count', + select: [ + { + aggFn: 'count', + aggCondition: '', + valueExpression: '', + aggConditionLanguage: 'lucene', + }, + ], + where: "ServiceName = '$abc' OR ServiceName = '$hidden'", + whereLanguage: 'sql', + displayType: 'line', + granularity: 'auto', + source: source.id, + groupBy: '', + }, + }, + ], + }).save(); + + const tile = dashboard.tiles?.find((t: any) => t.id === 'unknown-var'); + if (!tile) throw new Error('tile not found'); + + const details = await createAlertDetails( + team, + source, + tileAlertConfig(webhook._id.toString(), dashboard.id, 'unknown-var'), + { taskType: AlertTaskType.TILE, tile, dashboard }, + ); + + await processAlertAtTime( + NOW, + details, + clickhouseClient, + connection, + alertProvider, + teamWebhooksById, + ); + + // Both literals survived substitution and matched their rows; the + // `api` row did not. + expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT'); + expect(await lastValue(details.alert.id)).toBe(2); + }); + + it('expands $__filter in a raw SQL tile while leaving an undeclared literal alone', async () => { + const { + team, + webhook, + connection, + teamWebhooksById, + clickhouseClient, + } = await setupSavedSearchAlertTest(); + + await seedLogs(['api', 'web', '$abc']); + + const sqlTemplate = [ + 'SELECT toStartOfInterval(Timestamp, INTERVAL {intervalSeconds:Int64} second) AS ts,', + ' count() AS cnt', + ' FROM default.otel_logs', + ' WHERE Timestamp >= fromUnixTimestamp64Milli({startDateMilliseconds:Int64})', + ' AND Timestamp < fromUnixTimestamp64Milli({endDateMilliseconds:Int64})', + ' AND $__filter(svc)', + " AND ServiceName != '$abc'", + ' GROUP BY ts ORDER BY ts', + ].join(''); + + const dashboard = await new Dashboard({ + name: 'Raw SQL Variables Dashboard', + team: team._id, + filters: [serviceFilter()], + tiles: [ + { + id: 'rawsql-var', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + configType: 'sql', + displayType: 'line', + sqlTemplate, + connection: connection.id, + }, + }, + ], + }).save(); + + const tile = dashboard.tiles?.find((t: any) => t.id === 'rawsql-var'); + if (!tile) throw new Error('tile not found'); + + const details = await createAlertDetails( + team, + undefined, // No source for raw SQL tiles + tileAlertConfig(webhook._id.toString(), dashboard.id, 'rawsql-var'), + { taskType: AlertTaskType.TILE, tile, dashboard }, + ); + + await processAlertAtTime( + NOW, + details, + clickhouseClient, + connection, + alertProvider, + teamWebhooksById, + ); + + // `$__filter(svc)` expanded to its empty-selection no-op, so it matched + // everything; `'$abc'` was left as a literal and excluded its own row. + expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT'); + expect(await lastValue(details.alert.id)).toBe(2); + }); + }); + it('TILE alert (raw SQL) - multiple rows per time bucket from GROUP BY', async () => { const { team, webhook, connection, teamWebhooksById, clickhouseClient } = await setupSavedSearchAlertTest(); diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 1bb8fce290..26d8a3748b 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -24,6 +24,7 @@ import { isTimeSeriesDisplayType, } from '@hyperdx/common-utils/dist/core/utils'; import { timeBucketByGranularity } from '@hyperdx/common-utils/dist/core/utils'; +import { getDashboardVariableDeclarations } from '@hyperdx/common-utils/dist/filters'; import { isBuilderChartConfig, isBuilderSavedChartConfig, @@ -625,6 +626,14 @@ const getChartConfigFromAlert = ( } else if (details.taskType === AlertTaskType.TILE) { const tile = details.tile; + // Substitute empty selections for each variable the dashboard defines + const variables = getDashboardVariableDeclarations( + details.dashboard.filters, + ).map(declaration => ({ + ...declaration, + values: [], + })); + // Raw SQL tiles: build a RawSqlChartConfig if (isRawSqlSavedChartConfig(tile.config)) { if (displayTypeSupportsRawSqlAlerts(tile.config.displayType)) { @@ -637,6 +646,7 @@ const getChartConfigFromAlert = ( ]), connection, dateRange, + variables, // Only time-series charts use interval bucketing ...(isTimeSeriesDisplayType(tile.config.displayType) && { granularity: `${windowSizeInMins} minute`, @@ -706,6 +716,7 @@ const getChartConfigFromAlert = ( where: tile.config.where, whereLanguage: tile.config.whereLanguage, seriesReturnType: tile.config.seriesReturnType, + variables, }; } } diff --git a/packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx b/packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx index 4eb6fb870c..903cef223b 100644 --- a/packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx +++ b/packages/app/src/components/ChartEditor/RawSqlChartEditor.tsx @@ -118,6 +118,7 @@ export default function RawSqlChartEditor({ onSubmit, isDashboardForm, alert, + additionalWarnings, dashboardId, variables, }: { @@ -127,6 +128,7 @@ export default function RawSqlChartEditor({ onSubmit: (suppressErrorNotification?: boolean) => void; isDashboardForm: boolean; alert: ChartEditorFormState['alert']; + additionalWarnings?: string[]; dashboardId?: string; variables?: ChartVariable[]; }) { @@ -161,11 +163,13 @@ export default function RawSqlChartEditor({ const { alertErrorMessage, alertWarningMessage } = useMemo(() => { const { errors, warnings } = validateRawSqlForAlert(debouncedRawSqlConfig); + const allWarnings = [...warnings, ...(additionalWarnings ?? [])]; return { alertErrorMessage: errors.length > 0 ? errors.join(' ') : undefined, - alertWarningMessage: warnings.length > 0 ? warnings.join(' ') : undefined, + alertWarningMessage: + allWarnings.length > 0 ? allWarnings.join(' ') : undefined, }; - }, [debouncedRawSqlConfig]); + }, [additionalWarnings, debouncedRawSqlConfig]); const { chartErrors, chartWarnings, sqlValidationAlertVariant } = useMemo(() => { diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx index 4c33a6e7d8..f632326d23 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx @@ -63,6 +63,7 @@ type ChartEditorControlsProps = { seriesReturnType: ChartEditorFormState['seriesReturnType']; ratioMode: ChartEditorFormState['ratioMode']; alert: ChartEditorFormState['alert']; + additionalWarnings?: string[]; isRawSqlInput: boolean; dashboardId?: string; parentRef: HTMLElement | null; @@ -93,6 +94,7 @@ export function ChartEditorControls({ seriesReturnType, ratioMode, alert, + additionalWarnings, isRawSqlInput, dashboardId, parentRef, @@ -458,6 +460,11 @@ export function ChartEditorControls({ setValue={setValue} alert={alert} onRemove={() => setValue('alert', undefined)} + warning={ + additionalWarnings?.length + ? additionalWarnings.join(' ') + : undefined + } /> )} diff --git a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx index ebdd2364a5..591dc90dd6 100644 --- a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx @@ -24,6 +24,7 @@ import { SourceKind, TSource, } from '@hyperdx/common-utils/dist/types'; +import { getAlertVariableWarning } from '@hyperdx/common-utils/dist/variables'; import { Box, Divider, @@ -33,7 +34,7 @@ import { Text, Textarea, } from '@mantine/core'; -import { useDisclosure, usePrevious } from '@mantine/hooks'; +import { useDebouncedValue, useDisclosure, usePrevious } from '@mantine/hooks'; import { notifications } from '@mantine/notifications'; import { IconBracketsContain, @@ -392,6 +393,23 @@ export default function EditTimeChartForm({ [previewConfig, alert], ); + // Casting because `useWatch` returns a deep partial type, but we know that the + // form state is complete due to default values set above. + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const watchedForm = useWatch({ control }) as ChartEditorFormState; + const [debouncedForm] = useDebouncedValue(watchedForm, 300); + const additionalAlertWarnings = useMemo(() => { + if (alert == null) return []; + const config = convertFormStateToSavedChartConfig( + debouncedForm, + tableSource, + ); + const variableWarning = config + ? getAlertVariableWarning(config, variables) + : undefined; + return variableWarning ? [variableWarning] : []; + }, [alert, debouncedForm, tableSource, variables]); + const [saveToDashboardModalOpen, setSaveToDashboardModalOpen] = useState(false); @@ -872,6 +890,7 @@ export default function EditTimeChartForm({ onSubmit={onSubmit} isDashboardForm={isDashboardForm} alert={alert} + additionalWarnings={additionalAlertWarnings} dashboardId={dashboardId} variables={variables} /> @@ -897,6 +916,7 @@ export default function EditTimeChartForm({ seriesReturnType={seriesReturnType} ratioMode={ratioMode} alert={alert} + additionalWarnings={additionalAlertWarnings} isRawSqlInput={isRawSqlInput} dashboardId={dashboardId} parentRef={parentRef} diff --git a/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx b/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx index 7796fdee80..e029a3ca1a 100644 --- a/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/__tests__/DBEditTimeChartForm.test.tsx @@ -483,6 +483,64 @@ describe('DBEditTimeChartForm - Add/delete alerts for display type Number', () = }); }); +describe('DBEditTimeChartForm - Alert variable warning', () => { + const SVC = { name: 'svc', expression: 'ServiceName', values: [] }; + + const renderWithAlert = async ( + props: Partial> = {}, + ) => { + renderComponent({ + chartConfig: { ...defaultChartConfig, displayType: DisplayType.Number }, + dashboardId: 'test-dashboard-id', + ...props, + }); + await userEvent.click(screen.getByTestId('alert-button')); + }; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('warns that an alerting tile referencing a variable runs on its empty state', async () => { + await renderWithAlert({ + chartConfig: { + ...defaultChartConfig, + displayType: DisplayType.Number, + where: 'ServiceName:$svc', + }, + variables: [SVC], + }); + + const warning = await screen.findByText('Warning'); + await userEvent.hover(warning); + expect( + await screen.findByText( + 'This tile references $svc. Alerts run with every dashboard variable in its empty state, not the values selected here.', + ), + ).toBeInTheDocument(); + }); + + it('says nothing when the tile references no variable', async () => { + await renderWithAlert({ variables: [SVC] }); + + expect(screen.getByTestId('alert-details')).toBeInTheDocument(); + expect(screen.queryByText('Warning')).not.toBeInTheDocument(); + }); + + it('says nothing where no variables are in scope', async () => { + await renderWithAlert({ + chartConfig: { + ...defaultChartConfig, + displayType: DisplayType.Number, + where: 'ServiceName:$svc', + }, + }); + + expect(screen.getByTestId('alert-details')).toBeInTheDocument(); + expect(screen.queryByText('Warning')).not.toBeInTheDocument(); + }); +}); + describe('DBEditTimeChartForm - Duplicate series', () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/packages/app/src/components/alerts/AlertDetailChart.tsx b/packages/app/src/components/alerts/AlertDetailChart.tsx index d9270ab33e..d47d89a0d7 100644 --- a/packages/app/src/components/alerts/AlertDetailChart.tsx +++ b/packages/app/src/components/alerts/AlertDetailChart.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import Link from 'next/link'; import { pick } from 'lodash'; import { isTimeSeriesDisplayType } from '@hyperdx/common-utils/dist/core/utils'; +import { getDashboardVariableDeclarations } from '@hyperdx/common-utils/dist/filters'; import { isPromqlSavedChartConfig, isRawSqlSavedChartConfig, @@ -137,6 +138,14 @@ function TileAlertChart({ return undefined; } + // The alert (and its preview) runs with every dashboard variable in its empty state + const variables = getDashboardVariableDeclarations(dashboard?.filters).map( + declaration => ({ + ...declaration, + values: [], + }), + ); + // Raw SQL tiles: only time-series display types can be charted over the // alert window (mirrors what the alert task evaluates as a time series). if (isRawSqlSavedChartConfig(tile.config)) { @@ -144,13 +153,14 @@ function TileAlertChart({ return undefined; } if (!tile.config.source) { - return { ...tile.config, dateRange, granularity }; + return { ...tile.config, dateRange, granularity, variables }; } if (!source) { return undefined; } return { ...tile.config, + variables, ...pick(source, [ 'implicitColumnExpression', 'useTextIndexForImplicitColumn', @@ -181,6 +191,7 @@ function TileAlertChart({ const tableName = getMetricTableName(source, metricType); return { ...tile.config, + variables, displayType: tile.config.displayType === DisplayType.Number ? DisplayType.Line @@ -205,7 +216,7 @@ function TileAlertChart({ sampleWeightExpression: getSampleWeightExpression(source), metricTables: isMetricSource ? source.metricTables : undefined, }; - }, [tile, source, dateRange, granularity]); + }, [tile, source, dashboard?.filters, dateRange, granularity]); const referenceLines = React.useMemo( () => diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index 52040deef1..d44535eed9 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -3,9 +3,8 @@ import { useQueryState } from 'nuqs'; import { FilterState, filtersToQuery, - getFilterVariableName, + getDashboardVariableDeclarations, isFilterBroadcastEnabled, - isFilterVariableEnabled, } from '@hyperdx/common-utils/dist/filters'; import { ChartVariable, @@ -105,25 +104,19 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { } } - const variables: ChartVariable[] = []; - const takenNames = new Set(); - for (const definition of filters) { - if (!isFilterVariableEnabled(definition)) continue; - - // There shouldn't be any duplicate names, but if there are then the first one wins. - const name = getFilterVariableName(definition); - if (!name || takenNames.has(name)) continue; - takenNames.add(name); - - const selection = valuesForExistingFilters[definition.expression]; - variables.push({ - name, - expression: definition.expression, + const variables: ChartVariable[] = getDashboardVariableDeclarations( + filters, + ).map(definition => { + const selection = definition.expression + ? valuesForExistingFilters[definition.expression] + : undefined; + return { + ...definition, values: selection ? Array.from(selection.included).map(String).sort() // Sorted for deterministic react-query keys : [], - }); - } + }; + }); return { valuesForExistingFilters, diff --git a/packages/app/tests/e2e/components/ChartEditorComponent.ts b/packages/app/tests/e2e/components/ChartEditorComponent.ts index 746841458c..17b06efc6a 100644 --- a/packages/app/tests/e2e/components/ChartEditorComponent.ts +++ b/packages/app/tests/e2e/components/ChartEditorComponent.ts @@ -670,6 +670,24 @@ export class ChartEditorComponent { await this.save(); } + /** The badge the alert block shows when the tile's query has a warning. */ + alertWarningBadge(): Locator { + return this.page + .getByTestId('alert-details') + .getByText('Warning', { exact: true }); + } + + /** + * What the alert block's warning badge says, or '' when it shows none. The + * message only exists as a tooltip, so this hovers the badge to read it. + */ + async getAlertWarning(): Promise { + const badge = this.alertWarningBadge(); + if ((await badge.count()) === 0) return ''; + await badge.hover(); + return (await this.page.getByRole('tooltip').innerText()).trim(); + } + /** * Select a threshold type in the tile alert editor. * Pass the option value (e.g. 'between', 'above', 'below'). diff --git a/packages/app/tests/e2e/features/dashboard.spec.ts b/packages/app/tests/e2e/features/dashboard.spec.ts index a7956d6ce9..3f17eec3d9 100644 --- a/packages/app/tests/e2e/features/dashboard.spec.ts +++ b/packages/app/tests/e2e/features/dashboard.spec.ts @@ -1613,6 +1613,14 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => { }).toPass({ timeout: 15000 }); }); + await test.step('The alert says which variables it will empty', async () => { + await expect(async () => { + expect(await dashboardPage.chartEditor.getAlertWarning()).toContain( + 'This tile references $svc.', + ); + }).toPass({ timeout: 15000 }); + }); + await test.step('Removing the alert restores the selected value', async () => { await dashboardPage.chartEditor.clickRemoveAlert(); await expect(async () => { @@ -1912,6 +1920,73 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => { }, ); + test( + 'empties every variable in an alerting builder tile, and says so', + { tag: '@full-stack' }, + async () => { + test.setTimeout(90000); + + await test.step('Create a dashboard with a selected variable value', async () => { + await dashboardPage.createNewDashboard(); + await addServiceVariable(); + await dashboardPage.clickFilterOption('Service', 'accounting'); + await dashboardPage.page.keyboard.press('Escape'); + }); + + await test.step('Add a builder line tile whose WHERE references the variable', async () => { + await dashboardPage.addTile(); + await expect(dashboardPage.chartEditor.nameInput).toBeVisible(); + await dashboardPage.chartEditor.waitForDataToLoad(); + await dashboardPage.chartEditor.setChartType(DisplayType.Line); + await dashboardPage.chartEditor.setChartName( + `E2E Builder Alert Variable Tile ${Date.now()}`, + ); + await dashboardPage.chartEditor.selectSource( + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardPage.chartEditor.setSqlWhere( + '$__filter(ServiceName, svc)', + ); + await dashboardPage.chartEditor.runQuery(); + }); + + await test.step('Without an alert the preview uses the selected value', async () => { + await dashboardPage.chartEditor.openGeneratedSql(); + await expect(async () => { + const sql = await dashboardPage.chartEditor.getGeneratedSqlText(); + expect(sql).toContain("IN ('accounting')"); + }).toPass({ timeout: 15000 }); + }); + + await test.step('Adding an alert empties the variable in the preview', async () => { + // An alert runs on a schedule with no dashboard filter selection, so + // the preview must show what the alert will actually evaluate. + await dashboardPage.chartEditor.clickAddAlert(); + await expect(async () => { + const sql = await dashboardPage.chartEditor.getGeneratedSqlText(); + expect(sql).not.toContain('accounting'); + expect(sql).toMatch(/1\s*=\s*1/); + }).toPass({ timeout: 15000 }); + }); + + await test.step('The alert says which variables it will empty', async () => { + await expect(async () => { + expect(await dashboardPage.chartEditor.getAlertWarning()).toContain( + 'This tile references $svc. Alerts run with every dashboard ' + + 'variable in its empty state, not the values selected here.', + ); + }).toPass({ timeout: 15000 }); + }); + + await test.step('Dropping the reference clears the warning', async () => { + await dashboardPage.chartEditor.setSqlWhere("ServiceName = 'ad'"); + await expect( + dashboardPage.chartEditor.alertWarningBadge(), + ).toHaveCount(0, { timeout: 15000 }); + }); + }, + ); + test( 'expands the variable macros in a series agg condition', { tag: '@full-stack' }, diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index cbebcc2c59..d5f5fcf2d9 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -3,6 +3,7 @@ import { FilterState, filterStateToPredicate, filtersToQuery, + getDashboardVariableDeclarations, getFilterVariableName, hasFilterEffect, isFilterBroadcastEnabled, @@ -993,6 +994,87 @@ describe('filters', () => { }); }); + describe('getDashboardVariableDeclarations', () => { + const filter = (overrides: Partial): DashboardFilter => ({ + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'logs', + ...overrides, + }); + + it('returns nothing for a dashboard with no filters', () => { + expect(getDashboardVariableDeclarations(undefined)).toEqual([]); + expect(getDashboardVariableDeclarations([])).toEqual([]); + }); + + it('skips filters that do not expose a variable', () => { + expect( + getDashboardVariableDeclarations([ + filter({ id: 'broadcast-only', isVariableEnabled: false }), + filter({ id: 'unset', name: 'Env', expression: 'Env' }), + ]), + ).toEqual([]); + }); + + it('declares the name and the expression it filters on', () => { + expect( + getDashboardVariableDeclarations([ + filter({ isVariableEnabled: true, variableName: 'svc' }), + ]), + ).toEqual([{ name: 'svc', expression: 'ServiceName' }]); + }); + + it('falls back to the name derived from the display name', () => { + expect( + getDashboardVariableDeclarations([ + filter({ name: 'Total Requests', isVariableEnabled: true }), + ]), + ).toEqual([{ name: 'Total_Requests', expression: 'ServiceName' }]); + }); + + it('skips a filter whose display name derives nothing usable', () => { + expect( + getDashboardVariableDeclarations([ + filter({ name: '环境', isVariableEnabled: true }), + ]), + ).toEqual([]); + }); + + it('keeps the first of two filters claiming the same name', () => { + expect( + getDashboardVariableDeclarations([ + filter({ id: 'a', isVariableEnabled: true, variableName: 'svc' }), + filter({ + id: 'b', + expression: 'Other', + isVariableEnabled: true, + variableName: 'svc', + }), + ]), + ).toEqual([{ name: 'svc', expression: 'ServiceName' }]); + }); + + it('keeps the declarations in filter order', () => { + expect( + getDashboardVariableDeclarations([ + filter({ isVariableEnabled: true, variableName: 'svc' }), + filter({ + id: 'f2', + name: 'Env', + expression: 'Env', + isVariableEnabled: true, + variableName: 'env', + }), + ]), + ).toEqual([ + { name: 'svc', expression: 'ServiceName' }, + { name: 'env', expression: 'Env' }, + ]); + }); + }); + describe('validateVariableName', () => { const variableFilter = ( overrides: Partial, diff --git a/packages/common-utils/src/__tests__/variables.test.ts b/packages/common-utils/src/__tests__/variables.test.ts index 66cacd4a86..a23de5aa01 100644 --- a/packages/common-utils/src/__tests__/variables.test.ts +++ b/packages/common-utils/src/__tests__/variables.test.ts @@ -3,6 +3,7 @@ import type { BuilderChartConfig, ChartVariable } from '@/types'; import { filterReferencedVariables, formatVariableValues, + getAlertVariableWarning, getReferencedVariableNames, getVariableReferences, hasVariableMacro, @@ -714,6 +715,70 @@ describe('filterReferencedVariables', () => { }); }); +describe('getAlertVariableWarning', () => { + const variables = [SERVICE, variable('env', ['prod'])]; + + const rawSqlConfig = (sqlTemplate: string) => + ({ configType: 'sql', sqlTemplate, connection: 'local' }) as const; + + it('says nothing when no variables are in scope', () => { + const config = rawSqlConfig('WHERE ServiceName = $service'); + expect(getAlertVariableWarning(config, undefined)).toBeUndefined(); + expect(getAlertVariableWarning(config, [])).toBeUndefined(); + }); + + it('says nothing when the query references none of them', () => { + expect( + getAlertVariableWarning(rawSqlConfig('SELECT 1'), variables), + ).toBeUndefined(); + expect( + getAlertVariableWarning(builderConfig({ where: '' }), variables), + ).toBeUndefined(); + }); + + it('says nothing for a PromQL config, which cannot use variables', () => { + expect( + getAlertVariableWarning( + { + configType: 'promql', + promqlExpression: 'up{service="$service"}', + connection: 'local', + }, + variables, + ), + ).toBeUndefined(); + }); + + it('names only the variables the raw SQL references', () => { + expect( + getAlertVariableWarning( + rawSqlConfig('WHERE ServiceName = $service AND $nope'), + variables, + ), + ).toBe( + 'This tile references $service. Alerts run with every dashboard variable ' + + 'in its empty state, not the values selected here.', + ); + }); + + it('names every variable a builder config references, across its expressions', () => { + expect( + getAlertVariableWarning( + builderConfig({ + select: [ + { aggFn: 'count', valueExpression: '', aggCondition: '$env' }, + ], + where: '$__filter(ServiceName, service)', + }), + variables, + ), + ).toBe( + 'This tile references $service, $env. Alerts run with every dashboard ' + + 'variable in its empty state, not the values selected here.', + ); + }); +}); + describe('substituteChartConfigVariables', () => { it('returns the config untouched when there is no variable context', () => { const config = builderConfig({ where: 'ServiceName = $service' }); diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index f65e1309e8..f741178411 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -3,6 +3,7 @@ import * as SQLParser from 'node-sql-parser'; import { escapeSqlString, replaceJsonExpressions } from '@/core/utils'; import { parse } from '@/queryParser'; import { + ChartVariable, DASHBOARD_VARIABLE_NAME_MAX_LENGTH, DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED, DashboardFilter, @@ -778,6 +779,33 @@ export function getFilterVariableName(filter: { ); } +/** A dashboard variable's identity, before any selection is attached. */ +export type DashboardVariableDeclaration = Pick< + ChartVariable, + 'name' | 'expression' +>; + +/** The variables a dashboard declares, in filter order. */ +export function getDashboardVariableDeclarations( + filters: DashboardFilter[] | undefined, +): DashboardVariableDeclaration[] { + const declarations: DashboardVariableDeclaration[] = []; + const takenNames = new Set(); + + for (const filter of filters ?? []) { + if (!isFilterVariableEnabled(filter)) continue; + + // There shouldn't be any duplicate names, but if there are then the first one wins. + const name = getFilterVariableName(filter); + if (!name || takenNames.has(name)) continue; + takenNames.add(name); + + declarations.push({ name, expression: filter.expression }); + } + + return declarations; +} + /** * Validate a variable name against the token grammar and against the names * already taken by other variable-enabled filters on the same dashboard. diff --git a/packages/common-utils/src/variables.ts b/packages/common-utils/src/variables.ts index 0531a8b95b..6fe019ee6e 100644 --- a/packages/common-utils/src/variables.ts +++ b/packages/common-utils/src/variables.ts @@ -880,3 +880,20 @@ export function filterReferencedVariables( const referenced = new Set(names); return variables.filter(variable => referenced.has(variable.name)); } + +/** The warning an alerting tile shows when its query references dashboard variables. */ +export function getAlertVariableWarning( + config: ChartConfigWithOptDateRange | SavedChartConfig, + variables: ChartVariable[] | undefined, +): string | undefined { + if (!variables?.length) return undefined; + + const referenced = filterReferencedVariables(config, variables); + if (referenced.length === 0) return undefined; + + const names = referenced.map(variable => `$${variable.name}`).join(', '); + return ( + `This tile references ${names}. Alerts run with every dashboard variable ` + + `in its empty state, not the values selected here.` + ); +} diff --git a/scripts/ci/ratchet-baseline.json b/scripts/ci/ratchet-baseline.json index bf51d83bd0..3fd8aa4812 100644 --- a/scripts/ci/ratchet-baseline.json +++ b/scripts/ci/ratchet-baseline.json @@ -7,7 +7,7 @@ "app": { "as-any": 215, "ts-ignore": 0, - "eslint-disable": 143 + "eslint-disable": 144 }, "cli": { "as-any": 0, @@ -29,4 +29,4 @@ "ts-ignore": 0, "eslint-disable": 0 } -} +} \ No newline at end of file