Skip to content

Commit 3306e52

Browse files
authored
[HDX-5077] Alert-task coverage for multi-series metric tiles (#2907)
## Why Follow-up to #2859 (composed multi-series metric query). That PR verified the existing 268 checkAlerts int tests pass unchanged, but the alert suite had **zero multi-series metric tile coverage**, and the refactor intentionally changed two null/NaN edge cases in how alert evaluation sees multi-series results. This PR pins those semantics with tests written against the shipped implementation. ## Behavior deltas pinned (introduced by #2859, both fixes over the old node-side merge) 1. **Gap in the last series at a bucket**: the old `mergeResultSets` produced a row with the missing series' key simply *absent*, so `parseAlertData`'s "last value column wins" silently fell back to an earlier series' value — an arbitrary evaluation. The composed query pivots the gap to an explicit `NULL`, which the null guard in `processAlert` skips. 2. **Ratio with a zero denominator**: the old node-side `computeRatio` produced a JS `NaN` that slipped past the null guard (`NaN != null`) and pushed a `NaN` count into alert history. The composed query divides to SQL `NULL`, which the guard skips — exactly what its own comment said it was for. ## What - **New int tests** (first metric multi-series coverage in `checkAlerts.int.test.ts`): - `TILE alert (metrics, multi-series) - threshold compares the last series value` — fires on the last select item's value (6), not the first series' (100), and resolves next window - `TILE alert (metrics, multi-series) - a gap in the last series is skipped, not backfilled from an earlier series` — no ALERT from the first series' above-threshold value; single default OK history with no evaluated values - `TILE alert (metrics, ratio) - a zero denominator yields NULL and is skipped without NaN history` — no NaN ever lands in `lastValues` - **Comment-only production change**: retire the stale `TODO: we might want to fix the null value from the upstream (check if this is still needed)` on the null guard — the NULL is now intentional composed-query semantics (gap / div-by-zero), documented as the contract. No logic change. ## Test plan - `make dev-int FILE=checkAlerts.int` — 164/164 pass (incl. the 3 new tests) against docker CH - `npx nx run @hyperdx/api:ci:lint` green (at the 311-warning ratchet; new tests add zero warnings) - `tsc --noEmit` clean No changeset: tests + comment only. ## Issues Linear: [HDX-5077](https://linear.app/clickhouse/issue/HDX-5077/move-multi-series-metric-merge-computation-to-clickhouse)
1 parent 582f394 commit 3306e52

2 files changed

Lines changed: 282 additions & 2 deletions

File tree

‎packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts‎

Lines changed: 279 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6690,6 +6690,285 @@ describe('checkAlerts', () => {
66906690
).toBe(true);
66916691
});
66926692

6693+
// ── Multi-series metric tiles ──
6694+
// A multi-series metric chart is one ClickHouse query with a value column
6695+
// per series. The alert compares the threshold against the LAST series'
6696+
// value, and a series with no row at a bucket yields NULL, which
6697+
// processAlert skips.
6698+
6699+
const gaugePoint = (
6700+
metricName: string,
6701+
value: number,
6702+
timestampMs: number,
6703+
) => ({
6704+
MetricName: metricName,
6705+
ServiceName: 'api',
6706+
ResourceAttributes: { host: 'host1' },
6707+
Value: value,
6708+
TimeUnix: new Date(timestampMs),
6709+
});
6710+
6711+
const setupMetricTileAlert = async ({
6712+
select,
6713+
seriesReturnType,
6714+
threshold,
6715+
}: {
6716+
select: Array<Record<string, string>>;
6717+
seriesReturnType?: 'ratio' | 'column';
6718+
threshold: number;
6719+
}) => {
6720+
const { team, webhook, connection, teamWebhooksById, clickhouseClient } =
6721+
await setupSavedSearchAlertTest();
6722+
6723+
// Persistent mock: the beforeEach mockResolvedValueOnce only covers the
6724+
// first call, and resolve notifications can trigger a second one.
6725+
jest
6726+
.spyOn(slack, 'postMessageToWebhook')
6727+
.mockResolvedValue({ text: 'ok' });
6728+
6729+
const source = await Source.create({
6730+
kind: 'metric',
6731+
team: team._id,
6732+
from: {
6733+
databaseName: DEFAULT_DATABASE,
6734+
tableName: '',
6735+
},
6736+
metricTables: {
6737+
gauge: DEFAULT_METRICS_TABLE.GAUGE,
6738+
histogram: DEFAULT_METRICS_TABLE.HISTOGRAM,
6739+
sum: DEFAULT_METRICS_TABLE.SUM,
6740+
},
6741+
timestampValueExpression: 'TimeUnix',
6742+
connection: connection.id,
6743+
name: 'Metrics',
6744+
});
6745+
6746+
const dashboard = await new Dashboard({
6747+
name: 'My Dashboard',
6748+
team: team._id,
6749+
tiles: [
6750+
{
6751+
id: 'multi1',
6752+
x: 0,
6753+
y: 0,
6754+
w: 6,
6755+
h: 4,
6756+
config: {
6757+
name: 'Errors vs Requests',
6758+
select,
6759+
...(seriesReturnType ? { seriesReturnType } : {}),
6760+
where: '',
6761+
displayType: 'line',
6762+
source: source.id,
6763+
groupBy: '',
6764+
},
6765+
},
6766+
],
6767+
}).save();
6768+
6769+
const tile = dashboard.tiles?.find((t: any) => t.id === 'multi1');
6770+
if (!tile) {
6771+
throw new Error('tile not found for multi-series metric test');
6772+
}
6773+
6774+
const details = await createAlertDetails(
6775+
team,
6776+
source,
6777+
{
6778+
source: AlertSource.TILE,
6779+
channel: {
6780+
type: 'webhook',
6781+
webhookId: webhook._id.toString(),
6782+
},
6783+
interval: '5m',
6784+
thresholdType: AlertThresholdType.ABOVE,
6785+
threshold,
6786+
dashboardId: dashboard.id,
6787+
tileId: 'multi1',
6788+
},
6789+
{
6790+
taskType: AlertTaskType.TILE,
6791+
tile,
6792+
dashboard,
6793+
},
6794+
);
6795+
6796+
return { details, connection, teamWebhooksById, clickhouseClient };
6797+
};
6798+
6799+
it('TILE alert (metrics, multi-series) - threshold compares the last series value', async () => {
6800+
const now = new Date('2023-11-16T22:12:00.000Z');
6801+
// Alert window is [22:05, 22:10)
6802+
const eventMs = now.getTime() - ms('7m');
6803+
6804+
await bulkInsertMetricsGauge([
6805+
// Both series exceed the threshold; only the LAST one may drive the
6806+
// alert value.
6807+
gaugePoint('test.requests', 100, eventMs),
6808+
gaugePoint('test.errors', 6, eventMs),
6809+
]);
6810+
6811+
const { details, connection, teamWebhooksById, clickhouseClient } =
6812+
await setupMetricTileAlert({
6813+
select: [
6814+
{
6815+
aggFn: 'max',
6816+
valueExpression: 'Value',
6817+
metricType: 'gauge',
6818+
metricName: 'test.requests',
6819+
},
6820+
{
6821+
aggFn: 'max',
6822+
valueExpression: 'Value',
6823+
metricType: 'gauge',
6824+
metricName: 'test.errors',
6825+
},
6826+
],
6827+
threshold: 5,
6828+
});
6829+
6830+
await processAlertAtTime(
6831+
now,
6832+
details,
6833+
clickhouseClient,
6834+
connection.id,
6835+
alertProvider,
6836+
teamWebhooksById,
6837+
);
6838+
6839+
expect((await Alert.findById(details.alert.id))!.state).toBe('ALERT');
6840+
6841+
// The evaluated value is the last series' 6, not the first series' 100.
6842+
const [history] = await AlertHistory.find({
6843+
alert: details.alert.id,
6844+
}).sort({ createdAt: 1 });
6845+
expect(history.state).toBe('ALERT');
6846+
expect(history.lastValues.length).toBe(1);
6847+
expect(history.lastValues[0].count).toBe(6);
6848+
6849+
expect(slack.postMessageToWebhook).toHaveBeenCalledTimes(1);
6850+
expect(
6851+
jest.mocked(slack.postMessageToWebhook).mock.calls[0][1].text,
6852+
).toContain('6 meets or exceeds 5');
6853+
6854+
// Next window has no data -> resolves to OK.
6855+
const nextWindow = new Date('2023-11-16T22:16:00.000Z');
6856+
await processAlertAtTime(
6857+
nextWindow,
6858+
details,
6859+
clickhouseClient,
6860+
connection.id,
6861+
alertProvider,
6862+
teamWebhooksById,
6863+
);
6864+
expect((await Alert.findById(details.alert.id))!.state).toBe('OK');
6865+
});
6866+
6867+
it('TILE alert (metrics, multi-series) - a gap in the last series is skipped, not backfilled from an earlier series', async () => {
6868+
const now = new Date('2023-11-16T22:12:00.000Z');
6869+
// Alert window is [22:05, 22:10)
6870+
const eventMs = now.getTime() - ms('7m');
6871+
6872+
await bulkInsertMetricsGauge([
6873+
// The first series exceeds the threshold inside the window.
6874+
gaugePoint('test.requests', 100, eventMs),
6875+
// The last series exists but has no data points inside the window.
6876+
gaugePoint('test.errors', 999, now.getTime() - ms('2h')),
6877+
]);
6878+
6879+
const { details, connection, teamWebhooksById, clickhouseClient } =
6880+
await setupMetricTileAlert({
6881+
select: [
6882+
{
6883+
aggFn: 'max',
6884+
valueExpression: 'Value',
6885+
metricType: 'gauge',
6886+
metricName: 'test.requests',
6887+
},
6888+
{
6889+
aggFn: 'max',
6890+
valueExpression: 'Value',
6891+
metricType: 'gauge',
6892+
metricName: 'test.errors',
6893+
},
6894+
],
6895+
threshold: 5,
6896+
});
6897+
6898+
await processAlertAtTime(
6899+
now,
6900+
details,
6901+
clickhouseClient,
6902+
connection.id,
6903+
alertProvider,
6904+
teamWebhooksById,
6905+
);
6906+
6907+
// The last series is NULL for the bucket, so the row is skipped: no
6908+
// alert fires from the first series' 100, and the run records a single
6909+
// default OK history with no values.
6910+
expect((await Alert.findById(details.alert.id))!.state).toBe('OK');
6911+
6912+
const histories = await AlertHistory.find({ alert: details.alert.id });
6913+
expect(histories.length).toBe(1);
6914+
expect(histories[0].state).toBe('OK');
6915+
expect(histories[0].counts).toBe(0);
6916+
expect(histories[0].lastValues.length).toBe(0);
6917+
6918+
expect(slack.postMessageToWebhook).not.toHaveBeenCalled();
6919+
});
6920+
6921+
it('TILE alert (metrics, ratio) - a zero denominator yields NULL and is skipped without NaN history', async () => {
6922+
const now = new Date('2023-11-16T22:12:00.000Z');
6923+
// Alert window is [22:05, 22:10)
6924+
const eventMs = now.getTime() - ms('7m');
6925+
6926+
await bulkInsertMetricsGauge([
6927+
// Numerator present, denominator zero: the ratio is NULL.
6928+
gaugePoint('test.errors', 5, eventMs),
6929+
gaugePoint('test.requests', 0, eventMs),
6930+
]);
6931+
6932+
const { details, connection, teamWebhooksById, clickhouseClient } =
6933+
await setupMetricTileAlert({
6934+
select: [
6935+
{
6936+
aggFn: 'max',
6937+
valueExpression: 'Value',
6938+
metricType: 'gauge',
6939+
metricName: 'test.errors',
6940+
},
6941+
{
6942+
aggFn: 'max',
6943+
valueExpression: 'Value',
6944+
metricType: 'gauge',
6945+
metricName: 'test.requests',
6946+
},
6947+
],
6948+
seriesReturnType: 'ratio',
6949+
threshold: 0.1,
6950+
});
6951+
6952+
await processAlertAtTime(
6953+
now,
6954+
details,
6955+
clickhouseClient,
6956+
connection.id,
6957+
alertProvider,
6958+
teamWebhooksById,
6959+
);
6960+
6961+
expect((await Alert.findById(details.alert.id))!.state).toBe('OK');
6962+
6963+
const histories = await AlertHistory.find({ alert: details.alert.id });
6964+
expect(histories.length).toBe(1);
6965+
expect(histories[0].state).toBe('OK');
6966+
expect(histories[0].counts).toBe(0);
6967+
expect(histories[0].lastValues.length).toBe(0);
6968+
6969+
expect(slack.postMessageToWebhook).not.toHaveBeenCalled();
6970+
});
6971+
66936972
// The auto-resolve logic ensures that if a subsequent bucket within the same tick drops below the threshold,
66946973
// the alert state resets to OK, even though an alert notification might have already fired for the earlier bucket.
66956974
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 () => {

‎packages/api/src/tasks/checkAlerts/index.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1366,8 +1366,9 @@ export const processAlert = async (
13661366
for (const checkData of dataForBucket) {
13671367
const { value, extraFields } = parseAlertData(checkData, meta);
13681368

1369-
// TODO: we might want to fix the null value from the upstream (check if this is still needed)
1370-
// this happens when the ratio is 0/0
1369+
// NULL means no data: a metric series with no row at this bucket, or
1370+
// a ratio with a missing/zero denominator. Skip the row instead of
1371+
// fabricating a state from a gap.
13711372
if (value == null) {
13721373
continue;
13731374
}

0 commit comments

Comments
 (0)