From f20f3c81e703dbe37f5d2a687c34748f16f25ac4 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 18 Jul 2026 17:12:07 -0400 Subject: [PATCH] Unify collector-health ROW banding + fix daily-collector false-positive (#1573) The per-collector NEVER_RUN/NO_PERMISSIONS/FAILING/STALE/WARNING/HEALTHY status was three byte-identical copies -- Lite's grid, the Darling viewer's grid + Overview "collectors failing" count, and the service's get_collection_health MCP tool + web fleet failing-count reader -- with flat 4h-STALE / 24h-FAILING thresholds that assumed a ~1-min collector. index_object_stats is a daily (1440-min) collector that succeeds every run, so it always read STALE then FAILING between successes: a real field false-positive that also inflated the Overview failing count (which reuses the same row banding). - Extract one pure classifier, CollectorHealthClassifier, into PerformanceMonitor.Common (beside the #1562 card classifier). It centralizes the on-load set and makes the staleness thresholds relative to each collector's cadence: FAILING > max(24, 2 x freqHours), STALE > max(4, 1.5 x freqHours). The floors keep every frequent collector byte-for-byte identical; only a slow collector relaxes (daily: stale 36h, fail 48h), so index_object_stats at 27h now reads HEALTHY. - Refactor all three consumers onto it; each keeps its own SQL / row model / display and resolves the collector's cadence from the shared CollectorScheduleDefaults default (no consumer cheaply has a per-install override at the row level, so all three use the same default -- the parity guarantee). - Mirrored decision-table tests in Lite.Tests + Darling.Tests pin the whole table, including index_object_stats HEALTHY at 27h (the exact bug) and every frequent / on-load / NEVER_RUN / NO_PERMISSIONS / WARNING case unchanged. Closes #1573 Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 4 +- .../CollectorHealthClassifierTests.cs | 182 ++++++++++++++++++ .../Mcp/DarlingDataReader.cs | 49 ++--- .../ViewerDataService.CollectionHealth.cs | 50 ++--- Lite.Tests/CollectorHealthClassifierTests.cs | 181 +++++++++++++++++ .../LocalDataService.CollectionHealth.cs | 48 ++--- .../ServerHealthBands.cs | 132 +++++++++++++ 7 files changed, 550 insertions(+), 96 deletions(-) create mode 100644 Darling/Darling.Tests/CollectorHealthClassifierTests.cs create mode 100644 Lite.Tests/CollectorHealthClassifierTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index eefd63c7c..40ce50ed7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Darling scheduler: a service restart no longer starves long-frequency collectors - NextDue is seeded from the persisted collection watermark, not a full-interval offset** ([#1577]) - a daily collector (`index_object_stats`, CollectorScheduleDefaults frequency 1440 min) read as FAILING across most of the fleet even though every run it *did* complete was a SUCCESS; field-confirmed on DARLING01, where after a day with ~4 deploy restarts it had not run on 4 of 5 servers in ~27h. Root cause: on EVERY connect, `TryConnectAsync` seeded each collector's next-due as `now + CadencePhaseOffset(serverId, frequencySeconds)` (and `RecomputeNextDue`'s new-entry branch did the same) - a deterministic per-server phase (`(uint)serverId % periodSeconds`) that for a daily collector is *anywhere up to ~24h*. The persisted last-collection watermark was never consulted for scheduling, so each restart re-phased every collector up to a full interval forward, discarding when it actually last ran. Minute collectors were invisible to this (their offset is < 60s); long-frequency collectors were **starved indefinitely in a restart-heavy window** - each restart pushed the next run past the following restart. The full-interval offset (introduced as [#1553]'s anti-lockstep jitter) is simply too coarse for long cadences, and it also meant a FRESH install waited up to 24h for a daily collector's first run. **Fix:** the seed sites now read each collector's `MAX(collection_time)` in ONE batched round-trip per server and decide the next-due via a new pure `ComputeSeededNextDue(lastRunUtc, frequencyMinutes, nowUtc, jitter)`: a recently-run collector waits out the REMAINING interval (`lastRun + interval`), an overdue one runs promptly, and a never-run one runs shortly after connect - overdue/never-run stamped at `now + a small per-server jitter` capped at `min(interval, 150s)` (mirroring the analysis-phase jitter) to de-cluster the fleet WITHOUT the full-interval defer. A restart now RESUMES the real cadence instead of re-phasing it, a daily collector actually collects daily across restarts, and a fresh install runs it minutes after first connect (then daily). The watermark read is failure-isolated (a store hiccup falls back to a prompt jittered run, never aborting the connect), the new-entry branch of the reload recompute gets the same treatment (lazily, at most one extra round-trip per server that gains a collector), and the steady-state advance in `RunDueCollectorsAsync` (already the exact interval) is unchanged. Distinct from #1573, which fixes the *banding* (a daily collector shouldn't read FAILING at 25h); both are needed - with only #1573 the collector still wouldn't run, with only this the banding would still mis-flag the legitimate gap. Darling service only, no store schema change; the DarlingWorker field-hardening invariants (fire-and-track Retired flag, outer-thread-only sweep state, never-dispose gate, never await-all) are untouched. Verified: the Darling service builds clean in Release and the full `Darling.Tests` suite is green (**2403 passed / 0 failed / 138 gated-live skipped**), including a pure decision-table for the seed policy (daily recently-run waits ~23h; daily overdue runs promptly; 1-min recently-run keeps its ~30s feel; daily never-run seeds within ~150s - the fresh-install fix; the overdue boundary is inclusive) and the jitter cap (deterministic per FNV id, capped at `min(interval, 150s)`), plus a gated-live pin that the batched read returns `MAX(collection_time)` per collector relabeled Kind=Utc. - +- **Collector health: a healthy DAILY collector no longer bands STALE/FAILING, and no longer inflates the Overview "collectors failing" count** ([#1578]) - the per-collector health status used FLAT thresholds (STALE past 4h since last success, FAILING past 24h) that assumed a frequent (~1-min) collector. `index_object_stats` is a DAILY collector (`CollectorScheduleDefaults` cadence 1440 min) that succeeds every run, so between its once-a-day successes it always crossed 4h and then 24h and read STALE then FAILING - a real field false-positive. Because the Overview card's "Collectors: N failing" count reuses the SAME row banding (it counts `HealthStatus == "FAILING"`), a healthy daily collector also showed the fleet Overview a phantom failing collector. The staleness thresholds are now RELATIVE to each collector's own cadence: **FAILING when hours-since-success > max(24, 2 x freqHours)** and **STALE when > max(4, 1.5 x freqHours)**. The floors are the original flat values, so every FREQUENT collector bands byte-for-byte identically (a 1-min collector still goes stale at 4h and fails at 24h; an hourly collector too, since 1.5h/2h are under the floors); only a slow collector relaxes - a daily collector now goes stale at 36h and fails at 48h, so `index_object_stats` at 27h reads HEALTHY. On-load config snapshots (`server_config` / `database_config` / `database_scoped_config` / `trace_flags` / `server_properties`) stay staleness-exempt (banded by failure rate only), and NEVER_RUN / NO_PERMISSIONS / WARNING are unchanged. The banding was **three byte-identical copies** that nothing pinned together (the #1573 core) - Lite's grid, the Darling viewer's grid + Overview count, and the service's `get_collection_health` MCP tool + web fleet failing-count reader; all three now delegate to ONE pure classifier, `CollectorHealthClassifier` in `PerformanceMonitor.Common` (alongside the #1562 per-server card classifier), which also centralizes the on-load set so the surfaces cannot drift. Each surface resolves the collector's cadence from the shared `CollectorScheduleDefaults` default (no consumer cheaply has a per-install schedule override at the row level, so all three use the same default - which is itself the parity guarantee); an unknown collector name falls to the 24h/4h floors, i.e. the old flat behavior. A mirrored decision-table test in Lite.Tests and Darling.Tests pins the whole table - including that `index_object_stats`' default 1440-min cadence yields HEALTHY at 27h, the exact bug - so the two SKUs can never drift. - **Darling: the retention purge's batched DELETE no longer breaks on compressed chunks** ([#1567]) - the purge's row-capped `ctid IN (SELECT ctid ... LIMIT 10000)` batching idiom cannot execute against a TimescaleDB table with a compressed chunk in range: reading the `ctid` system column through transparent decompression is unsupported ("transparent decompression only supports tableoid system column", reproduced on the pinned 2.28.1), so the whole statement errored, the per-table isolation swallowed it as a warning, and the table silently kept its expired rows. In production that broke the DELETE fallback exactly when it matters - on a hypertable with compressed chunks whose `drop_chunks` transiently failed - and on plain-PostgreSQL stores pointed at a Timescale-compressed table. The batcher now deletes the OLDEST one-day slice of expired rows per statement (a plain time-range predicate rides TimescaleDB's supported DML decompression; the per-DML decompression cap is lifted on the purge connection, a placeholder-safe SET on stores without the extension). A day is one chunk on a hypertable and one day's arrival volume on a plain table - the same bounded-work goal the row cap served. Found via #1564: the gated purge tests flaked order-dependently because the shared fixture's compression policy jobs run immediately on creation and compressed the test chunks mid-suite; those tests now assert only their own-scoped contract (global activity counts stay order-dependent), carry a capturing logger so any purge warning lands in the failure text instead of a null logger, and a new deterministic E2E compresses a chunk synchronously and proves the DELETE path clears expired rows inside it. - **Darling: the viewer's store pool is now capped too, and the postgres.exe process count is documented for what it actually is** ([#1566]) — round-4 field verification correctly flagged that the [#1559] "≤24 backends" claim didn't hold: the process count oscillated to 44 during checkpoint/catch-up waves. Two truths behind that: **(1)** the peaks are overwhelmingly **TimescaleDB background policy workers**, not client connections — the managed conf deliberately sizes `timescaledb.max_background_workers` to hypertables + 2 (≈38), and every running compression/retention job is its own postgres.exe, so the count legitimately surges during policy waves and recedes (the field box's OS free memory was rock-stable through every swing); **(2)** the claim was also genuinely incomplete — the co-located **viewer built its connection string independently and rode Npgsql's default pool of 100**; it is now capped at 10 (a read-only UI seat on 30/60-second timers). The README's troubleshooting section gains the process-anatomy note with the `pg_stat_activity GROUP BY backend_type` decomposition query and the reminder that Windows charges the shared buffer segment to every attached process's working set, so per-process memory numbers cannot be summed. @@ -421,7 +421,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1571]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1571 [#1574]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1574 [#1577]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1577 -[#1536]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1536 +[#1578]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1578[#1536]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1536 [#1534]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1534 [#1533]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1533 [#1532]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1532 diff --git a/Darling/Darling.Tests/CollectorHealthClassifierTests.cs b/Darling/Darling.Tests/CollectorHealthClassifierTests.cs new file mode 100644 index 000000000..00d50ffe5 --- /dev/null +++ b/Darling/Darling.Tests/CollectorHealthClassifierTests.cs @@ -0,0 +1,182 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// Decision-table pins for the shared (#1573) - the one banding +/// the Lite grid, this viewer's grid + Overview "collectors failing" count, and the service's +/// get_collection_health / web fleet reader all delegate to. This SAME table is pinned identically in +/// Lite.Tests so the two SKUs' collector-health banding cannot drift. +/// +/// +/// The load-bearing case is the motivating bug: index_object_stats is a DAILY collector +/// ( cadence 1440 min) that succeeds every run, yet the old flat +/// 4h-STALE / 24h-FAILING thresholds flagged it STALE past 4h and FAILING past 24h. The thresholds are +/// now relative to each collector's cadence (FAILING = max(24, 2 x freqHours); STALE = max(4, 1.5 x +/// freqHours)), with floors that keep every FREQUENT collector byte-for-byte identical. +/// +/// +public sealed class CollectorHealthClassifierTests +{ + private const int OneMinute = 1; // wait_stats, cpu_utilization, etc. - the fastest cadence. + private const int Hourly = 60; // database_size_stats - still floor-bound (1.5h/2h < 4h/24h). + private const int Daily = 1440; // index_object_stats - the collector that relaxes. + private const int OnLoadFreq = 0; // config snapshots run on connect, not on the loop. + + [Theory] + /* NEVER_RUN wins first, whatever else is set (including on-load). */ + [InlineData(0, 0, 0, 0, 999, OneMinute, false, CollectorHealthClassifier.NeverRun)] + [InlineData(0, 0, 0, 0, 0, OnLoadFreq, true, CollectorHealthClassifier.NeverRun)] + + /* NO_PERMISSIONS (only permission denials) is checked before the on-load branch. */ + [InlineData(5, 0, 0, 5, 999, OneMinute, false, CollectorHealthClassifier.NoPermissions)] + [InlineData(5, 0, 0, 5, 999, OnLoadFreq, true, CollectorHealthClassifier.NoPermissions)] + + /* On-load collectors are staleness-exempt: banded by failure rate only, never STALE/FAILING. */ + [InlineData(10, 10, 0, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Healthy)] + [InlineData(10, 7, 3, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Warning)] // 30% > 20% + + /* Frequent (1-min) collector - the floors mean these are IDENTICAL to the old flat thresholds. */ + [InlineData(10, 10, 0, 0, 2, OneMinute, false, CollectorHealthClassifier.Healthy)] // < 4h + [InlineData(10, 7, 3, 0, 2, OneMinute, false, CollectorHealthClassifier.Warning)] // recent, 30% fail + [InlineData(10, 10, 0, 0, 5, OneMinute, false, CollectorHealthClassifier.Stale)] // > 4h, < 24h + [InlineData(10, 10, 0, 0, 25, OneMinute, false, CollectorHealthClassifier.Failing)] // > 24h + [InlineData(5, 0, 5, 0, 999, OneMinute, false, CollectorHealthClassifier.Failing)] // ran, never a success (999 sentinel) + + /* Hourly (60-min) collector - 1.5h/2h are both under the 4h/24h floors, so still floor-bound. */ + [InlineData(10, 10, 0, 0, 5, Hourly, false, CollectorHealthClassifier.Stale)] // > 4h floor + [InlineData(10, 10, 0, 0, 25, Hourly, false, CollectorHealthClassifier.Failing)] // > 24h floor + + /* Daily (1440-min) collector - the FIX. STALE line 36h, FAILING line 48h. */ + [InlineData(30, 30, 0, 0, 27, Daily, false, CollectorHealthClassifier.Healthy)] // THE BUG: was FAILING, now HEALTHY + [InlineData(30, 30, 0, 0, 35, Daily, false, CollectorHealthClassifier.Healthy)] // still under the 36h stale line + [InlineData(30, 30, 0, 0, 37, Daily, false, CollectorHealthClassifier.Stale)] // > 36h, < 48h + [InlineData(30, 30, 0, 0, 47, Daily, false, CollectorHealthClassifier.Stale)] // still stale, not failing + [InlineData(30, 30, 0, 0, 49, Daily, false, CollectorHealthClassifier.Failing)] // > 48h + [InlineData(10, 7, 3, 0, 1, Daily, false, CollectorHealthClassifier.Warning)] // recent, 30% fail -> WARNING not STALE + public void Classify_BandsTheDecisionTable( + long totalRuns, long successCount, long errorCount, long permissionDeniedCount, + double hoursSinceLastSuccess, int frequencyMinutes, bool isOnLoad, string expected) + { + Assert.Equal(expected, CollectorHealthClassifier.Classify( + totalRuns, successCount, errorCount, permissionDeniedCount, + hoursSinceLastSuccess, frequencyMinutes, isOnLoad)); + } + + [Theory] + [InlineData(OneMinute, 24.0)] // floor + [InlineData(Hourly, 24.0)] // 2 x 1h = 2h < 24h floor + [InlineData(720, 24.0)] // 2 x 12h = 24h, exactly the floor + [InlineData(Daily, 48.0)] // 2 x 24h = 48h relaxes past the floor + public void FailingThresholdHours_IsMaxOfFloorAndTwiceCadence(int frequencyMinutes, double expected) => + Assert.Equal(expected, CollectorHealthClassifier.FailingThresholdHours(frequencyMinutes), 3); + + [Theory] + [InlineData(OneMinute, 4.0)] // floor + [InlineData(Hourly, 4.0)] // 1.5 x 1h = 1.5h < 4h floor + [InlineData(Daily, 36.0)] // 1.5 x 24h = 36h relaxes past the floor + public void StaleThresholdHours_IsMaxOfFloorAndOnePointFiveCadence(int frequencyMinutes, double expected) => + Assert.Equal(expected, CollectorHealthClassifier.StaleThresholdHours(frequencyMinutes), 3); + + [Theory] + [InlineData("server_config", true)] + [InlineData("database_config", true)] + [InlineData("database_scoped_config", true)] + [InlineData("trace_flags", true)] + [InlineData("server_properties", true)] + [InlineData("SERVER_CONFIG", true)] // case-insensitive + [InlineData("wait_stats", false)] + [InlineData("index_object_stats", false)] + public void IsOnLoadCollector_MatchesTheFreqZeroSet(string collectorName, bool expected) => + Assert.Equal(expected, CollectorHealthClassifier.IsOnLoadCollector(collectorName)); + + /// The on-load set is exactly the FrequencyMinutes == 0 entries in the shared schedule table - + /// the invariant that lets a caller resolve cadence and on-load-ness from the same source. + [Fact] + public void OnLoadSet_EqualsTheFreqZeroScheduleEntries() + { + foreach (var (name, entry) in CollectorScheduleDefaults.All) + { + Assert.Equal(entry.FrequencyMinutes == 0, CollectorHealthClassifier.IsOnLoadCollector(name)); + } + } + + /* --- The bug, reproduced through the viewer's real CollectorHealthRow (it resolves the cadence from + CollectorScheduleDefaults by name). --- */ + + [Fact] + public void IndexObjectStats_DefaultCadence_IsDaily() + { + // Pins the exact input that produced the field false-positive: a 1440-min (daily) collector. + Assert.Equal(1440, CollectorScheduleDefaults.All["index_object_stats"].FrequencyMinutes); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At27Hours_IsHealthyNotFailing() + { + /* The motivating bug: index_object_stats succeeds daily; 27h since last success is well within a + daily cadence. Under the old flat 24h-FAILING threshold this read FAILING (and inflated the + Overview "collectors failing" count); it now reads HEALTHY. */ + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-27), + }; + Assert.Equal("HEALTHY", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At37Hours_IsStale() + { + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-37), + }; + Assert.Equal("STALE", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At49Hours_IsFailing() + { + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-49), + }; + Assert.Equal("FAILING", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_FrequentCollector_BandsUnchanged() + { + /* A 1-min collector is unaffected by the relative thresholds (the floors dominate): 30h -> FAILING, + exactly as before the #1573 change. */ + var row = new CollectorHealthRow + { + CollectorName = "wait_stats", + TotalRuns = 100, + SuccessCount = 100, + LastSuccessTime = DateTime.UtcNow.AddHours(-30), + }; + Assert.Equal("FAILING", row.HealthStatus); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs index 92d3f47d6..9daed1b80 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs @@ -12,6 +12,8 @@ using System.Threading.Tasks; using Npgsql; using NpgsqlTypes; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; namespace PerformanceMonitor.Darling.Service.Mcp; @@ -967,24 +969,16 @@ private static void AddTimestamp(NpgsqlCommand command, DateTime value) => /// /// One collector's 7-day health roll-up with its health band — a faithful service-side port of the /// viewer's CollectorHealthRow (itself Lite's), carrying just the fields the MCP -/// get_collection_health tool surfaces plus the computed / failure rate. The -/// on-load-collector staleness exemption and the NEVER_RUN / NO_PERMISSIONS / FAILING / STALE / WARNING / -/// HEALTHY banding match Lite exactly. arithmetic is correct against the -/// store's naive-UTC timestamps because both are UTC instants (tick subtraction ignores Kind). +/// get_collection_health tool surfaces plus the computed / failure rate. +/// delegates to the shared in +/// PerformanceMonitor.Common (#1573), so this service, Lite, and the viewer band identically and cannot +/// drift; it resolves the collector's cadence from the shared so a +/// healthy DAILY collector is no longer flagged stale/failing on the frequent-collector thresholds. +/// arithmetic is correct against the store's naive-UTC timestamps because +/// both are UTC instants (tick subtraction ignores Kind). /// internal sealed class CollectorHealth { - /// On-load collectors run once per tab open / on connect, not on the scheduled loop, so the - /// staleness thresholds do not apply to them (mirrors Lite / the viewer). - private static readonly HashSet OnLoadCollectors = new(StringComparer.OrdinalIgnoreCase) - { - "server_config", - "database_config", - "database_scoped_config", - "trace_flags", - "server_properties", - }; - public string CollectorName { get; set; } = ""; public long TotalRuns { get; set; } public long SuccessCount { get; set; } @@ -1002,20 +996,13 @@ internal sealed class CollectorHealth ? (DateTime.UtcNow - LastSuccessTime.Value).TotalHours : 999; - public string HealthStatus - { - get - { - if (TotalRuns == 0) return "NEVER_RUN"; - if (PermissionDeniedCount > 0 && ErrorCount == 0 && SuccessCount == 0) return "NO_PERMISSIONS"; - if (OnLoadCollectors.Contains(CollectorName)) - { - return FailureRatePercent > 20 ? "WARNING" : "HEALTHY"; - } - if (HoursSinceLastSuccess > 24) return "FAILING"; - if (HoursSinceLastSuccess > 4) return "STALE"; - if (FailureRatePercent > 20) return "WARNING"; - return "HEALTHY"; - } - } + /// The collector's default cadence from the shared + /// (0 for an on-load or unknown collector — both fall to the floor thresholds). The banding uses the + /// shipped default, not the resolved per-server override, so all three surfaces stay in parity. + private int FrequencyMinutes => + CollectorScheduleDefaults.All.TryGetValue(CollectorName, out var schedule) ? schedule.FrequencyMinutes : 0; + + public string HealthStatus => CollectorHealthClassifier.Classify( + TotalRuns, SuccessCount, ErrorCount, PermissionDeniedCount, + HoursSinceLastSuccess, FrequencyMinutes, CollectorHealthClassifier.IsOnLoadCollector(CollectorName)); } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.CollectionHealth.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.CollectionHealth.cs index ddd8748a6..c6654a4f3 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.CollectionHealth.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.CollectionHealth.cs @@ -11,6 +11,8 @@ using System.Threading; using System.Threading.Tasks; using Npgsql; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; namespace PerformanceMonitor.Darling.Viewer; @@ -313,27 +315,16 @@ public class CollectionLogRow /// One Collection Health "Health Summary" grid row — a collector's 7-day roll-up with its health band. /// Copied VERBATIM from Lite's rich CollectorHealthRow (LocalDataService.CollectionHealth.cs); /// it REPLACES the shell's placeholder CollectorHealthRow record (a single latest-run snapshot). -/// Every property is a pure computation over the aggregate — bands -/// NEVER_RUN / NO_PERMISSIONS / FAILING / STALE / WARNING / HEALTHY with the on-load-collector staleness -/// exemption, exactly as Lite. The arithmetic in +/// Every property is a pure computation over the aggregate — delegates to the +/// shared in PerformanceMonitor.Common (#1573), so Lite, this +/// viewer, and the service band identically and cannot drift; it resolves the collector's cadence from the +/// shared so a healthy DAILY collector is no longer flagged +/// stale/failing on the frequent-collector thresholds. The arithmetic in /// is correct against the store's naive-UTC timestamps because both /// sides are UTC instants (tick subtraction ignores Kind), matching Lite. /// public class CollectorHealthRow { - /// - /// On-load collectors run once per tab open, not on the scheduled loop. - /// Staleness thresholds don't apply to them. - /// - private static readonly HashSet OnLoadCollectors = new(StringComparer.OrdinalIgnoreCase) - { - "server_config", - "database_config", - "database_scoped_config", - "trace_flags", - "server_properties" - }; - public string CollectorName { get; set; } = ""; public long TotalRuns { get; set; } public long SuccessCount { get; set; } @@ -350,23 +341,16 @@ public class CollectorHealthRow ? (DateTime.UtcNow - LastSuccessTime.Value).TotalHours : 999; - public string HealthStatus - { - get - { - if (TotalRuns == 0) return "NEVER_RUN"; - if (PermissionDeniedCount > 0 && ErrorCount == 0 && SuccessCount == 0) return "NO_PERMISSIONS"; - if (OnLoadCollectors.Contains(CollectorName)) - { - if (FailureRatePercent > 20) return "WARNING"; - return "HEALTHY"; - } - if (HoursSinceLastSuccess > 24) return "FAILING"; - if (HoursSinceLastSuccess > 4) return "STALE"; - if (FailureRatePercent > 20) return "WARNING"; - return "HEALTHY"; - } - } + /// The collector's default cadence from the shared + /// (0 for an on-load or unknown collector — both fall to the floor thresholds). The banding uses the + /// shipped default, not any per-install override: the viewer has no cheap per-collector effective + /// frequency at the row level, and using the same default across all three surfaces keeps them in parity. + private int FrequencyMinutes => + CollectorScheduleDefaults.All.TryGetValue(CollectorName, out var schedule) ? schedule.FrequencyMinutes : 0; + + public string HealthStatus => CollectorHealthClassifier.Classify( + TotalRuns, SuccessCount, ErrorCount, PermissionDeniedCount, + HoursSinceLastSuccess, FrequencyMinutes, CollectorHealthClassifier.IsOnLoadCollector(CollectorName)); public string AvgDurationFormatted => AvgDurationMs < 1000 ? $"{AvgDurationMs:F0} ms" diff --git a/Lite.Tests/CollectorHealthClassifierTests.cs b/Lite.Tests/CollectorHealthClassifierTests.cs new file mode 100644 index 000000000..db7ece182 --- /dev/null +++ b/Lite.Tests/CollectorHealthClassifierTests.cs @@ -0,0 +1,181 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitorLite.Services; +using Xunit; + +namespace Lite.Tests; + +/// +/// Decision-table pins for the shared (#1573) - the one banding +/// the Lite grid, the Darling viewer's grid + Overview "collectors failing" count, and the service's +/// get_collection_health / web fleet reader all delegate to. This SAME table is pinned identically in +/// Darling.Tests so the two SKUs' collector-health banding cannot drift. +/// +/// +/// The load-bearing case is the motivating bug: index_object_stats is a DAILY collector +/// ( cadence 1440 min) that succeeds every run, yet the old flat +/// 4h-STALE / 24h-FAILING thresholds flagged it STALE past 4h and FAILING past 24h. The thresholds are +/// now relative to each collector's cadence (FAILING = max(24, 2 x freqHours); STALE = max(4, 1.5 x +/// freqHours)), with floors that keep every FREQUENT collector byte-for-byte identical. +/// +/// +public sealed class CollectorHealthClassifierTests +{ + private const int OneMinute = 1; // wait_stats, cpu_utilization, etc. - the fastest cadence. + private const int Hourly = 60; // database_size_stats - still floor-bound (1.5h/2h < 4h/24h). + private const int Daily = 1440; // index_object_stats - the collector that relaxes. + private const int OnLoadFreq = 0; // config snapshots run on connect, not on the loop. + + [Theory] + /* NEVER_RUN wins first, whatever else is set (including on-load). */ + [InlineData(0, 0, 0, 0, 999, OneMinute, false, CollectorHealthClassifier.NeverRun)] + [InlineData(0, 0, 0, 0, 0, OnLoadFreq, true, CollectorHealthClassifier.NeverRun)] + + /* NO_PERMISSIONS (only permission denials) is checked before the on-load branch. */ + [InlineData(5, 0, 0, 5, 999, OneMinute, false, CollectorHealthClassifier.NoPermissions)] + [InlineData(5, 0, 0, 5, 999, OnLoadFreq, true, CollectorHealthClassifier.NoPermissions)] + + /* On-load collectors are staleness-exempt: banded by failure rate only, never STALE/FAILING. */ + [InlineData(10, 10, 0, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Healthy)] + [InlineData(10, 7, 3, 0, 500, OnLoadFreq, true, CollectorHealthClassifier.Warning)] // 30% > 20% + + /* Frequent (1-min) collector - the floors mean these are IDENTICAL to the old flat thresholds. */ + [InlineData(10, 10, 0, 0, 2, OneMinute, false, CollectorHealthClassifier.Healthy)] // < 4h + [InlineData(10, 7, 3, 0, 2, OneMinute, false, CollectorHealthClassifier.Warning)] // recent, 30% fail + [InlineData(10, 10, 0, 0, 5, OneMinute, false, CollectorHealthClassifier.Stale)] // > 4h, < 24h + [InlineData(10, 10, 0, 0, 25, OneMinute, false, CollectorHealthClassifier.Failing)] // > 24h + [InlineData(5, 0, 5, 0, 999, OneMinute, false, CollectorHealthClassifier.Failing)] // ran, never a success (999 sentinel) + + /* Hourly (60-min) collector - 1.5h/2h are both under the 4h/24h floors, so still floor-bound. */ + [InlineData(10, 10, 0, 0, 5, Hourly, false, CollectorHealthClassifier.Stale)] // > 4h floor + [InlineData(10, 10, 0, 0, 25, Hourly, false, CollectorHealthClassifier.Failing)] // > 24h floor + + /* Daily (1440-min) collector - the FIX. STALE line 36h, FAILING line 48h. */ + [InlineData(30, 30, 0, 0, 27, Daily, false, CollectorHealthClassifier.Healthy)] // THE BUG: was FAILING, now HEALTHY + [InlineData(30, 30, 0, 0, 35, Daily, false, CollectorHealthClassifier.Healthy)] // still under the 36h stale line + [InlineData(30, 30, 0, 0, 37, Daily, false, CollectorHealthClassifier.Stale)] // > 36h, < 48h + [InlineData(30, 30, 0, 0, 47, Daily, false, CollectorHealthClassifier.Stale)] // still stale, not failing + [InlineData(30, 30, 0, 0, 49, Daily, false, CollectorHealthClassifier.Failing)] // > 48h + [InlineData(10, 7, 3, 0, 1, Daily, false, CollectorHealthClassifier.Warning)] // recent, 30% fail -> WARNING not STALE + public void Classify_BandsTheDecisionTable( + long totalRuns, long successCount, long errorCount, long permissionDeniedCount, + double hoursSinceLastSuccess, int frequencyMinutes, bool isOnLoad, string expected) + { + Assert.Equal(expected, CollectorHealthClassifier.Classify( + totalRuns, successCount, errorCount, permissionDeniedCount, + hoursSinceLastSuccess, frequencyMinutes, isOnLoad)); + } + + [Theory] + [InlineData(OneMinute, 24.0)] // floor + [InlineData(Hourly, 24.0)] // 2 x 1h = 2h < 24h floor + [InlineData(720, 24.0)] // 2 x 12h = 24h, exactly the floor + [InlineData(Daily, 48.0)] // 2 x 24h = 48h relaxes past the floor + public void FailingThresholdHours_IsMaxOfFloorAndTwiceCadence(int frequencyMinutes, double expected) => + Assert.Equal(expected, CollectorHealthClassifier.FailingThresholdHours(frequencyMinutes), 3); + + [Theory] + [InlineData(OneMinute, 4.0)] // floor + [InlineData(Hourly, 4.0)] // 1.5 x 1h = 1.5h < 4h floor + [InlineData(Daily, 36.0)] // 1.5 x 24h = 36h relaxes past the floor + public void StaleThresholdHours_IsMaxOfFloorAndOnePointFiveCadence(int frequencyMinutes, double expected) => + Assert.Equal(expected, CollectorHealthClassifier.StaleThresholdHours(frequencyMinutes), 3); + + [Theory] + [InlineData("server_config", true)] + [InlineData("database_config", true)] + [InlineData("database_scoped_config", true)] + [InlineData("trace_flags", true)] + [InlineData("server_properties", true)] + [InlineData("SERVER_CONFIG", true)] // case-insensitive + [InlineData("wait_stats", false)] + [InlineData("index_object_stats", false)] + public void IsOnLoadCollector_MatchesTheFreqZeroSet(string collectorName, bool expected) => + Assert.Equal(expected, CollectorHealthClassifier.IsOnLoadCollector(collectorName)); + + /// The on-load set is exactly the FrequencyMinutes == 0 entries in the shared schedule table - + /// the invariant that lets a caller resolve cadence and on-load-ness from the same source. + [Fact] + public void OnLoadSet_EqualsTheFreqZeroScheduleEntries() + { + foreach (var (name, entry) in CollectorScheduleDefaults.All) + { + Assert.Equal(entry.FrequencyMinutes == 0, CollectorHealthClassifier.IsOnLoadCollector(name)); + } + } + + /* --- The bug, reproduced through Lite's real CollectorHealthRow (it resolves the cadence from + CollectorScheduleDefaults by name). --- */ + + [Fact] + public void IndexObjectStats_DefaultCadence_IsDaily() + { + // Pins the exact input that produced the field false-positive: a 1440-min (daily) collector. + Assert.Equal(1440, CollectorScheduleDefaults.All["index_object_stats"].FrequencyMinutes); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At27Hours_IsHealthyNotFailing() + { + /* The motivating bug: index_object_stats succeeds daily; 27h since last success is well within a + daily cadence. Under the old flat 24h-FAILING threshold this read FAILING; it now reads HEALTHY. */ + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-27), + }; + Assert.Equal("HEALTHY", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At37Hours_IsStale() + { + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-37), + }; + Assert.Equal("STALE", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_IndexObjectStats_At49Hours_IsFailing() + { + var row = new CollectorHealthRow + { + CollectorName = "index_object_stats", + TotalRuns = 7, + SuccessCount = 7, + LastSuccessTime = DateTime.UtcNow.AddHours(-49), + }; + Assert.Equal("FAILING", row.HealthStatus); + } + + [Fact] + public void CollectorHealthRow_FrequentCollector_BandsUnchanged() + { + /* A 1-min collector is unaffected by the relative thresholds (the floors dominate): 30h -> FAILING, + exactly as before the #1573 change. */ + var row = new CollectorHealthRow + { + CollectorName = "wait_stats", + TotalRuns = 100, + SuccessCount = 100, + LastSuccessTime = DateTime.UtcNow.AddHours(-30), + }; + Assert.Equal("FAILING", row.HealthStatus); + } +} diff --git a/Lite/Services/LocalDataService.CollectionHealth.cs b/Lite/Services/LocalDataService.CollectionHealth.cs index 32e8e9564..d936331cb 100644 --- a/Lite/Services/LocalDataService.CollectionHealth.cs +++ b/Lite/Services/LocalDataService.CollectionHealth.cs @@ -10,6 +10,8 @@ using System.Collections.Generic; using System.Threading.Tasks; using DuckDB.NET.Data; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; namespace PerformanceMonitorLite.Services; @@ -198,21 +200,15 @@ public class CollectionLogRow public string DuckDbDurationFormatted => DuckDbDurationMs.HasValue ? $"{DuckDbDurationMs.Value} ms" : ""; } +/// +/// One Collection Health grid row — a collector's 7-day roll-up with its health band. +/// delegates to the shared in +/// PerformanceMonitor.Common (#1573), so Lite, the Darling viewer, and the service band identically and +/// cannot drift; it resolves the collector's cadence from the shared +/// so a healthy DAILY collector is no longer flagged stale/failing on the frequent-collector thresholds. +/// public class CollectorHealthRow { - /// - /// On-load collectors run once per tab open, not on the scheduled loop. - /// Staleness thresholds don't apply to them. - /// - private static readonly HashSet OnLoadCollectors = new(StringComparer.OrdinalIgnoreCase) - { - "server_config", - "database_config", - "database_scoped_config", - "trace_flags", - "server_properties" - }; - public string CollectorName { get; set; } = ""; public long TotalRuns { get; set; } public long SuccessCount { get; set; } @@ -229,23 +225,15 @@ public class CollectorHealthRow ? (DateTime.UtcNow - LastSuccessTime.Value).TotalHours : 999; - public string HealthStatus - { - get - { - if (TotalRuns == 0) return "NEVER_RUN"; - if (PermissionDeniedCount > 0 && ErrorCount == 0 && SuccessCount == 0) return "NO_PERMISSIONS"; - if (OnLoadCollectors.Contains(CollectorName)) - { - if (FailureRatePercent > 20) return "WARNING"; - return "HEALTHY"; - } - if (HoursSinceLastSuccess > 24) return "FAILING"; - if (HoursSinceLastSuccess > 4) return "STALE"; - if (FailureRatePercent > 20) return "WARNING"; - return "HEALTHY"; - } - } + /// The collector's default cadence from the shared + /// (0 for an on-load or unknown collector — both fall to the floor thresholds). The banding uses the + /// shipped default, not the per-install ScheduleManager override, so all three surfaces stay in parity. + private int FrequencyMinutes => + CollectorScheduleDefaults.All.TryGetValue(CollectorName, out var schedule) ? schedule.FrequencyMinutes : 0; + + public string HealthStatus => CollectorHealthClassifier.Classify( + TotalRuns, SuccessCount, ErrorCount, PermissionDeniedCount, + HoursSinceLastSuccess, FrequencyMinutes, CollectorHealthClassifier.IsOnLoadCollector(CollectorName)); public string AvgDurationFormatted => AvgDurationMs < 1000 ? $"{AvgDurationMs:F0} ms" diff --git a/PerformanceMonitor.Common/ServerHealthBands.cs b/PerformanceMonitor.Common/ServerHealthBands.cs index 500fae15a..18c881f8b 100644 --- a/PerformanceMonitor.Common/ServerHealthBands.cs +++ b/PerformanceMonitor.Common/ServerHealthBands.cs @@ -356,4 +356,136 @@ public static long FleetHealthScore(FleetHealthBand band, in ServerHealthMetrics _ => "Healthy", }; } + + /// + /// The one, app-agnostic source of truth for the collector-health ROW banding — the per-collector + /// NEVER_RUN / NO_PERMISSIONS / FAILING / STALE / WARNING / HEALTHY status shown on every Collection + /// Health surface (Lite's grid, the Darling WPF viewer's grid + Overview "collectors failing" count, + /// and the service's get_collection_health MCP tool + web fleet failing-count reader). It was + /// three byte-identical copies (Lite / viewer / service) with FLAT 4h-STALE / 24h-FAILING thresholds; + /// nothing pinned them together, so they could drift (#1573), and the flat numbers assumed a frequent + /// (~1-min) collector — a healthy DAILY collector that succeeds every run still read as STALE past 4h + /// and FAILING past 24h (index_object_stats at a 1440-min cadence, a real field false-positive). + /// + /// The fix makes the staleness thresholds RELATIVE to each collector's own cadence, with floors set to + /// the original flat values so every FREQUENT collector bands byte-for-byte identically — only a slow + /// collector (cadence past ~2.7h for STALE / 12h for FAILING) relaxes. Pure + static so all three + /// surfaces band identically and the whole decision table is unit-testable without a store. Each host + /// keeps its own SQL, row model, and brush/display mapping; only the band DECISION lives here. + /// + /// + public static class CollectorHealthClassifier + { + /* The band strings every surface's brush / display mapping already switches on — unchanged values. */ + public const string NeverRun = "NEVER_RUN"; + public const string NoPermissions = "NO_PERMISSIONS"; + public const string Failing = "FAILING"; + public const string Stale = "STALE"; + public const string Warning = "WARNING"; + public const string Healthy = "HEALTHY"; + + /// A collector with runs whose error rate exceeds this percent bands WARNING (when not STALE/FAILING). + public const double WarningFailureRatePercent = 20.0; + + /* Staleness cutoffs are max(floor, multiplier x the collector's own cadence in hours). The floors are + the original flat thresholds, so a collector with a cadence at/under the floor is unchanged; only a + slow collector relaxes. Chosen defaults (#1573): FAILING = max(24, 2 x freqHours) — a 1-min + collector still fails at 24h, a daily (1440-min) collector fails at 48h; STALE = max(4, 1.5 x + freqHours) — a 1-min collector still goes stale at 4h, a daily collector goes stale at 36h. So + index_object_stats (1440-min) at 27h since last success reads HEALTHY, not FAILING — the bug. */ + + /// Hours-since-last-success floor for FAILING — the original flat threshold. + public const double FailingFloorHours = 24.0; + + /// FAILING when hours-since-success exceeds this multiple of the collector's cadence (or the floor, whichever is larger). + public const double FailingCadenceMultiplier = 2.0; + + /// Hours-since-last-success floor for STALE — the original flat threshold. + public const double StaleFloorHours = 4.0; + + /// STALE when hours-since-success exceeds this multiple of the collector's cadence (or the floor, whichever is larger). + public const double StaleCadenceMultiplier = 1.5; + + /// + /// On-load collectors run once per server connect / tab open, NOT on the scheduled loop, so the + /// staleness thresholds never apply to them — they are banded by failure rate only (a 100-hour-old + /// last success is fine). Centralized here so the three surfaces cannot disagree on the set. These + /// are exactly the FrequencyMinutes == 0 entries in CollectorScheduleDefaults, kept as + /// an explicit name set so this classifier stays free of a dependency on the collector catalog. + /// + private static readonly HashSet OnLoadCollectorNames = new(StringComparer.OrdinalIgnoreCase) + { + "server_config", + "database_config", + "database_scoped_config", + "trace_flags", + "server_properties", + }; + + /// True for a collector that runs on connect rather than on the scheduled loop (staleness-exempt). + public static bool IsOnLoadCollector(string? collectorName) => + collectorName is not null && OnLoadCollectorNames.Contains(collectorName); + + /// The FAILING cutoff (hours since last success) for a collector of the given cadence. + public static double FailingThresholdHours(int frequencyMinutes) => + Math.Max(FailingFloorHours, FailingCadenceMultiplier * (frequencyMinutes / 60.0)); + + /// The STALE cutoff (hours since last success) for a collector of the given cadence. + public static double StaleThresholdHours(int frequencyMinutes) => + Math.Max(StaleFloorHours, StaleCadenceMultiplier * (frequencyMinutes / 60.0)); + + /// + /// Band one collector's trailing-window roll-up. Order is fixed: NEVER_RUN (no runs at all) -> + /// NO_PERMISSIONS (only permission denials) -> on-load (failure-rate only, never STALE/FAILING) -> + /// FAILING -> STALE -> WARNING (failure rate over the threshold) -> HEALTHY. + /// is the caller's elapsed-hours value — its 999 sentinel + /// for "ran but never a success" flows straight through to FAILING, exactly as before. + /// is the collector's cadence (callers resolve it from + /// CollectorScheduleDefaults; 0 for on-load or an unknown collector, which yields the floor + /// thresholds = the old flat behavior). is . + /// + public static string Classify( + long totalRuns, + long successCount, + long errorCount, + long permissionDeniedCount, + double hoursSinceLastSuccess, + int frequencyMinutes, + bool isOnLoad) + { + if (totalRuns == 0) + { + return NeverRun; + } + + if (permissionDeniedCount > 0 && errorCount == 0 && successCount == 0) + { + return NoPermissions; + } + + var failureRatePercent = totalRuns > 0 ? (double)errorCount / totalRuns * 100 : 0; + + if (isOnLoad) + { + return failureRatePercent > WarningFailureRatePercent ? Warning : Healthy; + } + + if (hoursSinceLastSuccess > FailingThresholdHours(frequencyMinutes)) + { + return Failing; + } + + if (hoursSinceLastSuccess > StaleThresholdHours(frequencyMinutes)) + { + return Stale; + } + + if (failureRatePercent > WarningFailureRatePercent) + { + return Warning; + } + + return Healthy; + } + } }