Stop metric_staleness alerts firing and clearing in a loop - #412
Conversation
While any probe was stale, the alerter raised and cleared the same metric_staleness alert on every cycle, sending a fire and a clear notification each time. The cleaner resolved the alert's metric, probe_staleness_ratio, through the metric registry, where the bespoke staleness metric deliberately has no entry, and read the resulting lookup error as proof that the condition had gone away; the evaluator then re-raised the alert on its next pass, because unlike triggerThresholdAlert it applied no cooldown guard. GetLatestMetricValues now returns distinguishable errors, so callers can tell a metric that cannot be evaluated at all, or a query that failed, apart from one that ran and reported nothing. checkAlertResolved clears an alert only in the last of those cases, since neither of the other two says anything about whether the alerting condition still holds. Probe scoped alerts skip the registry path entirely and resolve against probe availability instead, so they still clear promptly once the probe collects again or stops being reported. The staleness evaluator now applies the same recently-cleared cooldown guard as every other rule, and keys both its active-alert lookup and its cooldown check by probe, so several stale probes on one connection raise one alert each rather than fighting over a single row whose title and description were overwritten arbitrarily. Closes #405
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds classified metric lookup errors, probe-scoped staleness alert queries, cooldown enforcement, and probe-aware resolution. Integration tests cover alert creation, recovery, failures, suppression, probe isolation, and metric result handling. Documentation records the corrected lifecycle. ChangesStaleness alert lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 158 |
| Duplication | 59 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/golang-expert/metrics-queries.md:
- Around line 199-202: Update the documentation around checkAlertResolved to
clarify that ErrNoMetricData is the only error result that clears an alert; also
mention that alerts clear when no current value matches or the current value no
longer violates the threshold, while unsupported metrics and failed queries
leave alerts active.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2e7fd71-a115-4521-87da-a187b21b7f77
📒 Files selected for processing (9)
.claude/golang-expert/metrics-queries.mdalerter/src/internal/database/alert_queries.goalerter/src/internal/database/metric_queries.goalerter/src/internal/database/probe_alert_queries_integration_test.goalerter/src/internal/engine/cleanup.goalerter/src/internal/engine/staleness_alerts_errors_integration_test.goalerter/src/internal/engine/staleness_alerts_integration_test.goalerter/src/internal/engine/thresholds.godocs/changelog.md
CodeRabbit rightly pointed out that checkAlertResolved also clears an alert on paths that involve no error at all, so the note now scopes the ErrNoMetricData statement to the error case.
PR #410 adds audit_defects_test.go to the same package with fixtures called insertStalenessRuleSQL, insertProbeConfigSQL, and friends, so the two files would have collided on those identifiers once both landed. Prefixing the fixtures here with 'staleness' keeps the two sets apart and lets them coexist without either side needing edits at merge time.
|
@coderabbitai review |
|
1 participant
Summary
While any probe was stale, the alerter raised and cleared the same
metric_stalenessalert on every cycle, sending a fire and a clearnotification each time. The cleaner resolved the alert's metric,
probe_staleness_ratio, throughmetricRegistry, where the bespokestaleness metric deliberately has no entry, and read the resulting
lookup error as proof that the condition had gone away; the evaluator
then re-raised the alert on its next pass, because unlike
triggerThresholdAlertit applied no cooldown guard.The change is in three parts:
GetLatestMetricValuesnow returns distinguishable errors(
ErrMetricNotSupportedandErrNoMetricData), so callers can tell ametric that cannot be evaluated at all, or a query that failed, apart
from one that ran and reported nothing.
checkAlertResolvedclears analert only in the last of those cases.
Probe-scoped alerts skip the registry path entirely and resolve
through
checkStalenessAlertResolved, which re-reads probeavailability, so they still clear promptly once the probe collects
again or stops being reported; suppressing the spurious clears does
not leave the alert latched.
evaluateMetricStalenessapplies the same recently-cleared cooldownguard as every other rule, and keys both its active-alert lookup and
its cooldown check by probe, so several stale probes on one connection
raise one alert each rather than fighting over a single row whose
title and description were overwritten arbitrarily.
Issue #407 shares the root cause in
checkAlertResolved, and the errorhandling half of it is fixed here: a query error no longer clears an
alert. The rest of #407 is deliberately out of scope, because it needs
semantic rewrites of several
metric_registry.goqueries(
cache_hit_ratiolatest-sample reduction, an interval mean forslow_query_count, wider delta windows, and freshness cutoffs on thereplication slot metrics) that are independent of this fix.
Test plan
alerter/src/internal/engine/staleness_alerts_integration_test.gocovers the loop itself, resolution when the probe recovers, resolution
when the probe stops being reported, one alert per stale probe, the
cooldown guard, and both sides of the new error handling in
checkAlertResolved. Every one of those tests fails against the codeas it stood before this change, verified by reverting
cleanup.goandthresholds.goalone.alerter/src/internal/engine/staleness_alerts_errors_integration_test.godrives the evaluator's and cleaner's database error branches by
breaking exactly one operation at a time.
alerter/src/internal/database/probe_alert_queries_integration_test.gocovers the two new probe-scoped queries and every branch of
GetLatestMetricValues.cd alerter && make coveragepasses with no failures. The new andmodified units are at 100% (
checkAlertResolved,checkStalenessAlertResolved,GetLatestMetricValues,GetActiveThresholdAlertForProbe,GetRecentlyClearedAlertForProbe)and 96.4% (
evaluateMetricStaleness), against the project's 90%floor.
make lintandmake fmt-checkare clean.Closes #405
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Interaction with PR #410
PR #410 (branch
claude/dashboard-chart-stats-review-0iq76e) addsTestAudit*regression tests that deliberately pin the current,defective behaviour for issues #405 to #409. It is test-only and is not
yet merged, so nothing in it needed changing here, but two of its tests
document behaviour that this PR removes and will legitimately start
failing once both land. I verified that by copying #410's two test files
into this branch and running them; the results were:
TestAuditC1StalenessAlertFireClearLoopfails. It asserts three alertrows, all cleared, and three fire plus three clear notifications after
three cycles; it now sees one row, active, and one fire notification.
It should be inverted to assert exactly that, which is what its
Democounterpart already asserts.
TestAuditC1StalenessAlertFireClearLoopDemonow passes. ItsALERTER_DEFECT_DEMOskip guard should be removed. Once the test aboveis inverted the two are equivalent, so one of them can go; the
Demovariant's assertions are the clearer of the pair.TestAuditC1StalenessPathSkipsCooldownGuardfails on its finalassertion, "expected the staleness path to re-fire inside the
cooldown". The staleness path now honours
AlertCooldownPeriodexactlyas the registry-backed control rule in the same test does, so the
assertion should be flipped to require that neither path re-fires.
Everything else in #410 is unaffected: the whole
internal/databaseaudit suite still passes, as do
TestAuditC2,C6,C7,C8andC10in
internal/engine. In particular theC8andC9defects areuntouched here, because this PR changes only the error handling in
checkAlertResolved, not the row-matching path those tests exercise.I also renamed this PR's test fixtures with a
stalenessprefix, because#410 defines constants of the same names (
insertStalenessRuleSQL,insertProbeConfigSQL, and friends) in the same package; with the renameboth files compile together, which I confirmed with
go vetover thecombined tree.