fix: report only the delivery time as an alert's notification duration - #3110
fix: report only the delivery time as an alert's notification duration#3110jordan-simonovski wants to merge 5 commits into
Conversation
The notification duration on an alert's evaluation list timed the whole fireChannelEvent call - building the title and links, querying the log lines for the message body, compiling the template, then dispatching - while the per-target breakdown only covered the dispatch. With one target the two figures should match, and instead the target read as 4ms against a 2.69s total. Record the render share as analytics.renderDurationMs and show it as its own row, so the expansion accounts for the total. The column tooltip claimed the slowest target sets the figure, which was never true.
🦋 Changeset detectedLatest commit: eed9fb8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
|
E2E Test Results✅ All tests passed • 357 passed • 1 skipped • 1413s
Tests ran across 4 shards in parallel. |
Greptile SummaryThis PR narrows alert notification duration to the concurrent dispatch phase and updates the persisted contract, UI explanation, and integration coverage.
Confidence Score: 4/5The PR is not yet safe to merge because alerts with no resolvable notification targets report a delivery time even though nothing was dispatched. The unconditional accumulation initializes Files Needing Attention: packages/api/src/tasks/checkAlerts/index.ts and packages/api/src/tasks/checkAlerts/template.ts
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Measures the concurrent dispatch phase and returns its rounded wall time, including for an empty job list. |
| packages/api/src/tasks/checkAlerts/index.ts | Accumulates dispatch-only duration but incorrectly initializes the metric when no target was dispatched. |
| packages/common-utils/src/types.ts | Updates the analytics schema documentation to define dispatch-only timing and absence when nothing was dispatched. |
| packages/app/src/components/alerts/AlertEvaluationsTable.tsx | Correctly clarifies that the slowest target determines timing within each dispatch round. |
| packages/app/src/components/alerts/NotificationDurationCell.tsx | Removes render-time presentation and retains the dash for absent delivery timing. |
| packages/api/src/tasks/checkAlerts/tests/checkAlerts.int.test.ts | Covers single-target timing and compile failures but not successful renders with zero dispatch jobs. |
Sequence Diagram
sequenceDiagram
participant Evaluation
participant Renderer
participant Targets
participant History
Evaluation->>Renderer: Build alert message
Renderer->>Targets: Dispatch jobs concurrently
Targets-->>Renderer: Delivery results and timings
Renderer-->>Evaluation: dispatchDurationMs
Evaluation->>History: Accumulate webhookDurationMs
Reviews (3): Last reviewed commit: "fix: report only the delivery time as no..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Deep Review✅ No critical issues found. The dispatch/render timing split is correct: 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (7): correctness, reliability, testing, adversarial, api-contract, kieran-typescript, previous-comments. Testing gaps:
|
Each field was documented in the Mongoose interface, the zod schema, the implementation and the tests. The zod schema owns the description now; the rest get a line.
A grouped evaluation notifies once per group, and the delivery share takes the slowest target of each round, so no single target sets it for the evaluation.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,15 @@ | |||
| --- | |||
| '@hyperdx/app': patch | |||
| '@hyperdx/api': patch | |||
There was a problem hiding this comment.
🔵 minor — Changeset omits @hyperdx/common-utils although types.ts changed
packages/common-utils/src/types.ts gains renderDurationMs on AlertHistoryAnalyticsSchema, but the changeset only lists @hyperdx/app and @hyperdx/api. common-utils is not in the fixed group in .changeset/config.json, so it gets no bump or changelog entry from this PR — unlike the analogous per-target-timing change (see the 0558f77e entry in packages/common-utils/CHANGELOG.md) and the five pending changesets that do declare it (e.g. .changeset/tile-editor-dashboard-filters.md). Add '@hyperdx/common-utils': patch.
| expect(renderDurationMs).toBeGreaterThan(0); | ||
| expect( | ||
| renderDurationMs! + targets![0].durationMs, | ||
| ).toBeGreaterThanOrEqual(webhookDurationMs! - 2); |
There was a problem hiding this comment.
🔵 minor — ±2 ms reconciliation tolerance is tighter than the quantity it bounds
Substituting renderDurationMs = round(T) − round(D), the assertion reduces to |round(d_job) − round(D_promiseAll)| ≤ 2 — i.e. it bounds the gap between the per-job timer stopping in the job's finally (template.ts:946) and performance.now() after await Promise.all settles (template.ts:957). That gap is a microtask hop plus whatever the event loop is doing, so a GC pause or scheduler stall on a loaded CI runner makes this fail spuriously with no regression present. Widen to something like ±25 ms, which still catches the mismatch the test targets (4 ms vs 2.69 s).
PR Review2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor 2 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
Building the message — the title and links, the log-line query for the body, the template render — is not what the column is for. Time the dispatch alone and drop the render share rather than displaying it. Per the team's call in #alerting: the intent of the column is the webhook response time, and the body-building latency isn't something users need.
jordan-simonovski
changed the title
| evaluationAnalytics.webhookDurationMs = | ||
| (evaluationAnalytics.webhookDurationMs ?? 0) + dispatchDurationMs; |
There was a problem hiding this comment.
When a notification round has no dispatch jobs—for example, because its configured webhook was deleted or all mentioned targets are unresolvable—Promise.all([]) succeeds and returns a rounded duration, normally 0. These lines then create webhookDurationMs, so the evaluation displays 0ms even though no delivery occurred. This conflicts with the new contract that the field is absent when nothing was dispatched; only pre-dispatch exceptions currently produce the intended dash. Avoid setting the aggregate unless at least one job was dispatched.
| }); | ||
| // Only the dispatch phase: the column reports how long the targets | ||
| // took to respond, not the time spent building the message. | ||
| evaluationAnalytics.webhookDurationMs = |
There was a problem hiding this comment.
🔵 minor — An evaluation that dispatches nothing records webhookDurationMs: 0, not absent, so the cell reads "0ms"
When the render succeeds but no job is queued — e.g. the alert's configured webhook was deleted, so resolveConfiguredChannel (packages/api/src/tasks/checkAlerts/template.ts:673-687) records a pre-failure and returns [] — Promise.all([]) gives dispatchDurationMs: 0 (template.ts:957), and this line stores 0. NotificationDurationCell only dashes on total == null, so formatDurationMs(0) renders "0ms" (packages/app/src/utils.ts:1066) next to a WEBHOOK error, reading as an instant successful delivery; it also contradicts the "Absent when nothing was dispatched" contract this PR adds at packages/common-utils/src/types.ts:1112. Return dispatchDurationMs as undefined when jobs.length === 0 and only accumulate here when it is defined, so the no-dispatch path dashes like the compile-failure path the new test covers.
| * time — so this is not the complement of `failures`. | ||
| */ | ||
| timings: NotificationTiming[]; | ||
| /** Wall time of the concurrent dispatch phase (ms); the caller subtracts it to get the render share. */ |
There was a problem hiding this comment.
🔵 minor — Doc comment on dispatchDurationMs describes a subtraction no caller performs
The only caller adds the value straight onto evaluationAnalytics.webhookDurationMs (packages/api/src/tasks/checkAlerts/index.ts:1357) and the render share is deliberately discarded per this PR's own description, so "the caller subtracts it to get the render share" will mislead the next reader into hunting for a render metric that does not exist — drop that clause and say it is the wall time of the concurrent dispatch phase, reported as the evaluation's delivery time.
1 participant
The notification duration on an alert's evaluation list was timing everything an alert does once it decides to fire: building the message title and links, querying the log lines that go in the body, rendering the template, and then delivering it. The result was a column reading 2.69s directly above its own per-target breakdown reading 4ms — the webhook answered in milliseconds and almost all of the rest was spent assembling the message.
The column is meant to be the webhook response time, so it now times the dispatch alone. The message-building time is no longer recorded or displayed anywhere; we agreed that isn't something users need to see.
Two behaviour notes for review. Evaluations already in the database keep their old figure and will read high, with nothing to distinguish them — the field's meaning changes rather than a new field being added, which is the simpler option given the column was its only consumer. And an evaluation that fails before dispatch (a template that won't compile, for instance) now records no delivery time at all and shows a dash, where previously it reported the time it spent failing to build the message.
Implementation detail
renderAlertTemplatereturns the wall time of its dispatchPromise.all, which is the concurrent phase, so the slowest target of each round sets it. The accumulation moved out of thefinallyand into thetry: a pre-dispatch throw then records nothing instead of attributing its own render time to delivery.An integration test asserts the recorded figure matches the single target's own response time within rounding, which fails if the render time leaks back in, and another asserts a compile failure records no delivery time. The tooltip also picked up a per-round qualifier from review: a grouped alert notifies once per group, so no single target sets the figure across the whole evaluation.