Add baseline-deviation database-state alert - #2002
Conversation
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thorough, high-quality contribution — thank you. You clearly read the house patterns: the V47 hand-create-table-plus-catalog-driven-hypertable idiom, the engine's family conventions (cooldown stamped even when muted, no fabricated recovery on fetch failure, resolution gated on enable), the first-observation fix implemented symmetrically in both stores with both halves pinned by tests, and a CHANGELOG entry in the right shape. I fetched the branch, read the full diff, and built the service chain locally (Alerting/Collectors/Storage/Notifications/Darling.Service: 0 warnings, 0 errors). Requesting changes for one mechanical reason (the fork is two collector-generations stale) and one behavioral gap worth closing while you're in here.
Blocking: rebase against current dev (dev moved twice since your base)
Your base ae278b72 predates the PVS alert (#1984) and this week's other merges, which took the exact surfaces this PR touches. After rebasing on dev:
- Migration
45→49,StorageVersion.SchemaVersion→49, CHANGELOG text "schema v45" → v49. Dev is at 48; per the applier's ascent rule your V45 would silently skip on upgraded stores and the feature would be dead there (the tables would never exist). - CI worker sizing: dev is already at
timescaledb.max_background_workers = 43/max_worker_processes = 54(hypertable count 41). Your +1 makes it 44 / 55, not 42/53. - The six schedule-preset tables and
DarlingManagedRolesnow carry pvs_stats rows and a restructured role model (mcprole writesconfig_alert_settingsetc.) — your additions are right in spirit but will need re-applying against the new shape. Same for the probe:hasDatabaseStateAlertslots afterhasCollectorStateplus whatever #1984 added. git merge devcurrently conflicts; CI never ran on this PR because GitHub can't build the merge commit. Once you push the rebase I'll approve the workflow runs promptly.
Behavioral: two transient-state scenarios that will page people
1. Instance restarts produce an alert storm (including CRITICALs). After any restart — patch window, failover, crash — databases pass through RECOVERY_PENDING and RECOVERING before ONLINE. The collector reconnects while crash recovery is still running (minutes, on busy instances), the engine's next sweep reads that latest snapshot, and every affected database fires: RECOVERING → WARNING, RECOVERY_PENDING → CRITICAL (it's in your critical set), each followed by a resolution notice minutes later. A monthly patch window across a fleet turns that into dozens of paired fire/resolve pairs for a completely normal restart.
Proposed fix: require the deviation to be present in the two most recent collections before firing (change the deviation read to join the two newest snapshots and require the mismatch in both). At the per-minute default that adds ~1 minute of latency to a genuine SUSPECT — cheap — while making restart transients invisible unless recovery actually sticks, which is exactly the case you do want to page on (a stuck RECOVERY_PENDING).
2. is_in_standby is collected but never read. Your collector doc says it's captured so a standby log-shipping secondary is "distinguishable from a genuinely stuck restore" — but nothing downstream uses it (I grepped: it exists only in the collector, schema, and golden-schema pin). Concretely: a STANDBY-mode secondary reports state_desc = ONLINE with is_in_standby = 1, so it baselines as ONLINE — then every periodic log restore flips it through RESTORING, and each flip the sampler catches fires a WARNING + later a resolution. Suggest composing an effective state — is_in_standby ? 'STANDBY' : state_desc — through the seed, the deviation read, and the override editor (plus a STANDBY token in the dropdown), so standby secondaries baseline as STANDBY; the two-sample rule from finding 1 then absorbs the restore window itself. Worth validating the exact state_desc/flag sequence against a live standby restore, which I'm happy to do on the next round. If you'd rather not model STANDBY at all, drop the column and the doc claim rather than shipping it write-only.
Doc nits (stale relative to your own fix)
DatabaseStateInfoand the Lite store doc both still say a database "never alerts on first sight" — your pending-critical design (correctly) alerts on first sight for SUSPECT/RECOVERY_PENDING/EMERGENCY. Reword to "never alerts on a healthy first sight."DatabaseStateCollector's class doc has a leftover paragraph saying Azure SQL DB "needs no special path — the same single query runs everywhere," which contradicts the (correct)AppliesTogate and its own doc right below.- The engine's field comment says the cooldown key is
{serverKey}{dbName}; the implementation (correctly) usesserverKey + "|" + dbName.
Minor, non-blocking
- Dropped databases leave their
database_state_expectedrows behind forever. Bounded growth, but a delete-on-absent sweep (or a note that the editor shows them for manual cleanup) would tidy it. - The engine compares database names
OrdinalIgnoreCasewhile both stores' joins are case-sensitive — a rename differing only by case would strand the old row and could seed a duplicate. Edge-case; noting for completeness.
What I verified against the checklist
First-observation hole: closed correctly in both stores (critical states never seed; pending rows alert via the no-baseline arm) with store- and engine-level tests. Collector tax: schedule defaults (1/30) ✓, pinned set ✓, presets ✓ (will re-conflict), sizing ✓ (numbers need the rebase update). Grants: viewer write on the override table matches the custom_views model ✓ (re-check placement after rebase). Azure gating: AppliesTo(!IsAzureSqlDb) with a reasoned doc ✓. Base dev ✓, CHANGELOG + bottom link ref ✓, tests in-PR ✓. Security scan of the full diff: clean.
After you push the rebase I'll approve CI, and before merge I'll run the whole path live against a Docker SQL Server (offline/suspect/recovery transitions) — that will also settle the standby question empirically.
When the CPU and IO anomaly arms moved to reading the raw hypertables (erikdarlingdata#1743/erikdarlingdata#1995 - medians cannot be computed from the aggregates' sufficient statistics), cpu_utilization_baseline and file_io_baseline lost their only reader - but stayed registered, kept materializing hourly, and kept holding storage on every store. Fresh stores stop creating them: both entries leave BaselineAggregates (the single list the ensure sweep, fallback-view fill, policy list and retention all derive from), their Create SQL constants and SourceTableFor arms go with them, and the tier is seven aggregates. Upgraded stores get a startup sweep (DropRetiredBaselineAggregatesAsync, the reshape-drop shape) that removes BOTH implementations the field can carry: the continuous aggregate on TimescaleDB stores - policies drop with it - and the plain fallback view on plain-PostgreSQL stores, discriminated the same way DropBaselineFallbackViewSql does (a CAGG is also a relkind='v' view; the two need different DROP verbs). It runs in the worker's UNGATED fallback block so every store shape is cleaned, no-ops on fresh stores, is idempotent, and is failure-isolated per relation. NO schema migration - which also sidesteps the migration number erikdarlingdata#2002's rebase has claimed. Live-validated on a scratch TimescaleDB store in all three states an upgraded store presents: the CAGG shape (2 dropped, refresh+retention policy jobs confirmed gone with them), the plain-view shape, and already-gone (second pass drops 0). Tests: pins for the retired list, the living list excluding the retired names (a re-add would resurrect a create/drop fight), the drop SQL's dual-verb discrimination, and the worker's ungated call placement; a live three-state sweep test against dev Postgres; existing pins that referenced the retired constants move to perfmon_baseline, and the file_io CAGG cast pin dies with the CAGG (the raw-arm pins are the surviving guarantee). Closes erikdarlingdata#2007 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1a4bbd6 to
59a4fd0
Compare
|
Thanks for the thorough review — all points addressed and pushed. Summary: Behavioral changes you asked for
Rebase onto current
|
- New per-minute database_states collector (sys.databases.state_desc + is_in_standby time series); registered in CollectorCatalog + CollectorScheduleDefaults, dispatched in both SKUs - Per-(server, database) expected-state store: an auto-seeded first-observation baseline plus a user-editable override with an (ignore) opt-out, so a legitimately offline or log-shipping database stays quiet without any tuning. A critical first observation (SUSPECT/RECOVERY_PENDING/EMERGENCY) is left pending and alerts rather than learning the bad state as the baseline - Two-sample rule: the alert fires only when the deviation is present in the two most recent collections, so a restart's brief RECOVERING/RECOVERY_PENDING transient does not false-fire. Auto-baselines for databases absent from the newest snapshot are pruned (user overrides preserved) - Effective STANDBY state: a read-only log-shipping secondary (is_in_standby) is modelled at a single stable STANDBY rather than the RESTORING it flickers through on every log restore, so it baselines once and does not churn - Shared AlertEngine check fires when a database's current effective state differs from its expected state: CRITICAL for SUSPECT/RECOVERY_PENDING/EMERGENCY, WARNING otherwise, per-database cooldown and resolution, routed through the existing mute + delivery paths - Stores: Lite DuckDB (auto-created tables) and Darling Postgres (migration V49, schema version -> 49, viewer schema-probe sentinel, CI worker sizing 44/55) - Settings enable toggle + per-database override editor window in Lite and the Darling Viewer - MCP get/update_alert_settings expose the master toggle; web viewer auto-displays fired alerts - Tests: engine fire/severity/cooldown/resolve + real-DuckDB store round-trips including the two-sample debounce, critical-transient, and standby-secondary cases; Lite 2059, Darling 3991 pass
59a4fd0 to
f6948fd
Compare
Implements the database-state alert proposed in #1986. Thanks for the green light and the review notes — this addresses all of them.
What it does
Fires when a monitored database's current state deviates from its expected state, rather than on any non-ONLINE state (which would page every log-shipping secondary).
database_configonly capturesstate_descat server-load, so a database going OFFLINE / SUSPECT / RECOVERY_PENDING / RESTORING after onboarding was invisible until now.database_statescollector capturessys.databases.state_desc(+is_in_standby) as a time series.(ignore)to opt a database out.First-observation hole — closed (your review point)
A critical first observation does not baseline itself. Onboard a server while a database is SUSPECT and the engine will not learn SUSPECT as expected (which would go quiet through the outage, then fire on recovery). Instead:
"Pending" is modelled as the absence of a baseline row, so the deviation read is
(no baseline AND current is critical) OR (baseline exists, not ignored, current != baseline). Covered by new store-level and engine-level tests.Logistics from your checklist
database_statescollector table +config.database_state_expected+ thedatabase_state_enabledcolumn ship as V45;StorageVersion→ 45; the viewer schema-probe gains a matching newest-first sentinel. Lite's DuckDB tables are catalog-generated / non-collector-added.CollectorScheduleDefaults, the pinned collector set, and all six schedule preset tables (Lites_presets+ DarlingCollectorSchedulePresets, @ 1/1/5); the two preset pin tests and the cross-app parity test pass.build.yml/nightly.ymlmove totimescaledb.max_background_workers = 42,max_worker_processes = 53(kept in step withCiClusterWorkerSizingTests).config.database_state_expected(for the override editor) is added to bothDarling/tools/provision-roles.sqlandDarlingManagedRoles, mirroring thecustom_viewsmodel; SELECT is covered by the blanket config grant.AppliesTo => !IsAzureSqlDb(Managed Instance stays on), matching the AG / system-health collectors.dev; CHANGELOG entry added with a bottom link-ref; tests are in this PR.UI / surfaces
A Configure… button in each desktop app's Settings → Alerts opens a per-database editor (current state, expected state, re-baseline,
(ignore); pending databases are labelled). The web viewer auto-displays fired alerts. The MCPget_alert_settings/update_alert_settingsexpose the master toggle.Testing
dev: Lite 2002, Darling 3936 (live-Postgres legs skipped locally). Builds clean, no warnings.Happy to adjust anything to fit the project's direction.