Repository navigation
feat(alerts): send the alert.resolved webhook - #6373
claude[bot] wants to merge 1 commit into
Conversation
|
Heads-up on the red shard, not a request for anyone to act.
The assertion is order-dependent: Generated by Claude Code |
|
Automated pre-review (advisory, not a required check) — verdict: PASS · CI green PASS — The resolution webhook is wired consistently with the existing alert webhook path, including endpoint registration and schema exposure. Focused specs cover enqueue/no-enqueue behavior, webhook creation, and partial and full resolution payloads. |
eceedf9 to
0d23b99
Compare
A threshold that opted in to resolution now emits alert.resolved when the value comes back past it, carrying the same shape as the triggered event so a consumer can pair the two by threshold code. Rebuilt on the current recovery engine. The branch had repeatedly merged its base instead of rebasing, so it carried three copies of the engine commit and could not replay after the locking work was squashed into main; the content is unchanged from the reviewed version.
0d23b99 to
d6c6feb
Compare
Requested by Felipe Rodrigues · Slack thread
Before: The recovery engine notices that a value has come back to the safe side and writes a
resolvedrow, but nothing leaves Lago. A customer who blocks something when usage climbs is told about the crossing and never told about the return, so the block stays on until someone looks.After: The same moment that records the recovery also sends
alert.resolved. Only lines that asked to be watched are announced, and only when Lago has an alarm for them on record — that pairing is the one #6131 already applies before writing the row, so the webhook inherits it rather than repeating it. Every threshold that exists today notifies ontriggeredonly, so nothing fires for anyone until they opt a line in. Seeded rows are written on a different path and never reach here.How: delivery hangs off
record_resolutioninUsageMonitoring::ProcessAlertService, next to thealert.triggeredcall it mirrors, so the record and the send can never disagree.Webhooks::UsageMonitoring::AlertResolvedServicemirrors the triggered service and is registered inSendWebhookJob::WEBHOOK_SERVICES;alert_resolvedjoinsconfig/webhook_event_types.yml, which is what feeds the endpoint subscription list and the GraphQLEventTypeEnum(both schema dumps regenerated by hand — see the note below). The payload isTriggeredAlertSerializerplus the three things a resolution adds, via a subclass.Merge order
This stacks on #6131 and must merge after it. Its base here is
ing-509-recovery-engine, notmain; GitHub will retarget once that lands. Nothing in the stack below changes, and #6131's own merge-order note still applies — in particular the renewal fix, since without it a period boundary can record a spurious resolution row, and this PR is what turns such a row into a delivered webhook.Open questions for review
object_typestaystriggered_alert. The record is aUsageMonitoring::TriggeredAlertwithkind: resolved, and Lago's convention is that one object type carries several event names (invoice.created/invoice.one_off_created). Consumers separate the two onwebhook_type. If you would rather the resolution get its ownresolved_alertroot, say so now — it is a one-line change here and a breaking one later.resolved_atduplicatestriggered_at. On a resolved row thetriggered_atcolumn holds the time the resolution was recorded. The payload keeps the inheritedtriggered_atand addsresolved_atwith the same value, so a consumer reading the resolution does not have to know that. Happy to dropresolved_atif the duplication is worse than the ambiguity.crossed_thresholdscarries the recovered lines. Inherited from the triggered payload, where the name fits; on a resolution it lists the lines that were left behind. Renaming it would mean not reusing the serializer.ing-510-alert-resolved-webhookand the title carries no id. Please confirm the real one and I will rename.Testing
I could not run the suite or Rubocop in this environment, so nothing here is verified by a green run. The container has Ruby 3.3.6 and the Gemfile requires 4.0.6, so
bundle installrefuses and neitherrspecnorrubocopis installed; there is also no Postgres running. What I did instead:ruby -con every changed Ruby file (clean, aside from the pre-existing 3.4itwarnings), a YAML parse ofconfig/webhook_event_types.yml, and a JSON parse ofschema.json. CI is the first real run — please treat a red build as expected information, not a surprise.Two things worth a reviewer's eye because of that:
schema.graphqlandschema.jsonwere edited by hand rather than regenerated, since the app cannot boot here.spec/graphql/lago_api_schema_spec.rbcompares both against the live schema, so it will say immediately if the hand edit is off.spec/models/webhook_endpoint_spec.rbassertsWEBHOOK_EVENT_TYPESmatchesSendWebhookJob::WEBHOOK_SERVICES.keys, which is why the YAML entry is part of this PR and not a follow-up.Specs added: a webhook-service spec and a serializer spec mirroring the triggered ones, and in
process_alert_service_spec.rbthe existing "does not send a webhook yet" example becomes an assertion that the resolution webhook is enqueued, with the not-opted-in context now also asserting nothing is sent.🤖 Generated with Claude Code
https://claude.ai/code/session_01RLys3y8nVyx1rgUqqXDFP2
Generated by Claude Code