Repository navigation
Ask whether the blocking collectors ran, not whether they found anything - #2508
Conversation
…anything get_blocking_stats shipped in #2497 with an event-existence probe: had a blocking or deadlock row ever been stored. That is wrong on an edge table. A server collected perfectly for months that simply never blocked has no rows at all, so the probe answers no and the read tells a healthy server this is NOT a clean bill of health and to go check its XE session -- a false alarm that sends someone to fix collection that is working. #2500 got this right for the trends and left a note saying the event probe was nonetheless correct for get_blocking_stats, because that verdict is about severity. That reasoning does not survive contact with a healthy server: zero severity is the NORMAL state, so the same false alarm fires. The note is corrected along with the code. The denominator is now whether we LOOKED: a successful run of either blocking capture path, or of the deadlock collector, in collection_log. Both paths because either can be off alone; deadlocks because the verdict covers that series too. The live test now pins the case the first design got backwards -- a collector that RAN and saw nothing, with no event rows anywhere, must read as a clear window. That is the ordinary state of a well-behaved server and it was the one state the original probe could not describe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return everCaptured | ||
| var everRan = | ||
| await DarlingBlockingTrendReader.HasAnyBlockingCollectorRunAsync(postgres, resolved.ServerId) | ||
| || await DarlingBlockingTrendReader.HasAnyDeadlockCollectorRunAsync(postgres, resolved.ServerId); |
There was a problem hiding this comment.
Now that get_blocking_stats calls HasAnyBlockingCollectorRunAsync/HasAnyDeadlockCollectorRunAsync instead, DarlingDataReader.HasAnyBlockingCaptureAsync (and its HasAnyBlockingCaptureSql const) in DarlingDataReader.cs has no remaining callers anywhere in the repo. Same on the Lite side: LocalDataService.HasAnyBlockingCaptureAsync in Lite/Services/LocalDataService.BlockingStats.cs is now dead too.
Worse, Lite's doc comment on that now-dead method (LocalDataService.Blocking.cs around line 795, on HasAnyBlockingCollectorRunAsync) still says:
That one is right for get_blocking_stats, whose verdict is about severity; it is wrong here...
That's exactly the stale rationale this PR's own description warns about ("a wrong rationale sitting next to the right implementation is how the mistake comes back") — except it's now sitting next to dead code, and it's factually wrong about which method get_blocking_stats uses. Darling's equivalent comment in DarlingBlockingTrendReader.cs was updated in this PR; the Lite counterpart in LocalDataService.Blocking.cs wasn't touched.
Worth either deleting the now-unused HasAnyBlockingCaptureAsync/HasAnyBlockingCaptureSql in both DarlingDataReader.cs and LocalDataService.BlockingStats.cs, or at minimum fixing the stale doc comment in LocalDataService.Blocking.cs so it doesn't point future readers back at the wrong method.
Review summaryReviewed the diff against One issue worth addressing before merge — left as an inline comment: switching the call site off No T-SQL touched by this PR, no security-relevant input handling changed, and no other Lite/Darling parity drift found. |
…e horizon age (#2540) get_pg_session_states rolls the stored samples up per BACKEND rather than per sample - a session parked idle in transaction for two hours is in a hundred and twenty captures, and a raw read would report one problem a hundred and twenty times and push everything else under the limit. Grouping on the collector's synthetic backend id rather than the pid means a reused pid inside the window is two findings, not one merged average. horizon_holder_samples is a COUNT, not a flag, because the two ends of it are opposite findings: every write transaction is momentarily the oldest holder, so one sighting in a hundred is normal traffic while ninety-eight is the reason vacuum reclaims nothing. The tool only calls a session a sustained holder past half the samples that saw it, and reports the raw counts so a caller can disagree with the threshold instead of trusting it. peak_horizon_age of -1 means the session pinned NOTHING, not something small, and the tool's finding text refuses the vacuum argument on those rows explicitly - killing them reclaims not one dead row. It offers a different argument instead where the duration warrants one: a five-minute idle transaction that pins nothing is still a forgotten commit holding a connection and its locks. Redaction outranks every other branch. If the state columns came back NULL for want of pg_monitor then every input to every other finding is an artefact of a missing GRANT, so the row gets its own severity band ('unknown') rather than being painted healthy or critical - the same treatment estimate_unavailable gets on pg_table_bloat_stats. Capture counts come from collection_log, not from the data: this is an exception surface like pg_blocking_edges, so zero rows is the HEALTHY answer and probing the table would report a well-monitored server as uncollected (#2508). Also fixes an artifact in ViewerDataService: three comment blocks had stacked above the top arm, each new rung inserting its own above the last, so #2530's prose was explaining an arm three rungs away. Each block is back with the arm it describes. CI worker sizing re-derived in both workflows: 54-collector catalog + collection_log = 55 hypertables, 57 background workers, 68 worker processes. Ladder-generator diff run against all twelve PostgreSQL rungs and proven RED first by breaking V86's pid column type.
…gdata#2484) The last of the ten viewer surfaces with no /api/read endpoint. The interactive plot stays desktop-only by design; the read behind it is portable, and a bucketed table is the same answer. The bucketing is the viewer's, not a new one. Its bin width turned out to be a CONSTANT — INTERVAL '5 minutes', hardcoded whatever range is on screen, not derived from the window length — so there was nothing to reproduce, only a default to honor. bucket_minutes exposes it and defaults to 5, because a browser, an agent and a desktop pointed at the same server over the same window must not draw different pictures. The seven log-magnitude bands are the viewer's too. Making the width a parameter exposed a cross-SKU divergence the default hid. Postgres date_bin takes an explicit origin and the viewer passes the Unix epoch; DuckDB's time_bucket defaults to 2000-01-03 instead. The two origins are 15,780,960 minutes apart, which 5 divides exactly — so the default agrees by luck — but 7 does not, and the two SKUs would have binned the same row a minute apart the first time anyone passed an odd width. Lite's read now passes the epoch explicitly. Verified identical across {1, 5, 7, 13, 60, 90, 360, 1440}-minute strides on PostgreSQL 17 and DuckDB 1.5.5. query_stats is a PERIODIC table, not an edge table: the collector writes rows every cycle for whatever is in the plan cache, so an existence probe on the data is the right denominator here — the opposite of what erikdarlingdata#2508 corrected for blocking and deadlocks. Zero cells is three states: never collected (unavailable, nobody looked), nothing in the window (empty, widen it), and one only this read has — captures exist and every one recorded a zero execution delta, which is an idle server rather than a broken one, where "widen the window" would be advice pointed at the wrong problem. The cap is on CELLS and it cuts the OLDEST bins, so a capped call keeps the recent end of an incident window. That cut can land inside a bin, so the partial column is dropped rather than returned with holes: a column missing its low buckets reads as "nothing fast ran then" rather than "we stopped looking". Also closes out uat-onboarding §3.4, whose "data with no read endpoint at all" list named the heatmap and is now empty. Its counts were seven tools stale; they are re-derived rather than incremented — 90 read endpoints of 109 tools, 69 of them reached by the per-server page. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ngdata#2484) The last two of the ten viewer surfaces with no /api/read endpoint. Both readers already existed on both SKUs and nothing called either, so this is wiring rather than collection: GetDailySummaryRangeAsync had been sitting in DarlingHealthReader uncalled since it was written, and the viewer's LockWaitTrendSql had no port into the service at all. get_daily_summary_range is a SIBLING of get_daily_summary rather than a wider version of it, and the mechanical reasons decide it before the taste ones do. The single-day tool returns a flat object of scalars and a range returns rows, so one tool answering either shape depending on whether a span argument arrived would make every consumer branch on a parameter it may not have sent - the web stat tile would simply stop rendering. And a tab must not fetch one read twice (there is a pin for it), so the Overview tab's today tile and the month grid beside it CANNOT be the same read. They share the one aggregate underneath, which is what stops them ever disagreeing about a day. The two reads sit on opposite sides of the denominator question, and both are the PERIODIC case rather than the edge case erikdarlingdata#2508 corrected. - Lock waits read v_wait_stats, where the collector writes a row every cycle for every wait type it observes. The probe asks whether ANY wait sample exists and deliberately does NOT carry the read's own LIKE 'LCK%' filter: a server collected for months that never took a lock wait is precisely the all-clear this branch exists to give, and an LCK-filtered probe would report it as uncollected and send someone to fix collection that works. - The daily range probes v_collection_log, which is the day spine's own member - ANY run marks a day collected, which is why a quiet but monitored day comes back Healthy rather than absent. That is also what makes a HOLE in the returned days diagnostic: a missing day is a gap in collection, never a quiet one. Both take the erikdarlingdata#2504 anchor, and both are proven to use it BY CONTENT rather than by signature - rows seeded two days back come back when anchored there and the recent rows do not. The daily range anchors on the LAST DAY of the span, which needed its own shared description constant: the existing AsOfDescription names hours_back in its own text, and a parameter description naming a parameter the tool does not have is worse than a generic one, because the caller sends that key and it is ignored rather than rejected. Same contract, same resolver, same ceiling constant shared across the SKUs so neither can accept a span the other refuses. The lock-wait rate is a double the whole way down, and the test asserts a 3 ms delta over 60 seconds comes back as 0.05 rather than 0. That is erikdarlingdata#2507's defect one read over: a rate shipped as an integer reports a quiet server as an idle one. Counts in uat-onboarding §3.4 are re-derived from source rather than incremented - 92 read endpoints of 111 tools, 71 of them reached by the per-server page - as is the instructions census (111 / 86 shared / 25 Darling-only), recounted by scanning the McpServerTool attributes in both trees. Verified locally: the whole solution builds; a DOM shim built all twelve tabs at three time ranges with every panel a real node, and both new reads were actually FETCHED (the only two named-but-unfetched reads are get_wait_trend and get_perfmon_trend, which fetch on a picker by design). Test suites are net10.0-windows and CI is the arbiter for running them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to my own #2497, prompted by the design in #2500.
The bug
get_blocking_statsused an event-existence probe: had a blocking or deadlock row ever been stored for this server?That is wrong on an edge table. A server collected perfectly for months that simply never blocked has no rows at all — so the probe answers no, and the read tells a healthy server:
A false alarm that sends someone to fix collection that is working. It is the reassuring-answer failure inverted, and I built the probe reasoning carefully about capture paths while never asking what it says about a server with nothing to capture.
The correction, including a note that defended the mistake
#2500 got this right for the trends and left a note saying the event probe was nonetheless correct for
get_blocking_stats, because that verdict is about severity rather than occurrence. That reasoning does not survive contact with a healthy server: zero severity is the normal state, so the same false alarm fires. The note is corrected along with the code, because a wrong rationale sitting next to the right implementation is how the mistake comes back.The denominator is now whether we looked: a successful run of either blocking capture path, or of the deadlock collector, in
collection_log. Both paths, because either can be off alone; deadlocks included, because the verdict covers that series too.Verification
DarlingBlockingStatsReadTestsnow pins the case the first design got backwards:empty, "genuinely clear", and asserts the message does not sayNEVER. This is the ordinary state of a well-behaved server, and it is precisely the state the original probe could not describe.unavailable, "NOT a clean bill of health".Messages stay byte-identical across both SKUs. Service, Lite and both test projects build clean.