Fix five built-in alert rules that could never fire - #418
Conversation
Each of the five rules failed for a different reason, and every one of them failed silently, because a metric query error is logged at debug level by evaluateRuleForAllConnections and otherwise ignored; a broken rule is indistinguishable from a quiet one. wal_archive_failed selected FROM metrics.pg_stat_archiver, a relation the collector has never created: the archiver counters are consolidated onto metrics.pg_stat_wal, so every evaluation raised 42P01 and WAL archiving failures have never been detectable. The query now reads metrics.pg_stat_wal. transaction_wraparound evaluated a query that joined pg_stat_all_tables and pg_settings, discarded both, and returned the literal 50.0 against a threshold of 75. metrics.pg_stat_all_tables carries no frozen-xid column, so the value now comes from metrics.pg_database.age_datfrozenxid and is expressed as a percentage of the 2^31-1 wraparound limit, which matches the definition the dashboard's XID Age tile and the server's performance summary already use. Template databases are excluded. high_max_connections and connection_utilization both required a metrics.pg_settings row written within the last hour, but the settings probe is change-tracked and skips the write whenever the settings hash is unchanged, with no max-age heartbeat; a stable server therefore receives one snapshot at onboarding and nothing after, and both metrics died within the hour. They now take the newest row per connection, as the historical variants already did. The same one-hour window was silently defeating table_last_autovacuum_hours, which fell back through its COALESCE defaults and ignored tuned autovacuum settings, so that query is fixed alongside them. cpu_usage_high keyed on processor_time_percent, which the system_stats extension populates only on Windows; on Linux the column is NULL and the COALESCE yielded 0, so the rule read an idle host on a saturated one. A shared expression now prefers the Windows column, falls back to 100 minus idle_mode_percent, and finally sums the per-mode buckets. checkpoint_warning compared "more than 50 requested checkpoints" against the largest increase between two consecutive samples of a 600 second probe, which made the threshold unreachable, and its window held two samples only about half the time, so the rule evaluated intermittently. Both this metric and the archiver metric now sum positive per-sample deltas over an hour, which survives a statistics reset and reports zero rather than vanishing when only one sample lands in the window. Collector migration 8 realigns the seeded rules with those semantics: it rewrites the descriptions and units unconditionally, but only rewrites the checkpoint threshold where the row still carries the old shipped default of 50, so operator tuning survives the upgrade. Closes #406
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 31 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 (10)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 112 |
| Duplication | 35 |
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.
The metrics.* tables do carry a cascading foreign key to connections now, added by addConstraintIfMissing in the consolidated migration, so the note claiming they deliberately carry none was wrong. The orphan-filtering convention stays, because the joins cost nothing and the queries that predate the constraint still rely on them.
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
1 participant
Summary
Five built-in alert rules were incapable of firing, each for a different
reason, and all five failed silently: a metric query error is logged at
debug level in
evaluateRuleForAllConnectionsand otherwise discarded,so a broken rule looks exactly like a quiet one. Every root cause was
reproduced against a live PostgreSQL 18 running the real collector
SchemaManagerschema before it was fixed.wal_archive_failedselectedFROM metrics.pg_stat_archiver, whichthe collector has never created; applying the schema to a fresh
database yields 36
metrics.*tables and no views, and the archivercounters are consolidated onto
metrics.pg_stat_wal. Every evaluationraised
42P01, so WAL archiving failure has never been detectable.The query now reads
metrics.pg_stat_wal.transaction_wraparoundevaluated a query that joinedpg_stat_all_tablesandpg_settings, discarded both, and returnedthe literal
50.0against a threshold of 75; seeding tables at 1 and5e9 live tuples still returned exactly 50.0.
metrics.pg_stat_all_tableshas no frozen-xid column, so the value now comes from
metrics.pg_database.age_datfrozenxidas a percentage of the 2^31-1wraparound limit, matching the XID Age tile and the server's
performance summary. Template databases are excluded.
high_max_connectionsandconnection_utilizationrequired ametrics.pg_settingsrow newer than one hour, but the settings probeis change-tracked and skips the write when the hash is unchanged, with
no heartbeat; a stable server stores one snapshot at onboarding and
nothing after, and both metrics returned
no data foundonce that rowpassed 61 minutes. Both now take the newest row per connection, as the
historical variants already did. The same window was quietly defeating
table_last_autovacuum_hours, which fell through itsCOALESCEdefaults and ignored tuned
autovacuum_vacuum_thresholdand scalefactor, so that query is fixed alongside them.
cpu_usage_highkeyed onprocessor_time_percent, whichsystem_statspopulates only on Windows; on Linux it is NULL and theCOALESCEyielded 0, so a 95% busy host reported 0. A sharedexpression now prefers the Windows column, falls back to
100 - idle_mode_percent, and finally sums the per-mode buckets,clamped to 0-100.
checkpoint_warningcompared "more than 50 requested checkpoints"against the largest increase between two consecutive samples of a 600
second probe, and returned no row at all when only one sample fell
inside its 15 minute window. Both this metric and the archiver metric
now sum positive per-sample deltas over an hour, which survives a
statistics reset and reports 0 rather than vanishing on a single
sample.
Collector migration 8 realigns the seeded rules with those semantics. It
rewrites descriptions and units unconditionally, but only rewrites the
checkpoint threshold (50 to 12 per hour) where the row still carries the
old shipped default, so operator tuning survives the upgrade. The v1
seed data carries the same values for fresh installs.
Interaction with the other open alerter PRs
Based on
origin/main, and deliberately confined tometric_registry.goand the seeded rules; nothing here touchesengine/cleanup.goorengine/thresholds.go, so there is no overlapwith #412.
#410 asserts the current defective behaviour, and this change
legitimately invalidates four of its
TestAudit*tests. None of themshould be deleted or weakened; they should be inverted to assert the
fixed behaviour:
TestAuditC2ArchiverTableDoesNotExistandTestAuditC2ArchiverRuleErrorIsSwallowed: the first subtest, that thecollector never creates
metrics.pg_stat_archiver, stays true andshould be kept. The assertions that the metric errors, and that the
error is swallowed, no longer hold; the metric now returns a value
from
metrics.pg_stat_wal.TestDeadRuleArchiverMetricReadsPgStatWalin this PR pins the replacement behaviour.
TestAuditC3TransactionWraparoundReturnsConstant:age_percentnolonger returns 50.0. Invert it to assert the value tracks
age_datfrozenxid, asTestDeadRuleTransactionWraparoundFiresdoes.TestAuditC4PgSettingsMetricsExpireAfterOneHour: both metrics nowresolve from a snapshot older than an hour. Invert it to assert
survival, as
TestDeadRulePgSettingsMetricsSurviveStaleSnapshotdoes.The collector's
TestPgSettingsProbe_StoreUnchangedis unaffected,because the probe's change-tracking behaviour is unchanged; only the
alerter's assumption about it changed.
TestAuditC5CPUUsageNullProcessorTimeReadsZero: a NULLprocessor_time_percentno longer reads 0. Invert it to assert theLinux fallback, as
TestDeadRuleCPUUsageFiresOnLinuxandTestDeadRuleCPUUsageWindowsAndIdleFallbacksdo.TestAuditC7HistoricalSQLCoverageis unaffected:age_percentand bothdelta metrics still have no
historicalSQL, and backfilling those isout of scope here. The other
TestAudit*tests cover behaviour this PRdoes not touch.
Test plan
alerter/src/internal/database/dead_alert_rules_integration_test.go(13 tests) seeds each rule's scenario and asserts the metric now
returns a value that crosses the rule's shipped default threshold, and
also covers the stats-reset, single-sample, Windows, idle-fallback and
bucket-sum branches.
collector/src/database/migration_v8_test.go(4 tests) covers thefresh-install values, the upgrade path over rewound legacy rows, the
preservation of a tuned threshold, and the migration's error path.
collector/src/database/migration_v7_test.goandschema_test.goupdated for the new migration count;
Migratecompares against thehighest recorded version, so the v7 rewind now deletes rows
>= 7.queries_integration_test.go'spg_sys_cpu_usage_infofixture gainedthe Linux columns the portable CPU expression reads.
cd alerter && make coverage: all tests pass,internal/databaseat86.4%. The registry change is a package-level map and const literal
with no coverable statements; it is exercised end-to-end by the new
integration tests.
cd collector && make coverage: all tests pass,src/databaseat81.2%; migration 8's
Upis at 100% (5/5 statements).make lintclean for both collector and alerter.make test-allfrom the root: collector, alerter and the rest of theserver suite pass. The two pre-existing
server/internal/toolsmemoryembedding failures (
expected 3 dimensions, not 4000) are unrelatedand are the subject of fix(server): widen memory embedding test fixture to halfvec(4000) #382.
Closes #406