From 6be4926bcc346ab6715f9e5f054c0adc425d3e60 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Thu, 5 Nov 2026 13:56:16 -0500 Subject: [PATCH 1/5] feat(api): add an alert evaluator extension point processAlert hard-codes threshold evaluation after its scheduling and config setup. Add an AlertEvaluator interface: the first registered evaluator whose appliesTo matches evaluates the alert through an AlertEvaluationContext (the same history, notification, and persistence helpers the threshold path uses); otherwise the threshold path runs unchanged. An evaluator can also shift the evaluation clock, applied in processAlert and in getAlertWindowStart so window and history stay aligned. Evaluators are registered from @/extensions, called once on first lookup. It registers none here. The evaluator name is recorded on alerts.evaluate. Co-Authored-By: Claude Opus 5.5 --- packages/api/src/extensions/index.ts | 5 + .../checkAlerts/__tests__/evaluators.test.ts | 169 +++++++++++++ .../api/src/tasks/checkAlerts/evaluators.ts | 118 +++++++++ packages/api/src/tasks/checkAlerts/index.ts | 227 ++++++++++++------ 4 files changed, 439 insertions(+), 80 deletions(-) create mode 100644 packages/api/src/extensions/index.ts create mode 100644 packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts create mode 100644 packages/api/src/tasks/checkAlerts/evaluators.ts diff --git a/packages/api/src/extensions/index.ts b/packages/api/src/extensions/index.ts new file mode 100644 index 0000000000..25cfbc9437 --- /dev/null +++ b/packages/api/src/extensions/index.ts @@ -0,0 +1,5 @@ +// Registers optional extensions with the API's extension points, such as alert +// evaluators (see tasks/checkAlerts/evaluators.ts). Extension points call this +// once, on first use, so it runs after every module has loaded. This build +// registers none. +export const registerExtensions = (): void => {}; diff --git a/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts new file mode 100644 index 0000000000..4256eeb978 --- /dev/null +++ b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts @@ -0,0 +1,169 @@ +import { DisplayType } from '@hyperdx/common-utils/dist/types'; +import { Types } from 'mongoose'; + +import { AlertState } from '@/models/alert'; +import { processAlert } from '@/tasks/checkAlerts'; +import { + AlertEvaluationContext, + AlertEvaluator, + clearAlertEvaluators, + findAlertEvaluator, + registerAlertEvaluator, +} from '@/tasks/checkAlerts/evaluators'; +import { AlertTaskType } from '@/tasks/checkAlerts/providers'; + +const makeAlert = (overrides: Record = {}): any => ({ + id: new Types.ObjectId().toString(), + interval: '5m', + threshold: 1, + thresholdType: 'above', + channel: { type: 'webhook' }, + source: 'tile', + ...overrides, +}); + +// A raw SQL inline alert needs no source and skips materialized-view +// optimization, so processAlert reaches evaluation without ClickHouse. +const makeDetails = (alert: any): any => ({ + alert, + previousMap: new Map(), + taskType: AlertTaskType.INLINE, + chartConfig: { + configType: 'sql', + sqlTemplate: 'SELECT 1', + displayType: DisplayType.Line, + }, +}); + +const makeProvider = (): any => ({ + updateAlertState: jest.fn().mockResolvedValue(undefined), + recordAlertErrors: jest.fn().mockResolvedValue(undefined), +}); + +const makeClient = (): any => ({ + queryChartConfig: jest.fn().mockRejectedValue(new Error('not expected')), + requestTimeoutMs: 1000, +}); + +const run = (alert: any, now: Date, provider = makeProvider()) => { + const client = makeClient(); + return processAlert( + now, + makeDetails(alert), + client, + 'connection-1', + provider, + new Map(), + ).then(() => ({ client, provider })); +}; + +describe('alert evaluators', () => { + afterEach(() => { + clearAlertEvaluators(); + }); + + it('finds the first evaluator that applies to the alert', () => { + const custom: AlertEvaluator = { + name: 'custom', + appliesTo: alert => alert.threshold === 42, + evaluate: jest.fn(), + }; + registerAlertEvaluator(custom); + + expect(findAlertEvaluator(makeAlert({ threshold: 42 }))).toBe(custom); + expect(findAlertEvaluator(makeAlert())).toBeUndefined(); + }); + + it('rejects a second evaluator with the same name', () => { + const evaluator: AlertEvaluator = { + name: 'custom', + appliesTo: () => true, + evaluate: jest.fn(), + }; + registerAlertEvaluator(evaluator); + expect(() => registerAlertEvaluator(evaluator)).toThrow( + 'already registered', + ); + }); + + it('evaluates with the registered evaluator instead of the threshold query', async () => { + let ctx: AlertEvaluationContext | undefined; + registerAlertEvaluator({ + name: 'custom', + appliesTo: () => true, + evaluate: async c => { + ctx = c; + const history = c.getOrCreateHistory(''); + history.state = AlertState.OK; + await c.persistHistories([history]); + }, + }); + + const alert = makeAlert(); + const { client, provider } = await run( + alert, + new Date('2025-01-01T00:07:00Z'), + ); + + expect(client.queryChartConfig).not.toHaveBeenCalled(); + expect(ctx?.alert).toBe(alert); + expect(ctx?.nowInMinsRoundDown).toEqual(new Date('2025-01-01T00:05:00Z')); + expect(provider.updateAlertState).toHaveBeenCalledTimes(1); + const [alertId, records, errors] = provider.updateAlertState.mock.calls[0]; + expect(alertId).toBe(alert.id); + expect(records).toEqual([ + expect.objectContaining({ state: AlertState.OK }), + ]); + expect(errors).toEqual([]); + }); + + it('schedules the window from the evaluator clock', async () => { + let ctx: AlertEvaluationContext | undefined; + registerAlertEvaluator({ + name: 'delayed', + appliesTo: () => true, + // Two minutes behind: 00:06 on the wall clock evaluates as 00:04. + evaluationNow: (_alert, now) => new Date(now.getTime() - 2 * 60_000), + evaluate: async c => { + ctx = c; + }, + }); + + await run(makeAlert(), new Date('2025-01-01T00:06:00Z')); + + expect(ctx?.nowInMinsRoundDown).toEqual(new Date('2025-01-01T00:00:00Z')); + }); + + it('records an error thrown by the evaluator on the alert', async () => { + registerAlertEvaluator({ + name: 'failing', + appliesTo: () => true, + evaluate: async () => { + throw new Error('boom'); + }, + }); + + const { provider } = await run( + makeAlert(), + new Date('2025-01-01T00:07:00Z'), + ); + + expect(provider.updateAlertState).not.toHaveBeenCalled(); + expect(provider.recordAlertErrors).toHaveBeenCalledTimes(1); + const [, errors, windowStart] = provider.recordAlertErrors.mock.calls[0]; + expect(errors).toEqual([expect.objectContaining({ type: 'UNKNOWN' })]); + expect(windowStart).toEqual(new Date('2025-01-01T00:05:00Z')); + }); + + it('uses the threshold query when no evaluator applies', async () => { + registerAlertEvaluator({ + name: 'other', + appliesTo: () => false, + evaluate: jest.fn(), + }); + + const { client } = await run(makeAlert(), new Date('2025-01-01T00:07:00Z')); + + expect(client.queryChartConfig).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/api/src/tasks/checkAlerts/evaluators.ts b/packages/api/src/tasks/checkAlerts/evaluators.ts new file mode 100644 index 0000000000..73bb9ab5e7 --- /dev/null +++ b/packages/api/src/tasks/checkAlerts/evaluators.ts @@ -0,0 +1,118 @@ +import type { ClickhouseClient } from '@hyperdx/common-utils/dist/clickhouse/node'; +import type { Metadata } from '@hyperdx/common-utils/dist/core/metadata'; +import type { ChartConfigWithOptDateRange } from '@hyperdx/common-utils/dist/types'; + +import { registerExtensions } from '@/extensions'; +import type { AlertState, IAlert, IAlertError } from '@/models/alert'; +import type { + IAlertHistory, + IAlertHistoryAnalytics, +} from '@/models/alertHistory'; +import type { ISource } from '@/models/source'; +import type { AggregatedAlertHistory } from '@/tasks/checkAlerts'; +import type { + AlertDetails, + AlertProvider, +} from '@/tasks/checkAlerts/providers'; + +/** + * What processAlert has resolved by the time it evaluates an alert: the + * scheduled window, the chart config to query, and the helpers that record + * state and send notifications. The threshold path in processAlert uses the + * same helpers, so an evaluator gets identical silencing, resolve, and + * persistence behavior. + */ +export interface AlertEvaluationContext { + alert: IAlert; + details: AlertDetails; + source: ISource | undefined; + /** Unoptimized config: evaluators that change the date range or + * granularity must re-run materialized-view selection themselves. */ + chartConfig: ChartConfigWithOptDateRange; + clickhouseClient: ClickhouseClient; + metadata: Metadata; + dateRange: [Date, Date]; + windowSizeInMins: number; + /** Scheduled start of the window being evaluated. */ + nowInMinsRoundDown: Date; + hasGroupBy: boolean; + previousMap: Map; + alertProvider: AlertProvider; + /** Persisted on every history record this evaluation writes. */ + evaluationAnalytics: IAlertHistoryAnalytics; + /** Errors collected so far (e.g. failed notification targets). */ + executionErrors: IAlertError[]; + getOrCreateHistory: (groupKey: string) => IAlertHistory; + /** Resolves false when the send was suppressed (the alert is silenced). */ + trySendNotification: (args: { + state: AlertState; + totalCount: number; + group: string; + startTime?: Date; + attributes?: Record; + }) => Promise; + sendNotificationIfResolved: ( + previousHistory: AggregatedAlertHistory | undefined, + currentHistory: IAlertHistory, + groupKey: string, + ) => Promise; + /** Writes the records and alert state the way the threshold path does, + * with `extraErrors` added to the collected execution errors. Attaches + * `evaluationAnalytics` to each record when anything was measured. */ + persistHistories: ( + records: IAlertHistory[], + extraErrors?: IAlertError[], + ) => Promise; +} + +/** + * A detection strategy for alerts. processAlert evaluates an alert with the + * first registered evaluator whose `appliesTo` returns true, and falls back to + * the built-in threshold evaluation when none does. + */ +export interface AlertEvaluator { + /** Low-cardinality name, recorded on the alerts.evaluate metric. */ + name: string; + appliesTo: (alert: IAlert) => boolean; + /** + * The clock to evaluate against. Defaults to the wall clock. Applied both + * in processAlert and when fetching history up front, so the scheduled + * window and the history it is compared to stay aligned. + */ + evaluationNow?: (alert: IAlert, wallClockNow: Date) => Date; + /** Evaluates the window and persists state through `ctx`. Throwing records + * the error on the alert, as for the threshold path. */ + evaluate: (ctx: AlertEvaluationContext) => Promise; +} + +export const THRESHOLD_EVALUATOR_NAME = 'threshold'; + +const evaluators: AlertEvaluator[] = []; +let extensionsRegistered = false; + +export const registerAlertEvaluator = (evaluator: AlertEvaluator) => { + if (evaluators.some(e => e.name === evaluator.name)) { + throw new Error( + `Alert evaluator "${evaluator.name}" is already registered`, + ); + } + evaluators.push(evaluator); +}; + +/** The registered evaluator for this alert, or undefined for the threshold + * path. */ +export const findAlertEvaluator = ( + alert: IAlert, +): AlertEvaluator | undefined => { + if (!extensionsRegistered) { + extensionsRegistered = true; + registerExtensions(); + } + return evaluators.find(e => e.appliesTo(alert)); +}; + +/** @internal Test helper. Extensions are not registered again afterwards. */ +export const clearAlertEvaluators = () => { + extensionsRegistered = true; + evaluators.length = 0; +}; diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 6381d90507..d5c715276e 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -79,6 +79,11 @@ import { WebhookNotFoundError, WebhookRedirectError, } from '@/tasks/checkAlerts/errors'; +import { + AlertEvaluationContext, + findAlertEvaluator, + THRESHOLD_EVALUATOR_NAME, +} from '@/tasks/checkAlerts/evaluators'; import { AlertDetails, AlertProvider, @@ -436,13 +441,21 @@ export const getScheduledWindowStart = ( return fns.addMinutes(roundedShiftedNow, scheduleOffsetMinutes); }; +/** + * The clock an alert is evaluated against: the wall clock, unless the alert's + * evaluator shifts it (see AlertEvaluator.evaluationNow). + */ +const getAlertEvaluationNow = (alert: IAlert, now: Date): Date => + findAlertEvaluator(alert)?.evaluationNow?.(alert, now) ?? now; + /** * Compute the scheduled window start ("now rounded down to the window") for an * alert at the given time. This mirrors the computation inside processAlert so * that history fetched up-front (see getConsecutiveWindowHistories) lines up * exactly with the window processAlert evaluates. */ -const getAlertWindowStart = (alert: IAlert, now: Date): Date => { +const getAlertWindowStart = (alert: IAlert, wallClockNow: Date): Date => { + const now = getAlertEvaluationNow(alert, wallClockNow); const windowSizeInMins = ms(alert.interval) / 60000; const scheduleStartAt = normalizeScheduleStartAt({ alertId: alert.id, @@ -976,7 +989,7 @@ export const alertQueryAttribution = ( }); export const processAlert = async ( - now: Date, + wallClockNow: Date, details: AlertDetails, clickhouseClient: ClickhouseClient, connectionId: string, @@ -984,6 +997,8 @@ export const processAlert = async ( teamWebhooksById: Map, ) => { const { alert, previousMap, recentHistoryMap } = details; + const now = getAlertEvaluationNow(alert, wallClockNow); + const evaluator = findAlertEvaluator(alert); const source = 'source' in details ? details.source : undefined; // Errors collected during this execution. Webhook errors accumulate here; query // and validation errors are recorded via recordAlertErrors before returning. @@ -1206,81 +1221,6 @@ export const processAlert = async ( ? { readonly: '2' } : {}; - // Query for alert data. If the query fails, record the error and exit - // without touching alert state or creating an AlertHistory. - let checksData; - const queryStartedAt = performance.now(); - try { - checksData = await clickhouseClient.queryChartConfig({ - config: optimizedChartConfig, - metadata, - opts: { clickhouse_settings: clickHouseSettings }, - querySettings: source?.querySettings, - }); - // SLO signal for alert data fetching (distinct from the end-to-end - // evaluation SLI): did ClickHouse serve the alert query, and how fast. - const queryDurationMs = performance.now() - queryStartedAt; - evaluationAnalytics.queryDurationMs = Math.round(queryDurationMs); - recordOperationOutcome({ - operation: 'alerts.query', - outcome: 'success', - durationMs: queryDurationMs, - attributes: { alert_source: alert.source ?? 'unknown' }, - }); - } catch (e) { - const queryDurationMs = performance.now() - queryStartedAt; - // Time-to-failure — for QUERY_TIMEOUT this is roughly the configured - // evaluation timeout. - evaluationAnalytics.queryDurationMs = Math.round(queryDurationMs); - recordOperationOutcome({ - operation: 'alerts.query', - outcome: 'error', - durationMs: queryDurationMs, - attributes: { alert_source: alert.source ?? 'unknown' }, - }); - evalOutcome = 'error'; - const alertError = makeQueryAlertError( - e, - clickhouseClient.requestTimeoutMs, - ); - alertQueryFailuresCounter.add(1, { - error_type: - alertError.type === AlertErrorType.QUERY_TIMEOUT - ? 'timeout' - : 'error', - }); - logger.error( - { - alertId: alert.id, - errorType: alertError.type, - error: serializeError(e), - }, - 'Alert query failed, skipping state/history update', - ); - // Record the error on the alert and as an ERROR history row for this - // window. ERROR rows are excluded from the due-ness gate and date-range - // computation, so the failed window is still retried/backfilled. - await alertProvider.recordAlertErrors( - alert.id, - [alertError], - nowInMinsRoundDown, - evaluationAnalytics, - ); - return; - } - - logger.info( - { - alertId: alert.id, - chartConfig, - optimizedChartConfig, - checksData, - checkStartTime: dateRange[0], - checkEndTime: dateRange[1], - }, - `Received alert metric [${alert.source} source]`, - ); - // Track state per group (or one history if no groupBy) const histories = new Map(); const latestAlertContext = new Map< @@ -1331,6 +1271,10 @@ export const processAlert = async ( : undefined; // Helper to send a notification, catching and logging any errors. + // Resolves true when a dispatch was attempted (even if delivery later + // fails — failures surface via executionErrors, mirroring the threshold + // path which marks history.fired before dispatching), and false when the + // send was suppressed entirely (silenced). const trySendNotification = async ({ group, totalCount, @@ -1343,7 +1287,7 @@ export const processAlert = async ( group: string; startTime?: Date; attributes?: Record; - }) => { + }): Promise => { // KNOWN LIMITATION: Alert data (including silenced state) is fetched when // the task is queued via AlertProvider, not when it processes. If a user // silences an alert after it's queued but before it processes, this @@ -1359,7 +1303,7 @@ export const processAlert = async ( }, 'Skipped firing alert due to silence', ); - return; + return false; } alertEvaluationsCounter.add(1, { @@ -1430,6 +1374,7 @@ export const processAlert = async ( ); executionErrors.push(makeWebhookAlertError(e)); } + return true; }; const numWindowsToLookBack = alert.numConsecutiveWindows ?? 1; @@ -1483,6 +1428,125 @@ export const processAlert = async ( } }; + const persistHistories = async ( + records: IAlertHistory[], + extraErrors: IAlertError[] = [], + ) => { + flushNotificationTimings(); + // An evaluation that measured nothing persists no analytics object. + if (Object.keys(evaluationAnalytics).length > 0) { + for (const record of records) { + record.analytics = evaluationAnalytics; + } + } + await alertProvider.updateAlertState( + alert.id, + records, + [...executionErrors, ...extraErrors], + dateRange, + ); + }; + + if (evaluator) { + const ctx: AlertEvaluationContext = { + alert, + details, + source, + chartConfig, + clickhouseClient, + metadata, + dateRange, + windowSizeInMins, + nowInMinsRoundDown, + hasGroupBy, + previousMap, + alertProvider, + evaluationAnalytics, + executionErrors, + getOrCreateHistory, + trySendNotification, + sendNotificationIfResolved, + persistHistories, + }; + await evaluator.evaluate(ctx); + return; + } + + // Query for threshold alert data. If the query fails, record the error and exit + // without touching alert state or creating an AlertHistory. + let checksData; + const queryStartedAt = performance.now(); + try { + checksData = await clickhouseClient.queryChartConfig({ + config: optimizedChartConfig, + metadata, + opts: { clickhouse_settings: clickHouseSettings }, + querySettings: source?.querySettings, + }); + // SLO signal for alert data fetching (distinct from the end-to-end + // evaluation SLI): did ClickHouse serve the alert query, and how fast. + const queryDurationMs = performance.now() - queryStartedAt; + evaluationAnalytics.queryDurationMs = Math.round(queryDurationMs); + recordOperationOutcome({ + operation: 'alerts.query', + outcome: 'success', + durationMs: queryDurationMs, + attributes: { alert_source: alert.source ?? 'unknown' }, + }); + } catch (e) { + const queryDurationMs = performance.now() - queryStartedAt; + // Time-to-failure — for QUERY_TIMEOUT this is roughly the configured + // evaluation timeout. + evaluationAnalytics.queryDurationMs = Math.round(queryDurationMs); + recordOperationOutcome({ + operation: 'alerts.query', + outcome: 'error', + durationMs: queryDurationMs, + attributes: { alert_source: alert.source ?? 'unknown' }, + }); + evalOutcome = 'error'; + const alertError = makeQueryAlertError( + e, + clickhouseClient.requestTimeoutMs, + ); + alertQueryFailuresCounter.add(1, { + error_type: + alertError.type === AlertErrorType.QUERY_TIMEOUT + ? 'timeout' + : 'error', + }); + logger.error( + { + alertId: alert.id, + errorType: alertError.type, + error: serializeError(e), + }, + 'Alert query failed, skipping state/history update', + ); + // Record the error on the alert and as an ERROR history row for this + // window. ERROR rows are excluded from the due-ness gate and date-range + // computation, so the failed window is still retried/backfilled. + await alertProvider.recordAlertErrors( + alert.id, + [alertError], + nowInMinsRoundDown, + evaluationAnalytics, + ); + return; + } + + logger.info( + { + alertId: alert.id, + chartConfig, + optimizedChartConfig, + checksData, + checkStartTime: dateRange[0], + checkEndTime: dateRange[1], + }, + `Received alert metric [${alert.source} source]`, + ); + const meta = getResponseMetadata(chartConfig, checksData); if (!meta) { evalOutcome = 'error'; @@ -1821,7 +1885,10 @@ export const processAlert = async ( operation: 'alerts.evaluate', outcome: evalOutcome, durationMs: performance.now() - evalStartedAt, - attributes: { alert_source: alert.source ?? 'unknown' }, + attributes: { + alert_source: alert.source ?? 'unknown', + alert_evaluator: evaluator?.name ?? THRESHOLD_EVALUATOR_NAME, + }, }); } } From 48cc76bc5163932a0eb8445a70c7ea22be6a16a9 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Nov 2026 09:26:40 -0500 Subject: [PATCH 2/5] fix(api): contain evaluator lookup failures and clamp the evaluation clock - Resolve the evaluator and clock inside processAlert's try, so a throwing appliesTo/evaluationNow/registerExtensions is recorded on the alert and counted on alerts.evaluate instead of escaping. - Mark extensions registered only after registerExtensions succeeds; roll back a partial registration and retry on the next lookup, rather than silently routing alerts to the threshold path. - Ignore an evaluationNow later than the wall clock: previous-state lookups bound history by the wall clock, which only covers every window when the evaluation clock is not ahead of it. - Test the context contract: suppressed vs attempted sends, resolve via the context, merged persist errors, conditional analytics, and the up-front history window under a shifted or clamped clock. - Note that AlertEvaluationContext is not a stable API. Co-Authored-By: Claude Opus 5.5 --- .../checkAlerts/__tests__/evaluators.test.ts | 178 +++++++++++++++++- .../api/src/tasks/checkAlerts/evaluators.ts | 35 +++- packages/api/src/tasks/checkAlerts/index.ts | 23 ++- 3 files changed, 222 insertions(+), 14 deletions(-) diff --git a/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts index 4256eeb978..6d0108876a 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts @@ -2,7 +2,7 @@ import { DisplayType } from '@hyperdx/common-utils/dist/types'; import { Types } from 'mongoose'; import { AlertState } from '@/models/alert'; -import { processAlert } from '@/tasks/checkAlerts'; +import { getAlertWindowStart, processAlert } from '@/tasks/checkAlerts'; import { AlertEvaluationContext, AlertEvaluator, @@ -57,6 +57,24 @@ const run = (alert: any, now: Date, provider = makeProvider()) => { ).then(() => ({ client, provider })); }; +/** Registers an evaluator that runs `body` and returns its context. */ +const evaluateWith = async ( + body: (ctx: AlertEvaluationContext) => Promise, + alert = makeAlert(), +) => { + let ctx: AlertEvaluationContext | undefined; + registerAlertEvaluator({ + name: 'custom', + appliesTo: () => true, + evaluate: async c => { + ctx = c; + await body(c); + }, + }); + const result = await run(alert, new Date('2025-01-01T00:07:00Z')); + return { ...result, ctx: ctx! }; +}; + describe('alert evaluators', () => { afterEach(() => { clearAlertEvaluators(); @@ -166,4 +184,162 @@ describe('alert evaluators', () => { expect(client.queryChartConfig).toHaveBeenCalledTimes(1); }); + + it('reports a silenced send as suppressed', async () => { + let sent: boolean | undefined; + const { ctx } = await evaluateWith( + async c => { + sent = await c.trySendNotification({ + state: AlertState.ALERT, + group: '', + totalCount: 5, + }); + }, + makeAlert({ silenced: { until: new Date(Date.now() + 60_000) } }), + ); + + expect(sent).toBe(false); + expect(ctx.executionErrors).toEqual([]); + }); + + it('reports an attempted send even when delivery fails', async () => { + let sent: boolean | undefined; + // The test alert has no team, so the attempt records a webhook error. + const { ctx } = await evaluateWith(async c => { + sent = await c.trySendNotification({ + state: AlertState.ALERT, + group: '', + totalCount: 5, + }); + }); + + expect(sent).toBe(true); + expect(ctx.executionErrors).toHaveLength(1); + }); + + it('sends a resolve notification through the context', async () => { + const { ctx } = await evaluateWith(async c => { + const history = c.getOrCreateHistory(''); + history.state = AlertState.OK; + await c.sendNotificationIfResolved( + { state: AlertState.ALERT, fired: true } as any, + history, + '', + ); + }); + + // One dispatch attempt, failing on the missing team. + expect(ctx.executionErrors).toHaveLength(1); + }); + + it('persists extra errors with the collected ones', async () => { + const extra = { type: 'INVALID_ALERT', message: 'extra' } as any; + const { provider } = await evaluateWith(async c => { + c.executionErrors.push({ type: 'WEBHOOK_ERROR', message: 'a' } as any); + await c.persistHistories([c.getOrCreateHistory('')], [extra]); + }); + + const [, , errors] = provider.updateAlertState.mock.calls[0]; + expect(errors).toEqual([expect.objectContaining({ message: 'a' }), extra]); + }); + + it('attaches analytics only when something was measured', async () => { + const measured = await evaluateWith(async c => { + c.evaluationAnalytics.queryDurationMs = 12; + await c.persistHistories([c.getOrCreateHistory('')]); + }); + clearAlertEvaluators(); + const unmeasured = await evaluateWith(async c => { + await c.persistHistories([c.getOrCreateHistory('')]); + }); + + const [[, [measuredRecord]]] = + measured.provider.updateAlertState.mock.calls; + const [[, [unmeasuredRecord]]] = + unmeasured.provider.updateAlertState.mock.calls; + expect(measuredRecord.analytics).toEqual({ queryDurationMs: 12 }); + expect(unmeasuredRecord.analytics).toBeUndefined(); + }); + + it('aligns the up-front history window with the evaluator clock', () => { + registerAlertEvaluator({ + name: 'delayed', + appliesTo: () => true, + evaluationNow: (_alert, now) => new Date(now.getTime() - 2 * 60_000), + evaluate: jest.fn(), + }); + + expect( + getAlertWindowStart(makeAlert(), new Date('2025-01-01T00:06:00Z')), + ).toEqual(new Date('2025-01-01T00:00:00Z')); + }); + + it('ignores an evaluator clock that is ahead of the wall clock', () => { + registerAlertEvaluator({ + name: 'ahead', + appliesTo: () => true, + evaluationNow: (_alert, now) => new Date(now.getTime() + 10 * 60_000), + evaluate: jest.fn(), + }); + + expect( + getAlertWindowStart(makeAlert(), new Date('2025-01-01T00:06:00Z')), + ).toEqual(new Date('2025-01-01T00:05:00Z')); + }); + + it('records a failing evaluator lookup on the alert', async () => { + registerAlertEvaluator({ + name: 'broken', + appliesTo: () => { + throw new Error('lookup failed'); + }, + evaluate: jest.fn(), + }); + + const { provider } = await run( + makeAlert(), + new Date('2025-01-01T00:07:00Z'), + ); + + expect(provider.recordAlertErrors).toHaveBeenCalledTimes(1); + const [, errors] = provider.recordAlertErrors.mock.calls[0]; + expect(errors).toEqual([expect.objectContaining({ type: 'UNKNOWN' })]); + }); +}); + +describe('extension registration', () => { + it('rolls back and retries a failed registration', () => { + jest.isolateModules(() => { + const registerExtensions = jest + .fn() + .mockImplementationOnce(() => { + evaluatorsModule.registerAlertEvaluator({ + name: 'partial', + appliesTo: () => true, + evaluate: jest.fn(), + }); + throw new Error('registration failed'); + }) + .mockImplementationOnce(() => { + evaluatorsModule.registerAlertEvaluator({ + name: 'partial', + appliesTo: () => true, + evaluate: jest.fn(), + }); + }); + jest.doMock('@/extensions', () => ({ registerExtensions })); + // eslint-disable-next-line @typescript-eslint/no-require-imports, n/no-missing-require + const evaluatorsModule = require('@/tasks/checkAlerts/evaluators'); + + expect(() => evaluatorsModule.findAlertEvaluator(makeAlert())).toThrow( + 'registration failed', + ); + // The partial registration was rolled back, so the retry can register + // the same name again. + expect(evaluatorsModule.findAlertEvaluator(makeAlert())?.name).toBe( + 'partial', + ); + expect(registerExtensions).toHaveBeenCalledTimes(2); + }); + }); }); diff --git a/packages/api/src/tasks/checkAlerts/evaluators.ts b/packages/api/src/tasks/checkAlerts/evaluators.ts index 73bb9ab5e7..396f00cd90 100644 --- a/packages/api/src/tasks/checkAlerts/evaluators.ts +++ b/packages/api/src/tasks/checkAlerts/evaluators.ts @@ -16,6 +16,9 @@ import type { } from '@/tasks/checkAlerts/providers'; /** + * Internal extension point, not a stable API: members follow processAlert and + * can change in any release. + * * What processAlert has resolved by the time it evaluates an alert: the * scheduled window, the chart config to query, and the helpers that record * state and send notifications. The threshold path in processAlert uses the @@ -75,9 +78,10 @@ export interface AlertEvaluator { name: string; appliesTo: (alert: IAlert) => boolean; /** - * The clock to evaluate against. Defaults to the wall clock. Applied both - * in processAlert and when fetching history up front, so the scheduled - * window and the history it is compared to stay aligned. + * The clock to evaluate against, e.g. shifted back to wait for late data. + * Defaults to the wall clock, and a result later than the wall clock is + * ignored. Applied both in processAlert and when fetching history up front, + * so the scheduled window and the history it is compared to stay aligned. */ evaluationNow?: (alert: IAlert, wallClockNow: Date) => Date; /** Evaluates the window and persists state through `ctx`. Throwing records @@ -101,13 +105,30 @@ export const registerAlertEvaluator = (evaluator: AlertEvaluator) => { /** The registered evaluator for this alert, or undefined for the threshold * path. */ +/** + * Runs registerExtensions once. A failed registration is rolled back and + * rethrown, and retried on the next lookup: routing alerts to the threshold + * path because their evaluator failed to register would evaluate them wrongly + * without any error. + */ +const ensureExtensionsRegistered = () => { + if (extensionsRegistered) { + return; + } + const registeredBefore = evaluators.length; + try { + registerExtensions(); + } catch (e) { + evaluators.length = registeredBefore; + throw e; + } + extensionsRegistered = true; +}; + export const findAlertEvaluator = ( alert: IAlert, ): AlertEvaluator | undefined => { - if (!extensionsRegistered) { - extensionsRegistered = true; - registerExtensions(); - } + ensureExtensionsRegistered(); return evaluators.find(e => e.appliesTo(alert)); }; diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index d5c715276e..ee10313c2d 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -81,6 +81,7 @@ import { } from '@/tasks/checkAlerts/errors'; import { AlertEvaluationContext, + AlertEvaluator, findAlertEvaluator, THRESHOLD_EVALUATOR_NAME, } from '@/tasks/checkAlerts/evaluators'; @@ -443,10 +444,14 @@ export const getScheduledWindowStart = ( /** * The clock an alert is evaluated against: the wall clock, unless the alert's - * evaluator shifts it (see AlertEvaluator.evaluationNow). + * evaluator shifts it back (see AlertEvaluator.evaluationNow). Never later + * than the wall clock: previous-state lookups bound history by the wall clock, + * which only covers every window when the evaluation clock is not ahead of it. */ -const getAlertEvaluationNow = (alert: IAlert, now: Date): Date => - findAlertEvaluator(alert)?.evaluationNow?.(alert, now) ?? now; +const getAlertEvaluationNow = (alert: IAlert, now: Date): Date => { + const shifted = findAlertEvaluator(alert)?.evaluationNow?.(alert, now); + return shifted != null && shifted < now ? shifted : now; +}; /** * Compute the scheduled window start ("now rounded down to the window") for an @@ -454,7 +459,10 @@ const getAlertEvaluationNow = (alert: IAlert, now: Date): Date => * that history fetched up-front (see getConsecutiveWindowHistories) lines up * exactly with the window processAlert evaluates. */ -const getAlertWindowStart = (alert: IAlert, wallClockNow: Date): Date => { +export const getAlertWindowStart = ( + alert: IAlert, + wallClockNow: Date, +): Date => { const now = getAlertEvaluationNow(alert, wallClockNow); const windowSizeInMins = ms(alert.interval) / 60000; const scheduleStartAt = normalizeScheduleStartAt({ @@ -997,8 +1005,9 @@ export const processAlert = async ( teamWebhooksById: Map, ) => { const { alert, previousMap, recentHistoryMap } = details; - const now = getAlertEvaluationNow(alert, wallClockNow); - const evaluator = findAlertEvaluator(alert); + // Resolved inside the try so a failing evaluator lookup is recorded on the + // alert; hoisted so the finally can label the evaluation. + let evaluator: AlertEvaluator | undefined; const source = 'source' in details ? details.source : undefined; // Errors collected during this execution. Webhook errors accumulate here; query // and validation errors are recorded via recordAlertErrors before returning. @@ -1056,6 +1065,8 @@ export const processAlert = async ( .slice(0, ALERT_NOTIFICATION_TARGETS_LIMIT); }; try { + evaluator = findAlertEvaluator(alert); + const now = getAlertEvaluationNow(alert, wallClockNow); const windowSizeInMins = ms(alert.interval) / 60000; const scheduleStartAt = normalizeScheduleStartAt({ alertId: alert.id, From bf29ce8e3c7817e436769f83132067f5ef4054a9 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Fri, 6 Nov 2026 09:28:04 -0500 Subject: [PATCH 3/5] refactor(api): look up the alert evaluator once and persist threshold results through persistHistories Co-Authored-By: Claude Opus 5.5 --- packages/api/src/tasks/checkAlerts/index.ts | 34 ++++++--------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index ee10313c2d..389d4c4ef3 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -448,8 +448,12 @@ export const getScheduledWindowStart = ( * than the wall clock: previous-state lookups bound history by the wall clock, * which only covers every window when the evaluation clock is not ahead of it. */ -const getAlertEvaluationNow = (alert: IAlert, now: Date): Date => { - const shifted = findAlertEvaluator(alert)?.evaluationNow?.(alert, now); +const getAlertEvaluationNow = ( + alert: IAlert, + now: Date, + evaluator = findAlertEvaluator(alert), +): Date => { + const shifted = evaluator?.evaluationNow?.(alert, now); return shifted != null && shifted < now ? shifted : now; }; @@ -1066,7 +1070,7 @@ export const processAlert = async ( }; try { evaluator = findAlertEvaluator(alert); - const now = getAlertEvaluationNow(alert, wallClockNow); + const now = getAlertEvaluationNow(alert, wallClockNow, evaluator); const windowSizeInMins = ms(alert.interval) / 60000; const scheduleStartAt = normalizeScheduleStartAt({ alertId: alert.id, @@ -1604,17 +1608,7 @@ export const processAlert = async ( // Single-value evaluations always cover exactly the current window. evaluationAnalytics.backfilledBuckets = 0; - flushNotificationTimings(); - const historyRecords = Array.from(histories.values()); - for (const record of historyRecords) { - record.analytics = evaluationAnalytics; - } - await alertProvider.updateAlertState( - alert.id, - historyRecords, - executionErrors, - dateRange, - ); + await persistHistories(Array.from(histories.values())); return; } @@ -1838,17 +1832,7 @@ export const processAlert = async ( } // Save all history records and update alert state - flushNotificationTimings(); - const historyRecords = Array.from(histories.values()); - for (const record of historyRecords) { - record.analytics = evaluationAnalytics; - } - await alertProvider.updateAlertState( - alert.id, - historyRecords, - executionErrors, - dateRange, - ); + await persistHistories(Array.from(histories.values())); } catch (e) { // Uncomment this for better error messages locally // console.error(e); From 0d94bcf468821dd211fe37bfbc39ebc31896b086 Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 9 Nov 2026 12:44:51 -0500 Subject: [PATCH 4/5] fix(api): contain evaluator lookup failures in the history prefetch and keep errors from non-persisting evaluators - getConsecutiveWindowHistories catches a failing window-start lookup per alert, so one broken evaluator no longer rejects every alert's prefetch - an evaluator that returns without persistHistories still writes the collected execution errors to the alert - replace `as any` casts in evaluators.test.ts, which put the api package over its ESLint warning ceiling - add a changeset Co-Authored-By: Claude Opus 5.5 --- .changeset/alert-evaluator-extension-point.md | 5 ++ .../checkAlerts/__tests__/evaluators.test.ts | 88 ++++++++++++++++--- .../api/src/tasks/checkAlerts/evaluators.ts | 8 +- packages/api/src/tasks/checkAlerts/index.ts | 23 ++++- 4 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 .changeset/alert-evaluator-extension-point.md diff --git a/.changeset/alert-evaluator-extension-point.md b/.changeset/alert-evaluator-extension-point.md new file mode 100644 index 0000000000..ed39fc7334 --- /dev/null +++ b/.changeset/alert-evaluator-extension-point.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/api': patch +--- + +feat: add an internal extension point for alert detection strategies; threshold alerts are evaluated as before, and the `alerts.evaluate` metric now carries an `alert_evaluator` attribute diff --git a/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts index 6d0108876a..77b317248b 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/evaluators.test.ts @@ -1,8 +1,13 @@ -import { DisplayType } from '@hyperdx/common-utils/dist/types'; +import { AlertErrorType, DisplayType } from '@hyperdx/common-utils/dist/types'; import { Types } from 'mongoose'; -import { AlertState } from '@/models/alert'; -import { getAlertWindowStart, processAlert } from '@/tasks/checkAlerts'; +import { AlertState, IAlertError } from '@/models/alert'; +import AlertHistory from '@/models/alertHistory'; +import { + getAlertWindowStart, + getConsecutiveWindowHistories, + processAlert, +} from '@/tasks/checkAlerts'; import { AlertEvaluationContext, AlertEvaluator, @@ -35,6 +40,13 @@ const makeDetails = (alert: any): any => ({ }, }); +const makeFiredHistory = (): any => ({ + _id: new Types.ObjectId(), + createdAt: new Date(), + state: AlertState.ALERT, + fired: true, +}); + const makeProvider = (): any => ({ updateAlertState: jest.fn().mockResolvedValue(undefined), recordAlertErrors: jest.fn().mockResolvedValue(undefined), @@ -221,11 +233,7 @@ describe('alert evaluators', () => { const { ctx } = await evaluateWith(async c => { const history = c.getOrCreateHistory(''); history.state = AlertState.OK; - await c.sendNotificationIfResolved( - { state: AlertState.ALERT, fired: true } as any, - history, - '', - ); + await c.sendNotificationIfResolved(makeFiredHistory(), history, ''); }); // One dispatch attempt, failing on the missing team. @@ -233,9 +241,17 @@ describe('alert evaluators', () => { }); it('persists extra errors with the collected ones', async () => { - const extra = { type: 'INVALID_ALERT', message: 'extra' } as any; + const extra: IAlertError = { + timestamp: new Date(), + type: AlertErrorType.INVALID_ALERT, + message: 'extra', + }; const { provider } = await evaluateWith(async c => { - c.executionErrors.push({ type: 'WEBHOOK_ERROR', message: 'a' } as any); + c.executionErrors.push({ + timestamp: new Date(), + type: AlertErrorType.WEBHOOK_ERROR, + message: 'a', + }); await c.persistHistories([c.getOrCreateHistory('')], [extra]); }); @@ -287,6 +303,58 @@ describe('alert evaluators', () => { ).toEqual(new Date('2025-01-01T00:05:00Z')); }); + it('isolates a failing evaluator lookup during the history prefetch', async () => { + registerAlertEvaluator({ + name: 'broken', + appliesTo: alert => { + if (alert.threshold === 42) { + throw new Error('lookup failed'); + } + return false; + }, + evaluate: jest.fn(), + }); + const healthy = makeAlert({ numConsecutiveWindows: 2 }); + const broken = makeAlert({ numConsecutiveWindows: 2, threshold: 42 }); + const aggregate = jest + .spyOn(AlertHistory, 'aggregate') + .mockResolvedValue([ + { createdAt: new Date(), state: AlertState.ALERT, group: '' }, + ]); + + try { + const map = await getConsecutiveWindowHistories( + [broken, healthy], + new Date('2025-01-01T00:07:00Z'), + ); + + expect(aggregate).toHaveBeenCalledTimes(1); + expect(map.has(healthy.id)).toBe(true); + expect(map.has(broken.id)).toBe(false); + } finally { + aggregate.mockRestore(); + } + }); + + it('writes collected errors when the evaluator does not persist', async () => { + const { provider } = await evaluateWith(async c => { + await c.trySendNotification({ + state: AlertState.ALERT, + group: '', + totalCount: 5, + }); + }); + + expect(provider.updateAlertState).not.toHaveBeenCalled(); + expect(provider.recordAlertErrors).toHaveBeenCalledTimes(1); + const [, errors, windowStart] = provider.recordAlertErrors.mock.calls[0]; + expect(errors).toEqual([ + expect.objectContaining({ type: AlertErrorType.WEBHOOK_ERROR }), + ]); + // No history row: the evaluator decided not to record this window. + expect(windowStart).toBeUndefined(); + }); + it('records a failing evaluator lookup on the alert', async () => { registerAlertEvaluator({ name: 'broken', diff --git a/packages/api/src/tasks/checkAlerts/evaluators.ts b/packages/api/src/tasks/checkAlerts/evaluators.ts index 396f00cd90..c2504fffdb 100644 --- a/packages/api/src/tasks/checkAlerts/evaluators.ts +++ b/packages/api/src/tasks/checkAlerts/evaluators.ts @@ -85,7 +85,9 @@ export interface AlertEvaluator { */ evaluationNow?: (alert: IAlert, wallClockNow: Date) => Date; /** Evaluates the window and persists state through `ctx`. Throwing records - * the error on the alert, as for the threshold path. */ + * the error on the alert, as for the threshold path. Returning without + * calling `persistHistories` leaves state and history unchanged, but still + * writes `executionErrors` to the alert. */ evaluate: (ctx: AlertEvaluationContext) => Promise; } @@ -103,8 +105,6 @@ export const registerAlertEvaluator = (evaluator: AlertEvaluator) => { evaluators.push(evaluator); }; -/** The registered evaluator for this alert, or undefined for the threshold - * path. */ /** * Runs registerExtensions once. A failed registration is rolled back and * rethrown, and retried on the next lookup: routing alerts to the threshold @@ -125,6 +125,8 @@ const ensureExtensionsRegistered = () => { extensionsRegistered = true; }; +/** The registered evaluator for this alert, or undefined for the threshold + * path. */ export const findAlertEvaluator = ( alert: IAlert, ): AlertEvaluator | undefined => { diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 389d4c4ef3..3ad18042f1 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -1443,10 +1443,12 @@ export const processAlert = async ( } }; + let persisted = false; const persistHistories = async ( records: IAlertHistory[], extraErrors: IAlertError[] = [], ) => { + persisted = true; flushNotificationTimings(); // An evaluation that measured nothing persists no analytics object. if (Object.keys(evaluationAnalytics).length > 0) { @@ -1484,6 +1486,12 @@ export const processAlert = async ( persistHistories, }; await evaluator.evaluate(ctx); + if (!persisted) { + // Every completed evaluation overwrites the alert's execution errors, + // as the threshold path does through persistHistories. Without this, + // failed notifications would be dropped and stale errors would stay. + await alertProvider.recordAlertErrors(alert.id, executionErrors); + } return; } @@ -2031,7 +2039,20 @@ export const getConsecutiveWindowHistories = async ( queue.add(async () => { const numWindowsToLookBack = alert.numConsecutiveWindows ?? 1; const windowSizeInMins = ms(alert.interval) / 60000; - const windowStart = getAlertWindowStart(alert, now); + let windowStart: Date; + try { + windowStart = getAlertWindowStart(alert, now); + } catch (e) { + // The evaluator lookup runs extension code. A failure must not + // reject this batch, which would stop every alert loading this + // tick. processAlert repeats the lookup and records the error on + // this alert. + logger.error( + { alertId: alert.id, error: serializeError(e) }, + 'Failed to compute alert window start for history prefetch', + ); + return undefined; + } const earliestAllowedTime = new Date( windowStart.getTime() - (numWindowsToLookBack - 1) * windowSizeInMins * 60_000, From b563d7902c2131c58b6af26dfb7ad38876b0c61a Mon Sep 17 00:00:00 2001 From: Tom Alexander Date: Mon, 9 Nov 2026 14:09:51 -0500 Subject: [PATCH 5/5] chore: retrigger changeset-bot