Add HTTP webhook notifications for alert transitions (A3)#51
Conversation
Deliver alert fired/cleared events (A2) to configurable HTTP webhooks so alerts leave the box: a generic JSON POST covers Slack, Discord, Home Assistant, ntfy, etc. via their incoming-webhook formats. The alert Engine's Evaluate now returns the events it emits on each tick, which the collector forwards to a new Notifier (internal/alert/notify.go). Delivery is fully decoupled from collection: Notify only enqueues onto a bounded channel and a single background worker performs the POSTs, so a slow or failing webhook can never block metric sampling. Each webhook supports a per-severity filter (min_level), an optional Go text/template body, retry with exponential backoff, and a per-metric rate limit that stops a flapping alert from flooding a destination without suppressing distinct alerts. Configuration only (std-lib net/http, no new runtime dependency): webhooks and notifier tuning live under the alerts: block; malformed templates and invalid webhook fields are rejected at startup. Closes #4
There was a problem hiding this comment.
Review: Alert webhook notifications (A3)
Reviewed against the project's Go/security/API conventions. On claude/elegant-feynman-aur2l5:
go build ./...— cleango vet ./...— cleango test ./...— all packages pass
This is a well-structured change. The decoupling of delivery from collection is exactly right: Notify only enqueues onto a bounded channel and never blocks the fast tick, per-attempt timeouts are enforced via context.WithTimeout, resp.Body is always closed, backoff respects context cancellation, and deliver can't panic the worker. No exec.Command, no new dependencies (std-lib net/http + text/template), no /api/v1 shape change (the message field lives only in the webhook payload). Config validation runs regardless of Enabled so typos fail fast at startup. Test coverage is thorough — default payload, template, bad-template rejection, retries-then-give-up, min_level filter, per-metric rate limit, and the eventReaches table.
Nothing here is blocking. A few observations:
Worth considering: rate limiter can drop a cleared event
rateLimited is keyed per (url, metric, resource) regardless of Kind, and dispatch applies it to every event. If a cleared arrives within notify_min_interval_seconds of the preceding fired delivery, the clear is dropped — leaving a state-based downstream consumer (e.g. a Home Assistant binary_sensor) stuck "on" with no correction until the next transition.
With the defaults this can't happen: the debounce (for_seconds: 30) applies symmetrically, so any two transitions for one metric are ≥30s apart, comfortably outside the 5s window. It only becomes reachable when a user sets for_seconds below notify_min_interval_seconds and the metric flaps. Given that, it's a latent edge rather than a live bug — but since the rate limit exists to coalesce repeated firings, it may be worth exempting cleared events (or keying the limit on Kind) so a clear is never suppressed. A small test pinning the intended behavior would document the decision either way.
Minor
Notifier.wgis never awaited.Startdoeswg.Add(1)/defer wg.Done(), but nothing callswg.Wait(), so the WaitGroup currently has no effect — on ctx cancel the worker just returns and queued events are dropped (which the doc comment does state). Either drop the field or add aStop()/drain that waits on it, so the intent is unambiguous.Content-Type: application/jsonis sent unconditionally, including for a customtemplatethat may render non-JSON. Fine for Slack/Discord/ntfy's JSON payloads, but a user templating a plain-text body would get a mislabeled request. Low impact; noting for completeness.lastSentgrows unbounded, keyed per(url, metric, resource). Bounded in practice by the small set of metric/mountpoint combinations, so not a concern on a Pi — just flagging that pruned disk mountpoints leave stale entries behind, unlike the engine'spruneDisks.
Overall this looks ready to merge; the cleared-vs-rate-limit point is the only one I'd ask you to make a deliberate call on.
…-type Follow-up to the A3 webhook review: - Never rate-limit `cleared` events. A recovery signal must always be delivered, otherwise a state-based downstream consumer (e.g. a Home Assistant binary_sensor) could get stuck reporting an alert that has actually cleared when a clear lands inside notify_min_interval_seconds of the preceding fired. The limiter now purely coalesces repeated firings. - Make Notifier.Start's WaitGroup meaningful: add Stop() to join the delivery worker, called from the collector on shutdown so no webhook POST is left in flight past exit. - Add a per-webhook content_type (default application/json) so a custom template rendering a non-JSON body isn't mislabeled. Tests cover the cleared-bypass, per-metric firing coalescing, and the content-type override; docs/example config updated accordingly.
|
Thanks for the thorough review. Addressed in Rate limiter dropping a
Content-Type unconditional: added an optional per-webhook
|
Summary
Delivers alert
fired/clearedevents (from the A2 engine, #50) toconfigurable HTTP webhooks, so alerts leave the box. A single generic JSON
POST covers Slack, Discord, Home Assistant, ntfy, etc. via their
incoming-webhook formats.
internal/alert/notify.go— a newNotifier.Engine.Evaluatenowreturns the transition events it emits each tick; the collector forwards
them to the notifier.
Notifyonly enqueuesonto a bounded channel and a single background worker performs the POSTs,
so a slow or failing webhook can never block metric sampling (the queue
drops with a warning if it ever backs up).
min_level:warn/crit), anoptional Go
text/templaterequest body (defaulting to a JSON objectdescribing the event), and a per-attempt timeout.
give up without crashing; backoff waits respect context cancellation so
shutdown stays prompt.
(url, metric, resource)so a fast-flappingalert can't flood a destination, while a distinct metric alerting in the
same tick is still delivered.
net/http, no new runtime dependency): webhooksand notifier tuning live under the
alerts:block. Malformed templates andinvalid webhook fields are rejected at startup.
Both acceptance criteria are covered by tests in
notify_test.goagainst alocal
httptestserver: firing an alert delivers the expected JSON payload,and a failing delivery retries with backoff then gives up without crashing or
blocking. Verified end-to-end by running the binary against a real local
webhook sink. Per the issue, email/SMTP (A4) is deliberately left out.
Related Issue
Closes #4
Checklist
go test ./...passes locally)go vet ./...andgolangci-lint runare cleandocs/API.md),configuration (
README.md,packaging/pimonitor.example.yaml), orinstallation/packaging (
packaging/install.sh, systemd units)/api/v1/...response shapes, or a new APIversion was introduced instead