refactor(alerts): extract notification transports behind a registry - #2844
refactor(alerts): extract notification transports behind a registry#2844jordan-simonovski wants to merge 3 commits into
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔴 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
|
github-actions
Bot
added
the
review/tier-4
Greptile SummaryThis PR separates alert-notification delivery from template rendering and introduces registry-based transport and dispatcher seams while preserving synchronous delivery.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/template.ts | Replaces direct webhook dispatch with construction and synchronous dispatch of a NotificationJob. |
| packages/api/src/tasks/checkAlerts/notifications.ts | Defines the notification job, dispatcher contract, default delivery function, and inline implementation. |
| packages/api/src/tasks/checkAlerts/transports/index.ts | Adds channel-type and webhook-service registries with explicit unsupported-value errors. |
| packages/api/src/tasks/checkAlerts/transports/generic.ts | Extracts generic webhook rendering, validation, metrics, retry, and HTTP error handling. |
| packages/api/src/tasks/checkAlerts/transports/slack.ts | Extracts Slack webhook validation, delivery, and instrumentation. |
| packages/api/src/tasks/checkAlerts/errors.ts | Adds a typed HTTP response error carrying the destination status code. |
| packages/api/src/routers/api/webhooks.ts | Updates test-webhook delivery to pass a populated webhook channel to the extracted transports. |
Sequence Diagram
sequenceDiagram
participant Alert as Alert evaluation
participant Template as renderAlertTemplate
participant Dispatcher as NotificationDispatcher
participant Registry as Transport registry
participant Transport as Webhook transport
participant Destination as Webhook destination
Alert->>Template: Render notification
Template->>Dispatcher: dispatch(NotificationJob)
Dispatcher->>Registry: deliverToChannel(channel, message, ctx)
Registry->>Registry: Resolve channel type
Registry->>Registry: Resolve webhook service
Registry->>Transport: Invoke selected transport
Transport->>Destination: POST notification
Destination-->>Transport: HTTP response
Transport-->>Dispatcher: Resolve or reject
Dispatcher-->>Template: Propagate result
Reviews (8): Last reviewed commit: "refactor(alerts): add a notification dis..." | Re-trigger Greptile
Deep ReviewScope: Intent: Structural refactor of webhook notification dispatch; author asserts no behavior change (core alert integration tests unmodified). ✅ No critical issues found. Error propagation ( 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (7): correctness, reliability, maintainability, testing, kieran-typescript, api-contract, adversarial. Testing gaps:
|
E2E Test Results✅ All tests passed • 229 passed • 1 skipped • 839s
Tests ran across 4 shards in parallel. |
1879a85 to
764ceb4
Compare
764ceb4 to
e56dd4d
Compare
jordan-simonovski
changed the title
77ccf05 to
f17f741
Compare
| // Webhook delivery is the last (and most failure-prone) hop of an alert. It | ||
| // happens in the background task, so failures only show up in logs today. | ||
| // `service` and `outcome` are bounded enums (see agent_docs/observability.md). | ||
| export const webhookDeliveryCounter = getCounter( | ||
| 'hyperdx.alerts.webhook_deliveries', | ||
| { | ||
| description: | ||
| 'Count of alert webhook delivery attempts, labeled by service (slack, generic, incidentio) and outcome (success, error).', | ||
| }, | ||
| ); | ||
| export const webhookDeliveryDuration = getHistogram( | ||
| 'hyperdx.alerts.webhook_delivery.duration_ms', | ||
| { | ||
| description: | ||
| 'Duration of an alert webhook delivery attempt, labeled by service.', | ||
| unit: 'ms', | ||
| }, | ||
| ); |
There was a problem hiding this comment.
nit: Would these fit better in a shared file, since they aren't specific to generic webhooks?
There was a problem hiding this comment.
Fair point. I'll do this in a follow-up 🫡
template.ts mixed Handlebars templating with the HTTP transport for Slack/generic/incident.io webhooks. Move the transport (notifyChannel, handleSendSlackWebhook, handleSendGenericWebhook, sendGenericWebhook, delivery metrics) into tasks/checkAlerts/notifications.ts unchanged, so the upcoming multi-channel dispatch work lands in a focused module.
c6bd23d to
50a5db0
Compare
2 participants
Why
Notification delivery was a single
if/elsechain overWebhookService. Adding a new webhook service meant editing that chain; adding a whole new channel type meant editing it and everything around it.This replaces the chain with a registry keyed on channel type first, webhook service second, so both kinds of extension become an added entry rather than an edit to shared code.
The double keying looks like unnecessary indirection given this repo has exactly one channel type today. That is deliberate and is the point of the change — it is the seam a downstream build extends without touching these lines. There is a comment in
transports/index.tssaying so, aimed at the next person tempted to flatten it.What changed
checkAlerts/notifications.tsis split intocheckAlerts/transports/{types,slack,generic,index}.tsand deleted. The name is deliberately freed — a follow-up PR reuses it for the notification job and dispatcher contract, which is a different concern entirely.deliverToChannel(channel, message, ctx)resolves channel type then webhook service, throwingUnsupported channel type: <t>/Unsupported webhook service: <s>.IWebhook, so a transport can read per-channel fields without a signature change later.WebhookResponseErrorcarries the destination's HTTP status, replacing anas anyon a plainError.retry.tsreads.statusunchanged.Not a behaviour change
checkAlerts.int.test.ts(277) andrenderAlertTemplate.int.test.ts(74) pass unmodified — that is the gate.webhooks.int.test.tsneeded 3 assertion updates (.calls[0][0].url->.channel.url), a direct consequence of the transport signature.getWebhookFetchTimeoutMsis exported without a caller: the fetch wiring arrives with the PR that removes the per-event deadline, and landing it here would change behaviour this PR is asserting it does not change.