From e06d291b1501b6b7f907869193d2b82d796b6eab Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:22:04 -0700 Subject: [PATCH 1/2] test(api): cover alerts on multi-series metric tiles (HDX-5077) The composed multi-series metric query (#2859) changed how gaps and ratio division-by-zero reach the alert task: a series with no row at a bucket now pivots to an explicit NULL instead of an absent key, and a zero denominator divides to NULL instead of a node-side NaN. Pin the new semantics with the first multi-series metric tile coverage in the checkAlerts int suite: - threshold compares against the LAST select item's value (not an earlier series' value) - a gap in the last series is skipped by the null guard rather than silently backfilled from an earlier series (the old node-side merge fell back to it) - a ratio with a zero denominator is skipped without pushing a NaN count into alert history Also retire the stale 'check if this is still needed' TODO on the null guard: the NULL is now intentional composed-query semantics. --- .../__tests__/checkAlerts.int.test.ts | 284 ++++++++++++++++++ packages/api/src/tasks/checkAlerts/index.ts | 7 +- 2 files changed, 289 insertions(+), 2 deletions(-) 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 80885513ff..a539853496 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -6690,6 +6690,290 @@ describe('checkAlerts', () => { ).toBe(true); }); + // ── Multi-series metric tiles ── + // Multi-series metric charts are composed into a single ClickHouse query + // (HDX-5077) that pivots every series into its own value column. The alert + // task compares the threshold against the LAST select item's value, and a + // series with no row at a bucket pivots to an explicit NULL — which the + // null guard in processAlert skips instead of the old node-side merge's + // behavior of silently falling back to an earlier series' value. + + const gaugePoint = ( + metricName: string, + value: number, + timestampMs: number, + ) => ({ + MetricName: metricName, + ServiceName: 'api', + ResourceAttributes: { host: 'host1' }, + Value: value, + TimeUnix: new Date(timestampMs), + }); + + const setupMetricTileAlert = async ({ + select, + seriesReturnType, + threshold, + }: { + select: Array>; + seriesReturnType?: 'ratio' | 'column'; + threshold: number; + }) => { + const { team, webhook, connection, teamWebhooksById, clickhouseClient } = + await setupSavedSearchAlertTest(); + + // Persistent mock: the beforeEach mockResolvedValueOnce only covers the + // first call, and resolve notifications can trigger a second one. + jest + .spyOn(slack, 'postMessageToWebhook') + .mockResolvedValue({ text: 'ok' }); + + const source = await Source.create({ + kind: 'metric', + team: team._id, + from: { + databaseName: DEFAULT_DATABASE, + tableName: '', + }, + metricTables: { + gauge: DEFAULT_METRICS_TABLE.GAUGE, + histogram: DEFAULT_METRICS_TABLE.HISTOGRAM, + sum: DEFAULT_METRICS_TABLE.SUM, + }, + timestampValueExpression: 'TimeUnix', + connection: connection.id, + name: 'Metrics', + }); + + const dashboard = await new Dashboard({ + name: 'My Dashboard', + team: team._id, + tiles: [ + { + id: 'multi1', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + name: 'Errors vs Requests', + select, + ...(seriesReturnType ? { seriesReturnType } : {}), + where: '', + displayType: 'line', + source: source.id, + groupBy: '', + }, + }, + ], + }).save(); + + const tile = dashboard.tiles?.find((t: any) => t.id === 'multi1'); + if (!tile) { + throw new Error('tile not found for multi-series metric test'); + } + + const details = await createAlertDetails( + team, + source, + { + source: AlertSource.TILE, + channel: { + type: 'webhook', + webhookId: webhook._id.toString(), + }, + interval: '5m', + thresholdType: AlertThresholdType.ABOVE, + threshold, + dashboardId: dashboard.id, + tileId: 'multi1', + }, + { + taskType: AlertTaskType.TILE, + tile, + dashboard, + }, + ); + + return { details, connection, teamWebhooksById, clickhouseClient }; + }; + + it('TILE alert (metrics, multi-series) - threshold compares the last series value', async () => { + const now = new Date('2023-11-16T22:12:00.000Z'); + // Alert window is [22:05, 22:10) + const eventMs = now.getTime() - ms('7m'); + + await bulkInsertMetricsGauge([ + // Both series exceed the threshold; only the LAST one may drive the + // alert value. + gaugePoint('test.requests', 100, eventMs), + gaugePoint('test.errors', 6, eventMs), + ]); + + const { details, connection, teamWebhooksById, clickhouseClient } = + await setupMetricTileAlert({ + select: [ + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.requests', + }, + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.errors', + }, + ], + threshold: 5, + }); + + await processAlertAtTime( + now, + details, + clickhouseClient, + connection.id, + alertProvider, + teamWebhooksById, + ); + + expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT'); + + // The evaluated value is the last series' 6, not the first series' 100. + const [history] = await AlertHistory.find({ + alert: details.alert.id, + }).sort({ createdAt: 1 }); + expect(history.state).toBe('ALERT'); + expect(history.lastValues.length).toBe(1); + expect(history.lastValues[0].count).toBe(6); + + expect(slack.postMessageToWebhook).toHaveBeenCalledTimes(1); + expect( + jest.mocked(slack.postMessageToWebhook).mock.calls[0][1].text, + ).toContain('6 meets or exceeds 5'); + + // Next window has no data -> resolves to OK. + const nextWindow = new Date('2023-11-16T22:16:00.000Z'); + await processAlertAtTime( + nextWindow, + details, + clickhouseClient, + connection.id, + alertProvider, + teamWebhooksById, + ); + expect((await Alert.findById(details.alert.id))!.state).toBe('OK'); + }); + + it('TILE alert (metrics, multi-series) - a gap in the last series is skipped, not backfilled from an earlier series', async () => { + const now = new Date('2023-11-16T22:12:00.000Z'); + // Alert window is [22:05, 22:10) + const eventMs = now.getTime() - ms('7m'); + + await bulkInsertMetricsGauge([ + // The first series exceeds the threshold inside the window. + gaugePoint('test.requests', 100, eventMs), + // The last series exists but has no data points inside the window. + gaugePoint('test.errors', 999, now.getTime() - ms('2h')), + ]); + + const { details, connection, teamWebhooksById, clickhouseClient } = + await setupMetricTileAlert({ + select: [ + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.requests', + }, + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.errors', + }, + ], + threshold: 5, + }); + + await processAlertAtTime( + now, + details, + clickhouseClient, + connection.id, + alertProvider, + teamWebhooksById, + ); + + // The last series pivots to NULL for the bucket, so the row is skipped: + // no alert fires from the first series' 100 (the old node-side merge + // fell back to it because the missing series' key was simply absent), + // and the run records a single default OK history with no values. + expect((await Alert.findById(details.alert.id))!.state).toBe('OK'); + + const histories = await AlertHistory.find({ alert: details.alert.id }); + expect(histories.length).toBe(1); + expect(histories[0].state).toBe('OK'); + expect(histories[0].counts).toBe(0); + expect(histories[0].lastValues.length).toBe(0); + + expect(slack.postMessageToWebhook).not.toHaveBeenCalled(); + }); + + it('TILE alert (metrics, ratio) - a zero denominator yields NULL and is skipped without NaN history', async () => { + const now = new Date('2023-11-16T22:12:00.000Z'); + // Alert window is [22:05, 22:10) + const eventMs = now.getTime() - ms('7m'); + + await bulkInsertMetricsGauge([ + // Numerator present, denominator zero: 5 / nullif(0, 0) is NULL in + // the composed query. The old node-side merge computed NaN and pushed + // a NaN count into history. + gaugePoint('test.errors', 5, eventMs), + gaugePoint('test.requests', 0, eventMs), + ]); + + const { details, connection, teamWebhooksById, clickhouseClient } = + await setupMetricTileAlert({ + select: [ + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.errors', + }, + { + aggFn: 'max', + valueExpression: 'Value', + metricType: 'gauge', + metricName: 'test.requests', + }, + ], + seriesReturnType: 'ratio', + threshold: 0.1, + }); + + await processAlertAtTime( + now, + details, + clickhouseClient, + connection.id, + alertProvider, + teamWebhooksById, + ); + + expect((await Alert.findById(details.alert.id))!.state).toBe('OK'); + + const histories = await AlertHistory.find({ alert: details.alert.id }); + expect(histories.length).toBe(1); + expect(histories[0].state).toBe('OK'); + expect(histories[0].counts).toBe(0); + expect(histories[0].lastValues.length).toBe(0); + + expect(slack.postMessageToWebhook).not.toHaveBeenCalled(); + }); + // The auto-resolve logic ensures that if a subsequent bucket within the same tick drops below the threshold, // the alert state resets to OK, even though an alert notification might have already fired for the earlier bucket. it('should check 3 time buckets [1 error, 3 errors, 1 error] with threshold 2 and auto-resolve to OK state with 3 lastValues entries', async () => { diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 3c0a1f5fc0..9f10ae6a7b 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -1366,8 +1366,11 @@ export const processAlert = async ( for (const checkData of dataForBucket) { const { value, extraFields } = parseAlertData(checkData, meta); - // TODO: we might want to fix the null value from the upstream (check if this is still needed) - // this happens when the ratio is 0/0 + // A NULL value is intentional composed-query semantics for + // multi-series metric tiles (HDX-5077): a series with no row at this + // (group, bucket) pivots to NULL, and a ratio with a missing or zero + // denominator divides to NULL. Skip the row — evaluating it (e.g. as + // 0, or as another series' value) would fabricate a state from a gap. if (value == null) { continue; } From f236c2c8c146f3e67b3a4880ea41ba3fc07c3791 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:25:07 -0700 Subject: [PATCH 2/2] chore(api): simplify alert null-guard and test comments --- .../__tests__/checkAlerts.int.test.ts | 21 +++++++------------ packages/api/src/tasks/checkAlerts/index.ts | 8 +++---- 2 files changed, 11 insertions(+), 18 deletions(-) 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 a539853496..e84a1651df 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -6691,12 +6691,10 @@ describe('checkAlerts', () => { }); // ── Multi-series metric tiles ── - // Multi-series metric charts are composed into a single ClickHouse query - // (HDX-5077) that pivots every series into its own value column. The alert - // task compares the threshold against the LAST select item's value, and a - // series with no row at a bucket pivots to an explicit NULL — which the - // null guard in processAlert skips instead of the old node-side merge's - // behavior of silently falling back to an earlier series' value. + // A multi-series metric chart is one ClickHouse query with a value column + // per series. The alert compares the threshold against the LAST series' + // value, and a series with no row at a bucket yields NULL, which + // processAlert skips. const gaugePoint = ( metricName: string, @@ -6906,10 +6904,9 @@ describe('checkAlerts', () => { teamWebhooksById, ); - // The last series pivots to NULL for the bucket, so the row is skipped: - // no alert fires from the first series' 100 (the old node-side merge - // fell back to it because the missing series' key was simply absent), - // and the run records a single default OK history with no values. + // The last series is NULL for the bucket, so the row is skipped: no + // alert fires from the first series' 100, and the run records a single + // default OK history with no values. expect((await Alert.findById(details.alert.id))!.state).toBe('OK'); const histories = await AlertHistory.find({ alert: details.alert.id }); @@ -6927,9 +6924,7 @@ describe('checkAlerts', () => { const eventMs = now.getTime() - ms('7m'); await bulkInsertMetricsGauge([ - // Numerator present, denominator zero: 5 / nullif(0, 0) is NULL in - // the composed query. The old node-side merge computed NaN and pushed - // a NaN count into history. + // Numerator present, denominator zero: the ratio is NULL. gaugePoint('test.errors', 5, eventMs), gaugePoint('test.requests', 0, eventMs), ]); diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 9f10ae6a7b..1bb8fce290 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -1366,11 +1366,9 @@ export const processAlert = async ( for (const checkData of dataForBucket) { const { value, extraFields } = parseAlertData(checkData, meta); - // A NULL value is intentional composed-query semantics for - // multi-series metric tiles (HDX-5077): a series with no row at this - // (group, bucket) pivots to NULL, and a ratio with a missing or zero - // denominator divides to NULL. Skip the row — evaluating it (e.g. as - // 0, or as another series' value) would fabricate a state from a gap. + // NULL means no data: a metric series with no row at this bucket, or + // a ratio with a missing/zero denominator. Skip the row instead of + // fabricating a state from a gap. if (value == null) { continue; }