feat: attribute alert notification time to each target - #3003
feat: attribute alert notification time to each target#3003jordan-simonovski wants to merge 3 commits into
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b8b30e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThe PR records notification delivery timing per target and exposes an expandable evaluation-history breakdown.
Confidence Score: 5/5The PR appears safe to merge with no blocking failure remaining. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Times each dispatched notification job and returns its target identity, duration, and outcome. |
| packages/api/src/tasks/checkAlerts/index.ts | Aggregates target timings across an evaluation and attaches a bounded, slowest-first breakdown to history analytics. |
| packages/api/src/models/alertHistory.ts | Adds the optional per-target notification timing subdocuments to the alert-history model. |
| packages/common-utils/src/types.ts | Defines the shared timing schema, analytics field, and maximum stored-target count. |
| packages/app/src/components/alerts/NotificationDurationCell.tsx | Renders the total notification duration and an expandable per-target breakdown. |
| packages/app/src/components/alerts/tests/NotificationDurationCell.test.tsx | Covers legacy data, empty timing data, expansion behavior, stable duplicate-name rendering, and click propagation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Alert evaluation] --> B[Render notification jobs]
B --> C[Dispatch targets concurrently]
C --> D[Record each target duration and outcome]
D --> E[Aggregate by webhook ID]
E --> F[Persist evaluation analytics]
F --> G[Expandable notification-duration cell]
Reviews (3): Last reviewed commit: "Merge branch 'main' into jordansimonovsk..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep Review✅ No critical issues found. This is a well-scoped change. The backend logic checks out on direct reading: 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (10): correctness, testing, maintainability, project-standards, kieran-typescript, reliability, performance, api-contract, adversarial, previous-comments. Coverage note: Only the previous-comments reviewer (which returned no unaddressed findings) completed before synthesis was required; the remaining findings above are grounded in direct analysis of the diff and surrounding code ( Testing gaps:
|
E2E Test Results✅ All tests passed • 308 passed • 1 skipped • 1266s
Tests ran across 4 shards in parallel. |
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.
1 participant
webhookDurationMsrecorded one number for an evaluation's whole notification delivery. Targets are dispatched concurrently, so the slowest one sets that number — a three-target alert where one webhook takes 4s and two take 50ms reports 4s, with nothing to say which webhook was responsible or that the other two were healthy. This times each dispatch and attributes the total.Follows #3001, which renamed the column to "Notification duration" and explained what the single number meant. This gives it a breakdown to expand into.
What changed
Each dispatch is timed individually inside the existing
Promise.allfan-out and aggregated per target across the evaluation. Each entry carries the target's summed duration, how many dispatches it took, and how many failed. The evaluation history cell expands in place to show it.Background
An evaluation can dispatch to the same target several times: a grouped alert notifies per firing group, and a resolve notification is a further dispatch. So "the time for this target" is a sum over dispatches, while the evaluation's total is a max over concurrent targets within each round. The two genuinely don't add up to each other, and the schema comment says so — otherwise the next reader will file the discrepancy as a bug.
Key decisions
Aggregated per evaluation, not per dispatch. A 50-group alert notifying 10 targets would write 500 entries onto every history row, on a collection that already has a TTL index because of its volume. Per-target totals are bounded by the distinct targets an evaluation can reach. The cost is that the expander shows "Team Slack — 4.1s across 50 dispatches" rather than which group was slow; the dispatch count is surfaced so the number doesn't read as one slow send.
Timed in the
finally, so failures count too. A target that fails after 30s spent 30s, andwebhookDurationMsalready includes it. Targets that fail before dispatch have no timing at all — there was nothing to time — sotimingsis not the complement offailures. Those still surface in the Errors column.Expands within the cell, not as another table row. The row already owns a chevron for groups and errors; a second row-level expander competing with it would be ambiguous to click. The cell's toggle stops propagation so it doesn't also fire the row's.
The label is the webhook's name at dispatch time. A renamed webhook leaves historical rows showing the old name, which is what was actually notified. Aggregation keys on the webhook id, so two webhooks sharing a name stay separate entries — they will render identically, which is the one case where the breakdown is ambiguous.
Impact
New optional field on alert history analytics; nothing reads it as required. Records written before this keep rendering their total with no expander, covered by a test. No migration — the field is absent on old documents and Mongoose leaves them alone.
Implementation detail
renderAlertTemplatereturnstimingsalongsidefailures;fireChannelEventwidens fromNotificationFailure[]toPick<RenderedAlert, 'failures' | 'timings'>. Aggregation lives inprocessAlertas aMapkeyed by webhook id, flushed onto the analytics object before the records are written — from the error path too, so an evaluation that notifies some targets and then fails still reports what it delivered.The stored array is sorted slowest-first and capped at
ALERT_NOTIFICATION_TARGETS_LIMIT, so the cap drops the least interesting rows rather than an arbitrary set.This branch deliberately leaves
AlertEvaluationsTable's heading alone — #3001 renames it and sentence-cases the rest, and touching the same line here would conflict for no benefit. If this lands first the heading reads "Webhook Duration" until #3001 merges.Tests: 4 on the cell (no-delivery dash, pre-change records with a total but no breakdown, the toggle, and the per-target rows). The toggle is asserted via
aria-expandedbecause Mantine'sCollapsekeeps children mounted — asserting on breakdown content alone passed without ever clicking, which I confirmed before rewriting it. Integration: the existingcheckAlertsanalytics assertion now also checks the per-target entry; 296 pass.make ci-unit3343 pass,make ci-lint0 errors.