Repository navigation
fix: alert on every monitor outage, and resolve the page when a snoozed rule's monitor recovers - #394
Open
FrameAutomata wants to merge 1 commit into
Open
FrameAutomata wants to merge 1 commit into
FrameAutomata wants to merge 1 commit into
Conversation
…ed rule's monitor recovers
Two defects in OnCheckStateChange, the notifier behind check_down rules.
A second outage inside the rule's cooldown was never alerted. The notifier
runs only on a state change, so an outage produces one "went down"
transition; the cooldown dedup dropped it, and nothing fired again while
the monitor stayed down. On an escalation channel no page opened. The
recovery was still sent. The dedup key is now forgotten when the monitor
recovers, so the cooldown covers one outage, as the docs describe it
("the same ongoing condition"), and the next outage alerts.
A page was left open when its monitor recovered while the rule was
snoozed: the snooze check ran before the recovery handling and skipped the
auto-resolve along with the notifications. Ending an outage is not a
notification, so the auto-resolve and the dedup reset now run before the
snooze check. A snoozed rule still sends nothing.
Closes #387
Closes #391
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #387
Closes #391
Two defects in
OnCheckStateChange(backend/app/notifications/check_state.go), the notifier behind Monitor Down rules. They sit a few lines apart in one loop, so they are fixed together.Problem
#387: a second outage inside the rule's cooldown was never alerted.
ProcessOutcomecalls the notifier only on a state change, so an outage produces exactly one "went down" transition. The notifier dropped that transition when the rule's dedup key for the monitor had been recorded withinCooldownMinutes(default 15), and nothing fired again while the monitor stayed down. The alert was not delayed, it was lost; on an escalation channel no page opened. The recovery was not deduplicated, so the channel still got a "recovered" notice for an outage nobody was told about.#391: a page stayed open when its monitor recovered while the rule was snoozed. The snooze check ran before both the "went down" and the recovery handling, so it skipped the page auto-resolve along with the notifications. A recovery produces a single transition, so nothing resolved the page later.
Fix
dedupTracker.forget). The cooldown then covers one outage, which is how the docs already describe it ("prevents a rule from firing repeatedly for the same ongoing condition"): a recovery ends the condition, and the next outage is a new one.Behaviour change to be aware of
A flapping monitor now alerts for every outage. Before, it sent the first alert and then only recoveries until the cooldown ran out, which is neither quiet nor informative. The damping for a flapping check is the monitor's failure threshold (consecutive failures before it counts as down), and the docs now say so.
On an escalation channel this means each outage opens its own page, after the previous one auto-resolved.
Verification
Tests, written first and run against unmodified production code, where the three regression tests failed on their assertions:
mainTestCheckDownAlertsForEveryOutage(Slack channel: down, up, down, up)down, recovered, recovered; wantsdown, recovered, down, recoveredTestSecondOutageInsideCooldownOpensNewPage(escalation channel)expected the second outage to open a pageTestRecoveryResolvesPageWhileRuleSnoozed(escalation channel)page status = open, want resolvedTwo guards pass on
mainand on this branch:TestCheckDownCooldownHoldsWithinOneOutage(a repeated down transition with no recovery between still alerts once) andTestSnoozedCheckDownRuleSendsNothing. The existingTestCheckDownOpensPageAndRecoveryAutoResolvesnow shares a seeding helper with the two new on-call tests.go testfornotifications,oncall,outbox,syntheticsandcontrollers,go vet ./app/...andgofmt -lare clean on the default build;go veton the two touched packages is clean undertelemetry_duckdbandtransactional_pg telemetry_ch.Live, the same script against a build of
main(895b03c1) and of this branch, default SQLite build: one TCP monitor (failureThreshold: 1) feeding a Slack rule and an on-call rule, both on the default 15 minute cooldown. The on-call rule is snoozed during the first outage and un-snoozed after it.mainThe monitor recorded 2 incidents in both runs.
CI note
Backend Vulncheckwill fail on this PR:mainfails that gate on GO-2026-6505 in a dependency, which #386 fixes. This branch does not carry the bump, so the gate clears once #386 has merged and thecilabel is re-applied.🤖 Generated with Claude Code