From 8ce84f5e41d5c4d28d8cbc45203600f4ec00f971 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 26 Aug 2026 11:43:06 +1000 Subject: [PATCH 1/2] feat: attribute alert notification time to each target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit webhookDurationMs covered the whole delivery, and since targets dispatch concurrently the slowest one sets it — so a multi-target alert reported a number with no way to tell which webhook was responsible. Time each dispatch and aggregate per target across the evaluation: a grouped alert notifies the same target once per firing group and again on resolve, so entries carry a summed duration, a dispatch count and a failure count. Stored per evaluation rather than per dispatch, since 50 groups x 10 targets would write 500 entries onto every history row. The evaluation history cell expands in place to show the breakdown, rather than adding a second row-level expander to compete with the existing one. --- .../alert-per-target-notification-timings.md | 11 +++ packages/api/src/models/alertHistory.ts | 24 +++++- .../__tests__/checkAlerts.int.test.ts | 10 +++ packages/api/src/tasks/checkAlerts/index.ts | 61 +++++++++++++- .../api/src/tasks/checkAlerts/template.ts | 34 +++++++- .../components/alerts/AlertEvaluationRow.tsx | 3 +- .../alerts/NotificationDurationCell.tsx | 80 +++++++++++++++++++ .../NotificationDurationCell.test.tsx | 77 ++++++++++++++++++ packages/common-utils/src/types.ts | 41 ++++++++++ 9 files changed, 334 insertions(+), 7 deletions(-) create mode 100644 .changeset/alert-per-target-notification-timings.md create mode 100644 packages/app/src/components/alerts/NotificationDurationCell.tsx create mode 100644 packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx diff --git a/.changeset/alert-per-target-notification-timings.md b/.changeset/alert-per-target-notification-timings.md new file mode 100644 index 0000000000..bfe7f63326 --- /dev/null +++ b/.changeset/alert-per-target-notification-timings.md @@ -0,0 +1,11 @@ +--- +'@hyperdx/api': minor +'@hyperdx/app': minor +'@hyperdx/common-utils': minor +--- + +Record and show which notification target an evaluation's delivery time went to. `webhookDurationMs` was a single number covering the whole delivery, and because targets are dispatched concurrently the slowest one sets it — so a multi-target alert reported a figure with no way to tell which webhook was responsible, or that the other targets were fine. + +Each dispatch is now timed individually and aggregated per target across the evaluation, since a grouped alert notifies the same target once per firing group and again on resolve. One entry per distinct target carries its summed duration, how many dispatches it took, and how many failed. The evaluation history's "Notification duration" cell expands in place to show the breakdown. + +Stored per evaluation rather than per dispatch: a 50-group alert notifying 10 targets would otherwise write 500 entries onto every history row. The array is capped at `ALERT_NOTIFICATION_TARGETS_LIMIT` and sorted slowest-first, so the cap drops the least interesting rows. Records written before this change keep rendering their total with nothing to expand. diff --git a/packages/api/src/models/alertHistory.ts b/packages/api/src/models/alertHistory.ts index 78b968208c..1f23b0e3dc 100644 --- a/packages/api/src/models/alertHistory.ts +++ b/packages/api/src/models/alertHistory.ts @@ -1,4 +1,7 @@ -import { AlertErrorType } from '@hyperdx/common-utils/dist/types'; +import { + AlertErrorType, + AlertNotificationTargetTiming, +} from '@hyperdx/common-utils/dist/types'; import mongoose, { Schema } from 'mongoose'; import ms from 'ms'; @@ -28,6 +31,12 @@ export interface IAlertHistoryAnalytics { * (expected buckets − 1). 0 in steady state. */ backfilledBuckets?: number; + /** + * Per-target breakdown of `webhookDurationMs`, one entry per distinct + * target, slowest first. Targets dispatch concurrently, so these do not sum + * to `webhookDurationMs`. Absent when the evaluation sent nothing. + */ + notificationTargets?: AlertNotificationTargetTiming[]; } export interface IAlertHistory { @@ -105,6 +114,19 @@ const AlertHistorySchema = new Schema({ queryDurationMs: { type: Number, required: false }, webhookDurationMs: { type: Number, required: false }, backfilledBuckets: { type: Number, required: false }, + notificationTargets: { + type: [ + { + _id: false, + target: { type: String, required: true }, + durationMs: { type: Number, required: true }, + dispatches: { type: Number, required: true }, + failures: { type: Number, required: true }, + }, + ], + required: false, + default: undefined, + }, }, required: false, default: undefined, 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 de95dc1fd8..38572a2092 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -3345,6 +3345,16 @@ describe('checkAlerts', () => { expect(normalHistories[0].analytics!.webhookDurationMs).toEqual( expect.any(Number), ); + // Per-target breakdown of that total: one entry for the alert's single + // configured webhook, named so the UI can attribute the time. Read + // field by field — these come back as Mongoose subdocuments, which + // don't deep-equal a plain object literal. + const targets = normalHistories[0].analytics!.notificationTargets; + expect(targets).toHaveLength(1); + expect(targets![0].target).toBe(webhook.name); + expect(targets![0].durationMs).toEqual(expect.any(Number)); + expect(targets![0].dispatches).toBe(1); + expect(targets![0].failures).toBe(0); }); it('keeps ERROR rows from older windows when a later window succeeds', async () => { diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index b5ee953242..0f11f496b8 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -33,7 +33,9 @@ import { isRawSqlSavedChartConfig, } from '@hyperdx/common-utils/dist/guards'; import { + ALERT_NOTIFICATION_TARGETS_LIMIT, AlertErrorType, + AlertNotificationTargetTiming, AlertThresholdType, BuilderChartConfigWithOptDateRange, ChartConfigWithOptDateRange, @@ -80,7 +82,9 @@ import { AlertMessageTemplateDefaultView, buildAlertMessageTemplateTitle, NotificationFailure, + NotificationTiming, renderAlertTemplate, + RenderedAlert, } from '@/tasks/checkAlerts/template'; import { handleSendGenericWebhook } from '@/tasks/checkAlerts/transports'; import { tasksTracer } from '@/tasks/tracer'; @@ -494,7 +498,7 @@ const fireChannelEvent = async ({ totalCount: number; windowSizeInMins: number; teamWebhooksById: Map; -}): Promise => { +}): Promise> => { const team = alert.team; if (team == null) { throw new Error('Team not found'); @@ -546,7 +550,7 @@ const fireChannelEvent = async ({ value: totalCount, }; - const { failures } = await renderAlertTemplate({ + const { failures, timings } = await renderAlertTemplate({ alertProvider, clickhouseClient, metadata, @@ -561,7 +565,7 @@ const fireChannelEvent = async ({ teamId, teamWebhooksById, }); - return failures; + return { failures, timings }; }; // Use a delimiter that's unlikely to appear in alert IDs or group names @@ -925,6 +929,49 @@ export const processAlert = async ( // (query duration, webhook delivery time, backfilled buckets). Populated // progressively; hoisted so the catch blocks can attach what was measured. const evaluationAnalytics: IAlertHistoryAnalytics = {}; + // Per-target notification timings, keyed by webhook id so the same target + // notified for several groups (and again on resolve) aggregates into one + // entry rather than one per dispatch. + const notificationTimings = new Map< + string, + AlertNotificationTargetTiming & { key: string } + >(); + const recordNotificationTimings = (timings: NotificationTiming[]) => { + for (const timing of timings) { + const existing = notificationTimings.get(timing.key); + if (existing == null) { + notificationTimings.set(timing.key, { + key: timing.key, + target: timing.target, + durationMs: timing.durationMs, + dispatches: 1, + failures: timing.ok ? 0 : 1, + }); + continue; + } + existing.durationMs += timing.durationMs; + existing.dispatches += 1; + existing.failures += timing.ok ? 0 : 1; + } + }; + /** + * Fold the aggregated timings onto the analytics object. Called before the + * records are written, from both the success and the error path, so a + * failed evaluation still reports what it managed to deliver. + */ + const flushNotificationTimings = () => { + if (notificationTimings.size === 0) { + return; + } + evaluationAnalytics.notificationTargets = Array.from( + notificationTimings.values(), + ) + // Slowest first: the point of the breakdown is finding what dominated + // the total, and the cap below should drop the least interesting rows. + .sort((a, b) => b.durationMs - a.durationMs) + .slice(0, ALERT_NOTIFICATION_TARGETS_LIMIT) + .map(({ key: _key, ...timing }) => timing); + }; try { const windowSizeInMins = ms(alert.interval) / 60000; const scheduleStartAt = normalizeScheduleStartAt({ @@ -1232,7 +1279,7 @@ export const processAlert = async ( // alert logic requiring large, nested objects. We should look at // cleaning this up next. fireChannelEvent guards against null values // for these properties. - const failures = await fireChannelEvent({ + const { failures, timings } = await fireChannelEvent({ alert, alertProvider, attributes, @@ -1250,6 +1297,7 @@ export const processAlert = async ( windowSizeInMins, teamWebhooksById, }); + recordNotificationTimings(timings); // Each entry is a target that didn't end up delivered: unresolvable, // capped, or (for the inline dispatcher) an actual send rejection — // see renderAlertTemplate. @@ -1373,6 +1421,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; @@ -1595,6 +1644,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; @@ -1623,6 +1673,9 @@ export const processAlert = async ( e instanceof InvalidAlertError ? AlertErrorType.INVALID_ALERT : AlertErrorType.UNKNOWN; + // An evaluation can notify some targets and then fail; report what it + // managed to deliver rather than dropping the timings with the error. + flushNotificationTimings(); try { await alertProvider.recordAlertErrors( alert.id, diff --git a/packages/api/src/tasks/checkAlerts/template.ts b/packages/api/src/tasks/checkAlerts/template.ts index 502d1a19fc..f3aa65b027 100644 --- a/packages/api/src/tasks/checkAlerts/template.ts +++ b/packages/api/src/tasks/checkAlerts/template.ts @@ -341,11 +341,30 @@ const channelKey = (c: PopulatedAlertChannel) => const channelLabel = (c: PopulatedAlertChannel) => c.type === 'webhook' ? c.channel.name : c.type; +/** + * One dispatch's wall time. Emitted per target per event, so a grouped alert + * produces one of these per (group, target); the caller aggregates. + */ +export type NotificationTiming = { + /** Stable identity for aggregation across events — the webhook id. */ + key: string; + /** Display label: the webhook's name. */ + target: string; + durationMs: number; + ok: boolean; +}; + export type RenderedAlert = { /** The rendered message body, as delivered to every target. */ body: string; /** One entry per target that did not end up delivered — see NotificationFailure. */ failures: NotificationFailure[]; + /** + * One entry per target that reached the dispatcher, delivered or not. + * Targets that failed before dispatch have no timing — there was nothing to + * time — so this is not the complement of `failures`. + */ + timings: NotificationTiming[]; }; // this method will build the body of the alert message and will be used to send the alert to the channel @@ -733,11 +752,17 @@ ${targetTemplate}`; // queued dispatcher resolves after enqueue and never rejects here; it // reports delivery outcomes through its own logs/metrics instead (see // agent_docs/observability.md). + const timings: NotificationTiming[] = []; await Promise.all( jobs.map(async job => { + // Per-job, not around the Promise.all: the whole point is attributing + // the total to a target, and the dispatches overlap. + const startedAt = performance.now(); + let ok = true; try { await dispatcher.dispatch(job); } catch (e) { + ok = false; logger.error( { alertId: alert.id, @@ -751,11 +776,18 @@ ${targetTemplate}`; type: job.populatedChannel.type, error: e, }); + } finally { + timings.push({ + key: channelKey(job.populatedChannel), + target: channelLabel(job.populatedChannel), + durationMs: Math.round(performance.now() - startedAt), + ok, + }); } }), ); - return { body, failures }; + return { body, failures, timings }; } throw new Error(`Unsupported alert source: ${alert.source}`); diff --git a/packages/app/src/components/alerts/AlertEvaluationRow.tsx b/packages/app/src/components/alerts/AlertEvaluationRow.tsx index d87ab766ee..d17bc1ea1b 100644 --- a/packages/app/src/components/alerts/AlertEvaluationRow.tsx +++ b/packages/app/src/components/alerts/AlertEvaluationRow.tsx @@ -15,6 +15,7 @@ import { AlertErrorsContent, } from '@/components/alerts/AlertHistoryCards'; import { AlertStateBadge } from '@/components/alerts/AlertStateBadge'; +import { NotificationDurationCell } from '@/components/alerts/NotificationDurationCell'; import { FormatTime } from '@/useFormatTime'; import { formatDurationMs } from '@/utils'; @@ -184,7 +185,7 @@ export function AlertEvaluationRow({ {durationCell(history.analytics?.queryDurationMs)} - {durationCell(history.analytics?.webhookDurationMs)} + {hasErrors ? ( diff --git a/packages/app/src/components/alerts/NotificationDurationCell.tsx b/packages/app/src/components/alerts/NotificationDurationCell.tsx new file mode 100644 index 0000000000..43b2a6e005 --- /dev/null +++ b/packages/app/src/components/alerts/NotificationDurationCell.tsx @@ -0,0 +1,80 @@ +import * as React from 'react'; +import type { AlertHistoryAnalytics } from '@hyperdx/common-utils/dist/types'; +import { Collapse, Group, Stack, Text, UnstyledButton } from '@mantine/core'; +import { IconChevronDown, IconChevronRight } from '@tabler/icons-react'; + +import { formatDurationMs } from '@/utils'; + +/** + * The evaluation's notification wall time, expandable in place into a + * per-target breakdown. + * + * It expands *within* the cell rather than adding child rows: the parent row + * already owns a chevron for groups and errors, and a second row-level + * expander competing with it would be ambiguous to click. + */ +export function NotificationDurationCell({ + analytics, +}: { + analytics?: AlertHistoryAnalytics; +}) { + const [expanded, setExpanded] = React.useState(false); + const total = analytics?.webhookDurationMs; + const targets = analytics?.notificationTargets ?? []; + + if (total == null) { + return <>–; + } + + // Records written before per-target timing existed have the total but no + // breakdown, so there is nothing to expand into. + if (targets.length === 0) { + return {formatDurationMs(total)}; + } + + return ( + + { + event.stopPropagation(); + setExpanded(value => !value); + }} + aria-expanded={expanded} + data-testid="notification-duration-toggle" + > + + {formatDurationMs(total)} + {expanded ? ( + + ) : ( + + )} + + + + + {targets.map(target => ( + + + {target.target} + + {formatDurationMs(target.durationMs)} + {target.dispatches > 1 && ( + + ×{target.dispatches} + + )} + {target.failures > 0 && ( + + {target.failures} failed + + )} + + ))} + + + + ); +} diff --git a/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx b/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx new file mode 100644 index 0000000000..1c4d1fd467 --- /dev/null +++ b/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx @@ -0,0 +1,77 @@ +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +import { NotificationDurationCell } from '@/components/alerts/NotificationDurationCell'; + +const analytics = { + webhookDurationMs: 4120, + notificationTargets: [ + { target: 'Team Slack', durationMs: 4120, dispatches: 50, failures: 0 }, + { target: 'Ops webhook', durationMs: 210, dispatches: 50, failures: 2 }, + ], +}; + +describe('NotificationDurationCell', () => { + it('shows a dash when the evaluation notified nothing', () => { + renderWithMantine(); + + expect(screen.getByText('–')).toBeInTheDocument(); + }); + + // Records written before per-target timing have the total but no breakdown. + it('shows the total with no expander when there is no breakdown', () => { + renderWithMantine( + , + ); + + expect(screen.getByText('210ms')).toBeInTheDocument(); + expect( + screen.queryByTestId('notification-duration-toggle'), + ).not.toBeInTheDocument(); + }); + + // Mantine's Collapse keeps its children mounted and hides them with height, + // so the breakdown is in the DOM either way — asserting on its content alone + // would pass without ever clicking. aria-expanded is the part jsdom can see + // change. + it('toggles expansion', async () => { + renderWithMantine(); + const toggle = screen.getByTestId('notification-duration-toggle'); + + expect(toggle).toHaveAttribute('aria-expanded', 'false'); + await userEvent.click(toggle); + expect(toggle).toHaveAttribute('aria-expanded', 'true'); + await userEvent.click(toggle); + expect(toggle).toHaveAttribute('aria-expanded', 'false'); + }); + + it('renders a row per target with its own duration', () => { + renderWithMantine(); + + const breakdown = screen.getByTestId('notification-duration-breakdown'); + expect(breakdown).toHaveTextContent('Team Slack'); + expect(breakdown).toHaveTextContent('Ops webhook'); + // The slow target's own time, which the collapsed total hides behind a + // single figure. + expect(breakdown).toHaveTextContent('210ms'); + expect(breakdown).toHaveTextContent('2 failed'); + // Repeated dispatches are marked, so a 50-group total doesn't read as one + // slow send. + expect(breakdown).toHaveTextContent('×50'); + }); + + // The parent row toggles its own expansion on click, so the cell's expander + // must not bubble into it. + it('does not propagate the toggle click to the row', async () => { + const onRowClick = jest.fn(); + renderWithMantine( +
+ +
, + ); + + await userEvent.click(screen.getByTestId('notification-duration-toggle')); + + expect(onRowClick).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 5a14c12825..eea6193f1f 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -972,6 +972,36 @@ export const AlertSchema = z.union([ export type Alert = z.infer; +/** + * Max per-target timing entries stored on one evaluation. An evaluation's + * distinct targets are already bounded (configured channels plus whatever the + * message body @mentions, itself capped per event), so this only guards + * against a pathological alert growing the document. Shared so the app can + * explain the truncation it renders. + */ +export const ALERT_NOTIFICATION_TARGETS_LIMIT = 20; + +/** + * One notification target's timing within an evaluation, summed across every + * dispatch the evaluation made to it — a grouped alert notifies the same + * target once per firing group, and a resolve notification is another + * dispatch. + */ +export const AlertNotificationTargetTimingSchema = z.object({ + /** Display label: the webhook's name as it was at dispatch time. */ + target: z.string(), + /** Summed wall time across this target's dispatches (ms). */ + durationMs: z.number(), + /** Dispatches attempted for this target in the evaluation. */ + dispatches: z.number(), + /** How many of those dispatches failed. */ + failures: z.number(), +}); + +export type AlertNotificationTargetTiming = z.infer< + typeof AlertNotificationTargetTimingSchema +>; + // Diagnostics for the evaluation that wrote a history record. Evaluation- // level: identical on every row one evaluation writes (incl. per-group rows). export const AlertHistoryAnalyticsSchema = z.object({ @@ -981,6 +1011,17 @@ export const AlertHistoryAnalyticsSchema = z.object({ webhookDurationMs: z.number().optional(), /** Earlier buckets backfilled in this run after missed ticks (expected buckets − 1). */ backfilledBuckets: z.number().optional(), + /** + * Per-target breakdown of `webhookDurationMs`, highest duration first. + * Targets are dispatched concurrently, so these do not sum to + * `webhookDurationMs` — the slowest target in each dispatch round sets the + * total. Absent on evaluations that sent nothing, and on records written + * before per-target timing existed. + */ + notificationTargets: z + .array(AlertNotificationTargetTimingSchema) + .max(ALERT_NOTIFICATION_TARGETS_LIMIT) + .optional(), }); export type AlertHistoryAnalytics = z.infer; From 6d2a9cd1f237193e5ac245cb1a99f0da51580ce4 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 26 Aug 2026 17:50:35 +1000 Subject: [PATCH 2/2] fix: key notification timings on the webhook id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two webhooks can share a display name, so keying the breakdown rows on the label collided. Persist the webhook id alongside it — the aggregation already keyed on it, the id was just stripped before storing. Also use the semantic danger token for the failure count rather than a raw Mantine colour. --- .../alert-per-target-notification-timings.md | 2 +- packages/api/src/models/alertHistory.ts | 1 + .../__tests__/checkAlerts.int.test.ts | 1 + packages/api/src/tasks/checkAlerts/index.ts | 10 +--- .../alerts/NotificationDurationCell.tsx | 5 +- .../NotificationDurationCell.test.tsx | 58 ++++++++++++++++++- packages/common-utils/src/types.ts | 5 ++ 7 files changed, 70 insertions(+), 12 deletions(-) diff --git a/.changeset/alert-per-target-notification-timings.md b/.changeset/alert-per-target-notification-timings.md index bfe7f63326..d685639604 100644 --- a/.changeset/alert-per-target-notification-timings.md +++ b/.changeset/alert-per-target-notification-timings.md @@ -6,6 +6,6 @@ Record and show which notification target an evaluation's delivery time went to. `webhookDurationMs` was a single number covering the whole delivery, and because targets are dispatched concurrently the slowest one sets it — so a multi-target alert reported a figure with no way to tell which webhook was responsible, or that the other targets were fine. -Each dispatch is now timed individually and aggregated per target across the evaluation, since a grouped alert notifies the same target once per firing group and again on resolve. One entry per distinct target carries its summed duration, how many dispatches it took, and how many failed. The evaluation history's "Notification duration" cell expands in place to show the breakdown. +Each dispatch is now timed individually and aggregated per target across the evaluation, since a grouped alert notifies the same target once per firing group and again on resolve. One entry per distinct target carries its webhook id, display name, summed duration, how many dispatches it took, and how many failed. The evaluation history's "Notification duration" cell expands in place to show the breakdown. Stored per evaluation rather than per dispatch: a 50-group alert notifying 10 targets would otherwise write 500 entries onto every history row. The array is capped at `ALERT_NOTIFICATION_TARGETS_LIMIT` and sorted slowest-first, so the cap drops the least interesting rows. Records written before this change keep rendering their total with nothing to expand. diff --git a/packages/api/src/models/alertHistory.ts b/packages/api/src/models/alertHistory.ts index 1f23b0e3dc..e1f574a3ec 100644 --- a/packages/api/src/models/alertHistory.ts +++ b/packages/api/src/models/alertHistory.ts @@ -118,6 +118,7 @@ const AlertHistorySchema = new Schema({ type: [ { _id: false, + targetId: { type: String, required: true }, target: { type: String, required: true }, durationMs: { type: Number, required: true }, dispatches: { type: Number, required: true }, 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 38572a2092..07065a4e77 100644 --- a/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts +++ b/packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts @@ -3351,6 +3351,7 @@ describe('checkAlerts', () => { // don't deep-equal a plain object literal. const targets = normalHistories[0].analytics!.notificationTargets; expect(targets).toHaveLength(1); + expect(targets![0].targetId).toBe(webhook._id.toString()); expect(targets![0].target).toBe(webhook.name); expect(targets![0].durationMs).toEqual(expect.any(Number)); expect(targets![0].dispatches).toBe(1); diff --git a/packages/api/src/tasks/checkAlerts/index.ts b/packages/api/src/tasks/checkAlerts/index.ts index 0f11f496b8..55a0890b14 100644 --- a/packages/api/src/tasks/checkAlerts/index.ts +++ b/packages/api/src/tasks/checkAlerts/index.ts @@ -932,16 +932,13 @@ export const processAlert = async ( // Per-target notification timings, keyed by webhook id so the same target // notified for several groups (and again on resolve) aggregates into one // entry rather than one per dispatch. - const notificationTimings = new Map< - string, - AlertNotificationTargetTiming & { key: string } - >(); + const notificationTimings = new Map(); const recordNotificationTimings = (timings: NotificationTiming[]) => { for (const timing of timings) { const existing = notificationTimings.get(timing.key); if (existing == null) { notificationTimings.set(timing.key, { - key: timing.key, + targetId: timing.key, target: timing.target, durationMs: timing.durationMs, dispatches: 1, @@ -969,8 +966,7 @@ export const processAlert = async ( // Slowest first: the point of the breakdown is finding what dominated // the total, and the cap below should drop the least interesting rows. .sort((a, b) => b.durationMs - a.durationMs) - .slice(0, ALERT_NOTIFICATION_TARGETS_LIMIT) - .map(({ key: _key, ...timing }) => timing); + .slice(0, ALERT_NOTIFICATION_TARGETS_LIMIT); }; try { const windowSizeInMins = ms(alert.interval) / 60000; diff --git a/packages/app/src/components/alerts/NotificationDurationCell.tsx b/packages/app/src/components/alerts/NotificationDurationCell.tsx index 43b2a6e005..24e6b48b75 100644 --- a/packages/app/src/components/alerts/NotificationDurationCell.tsx +++ b/packages/app/src/components/alerts/NotificationDurationCell.tsx @@ -56,7 +56,8 @@ export function NotificationDurationCell({ {targets.map(target => ( - + // Keyed on the id, not the label: two webhooks can share a name. + {target.target} @@ -67,7 +68,7 @@ export function NotificationDurationCell({ )} {target.failures > 0 && ( - + {target.failures} failed )} diff --git a/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx b/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx index 1c4d1fd467..50d671e7c1 100644 --- a/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx +++ b/packages/app/src/components/alerts/__tests__/NotificationDurationCell.test.tsx @@ -6,12 +6,66 @@ import { NotificationDurationCell } from '@/components/alerts/NotificationDurati const analytics = { webhookDurationMs: 4120, notificationTargets: [ - { target: 'Team Slack', durationMs: 4120, dispatches: 50, failures: 0 }, - { target: 'Ops webhook', durationMs: 210, dispatches: 50, failures: 2 }, + { + targetId: 'hook-1', + target: 'Team Slack', + durationMs: 4120, + dispatches: 50, + failures: 0, + }, + { + targetId: 'hook-2', + target: 'Ops webhook', + durationMs: 210, + dispatches: 50, + failures: 2, + }, ], }; describe('NotificationDurationCell', () => { + // Aggregation keys on the webhook id, so two webhooks sharing a display + // name are two legitimate entries. React only complains about the duplicate + // key through console.error, so watch for it directly. + it('renders same-named targets as distinct rows without key collisions', () => { + const consoleError = jest + .spyOn(console, 'error') + .mockImplementation(() => {}); + renderWithMantine( + , + ); + + expect(screen.getAllByText('Ops webhook')).toHaveLength(2); + // Scan every call rather than matching an argument list: React passes the + // message as a format string plus substitutions, so a fixed-arity + // toHaveBeenCalledWith matcher never matches and passes vacuously. + const warnings = consoleError.mock.calls + .map(args => args.map(String).join(' ')) + .filter(message => message.includes('same key')); + consoleError.mockRestore(); + expect(warnings).toHaveLength(0); + }); + it('shows a dash when the evaluation notified nothing', () => { renderWithMantine(); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index eea6193f1f..3aaacbe259 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -988,6 +988,11 @@ export const ALERT_NOTIFICATION_TARGETS_LIMIT = 20; * dispatch. */ export const AlertNotificationTargetTimingSchema = z.object({ + /** + * Stable identity for the target — the webhook id. Two webhooks can share a + * display name, so `target` alone does not identify a row. + */ + targetId: z.string(), /** Display label: the webhook's name as it was at dispatch time. */ target: z.string(), /** Summed wall time across this target's dispatches (ms). */