Repository navigation
Analysis facts divide by the time the collector actually observed, and a window with a hole in it says so, so collector downtime no longer reads as improvement (#3538 A2) - #3592
Conversation
…d a window with a hole in it says so (#3538 A2) Every rate and fraction fact in the analysis engine divided by the NOMINAL window (AnalysisContext.PeriodDurationMs), so collector downtime inside the window deflated severity in exact proportion to the data lost: a CXPACKET storm at a true 25% of observed time read as 6% when the collector was down for three of four hours, blocking at 40/hr read as 10/hr, and nothing consulted collection health before believing the numbers. compare_analysis caveated only an ENTIRELY empty window. Both SKUs' fact collectors now stamp AnalysisContext.Coverage first, from the wait_stats collection series itself: the sum of LAG intervals inside the window (the anomaly path's "never an assumed cadence" pattern), with an interval past the delta gap policy (CollectorDeltaCalculator .DefaultMaxGapSeconds, bound as a parameter) credited as zero because the calculator discarded that delta, the first row's interval clipped to the window, and an orphan row contributing nothing. Wait fractions, blocking and deadlock rates divide by ObservedDurationMs; the grouped LCK/CXPACKET families sum their constituents' fractions. An unobserved window emits no rate fact and composes with the #3524 unavailable envelope even when point-in-time config facts exist. Below 70% coverage a COLLECTION_GAP context fact (scored 0) carries the fraction and the largest gap, and analyze_server / get_analysis_facts / compare_analysis carry a coverage block and a partial-coverage caveat, byte-identical across SKUs and pinned by the parity test. Tests: Lite's seeder now plants a physically possible series (baseline reading, then deltas), with a collector-gap scenario pinning fractions 4x the nominal division and the gap fact at 25%; Darling's census gains the coverage witness and its SQL pins, and a live PG test walks the same scenario through collector, service and tools. Darling's live tests ran green here against PG 18.4 + TimescaleDB 2.28.1; Lite's are compile- verified and first execute in CI. Partial for #3538 (item A2).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| NominalMs = nominalMs, | ||
| ObservedMs = Math.Min(nominalMs, Math.Max(0, observedSeconds * 1000.0)), | ||
| SampleCount = (int)Math.Min(int.MaxValue, sampleCount), |
There was a problem hiding this comment.
Non-blocking: when the window's only in-window row is an orphan (no predecessor within the lookback), sampleCount is 1 but observedSeconds is 0 (the previous_time IS NULL branch contributes 0). BuildCoverage then returns SampleCount = 1, ObservedMs = 0 instead of going through WindowCoverage.Unobserved(nominalMs) (which sets SampleCount = 0). Since IsObserved only checks ObservedMs > 0, behavior is correct (treated as unobserved), but the coverage.sample_count field in the tool payload will read 1 next to unobserved: true, which could read as a contradiction to an API consumer. Same for Lite's twin at Lite/Analysis/DuckDbFactCollector.Waits.cs:138. Might be worth zeroing SampleCount in this branch for consistency with the fully-dead-window case, but this doesn't affect any rate calculation.
There was a problem hiding this comment.
Declined, deliberately: sample_count = 1 beside unobserved: true is the honest reading of that store, not a contradiction. The two say different things — sample_count counts the readings that landed inside the window (its doc says "the orphan first row included"), unobserved says none of them stood for any knowable interval. Together they distinguish the two dead-collector shapes an operator cares about: 0 / unobserved is a collector that wrote nothing for the whole window; 1 / unobserved is one that came back (or started) inside it and has exactly one reading so far, whose delta the calculator had to discard. Zeroing the count would fold those into one and lose the diagnosis. The Lite test CollectFacts_UnobservedWindow_EmitsNoRateFacts_AndStampsZeroCoverage pins SampleCount == 1 for the orphan case on purpose. Rate math unaffected either way, as you note.
| window — and scoring those alone would produce a pass whose every windowed rate is | ||
| absent and whose all-clear (or config-only findings) still reads as "analyzed this | ||
| window". Nothing was measured over the window; the same envelope says so. */ | ||
| var unobserved = facts.Count > 0; |
There was a problem hiding this comment.
Nit: unobserved is true when facts did survive (the point-in-time ones), which is the opposite of what the name suggests at a glance — it really means "the window itself was unobserved, but some facts survived anyway". A name like hasPointInTimeFactsOnly would read more directly at the call site below. Same variable/name in Lite's twin (Lite/Analysis/AnalysisService.cs:202). Non-blocking.
There was a problem hiding this comment.
Fixed in 0ace8b6 — renamed to hasPointInTimeFactsOnly in both SKUs, with a one-line comment at the declaration saying what the state is (the window went unobserved but config/trace-flag/hardware facts still read — the case that used to slip past the facts.Count == 0 check).
There was a problem hiding this comment.
LGTM — reviewed the coverage-witness SQL (Postgres/DuckDB, byte-identical between Darling and Lite), the WindowCoverage math and edge cases (zero/reversed window, orphan rows, gap-policy discard, clipping), the widened #3524 unavailable envelope, and the analyze_server/get_analysis_facts/compare_analysis payload and caveat changes on both SKUs. Service and MCP-tool layers are line-for-line identical between Lite and Darling, and the new parity pin test extends correctly to the added prose. Left two non-blocking nits inline (a sample_count/unobserved payload inconsistency in an edge case, and a confusingly-named local variable) — neither affects correctness.
…what it is (both SKUs)
|
Reviewed the coverage-witness change ( Things I specifically checked and found solid:
One minor (non-blocking) observation: in Nicely done — very tight parity discipline and thorough edge-case test coverage for a change this central to severity scoring. |
There was a problem hiding this comment.
LGTM — reviewed the coverage-witness (#3538 A2) denominator fix across both SKUs: shared/byte-identical SQL, parity in AnalysisService/MCP tools, gap-policy boundary consistency, parameterized queries, and edge-case test coverage. No correctness, security, or Lite/Darling parity issues found; one minor non-blocking observation left as a PR comment.
…side even when it produced no facts (both SKUs)
|
Fix-now for the one observation: the |
There was a problem hiding this comment.
LGTM — reviewed the coverage-witness SQL (CoverageSql) for boundary correctness against CollectorDeltaCalculator's own gap policy (both use strict >, so the denominator can never disagree with the numerator about a discarded interval), the C# arithmetic in WindowCoverage/AnalysisContext, and Lite/Darling parity. AnalysisContext, WindowCoverage, and FactCollectorHelpers are shared (one file, not duplicated), and the SKU-specific collector/service/MCP-tool code (PgFactCollector.Waits.cs vs DuckDbFactCollector.Waits.cs, DarlingAnalysisService vs AnalysisService, DarlingMcpTools vs McpAnalysisTools) is a faithful byte-for-byte port with matching guards (ObservedDurationMs <= 0 checks), matching metadata keys, and matching caveat prose (pinned by McpMissMessageParityPinTests). Verified: sample_count/orphan/lead-in/tail-gap logic in BuildCoverage is internally consistent and matches the live-PG and DuckDB test expectations; FactScorer correctly scores the new coverage source at 0 so COLLECTION_GAP can't root a story or trigger amplifiers; the grouped LCK/CXPACKET fractions correctly sum constituent Values rather than re-dividing, so they inherit whichever denominator was stamped; deprecated/Dashboard is correctly left untouched. No correctness, parity, or security issues found.
…l, as WindowCoverage documents, not the nominal window
|
CI's first execution of the Lite suite: 4,034/4,035 green on the first run — every re-planted scenario, the new coverage tests, the parity pin — with one failure in a NEW assertion of mine, not in the product: |
|
Reviewed for correctness, Lite/Darling parity, security, and performance. Verified independently (not just from the PR description):
Two things I considered flagging but ruled out as non-issues after checking further context:
No correctness bugs, Lite/Darling parity drift, security issues, or performance regressions found. |
There was a problem hiding this comment.
LGTM — verified CoverageSql byte-identity and parameter order across SKUs, edge-case arithmetic (0 rows, orphan first row, gap-threshold boundary), guarded divisions by ObservedDurationMs, Lite/Darling parity in AnalysisService/DarlingAnalysisService and McpAnalysisTools/DarlingMcpTools, the DominantLockMode metadata-collision claim, and query indexing; no substantive findings.
…folds one cause into one row, so same-hour-yesterday noise stops reading as a verdict (#3538 A3) The tool compared ONE window against ONE window and banded every key by its severity delta on a flat +/-0.1 dead-band. Severity is a threshold-formula artifact: a saturating ladder read a doubling from 30% to 60% of observed time as "stable", a trace of a 1%-bar wait read a few seconds an hour as "worse", one I/O stall surfaced as four to six worse keys, and BAD_ACTOR_<hash> keys churned into new_issues/resolved_issues on plan-cache eviction alone. Shared ComparisonBanding (PerformanceMonitor.Analysis) now decides every verdict for both SKUs: a key with a same-unit per-server baseline (CPU %, read latency, connections) is banded in that bucket's robust sigma (delta_sigma, band_source "baseline", trustworthy buckets only); every other key changes status only when the value moved >= 25% of the larger side AND the larger side sits >= 0.25 up its own Layer-1 ladder (the scorer's base severity, so every concerning bar is reused with no number copied); one-sided keys are new/resolved only when they register on their ladder; BAD_ACTOR_* appearances are plan_cache_churn; rows carry a physical-cause family and the payload carries one family row per cause; the rules are stated in band_rules; every verdict row carries coverage_caveat when either side was partly observed (#3592 composes). ComparePeriodsAsync on both SKUs returns the dispersion map, fenced so a failed baseline read degrades to the absolute rule.
…folds one cause into one row, so same-hour-yesterday noise stops reading as a verdict (#3538 A3) (#3634) * compare_analysis bands each delta by the server's own dispersion and folds one cause into one row, so same-hour-yesterday noise stops reading as a verdict (#3538 A3) The tool compared ONE window against ONE window and banded every key by its severity delta on a flat +/-0.1 dead-band. Severity is a threshold-formula artifact: a saturating ladder read a doubling from 30% to 60% of observed time as "stable", a trace of a 1%-bar wait read a few seconds an hour as "worse", one I/O stall surfaced as four to six worse keys, and BAD_ACTOR_<hash> keys churned into new_issues/resolved_issues on plan-cache eviction alone. Shared ComparisonBanding (PerformanceMonitor.Analysis) now decides every verdict for both SKUs: a key with a same-unit per-server baseline (CPU %, read latency, connections) is banded in that bucket's robust sigma (delta_sigma, band_source "baseline", trustworthy buckets only); every other key changes status only when the value moved >= 25% of the larger side AND the larger side sits >= 0.25 up its own Layer-1 ladder (the scorer's base severity, so every concerning bar is reused with no number copied); one-sided keys are new/resolved only when they register on their ladder; BAD_ACTOR_* appearances are plan_cache_churn; rows carry a physical-cause family and the payload carries one family row per cause; the rules are stated in band_rules; every verdict row carries coverage_caveat when either side was partly observed (#3592 composes). ComparePeriodsAsync on both SKUs returns the dispersion map, fenced so a failed baseline read degrades to the absolute rule. * Verdict order is direction before magnitude, so a mixed-direction family's worst member is its regression (#3538 A3 review) The row sort partitioned only stable from non-stable; worse and better shared a bucket ordered by relative move, so a family whose BLOCKING_EVENTS fell 60% while its LCK_M_S rose 30% took the improvement as its worst member and left families_worse. Rows and families now rank worse > better > stable, then move; a mixed-direction family pin covers it.
1 participant
What does this PR do?
Partial for #3538 — checklist item A2, coverage-blind normalization. Every rate and fraction fact the analysis engine emits now divides by the time the collector actually observed inside the window, not by the nominal window length, and a window the collector only partly observed says so in every analysis payload. The sibling A1 finding (a dead collector's empty window rendered as "All metrics are within normal ranges") was closed in wave 1 by #3553/#3565; this change composes with that envelope rather than redoing it.
The lie the product told
AnalysisContext.PeriodDurationMs—TimeRangeEnd − TimeRangeStart, the nominal window — was the denominator of every wait fraction (PgFactCollector.Waits.cs:57, Lite's twin at:54), of blocking and deadlock events per hour (:116,:165), and of the groupedLCK/CXPACKETfamily fractions inFactCollectorHelpers. Nothing consulted collection health before dividing. So collector downtime inside a window deflated severity in exact proportion to the data lost: with the collector down for three of a four-hour window during a blocking storm,CXPACKETat a true 25% of observed time read as 6%, blocking at 40/hr read as 10/hr — under every bar on the ladder — and the pass returned a near-clean bill with nothing to flag it.compare_analysiscaveated only a window with no facts at all; a half-collected one produced confident numbers and called the deflation "better".The honest pattern already existed one file over:
PgAnomalyDetector.WaitRateWindowSqlcomputes its ms/sec over realLAGintervals — "interval via LAG — never an assumed cadence". This change applies the same measurement to the denominator.How
One coverage witness, stamped first, on both SKUs.
CollectObservedCoverageAsyncis step 0 ofCollectFactsAsyncinPgFactCollectorandDuckDbFactCollector(byte-identical SQL, shared dialect —EXTRACT(EPOCH …),LAG,GREATEST,COALESCE; proven on PG 18.4 + TimescaleDB and on DuckDB 1.5.5 with the same text). It readsv_wait_stats— the series the wait fractions themselves are summed from, so numerator and denominator come from one set of rows and cannot disagree about when the collector was up; it is also already the product's canary in the data-span gate and the anomaly baseline gate. It sums, overDISTINCT collection_timeinside the window, each collection's interval to its predecessor, with three rules thatWindowCoverage's doc comment argues in full:CollectorDeltaCalculator.DefaultMaxGapSecondsis bound as a parameter ($5), never a literal, because the calculator discarded the delta over such a gap, so crediting any of that time would re-create the deflation in miniature (the task said "cap"; the faithful reading of the calculator is "discard", and the comment says why);$4) and its interval is clipped to the window start (GREATEST(previous_time, $2)), so observed time never exceeds the nominal window and the fraction needs no clamp;The lead-in and tail gaps (a first row with no predecessor; a collector that died mid-window and left nothing after itself to
LAGfrom) are finished in C# from the edge columns, because they are properties of the window's edges the row set cannot see.The stamp lives on the context.
AnalysisContext.Coverage(WindowCoverage:NominalMs,ObservedMs,SampleCount,LargestGapMs,Fraction,IsObserved,IsPartial) andObservedDurationMs— the divisor.PeriodDurationMsstays, re-documented as the nominal window, for the things legitimately about the window itself: the coverage fraction's own denominator, theperiod_duration_ms/period_hoursmetadata that states what was asked for, and the caveat prose. The stamp is done in the collector, not the service, so every path that collects — the scheduled pass,get_analysis_facts, both windows ofcompare_analysis, a test handing the collector a bare context — gets it before the first division and no caller can forget it. Like the wait read it precedes, it carries no catch: this is the canary series, and a store that cannot answer it should fail the pass loudly, not degrade into "unobserved" and report a dead collector for a store that merely timed out.Every rate divides by observed time. Wait fractions,
BLOCKING_EVENTS,DEADLOCKSin both collectors (rg PeriodDurationMssweep: no remaining divisor use). The groupedLCK/CXPACKEThelpers now sum their constituents' fractions instead of re-dividing, so they are correct under whichever denominator the collector chose without knowing which — and the frozen Dashboard twin, which does not stamp coverage, keeps its pre-#3538 shape exactly. Metadata: wait facts keepperiod_duration_ms(nominal) and gaincoverage_fraction(the divisor is their product); the name is deliberate —FactAdvice.DominantLockModereads every non-standard*_mskey on theLCKfact as a lock mode, so anobserved_duration_mswould have been reported as "the largest single contributor". Blocking/deadlocks keepperiod_hoursand gainobserved_hours. An unobserved window emits no rate fact — no fabricated 0, noInfinity; the honest-miss ladder handles it.Zero coverage composes with the wave-1 envelope. Both services'
#3524branch widens fromfacts.Count == 0to|| ObservedDurationMs <= 0: the facts that survive an unobserved window are point-in-time ones (server config, trace flags, hardware — read from the latest row regardless of window), and scoring those alone produced a pass whose every windowed rate was absent and whose all-clear or config-only findings still read as "analyzed this window". Sameunavailableenvelope, with aWindowEmptyMessagevariant that says which kind of nothing and how many point-in-time facts could still be read.get_analysis_factsreturnsunavailablein the same state, pointing ataudit_configfor the config facts (which deliberately discards coverage — a missed hour changes nothing about what the server is configured to).Below 70% coverage: a
COLLECTION_GAPcontext fact and caveats everywhere.WindowCoverage.PartialThreshold = 0.70, a judgment about materiality rather than a fleet measurement, and the comment says so and what would revise it: at 70% the old division understated every rate by ≈1.43×, enough to cross a severity band on the wait ladder; the rates themselves are corrected at every coverage, the bar only decides when the reader must be told. The fact hasSource = "coverage"—FactScorer.ScoreAll's default arm scores unknown sources 0, so no scorer registration was needed andFactScorer.csis untouched (another lane is in it). It cannot root a story or notify; it exists to be read inget_analysis_factsnext to the facts it qualifies.analyze_server(findings and the all-clear, which becomes "No significant findings in the 25% of this window the collector observed — a PARTIAL reading, not a full all-clear"),get_analysis_factsandcompare_analysiscarry acoverageblock always and acaveatbelow the bar;compare_analysisextends its existing empty-window caveat pattern to "the COMPARISON/BASELINE window was only partly collected: …" on either or both sides, and excludes the gap fact from the compared keys so it does not pad thestable/new_issuescounters (it is an observation of the collector, not the server). The prose is byte-identical across SKUs and the ten new sentences are added toMcpMissMessageParityPinTests.Services return coverage with the facts.
CollectAndScoreFactsAsync→(Facts, Coverage),ComparePeriodsAsync→(BaselineFacts, ComparisonFacts, BaselineCoverage, ComparisonCoverage), on both SKUs. Returned rather than parked on a property because those paths have noIsAnalyzingguard and two on-demand callers can overlap.AnalyzeAsyncexposesLastWindowCoveragebesideWindowEmptyMessage(same lifecycle) and logs a warning on a partial pass, because a scheduled pass has no payload to put the caveat in.Doc lines the change falsified — the "Value is wait_time_ms / period_duration_ms", "events per hour", "deadlocks per hour" summaries in both collectors — are rewritten; the wait fact's comment also documents the measurement lane's A3 note (the numerator sums concurrent tasks, so the same fraction means different things on 4 vs 64 schedulers) as documented-not-fixed, deferred to A5 with the threshold work.
Why this shape, and what was rejected
facts.Count == 0semantics the analyze_server answers "all metrics are within normal ranges" when the window collected nothing #3524 envelope keys on and pad every payload and count; the payloads carry acoverageblock unconditionally instead.Blast radius
Fully collected windows are unchanged to within one collection cadence — the existing scenario tests (
ScenarioTests,FactCollectorTests) assert the same documented fractions. Every partially collected window's rates go up toward their true value (they were only ever deflated), so some findings that were suppressed by a gap will now fire; each such pass carries the gap fact and the caveat. A single in-window reading with nothing before it is now, correctly, the dead-collector shape (unavailable), which is why the wave-1 control fixtures in both SKUs were re-planted as realistic series.deprecated/Dashboardcompiles unchanged (verified) and keeps its nominal division — the defect exists there too but porting it needs a T-SQL witness plus its service and tool surfaces, not a smallest-faithful-diff mirror; noted for the coordinator.What it does NOT do
collection_log/collector_health(see "rejected" above) and does not make any claim about the monitored server's uptime — a server that was down while the collector kept failing shows as unobserved, which is right for a rate and silent about the server.FactScorer.deprecated/Dashboardor touch CHANGELOG, migrations, or schema.Test plan
Darling (
Darling.Tests) — the live classes were executed here against a throwaway PG 18.4 + TimescaleDB 2.28.1 container (Docker contextdesktop-linux, torn down after):PgFactCollectorTests(census now includesCollectObservedCoverageAsync;AllSqlincludesCoverageSql; newCoverageSql_…text pin and a source pin, both SKUs, that every rate site divides byObservedDurationMsand bails when unobserved; the E2E fixture re-planted as baseline + delta and asserting full coverage / no gap fact),AnalyzeWindowEmptyLivePostgresTests(control re-planted as a 15-minute series, asserts the all-clear at ≈100% coverage), newAnalysisCoverageLivePostgresTests(the review's scenario end to end: fraction 0.25 and 10/hr against a 0.0625 / 2.5 nominal reading, gap fact at 0.25 with a 3h largest gap, caveats and coverage blocks in all three tools, a window anchored in the dead stretch →unavailable, then the same window fully collected → no caveat, 2.5/hr), plusAnalysisAsOfAnchorTestsandDarlingAnalysisPipelineTestsas regression: 32/32 passed, 0 skipped.Lite (
Lite.Tests) — compile-verified here; first execution is CI (net10.0-windows). Written right-by-construction against the same SQL (run on DuckDB 1.5.5 with the same text and the same fixtures: full/died/hole/orphan/predecessor/empty all agree with PG) and the same arithmetic as the Darling tests that ran:TestDataSeeder.SeedWaitStatsAsyncplants a physically possible series (baseline reading, then 15-minute deltas ending on the window end) with acoveredMinutesknob; newSeedCollectorGapServerAsyncscenario.FactCollectorTests: full coverage leaves the memory-starved 0.694 unchanged (the regression pin) and stamps ≈1.0; the gap scenario yields fractions 4× the nominal division,coverage_fraction0.25,period_duration_ms × coverage_fractionrecovering the divisor, blocking 40/hr and deadlocks 8/hr per observed hour, the gap fact with a 3h largest gap scored 0; an orphan-only window emits no rate facts and keeps point-in-time facts; an empty window stamps unobserved.AnalysisCoverageTests: the three tools on partial / full / unobserved windows (findings + caveat, scoped all-clear,unavailablewith config facts, gap fact in the facts payload,compare_analysiscaveat on the partial side only and no gap key among the compared facts), plusWindowCoveragearithmetic and prose.AnalysisWindowEmptyTestscontrol re-planted as a series;McpMissMessageParityPinTestsgains the ten shared sentences (replayed locally: all present in both SKUs, with a positive control).Builds (exact commands from the task):
PerformanceMonitor.Analysis,Darling.Service,Lite(EnableWindowsTargeting),Darling.Tests,Lite.Tests— all0 Warning(s) 0 Error(s);deprecated/Dashboardalso builds.Which component(s) does this affect?
How was this tested?
See the test plan above — Darling live tests run against PG 18.4 + TimescaleDB 2.28.1; Lite tests compile-verified and CI-first; the coverage SQL executed on both engines with identical results.
Checklist
dotnet build -c Debug)