Repository navigation
Fixes #3017 (items 1 and 2) - #3027
Conversation
… ingest cycle looked get_fleet_overview's total_deadlocks reads v_deadlocks, which is the SQL Server extended-event capture and nothing else, so it is structurally zero for a PostgreSQL server whatever its clusters do. Zero is also what a quiet SQL Server fleet reports, so the reading that needs no action and the reading that does not cover the fleet had the same character. The total now ships with a denominator: how many servers it read a deadlock source for, with the three causes named separately because they take opposite actions - a different tool for a PostgreSQL target, a collector to look at for one that is not being invoked, a grant for one that was refused. Coverage excludes NO_PERMISSIONS, STOPPED and NEVER_RUN through a named set on the shared classifier, and counts FAILING, STALE and WARNING as read: those collectors did read on some cycles and their rows are in the total, so excluding them would report a smaller fleet than exists. The note names each of the two windows against the figure it belongs to - the total between the caller's bounds, coverage over the fixed trailing seven days of collection health - and says outright that coverage makes no claim about the caller's window. The three AWS-API ingestors returned a bare int, so a cycle that never made an AWS call was indistinguishable from one that read the source and found nothing, and the runner stamped both with a sentence about the source's contents. They now return an outcome carrying rows and whether the source was reached, so a caller cannot take the count and drop the signal, and the runner renders a different note for each. pg_cpu_utilization meets this on every cycle of every self-hosted PostgreSQL target, because its dispatch is unconditional and there is no pg_read_file-shaped fallback for instance CPU.
ReviewThe two fixes (deadlock coverage denominator, RDS/PI ingest reach) are well-scoped and the test coverage for both is thorough — I traced One gap: the fix doesn't reach Darling's other fleet-totals reader
Worth at least an issue if not in-scope for this PR — since #3017 mentions this is intentionally partial (item 3 left open), this may be exactly the kind of thing item 3 or a follow-up should cover, but as written the PR title/description ("get_fleet_overview's total_deadlocks now says how much of the fleet it covers") could read as if the number is fixed everywhere it's shown, when one more surface still needs it. Minor: web dashboard tile doesn't surface the new note either
|
…ool caller The web rollup's Deadlocks tile rendered total_deadlocks bare, which is the number an operator actually looks at and the one place the ambiguity still had no answer: zero is structurally permanent on a PostgreSQL fleet and a genuine all-clear on a quiet SQL Server one, and the tile could not tell them apart. deadlock_coverage already ships on the same /api/fleet body the MCP tool reads, so this is rendering only - no server-side change. The figure is a VISIBLE sub-line under the label, not a tooltip: an operator who does not already suspect a problem never hovers. It is shown whenever the response carries coverage, including at full coverage, because a line that appeared only on a partial read would make its absence the load-bearing signal - and would leave "zero, whole fleet measured" indistinguishable from "zero, from a build that reports no coverage at all", which is the same defect one step out. The API's own note rides along as the tile's title, so the hover adds detail to a signal that is already on screen instead of being the only place it exists. A partly-covered fleet reporting zero deadlocks stops rendering as an unstyled all-clear and takes the warning class; a real deadlock keeps critical whatever the coverage, since a measured incident outranks an incomplete denominator and the sub-line still says the count may be short. The sub-line's own colour tracks coverage rather than the tile's severity, because "read all 12 servers" under a red count is good news about a bad number. Also strengthens the band-set pin: the set-equality assertion compared against a hand-written triple, so an eighth band added to the classifier and to neither list would have passed it and counted as read by default. The band constants are now read off the type and every one is required to sit in exactly one of the two decided lists.
|
Reviewed. This is Darling-only (fleet overview + RDS/PI ingest notes) — Lite has no fleet concept and no RDS ingestors, so the "Lite/Darling parity" requirement in CONTRIBUTING.md doesn't apply here (that section covers shared-library seams like Traced the new code paths and didn't find correctness bugs:
No security, performance, or correctness issues found. Nothing to flag as a parity gap since this feature has no Lite counterpart by design. |
* Say how much of the fleet the WPF Overview's deadlock total covers The Overview's "Deadlocks: N" is a third independent computation of the same fleet total, out of ViewerDataService.Fleet.cs's FleetTotalsSql. Like the two surfaces #3027 fixed, it reads v_deadlocks — the SQL Server extended-event capture and nothing else — so on a PostgreSQL fleet it is structurally zero forever, and zero is also what a genuinely quiet SQL Server fleet reports. FleetDeadlockSource, FleetDeadlockCoverage and ClassifyDeadlockSource move from the service's fleet reader into PerformanceMonitor.Common, which the viewer does reference, so the banding lives once. The service keeps the type verbatim, including its JSON attributes and its API-audience note, and each surface writes its own prose. The card grows an engine flag and the deadlocks collector's own band, both free: the flag comes off the registry row the Overview loader already holds, and the band comes out of the collection-health rows the collector tallies are already counted from. The roll-up's coverage denominator is the REGISTERED fleet, the same population the cross-server total counts over. Its four causes therefore need not sum to it — a server with no summary this cycle is classified by none of them, and that shortfall keeps its own words rather than being blamed on a collector nobody measured. * Make each coverage cause read correctly after a count of one The four causes are verb-free noun phrases now, and the count becomes words in one place rather than once per arm. "N are PostgreSQL targets" prints "1 are PostgreSQL targets" the moment a fleet has exactly one of something, which for the denied and silent arms is the common case. * Collapse the stray blank line after the namespace declaration
1 participant
Fixes items 1 and 2 of #3017. Item 3 is deliberately left open — do not close #3017 on this PR.
Both halves are the same shape: a surface reporting a measurement it did not take.
1.
get_fleet_overview'stotal_deadlocksnow says how much of the fleet it coversFleetDeadlockSqlreadsv_deadlocks, which isSELECT * FROM deadlocks, anddeadlocksis written by exactly one collector — the SQL Server extended-event capture. A PostgreSQL target's deadlocks go topg_deadlocks, there is nov_pg_deadlocks, and nothing joins the two, so on a PostgreSQL fleet this total is permanently zero for every server, whatever those clusters do and whatever grants the role holds. Zero is also exactly what a genuinely quiet SQL Server fleet reports, so the reading that needs no action and the reading that does not cover the fleet had the same character.FleetOverviewResultnow carriesdeadlock_coveragebeside the total: how many servers the total read a deadlock source for, out of how many, with the three uncovered causes counted separately because they take opposite actions —get_pg_deadlocksfor a PostgreSQL target, a collector to look at for one that is not being invoked, a grant for one that was refused. Each card carriesdeadlock_collector_band(the fact that explains its own zero) and a deriveddeadlock_source, and the rollup reduces from the cards, so the denominator and the total it qualifies reconcile by construction rather than by two queries agreeing.Coverage excludes
NO_PERMISSIONS,STOPPEDandNEVER_RUN, via a namedCollectorHealthClassifier.NothingReadBandsset rather than a comparison at the call site, so a band added later gets one decision instead of N silent omissions.NEVER_RUNis in the set on meaning, not on reachability: it istotalRuns == 0, which aGROUP BYover a run log cannot produce today, but that is a property of the query and not of the band — an outer join added later to make a never-invoked collector visible would make it reachable, and a set that had left it out would then count the most completely unread server of all as read.FAILING,STALEandWARNINGcount as read. Those collectors succeeded on some cycles and their rows really are in the total; excluding them would report a smaller fleet than exists, which is a new wrong number in place of the old one. They are not hidden by being counted —servers_with_collection_failuresand each card'sfailed_collector_countare where a degraded collector shows.The note names each window against the figure it belongs to. The total is counted between the caller's
window_startandwindow_end(one hour by default); coverage is banded over the fixed trailing seven days of collection health. That divergence is correct — whether a reader works is a durable fact, and the banding thresholds are themselves defined in days, so an hour-wide health window could not produce a band at all — but a sentence claiming both were read in this window would be the same defect one level up. The note therefore says outright that coverage makes no claim about what was read insidewindow_start..window_end.The denominator follows the convention already in the tree twice:
get_pg_blockingreportscaptures_totalbesidecaptures_with_blockingwith a sentence saying what an empty answer does and does not mean, andtarget_has_user_databases(#1852) states a fact about the target beside a persistently empty result rather than banding it. Deliberately not a new band, for #1852's reason: a quiet SQL Server fleet with full coverage is healthy and keeps reading that way.1b. And the same total on the web fleet page, which is the surface an operator actually looks at
wwwroot/js/pages/fleet.jsrenderedtotal_deadlocksas a bare tile. Rendering only — no server-side change;deadlock_coveragealready ships on the same/api/fleetbody.The figure is a visible sub-line under the tile's label, not a tooltip: an operator who does not already suspect a problem never hovers. It renders whenever the response carries coverage, including at full coverage — a line that appeared only on a partial read would make its absence the load-bearing signal, and would leave "zero, whole fleet measured" indistinguishable from "zero, from a build that reports no coverage at all", which is the same defect one step out. Absent coverage on the payload means exactly one thing: this response made no coverage claim. The API's own note rides along as the tile's
title, so hovering adds detail to a signal that is already on screen rather than being the only place it exists.The severity class moved too. A partly-covered fleet reporting zero deadlocks was rendering as the unstyled all-clear; it now takes
warning— "interrogate this number", without claiming a deadlock happened. A real deadlock keepscriticalwhatever the coverage: a measured incident outranks an incomplete denominator, and the sub-line under it already says the count may be short. The sub-line's own colour tracks coverage rather than the tile's severity, because "read all 12 servers" under a red count is good news about a bad number.2. The RDS/PI ingest note no longer describes a log the cycle never opened
RdsPlanIngestor,RdsDeadlockIngestorandRdsCpuIngestorreturned a bareint, so two states collapsed into one value: the AWS API answered and the source genuinely held nothing new, orRdsEndpoint.TryParsereturned null for the host, no AWS call was made at all, and the ingestor returned 0.DarlingCollectorRunnerstamped both with a positive claim about the source's contents.All three now return
RdsIngestOutcome(int Rows, bool SourceReached), so a caller cannot take the count and drop the signal, and the runner renders a different note for each outcome through one pureRdsIngestNote. The not-reached notes name the cause and end on #2633's own closing sentence — this cycle did not look — so an operator who learned the distinction from #2633'sPERMISSIONSrows meets the same words arriving through the one door that is not a failure. The read-and-empty notes are unchanged; those sentences are correct on their own path, and stopping them being borrowed is the whole point.pg_cpu_utilizationmeets this on every cycle of every self-hosted PostgreSQL target: its dispatch is unconditional, because there is nopg_read_file-shaped fallback for instance CPU, soRdsCpuIngestoris always called and always no-ops on a non-RDS host. For the two log readers the not-reached branch is reachable by construction — the dispatch gate isIsAurora || IsAwsRds, andIsAurorais probed from the database whileIsAwsRdsis derived from the endpoint, so an Aurora cluster reached through a hostnameRdsEndpointdoes not parse routes there — but that configuration is not asserted as observed on any fleet.Not #3008 (marker advanced inside the fetch, merged) or #3009 (report cut at a chunk boundary, open): both are
RdsLogSourceread-correctness defects, where this is the runner mislabelling an outcomeRdsLogSourcereported correctly.Review findings
The web dashboard tile — already fixed here, in
7d934a5f6. The review ran against the previous head, before that commit existed; section 1b above is that finding.ViewerDataService.Fleet.cs'sFleetTotalsSqlhas the same defect — correct, and not fixed here. ItsSELECT COUNT(*) FROM v_deadlocksis a third independent computation of the same total, rendered in the WPF Viewer's Overview as a bareDeadlocks: {0}. Measured cost before deciding, because it is not the rendering-only change the web tile was:PerformanceMonitor.Darling.Viewerdoes not referencePerformanceMonitor.Darling.Service, soFleetDeadlockSource,FleetDeadlockCoverageandClassifyDeadlockSourcewould have to move intoPerformanceMonitor.Common— a public type crossing assemblies. On top of that, the Viewer's Overview read selects noengine_kindat all andServerSummaryItemcarries no PostgreSQL flag, so the read and the DTO both have to grow one, and the summary path currently keeps only a FAILING count rather than thedeadlockscollector's band. With the XAML andViewerFleetRollupTeststhat is roughly five more files including a cross-assembly type move, against a diff already at ~1000 lines. Left for a routing decision rather than folded in unannounced — flagged to the coordinator with this costing, not filed as a fire-and-forget follow-up.Verification
Baseline committed first, then one mutation at a time through
redproof.sh: each restored via a stash of the mutation only, the fix re-asserted by content after restore, and the repo-wide stash list proven unchanged. Fourteen kill-mutations, each caught:STOPPEDremoved (the exact shape of an earlier attempt), withNEVER_RUNremoved, and — the other direction — withFAILINGadded, which is what a quietly shrinking denominator looks likeA comment-only control stayed green, so neither an assembly rebuild nor a source edit is being treated as evidence of behaviour.
The client half has no test harness in this repo — no
package.json, no eslint config, no JS suite, and nothing in CI that executeswwwroot/js. Saying so rather than implying coverage that does not exist. It was verified by hand in a browser against the real, unmodifiedwwwroot, served statically withwindow.fetchstubbed so/api/fleetreturned four payloads: full coverage, a PostgreSQL fleet at zero coverage, real deadlocks with partial coverage, and a payload carrying no coverage object at all. Six observables were recorded per run — whether the sub-line exists and has non-zero height (i.e. is visible without hovering), its text, itspartialclass, its computed colour, the tile's class, and whether the title is the API note.Baseline: all six hold. Then four independent reverts, each breaking a different observable set, with the tree committed first and each file restored by targeted checkout plus a content re-assertion:
partialclass goes, the colour drops to muted and the class to unstyled, while the sub-line's existence and the title are untouchedNo two variants fail the same set, and two of them fail exactly one observable each, so no single check is standing in for the others. Measured separately: at a 375px viewport the rollup's horizontal overflow is byte-identical with and without the coverage sub-line (
scrollWidth474 in both), so the pre-existing non-responsive sidebar is not made worse by this.The Windows-only
Darling.Testssuite cannot run on macOS, so the real test files were compiled into a throwawaynet10.0xunit v3 console host namedDarling.Tests(the product projects carryInternalsVisibleTo) staged outside the tree,Compile Include-ing the repo paths with the solution symlinked in so the source pins' upward walk resolves: 95 tests, 0 failed, 0 not run, 2 skipped (both theDARLING_TEST_PG-gated live round-trips). Seventeen of those are new. CI is the arbiter for the rest of the suite.CHANGELOG
Not edited here, per the multi-lane routing. Entry text for
[Unreleased]→### Fixed, with the link-ref definition it needs:get_fleet_overview'stotal_deadlocksnow reports how much of the fleet it covers ([Fixes #3017 (items 1 and 2) #3027]) —v_deadlocksis the SQL Server extended-event capture and nothing else, so on a PostgreSQL fleet the total was permanently zero for every server whatever those clusters did, and zero is also what a quiet SQL Server fleet reports. Adeadlock_coverageobject now sits beside the total — servers read out of servers total, with PostgreSQL targets, silent collectors and permission-denied collectors counted apart because they take opposite actions — plus a per-carddeadlock_collector_bandanddeadlock_source. Coverage excludesNO_PERMISSIONS,STOPPEDandNEVER_RUNthrough a named set on the shared classifier and countsFAILING/STALE/WARNINGas read, since those collectors' rows are genuinely in the total. Its note names each of the two windows against the figure it belongs to and states that coverage makes no claim about the caller's window. The web fleet page's Deadlocks tile carries the figure as a visible sub-line rather than a tooltip, and a partly-covered fleet reporting zero deadlocks stops rendering as an all-clear.RdsEndpoint.TryParsedeclined the host and no AWS call was made at all was indistinguishable from one that read the source and found nothing, and the runner stamped both with a claim about the source's contents. They now return an outcome carrying rows and whether the source was reached, and the runner renders a distinct note that names the cause and ends on RDS plan capture reports SUCCESS "no new plans" when the AWS call was DENIED #2633's own closing sentence.pg_cpu_utilizationmet this on every cycle of every self-hosted PostgreSQL target, its dispatch being unconditional.