diff --git a/.changeset/fix-alert-marker-bucket-alignment.md b/.changeset/fix-alert-marker-bucket-alignment.md new file mode 100644 index 0000000000..21bc168073 --- /dev/null +++ b/.changeset/fix-alert-marker-bucket-alignment.md @@ -0,0 +1,7 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/api": patch +"@hyperdx/app": patch +--- + +Align alert firing/recovery chart markers with the evaluated data: markers are now drawn at the start of the newest evaluated bucket (matching the evaluation history table and the plotted data point) instead of at the evaluation time, which sat one bucket to the right. diff --git a/packages/api/src/controllers/__tests__/alertHistory.int.test.ts b/packages/api/src/controllers/__tests__/alertHistory.int.test.ts index 71a1a31ade..12554ebcd8 100644 --- a/packages/api/src/controllers/__tests__/alertHistory.int.test.ts +++ b/packages/api/src/controllers/__tests__/alertHistory.int.test.ts @@ -1,4 +1,5 @@ import { ObjectId } from 'mongodb'; +import { Types } from 'mongoose'; import { ALERT_EVALUATION_GROUPS_LIMIT, @@ -498,7 +499,7 @@ describe('alertHistory controller', () => { }); }; - const createOkWindow = (alertId: any, createdAt: Date) => + const createOkWindow = (alertId: Types.ObjectId, createdAt: Date) => AlertHistory.create({ alert: alertId, createdAt, @@ -1052,7 +1053,7 @@ describe('alertHistory controller', () => { }; const createHistory = ( - alertId: any, + alertId: Types.ObjectId, createdAt: Date, state: AlertState, counts: number, @@ -1262,5 +1263,194 @@ describe('alertHistory controller', () => { expect(transitions).toHaveLength(1); expect(transitions[0].state).toBe(AlertState.ALERT); }); + + describe('bucketStart', () => { + const createHistoryWithBuckets = ( + alertId: Types.ObjectId, + createdAt: Date, + state: AlertState, + bucketStarts: Date[], + group?: string, + ) => + AlertHistory.create({ + alert: alertId, + createdAt, + state, + counts: state === AlertState.ALERT ? 1 : 0, + ...(group != null ? { group } : {}), + lastValues: bucketStarts.map(startTime => ({ + startTime, + count: 1, + })), + }); + + it('emits the newest evaluated bucket start for each transition', async () => { + // Evaluation at t(20) covered 1-minute buckets t(25)..t(21) (anomaly + // alerts bucket finer than the interval) — the marker belongs on the + // newest bucket, where the chart plots the transitioning value. + const alert = await createAlert(); + await createHistory(alert._id, t(25), AlertState.OK, 0); + await createHistoryWithBuckets(alert._id, t(20), AlertState.ALERT, [ + t(25), + t(24), + t(23), + t(22), + t(21), + ]); + + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime: t(22), + endTime: t(5), + }); + + expect(transitions).toHaveLength(1); + expect(transitions[0].createdAt).toBe(t(20).toISOString()); + expect(transitions[0].bucketStart).toBe(t(21).toISOString()); + }); + + it('falls back to createdAt − interval when the window has no lastValues', async () => { + const alert = await createAlert(); + await createHistory(alert._id, t(30), AlertState.OK, 0); + await createHistoryWithBuckets(alert._id, t(20), AlertState.ALERT, []); + + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime: t(28), + endTime: t(5), + }); + + expect(transitions).toHaveLength(1); + expect(transitions[0].bucketStart).toBe(t(25).toISOString()); + }); + + it("derives a recovery's bucketStart from the recovering window's lastValues", async () => { + const alert = await createAlert(); + await createHistory(alert._id, t(30), AlertState.OK, 0); + await createHistoryWithBuckets(alert._id, t(25), AlertState.ALERT, [ + t(27), + t(26), + ]); + await createHistoryWithBuckets(alert._id, t(20), AlertState.OK, [ + t(22), + t(21), + ]); + + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime: t(26), + endTime: t(5), + }); + + expect(transitions.map(tr => tr.state)).toEqual([ + AlertState.ALERT, + AlertState.OK, + ]); + expect(transitions[1].createdAt).toBe(t(20).toISOString()); + expect(transitions[1].bucketStart).toBe(t(21).toISOString()); + }); + + it('floors bucketStart at the range start for an edge crossing', async () => { + // A crossing whose evaluation lands just inside the range derives a + // bucketStart one bucket earlier — before startTime. It is floored at + // startTime so the marker never renders left of a carry-in pin. + const alert = await createAlert(); + await createHistory(alert._id, t(30), AlertState.OK, 0); + await createHistoryWithBuckets(alert._id, t(25), AlertState.ALERT, []); + + const startTime = t(25); // crossing evaluated exactly at range start + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime, + endTime: t(5), + }); + + expect(transitions).toHaveLength(1); + expect(transitions[0].state).toBe(AlertState.ALERT); + // Empty lastValues → createdAt − interval = t(30), before startTime — + // floored to startTime. + expect(transitions[0].bucketStart).toBe(startTime.toISOString()); + }); + + it('keeps a recovery after the carry-in pin it follows', async () => { + // Firing on entry, recovering on the first in-range tick: the + // recovery's derived bucket start precedes startTime, but the carry-in + // pin sits at startTime — the floor keeps firing → recovery ordered. + const alert = await createAlert(); + await createHistory(alert._id, t(30), AlertState.ALERT, 5); + await createHistoryWithBuckets(alert._id, t(25), AlertState.OK, []); + + const startTime = t(25); + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime, + endTime: t(5), + }); + + expect(transitions.map(tr => tr.state)).toEqual([ + AlertState.ALERT, + AlertState.OK, + ]); + expect(transitions[0].bucketStart).toBe(startTime.toISOString()); + expect( + new Date(transitions[1].bucketStart!) >= + new Date(transitions[0].bucketStart!), + ).toBe(true); + }); + + it('takes the newest bucket across group rows in one window', async () => { + // Group-by alerts write one history row per group; the marker should + // reflect the newest bucket evaluated by any of them. + const alert = await createAlert(); + await createHistory(alert._id, t(25), AlertState.OK, 0); + await createHistoryWithBuckets( + alert._id, + t(20), + AlertState.ALERT, + [t(23)], + 'group-a', + ); + await createHistoryWithBuckets( + alert._id, + t(20), + AlertState.OK, + [t(21)], + 'group-b', + ); + + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime: t(22), + endTime: t(5), + }); + + expect(transitions).toHaveLength(1); + expect(transitions[0].state).toBe(AlertState.ALERT); + expect(transitions[0].bucketStart).toBe(t(21).toISOString()); + }); + + it('pins the carry-in marker bucketStart to the range start', async () => { + const alert = await createAlert(); + await createHistory(alert._id, t(30), AlertState.ALERT, 5); + await createHistory(alert._id, t(25), AlertState.ALERT, 5); + + const startTime = t(27); + const transitions = await getAlertTransitionsInRange({ + alertId: new ObjectId(alert._id), + interval: '5m', + startTime, + endTime: t(5), + }); + + expect(transitions).toHaveLength(1); + expect(transitions[0].bucketStart).toBe(startTime.toISOString()); + }); + }); }); }); diff --git a/packages/api/src/controllers/alertHistory.ts b/packages/api/src/controllers/alertHistory.ts index 6ff1e9381b..0c120d05cf 100644 --- a/packages/api/src/controllers/alertHistory.ts +++ b/packages/api/src/controllers/alertHistory.ts @@ -462,11 +462,15 @@ export async function getRecentAlertHistoriesBatch( /** * Returns alert firing/recovery transitions (ALERT-boundary crossings) within - * [startTime, endTime] for one alert, for drawing chart annotations. One window - * before startTime is fetched to know the state on entry: if the alert is - * already firing then, a firing marker is pinned to startTime so a later - * in-range recovery isn't orphaned. PENDING/INSUFFICIENT_DATA count as - * non-firing, so only ALERT crossings are reported. + * [startTime, endTime] for one alert, for drawing chart annotations. Each + * transition carries `bucketStart` — the start of the newest bucket the + * transitioning evaluation covered — so markers land on the data point that + * produced the transition (charts plot buckets at their start, while the + * evaluation runs at the bucket end). One window before startTime is fetched + * to know the state on entry: if the alert is already firing then, a firing + * marker is pinned to startTime so a later in-range recovery isn't orphaned. + * PENDING/INSUFFICIENT_DATA count as non-firing, so only ALERT crossings are + * reported. */ export async function getAlertTransitionsInRange({ alertId, @@ -482,22 +486,51 @@ export async function getAlertTransitionsInRange({ const intervalMs = intervalToMs(interval); const lookbackStart = new Date(startTime.getTime() - intervalMs); - // Only the per-window state is needed to detect crossings. ERROR rows are - // failed evaluations, not state observations — excluding them prevents a + // Per-window state detects crossings; the newest evaluated bucket start + // positions the marker where the chart plots that bucket's value. ERROR rows + // are failed evaluations, not state observations — excluding them prevents a // query failure mid-firing from drawing a false recovery annotation. - const windows = await AlertHistory.aggregate<{ _id: Date; states: string[] }>( - [ - { - $match: { - alert: new ObjectId(alertId), - createdAt: { $gte: lookbackStart, $lte: endTime }, - state: { $ne: AlertState.ERROR }, - }, + const windows = await AlertHistory.aggregate<{ + _id: Date; + states: string[]; + // One array of bucket-start dates per row in the window (one row per + // group for group-by alerts). Push only the dates via a plain field path + // and derive the newest in JS — the file's DocumentDB-safe pattern (see + // mapGroupedHistories) avoids expression operators inside accumulators, + // and pushing dates instead of whole lastValues rows keeps the buffered + // payload small. The router caps the scanned span (MAX_HISTORY_SPAN_MS). + bucketStarts: Date[][]; + }>([ + { + $match: { + alert: new ObjectId(alertId), + createdAt: { $gte: lookbackStart, $lte: endTime }, + state: { $ne: AlertState.ERROR }, }, - { $group: { _id: '$createdAt', states: { $push: '$state' } } }, - { $sort: { _id: 1 } }, - ], - ); + }, + { + $group: { + _id: '$createdAt', + states: { $push: '$state' }, + bucketStarts: { $push: '$lastValues.startTime' }, + }, + }, + { $sort: { _id: 1 } }, + ]); + + // Newest bucket start across the window's rows; null when no row carries + // lastValues. Be defensive about missing arrays/fields in case of engine + // differences (e.g. DocumentDB). + const newestBucketStart = (bucketStarts: Date[][] | undefined): Date | null => + (bucketStarts ?? []) + .flat() + .reduce( + (max, startedAt) => + startedAt != null && (max == null || startedAt > max) + ? startedAt + : max, + null, + ); const transitions: AlertTransition[] = []; // Assume "not firing" before the earliest known window, so an alert whose @@ -506,12 +539,14 @@ export async function getAlertTransitionsInRange({ let enteredRange = false; // Pin a firing marker to the range start if the alert was already firing on - // entry (carried in from before startTime). + // entry (carried in from before startTime). The marker is synthetic ("firing + // when the window opens"), so it carries no bucketStart of its own. const pinCarryInIfFiring = () => { if (prevIsAlert) { transitions.push({ createdAt: startTime.toISOString(), state: AlertState.ALERT, + bucketStart: startTime.toISOString(), }); } }; @@ -527,9 +562,20 @@ export async function getAlertTransitionsInRange({ } if (inRange && isAlert !== prevIsAlert) { + // Fall back to createdAt − interval when the window has no lastValues + // (mirrors the evaluation table's fallback for failed evaluations). + const derived = + newestBucketStart(evalWindow.bucketStarts) ?? + new Date(evalWindow._id.getTime() - intervalMs); + // Floor at startTime: a derived bucket start one bucket earlier than an + // edge crossing's createdAt can precede the range start, and a marker + // before startTime would render at or left of a carry-in pin (which is + // pinned to startTime) — inverting the firing→recovery order. + const bucketStart = derived >= startTime ? derived : startTime; transitions.push({ createdAt: evalWindow._id.toISOString(), state: isAlert ? AlertState.ALERT : AlertState.OK, + bucketStart: bucketStart.toISOString(), }); } diff --git a/packages/app/src/hooks/__tests__/useAlertAnnotations.test.tsx b/packages/app/src/hooks/__tests__/useAlertAnnotations.test.tsx index b0138de720..b25228f9f0 100644 --- a/packages/app/src/hooks/__tests__/useAlertAnnotations.test.tsx +++ b/packages/app/src/hooks/__tests__/useAlertAnnotations.test.tsx @@ -11,7 +11,8 @@ import { getChartColorError, getChartColorSuccess } from '@/utils'; const makeTransition = ( createdAt: string, state: AlertState, -): AlertTransition => ({ createdAt, state }); + bucketStart?: string, +): AlertTransition => ({ createdAt, state, bucketStart }); describe('alertTransitionsToAnnotations', () => { it('returns no annotations for an empty list', () => { @@ -41,6 +42,64 @@ describe('alertTransitionsToAnnotations', () => { // Distinct keys so React can reconcile the markers. expect(annotations[0].key).not.toEqual(annotations[1].key); }); + + it('draws the marker at bucketStart so it lines up with the plotted bucket', () => { + // Evaluation ran at 00:30 over buckets whose newest starts at 00:29 — + // the chart plots that bucket at 00:29, so the marker must sit there too. + const evaluatedAt = '2026-07-01T00:30:00.000Z'; + const bucketStart = '2026-07-01T00:29:00.000Z'; + + const annotations = alertTransitionsToAnnotations([ + makeTransition(evaluatedAt, AlertState.OK, bucketStart), + ]); + + expect(annotations).toHaveLength(1); + expect(annotations[0]).toMatchObject({ time: bucketStart, label: 'OK' }); + }); + + it('maps each transition to its own bucketStart', () => { + // Fire and recovery in one range, each evaluated over different buckets — + // every marker must land on its own transition's newest bucket. + const firedAt = '2026-07-01T00:10:00.000Z'; + const firedBucket = '2026-07-01T00:09:00.000Z'; + const recoveredAt = '2026-07-01T00:30:00.000Z'; + const recoveredBucket = '2026-07-01T00:29:00.000Z'; + + const annotations = alertTransitionsToAnnotations([ + makeTransition(firedAt, AlertState.ALERT, firedBucket), + makeTransition(recoveredAt, AlertState.OK, recoveredBucket), + ]); + + expect(annotations).toHaveLength(2); + expect(annotations[0]).toMatchObject({ time: firedBucket, label: 'Alert' }); + expect(annotations[1]).toMatchObject({ + time: recoveredBucket, + label: 'OK', + }); + }); + + it('falls back to createdAt when bucketStart is absent (older API)', () => { + const evaluatedAt = '2026-07-01T00:30:00.000Z'; + + const annotations = alertTransitionsToAnnotations([ + makeTransition(evaluatedAt, AlertState.ALERT), + ]); + + expect(annotations[0]).toMatchObject({ time: evaluatedAt }); + }); + + it('keeps keys distinct for opposite-state transitions sharing a time', () => { + // Keys derive from the annotation time — the state must keep two markers + // at the same instant reconcilable as distinct React elements. + const bucketStart = '2026-07-01T00:29:00.000Z'; + + const annotations = alertTransitionsToAnnotations([ + makeTransition('2026-07-01T00:30:00.000Z', AlertState.ALERT, bucketStart), + makeTransition('2026-07-01T00:35:00.000Z', AlertState.OK, bucketStart), + ]); + + expect(annotations[0].key).not.toEqual(annotations[1].key); + }); }); describe('useAlertAnnotations', () => { diff --git a/packages/app/src/hooks/useAlertAnnotations.tsx b/packages/app/src/hooks/useAlertAnnotations.tsx index c1577927fb..291b4868bf 100644 --- a/packages/app/src/hooks/useAlertAnnotations.tsx +++ b/packages/app/src/hooks/useAlertAnnotations.tsx @@ -8,7 +8,12 @@ import { getChartColorError, getChartColorSuccess } from '@/utils'; /** * Maps alert state transitions to generic chart annotations: firing (→ ALERT) * is a red "Alert" marker, recovery (→ OK) a green "OK" marker. Colors come - * from the theme's semantic chart palette (error / success). + * from the theme's semantic chart palette (error / success). Markers are drawn + * at the transition's `bucketStart` — charts plot each bucket's value at its + * start, while the evaluation runs at the bucket end (`createdAt`) — so the + * marker lines up with the data point that produced the transition (and with + * the evaluation table, which shows the same bucket start). `createdAt` is the + * fallback for older API responses without `bucketStart`. */ export function alertTransitionsToAnnotations( transitions: AlertTransition[], @@ -18,11 +23,12 @@ export function alertTransitionsToAnnotations( const okColor = getChartColorSuccess(); return transitions.map(transition => { const isFiring = transition.state === AlertState.ALERT; + const time = transition.bucketStart ?? transition.createdAt; return { - time: transition.createdAt, + time, label: isFiring ? 'Alert' : 'OK', color: isFiring ? alertColor : okColor, - key: `alert-annotation-${transition.createdAt}-${transition.state}`, + key: `alert-annotation-${time}-${transition.state}`, }; }); } diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 5e60bd0276..e5ee367a93 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -882,8 +882,16 @@ export const ALERT_EVALUATION_GROUPS_LIMIT = 50; // firing/recovery annotations on dashboard charts. Only boundary crossings are // emitted: ALERT = fired, OK = recovered. export const AlertTransitionSchema = z.object({ - createdAt: z.string(), + createdAt: z.string().datetime(), state: z.nativeEnum(AlertState), + // Start of the newest bucket evaluated by the transitioning window. Charts + // plot each bucket's value at its *start*, while the evaluation runs at the + // bucket *end* (createdAt) — markers drawn at bucketStart line up with the + // data point that produced the transition. Optional for compatibility with + // older API responses; consumers fall back to createdAt. Floored at the + // requested range start so an edge crossing's marker never precedes a + // carry-in pin; charts clamp edge markers into their rendered domain. + bucketStart: z.string().datetime().optional(), }); export type AlertTransition = z.infer;