feat: attribute alert notification time to each target - #3003
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: 2a7f0b6 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 per-target notification timing analytics and adds an expandable breakdown to alert evaluation history.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Times each dispatched notification job and returns target identity, duration, and success status for aggregation. |
| packages/api/src/tasks/checkAlerts/index.ts | Aggregates notification timings by webhook ID and attaches a bounded, slowest-first breakdown to successful and failed evaluation history. |
| packages/api/src/models/alertHistory.ts | Adds the optional per-target notification timing subdocument to persisted alert analytics. |
| packages/common-utils/src/types.ts | Defines the shared per-target timing schema, storage limit, and optional alert-history analytics field. |
| packages/app/src/components/alerts/NotificationDurationCell.tsx | Displays the total notification duration and an expandable per-target breakdown using stable IDs and semantic failure styling. |
| packages/app/src/components/alerts/tests/NotificationDurationCell.test.tsx | Covers compatibility, expansion behavior, event propagation, timing details, and duplicate display-name handling. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Alert evaluation] --> B[Build notification jobs]
B --> C[Dispatch targets concurrently]
C --> D[Record each target duration and outcome]
D --> E[Aggregate by webhook ID]
E --> F[Persist bounded timing breakdown]
F --> G[Expand notification duration cell]
Reviews (5): 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 ReviewScope: PR #3003 — attribute alert notification time to each target. Diff base Intent: Replace the single The change is well-scoped and internally consistent: both the single-value and grouped evaluation paths route through ✅ No critical issues found.No P0/P1 findings. The items below are optional. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (11): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, reliability, api-contract, performance, adversarial, kieran-typescript, previous-comments. Testing gaps:
|
E2E Test Results✅ All tests passed • 323 passed • 1 skipped • 1354s
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.
2 participants
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.