Repository navigation
Make an empty enumeration visible in Collection Health, and let it say why (#1837) - #1854
Conversation
…y why (#1837) #1843 put a fixed message on the collection_log row of an enumerated collector that enumerated nothing. That row is only reachable from the raw collection-log detail grid - the surface nobody opens until they already suspect something - so "collected nothing for weeks" still read as healthy. Two changes finish it. Probe-failure contract. An enumeration's SQL may now return an OPTIONAL SECOND RESULT SET of (item_name, error_text) for items it could not probe. It has to be a second result set: the first one IS the item list both runners consume as database names, so failures added to it would be collected FROM. EnumeratedCollectorDriver.ReadEnumerationAsync reads the item list, the failures, and composes the note; both hosts route the whole enumeration read through it, so the reads and the note WORDING cannot drift. Each host logs the first five per-item errors through shared templates and collapses the rest into a count. query_store's on-prem cursor now records (database, ERROR_MESSAGE()) there instead of discarding it in an empty CATCH - the shape that made a login with no database access indistinguishable from a server with no Query Store. An enumeration that returns one result set behaves exactly as before; a malformed second set is reported AS a probe failure rather than thrown, because failing a whole cycle to announce a diagnostics defect trades an invisible problem for a loud unrelated one. Collection Health visibility. Lite's grid, the Viewer's grid, the web dashboard's table and both get_collection_health MCP tools gained a Note column: last_note / note_count, read off SUCCESS rows specifically (the looser "not a failure status" complement would have dragged Darling's SESSION_MISSING rows into a column whose tooltip says it is not an error). Rendered through one shared CollectorHealthClassifier.FormatCollectionNote as "<note> (all 96 runs)" or "(3 of 96 runs)" - always-empty is the signal, sometimes-empty is normal. The banding is deliberately untouched: a target with no user databases is legitimately empty and keeps reading HEALTHY, and nothing in Classify reads the new columns. Sharpening it to "empty AND the target HAS user databases" needs a database inventory at health-read time and is filed as #1852; the same empty-CATCH shape in database_size_stats is not an enumeration and is filed as #1851. Two defects found while building this, fixed here. query_store's enumeration was the only one without #1823's HAS_DBACCESS screen, so a least-privilege login took a 916 per inaccessible database per cycle - invisible while swallowed, a permanent probe-failure note and a warning burst every five minutes once recorded. And Lite's per-run collection-log fields were plain instance fields while a cycle runs the servers in PARALLEL on one service instance: server B's reset could blank server A's timings between A's write and A's read, and with a note attached, A's "enumeration yielded 0 items" could land on B's row for a collector that does not enumerate. They now live in one per-server telemetry slot, which is sufficient because collectors within a server run sequentially. Darling was never exposed - its note is a return value. Tests: paired suites in both apps pin the contract against a real multi-result-set reader (back-compat, partial failure, all-probes-failed, malformed set, NULL error text), the note composition, the shared log templates and cap, the query_store SQL, the health SQL gate, the fleet read's ordinal parity with its shared mapper, the web table's column, the per-server slot isolation, and - the one that matters most - that two collectors identical except for the note band identically. Watched red: dropping the probe note, restoring the empty CATCH, flattening the qualifier, removing the HAS_DBACCESS screen, and removing the web column each fail the tests that cover them. Lite 1684 passed, Darling 3623 passed / 193 gated-live skipped, solution rebuild 0 warnings 0 errors. The two source pins from #1843 that asserted a specific note ASSIGNMENT are replaced with stronger ones asserting both runners route through the shared read - a literal pin could no longer prove the hosts cannot drift now that the note has three possible shapes. Every behavioral pin from #1843, including health-neutrality and the last_error gating, is unchanged. Closes #1837 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…meration-health # Conflicts: # CHANGELOG.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* #1837: what a NON-failing run reported (an enumeration that came back with 0 items). Blank for a | ||
| plainly healthy collector; the same column the two WPF grids carry, so the web view is not the one | ||
| Collection Health surface that still hides it. */ | ||
| { key: "last_note", label: "Note", wrap: true }, |
There was a problem hiding this comment.
The web dashboard's new last_note column renders the raw text with no qualifier, unlike both WPF grids. NoteFormatted/CollectorHealthClassifier.FormatCollectionNote appends "(all N runs)" / "(N of M runs)" — described in the PR body as "the point": distinguishing a collector that's sometimes empty (normal) from one that's been persistently empty (the actual signal worth a look). The MCP JSON payload backing this table already includes total_runs and note_count (see DarlingMcpDataTools.GetCollectionHealth), but COLLECTOR_COLUMNS just does { key: "last_note", ... } with no format/derived field composing those two into the same qualifier client-side.
Net effect: on the web surface, a collector that's been silently empty for a week and one that went empty once this window render identically — the exact ambiguity this feature exists to resolve. Worth either adding a client-side formatter (mirroring FormatCollectionNote) or a server-side pre-formatted field in the tool's JSON.
| -- error. MAX ignores NULLs, so a collector whose runs left no note reads blank. Display text only: | ||
| -- no band, no count, no threshold reads it, and a legitimately empty target (no user databases, no | ||
| -- AGs) stays HEALTHY exactly as before. | ||
| MAX(CASE WHEN status = 'SUCCESS' THEN error_message END) AS last_note, |
There was a problem hiding this comment.
last_note is MAX(CASE WHEN status = 'SUCCESS' THEN error_message END) — a lexicographic max over the message text, not the most-recent-by-time note (that's what last_error_time exists to pin down for the sibling last_error column, but there's no last_note_time/ordering here). That was harmless for last_error, whose text is mostly stable per failure mode, but this PR is the first place a count-bearing note string gets aggregated this way: the probe-failure note is "{N} item(s) failed their enumeration probe...", and lexicographic comparison doesn't sort numbers correctly (e.g. "12 item(s)..." < "9 item(s)..." because '1' < '9'). So across a 7-day window where the failure count fluctuates cycle to cycle, the displayed note can be an arbitrary historical run's count rather than the latest one — while note_count/total_runs (correctly aggregate, order-independent) still claim it reflects "how much of the window" via FormatCollectionNote. Same query shape repeated in Darling's ViewerDataService.CollectionHealth.cs and DarlingDataReader.cs, so it'd need fixing in all three if addressed (e.g. error_message from the row with MAX(collection_time) among SUCCESS rows, via a window function or correlated subquery).
Review: #1837 -- Probe-failure contract + Collection Health note visibilityOverview: Extends the #1843 bare "enumeration yielded 0 items" breadcrumb with (1) an optional second result set on enumerating collectors SQL for per-item probe failures, routed through a new shared Correctness -- solid. Traced the enumeration read path end to end in both Parity -- good, with one gap. The shared driver, shared note-building ( Minor correctness note (left inline): Nit: Security / performance: No new SQL injection surface -- the probe-failure text lands via a parameterized Good test discipline overall -- the "watched red" section in the PR description and the paired independent pins in both suites are exactly the kind of drift-proofing this repo asks for. |
…grids show
Review of the previous commit caught the web dashboard rendering the raw
last_note with no "(all 96 runs)" qualifier, so a collector empty all week
and one empty once this window read identically there - the exact ambiguity
this feature exists to resolve.
Fixed server-side rather than with a JavaScript formatter: both MCP tools
now emit note_summary, composed with the same
CollectorHealthClassifier.FormatCollectionNote both grids call, and the
table binds that. One source for the text across all three surfaces instead
of a second copy in another language. The raw last_note / note_count stay
in the payload for machine consumers.
Also from review: last_note is a MAX over the message TEXT, which orders
lexicographically, so a probe note whose count varies across the window can
show an earlier cycle's number ("12 item(s)" sorts below "9 item(s)"). The
empty-enumeration note carries no number and is unaffected, and the
per-cycle count is exact on the Collection Log detail grid, but the
limitation is real. There is no portable "value at the greatest timestamp"
aggregate across DuckDB and Postgres, and these three reads are
deliberately one byte-portable string, so it is documented at the parameter
it affects and filed as #1855 rather than resolved with a dialect split.
Tests: the web-column pin now requires note_summary and rejects the raw
last_note; a new pin asserts both apps' MCP tools emit the composed field
(the response shape is deliberately identical across SKUs, and the web
table binds it). Lite 1690 passed, Darling 3623 passed / 194 gated-live
skipped, solution rebuild 0 warnings 0 errors.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewOverview. This finishes #1837: an optional second result set Correctness. Traced the full contract: back-compat for single-result-set enumerations ( Lite/Darling parity. Checked column ordinals across all three health projections (Lite Security/Performance. No new user-controlled input reaches SQL text; Two minor/low-severity observations, inline:
Nothing blocking. Well-scoped, well-tested (including the "watched red" verification described in the PR body), and the parity discipline is genuinely upheld throughout. |
| COUNT but a non-string column would throw InvalidCastException out of here and fail the | ||
| whole collection cycle — the outcome the arity check above deliberately avoids. Read | ||
| loosely and the malformed set still reports itself as a probe failure. */ | ||
| var item = reader.IsDBNull(0) ? ContractViolationItem : reader.GetValue(0).ToString() ?? ContractViolationItem; |
There was a problem hiding this comment.
Minor: ContractViolationItem ("(enumeration)") is used both as the diagnostic item-name for a malformed second result set (line 197) and here as the fallback when a genuine probe-failure row's item-name column is NULL. If some future enumeration ever emits a null item name for a real failure, it would render identically to a contract-violation diagnostic in the log/note, which could send an operator chasing a SQL defect that isn't there. Today's only producer (query_store, @db from sys.databases.name) can never be null, so this is latent, not active — but a distinct sentinel for "null item name" vs "malformed shape" would remove the ambiguity for future enumerations that adopt this contract.
|
|
||
| Assert.Equal(new[] { "A" }, outcome.Items); | ||
| var failure = Assert.Single(outcome.ProbeFailures); | ||
| Assert.Equal(EnumeratedCollectorDriver.ContractViolationItem, failure.Item); |
There was a problem hiding this comment.
Nit: unlike Lite.Tests' A_Malformed_Second_Result_Set_Is_Reported_Not_Thrown (which also asserts outcome.Note contains "1 item(s) failed their enumeration probe"), this test only checks Items/ProbeFailures, not the composed Note. Given the PR's stated goal of pinning identical expectations independently in both suites, this one's slightly weaker than its Lite counterpart — low risk since the driver is shared and Lite.Tests would still catch a regression, but worth aligning for symmetry.
…ract Both review nits from the second pass. The NULL item-name fallback reused ContractViolationItem, so a genuine probe failure the enumeration merely failed to NAME would have logged identically to a malformed-second-result-set diagnostic - sending an operator hunting a SQL defect that is not there. They now read differently. Latent rather than active: query_store takes the name from sys.databases.name and can never produce it, but the next enumeration to adopt this contract might. And the Darling malformed-result-set test was weaker than its Lite twin - it checked the failures but not the composed note. Aligned, since the point of the paired suites is that each app's expectations stand on their own. Both suites also gained the unnamed-vs-malformed distinction. Lite 1691 passed, Darling 3624 passed / 194 gated-live skipped, solution rebuild 0 warnings 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| Assert.True(items >= 0 && failures >= 0, "both result sets must be selected"); | ||
| Assert.True(items < failures, "the item list must be the first result set"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test-parity gap vs. Lite.Tests. Lite.Tests/EnumerationProbeFailureTests.cs has two tests that this file doesn't:
Null_Error_Text_Does_Not_Drop_The_Failure— asserts a probe failure withDBNullerror text still counts (analogous to the "no item name" test already present here).QueryStore_OnPrem_Enumeration_Screens_Databases_The_Login_Cannot_Enter— pinsHAS_DBACCESS(d.name) = 1inBuildEnumerationQuery's text, i.e. the [QUESTION] Regarding installing the windows service and integrated security #1823 screen this PR adds toquery_store.
QueryStoreCollector and EnumeratedCollectorDriver are shared code, so a regression here would still be caught by Lite.Tests in CI — this isn't a live production risk. But the PR's own stated methodology is pairing these tests "independently in both suites so editing one app's copy alone fails a build," and per the CHANGELOG this HAS_DBACCESS screen is called out as one of "two defects found while building this, fixed here" — it seems worth the same independent pin here that the empty-CATCH fix and the other probe-failure behaviors already got in this file.
Review summaryWent through the full diff (19 files) against Overall: solid. This is unusually well fenced against the two failure modes it's designed to guard:
One finding posted inline: No missing-index or other DMV-folklore suggestions offered, per project convention. |
Review's third pass, and it is right: the PR's own methodology is that each app's suite asserts the shared behavior independently, so editing one app's copy alone fails a build. Two pins were only in Lite.Tests - the NULL error-text case, and the HAS_DBACCESS screen this PR adds to query_store's enumeration, which the CHANGELOG calls out as one of the two defects found while building it. Both shared code, so a regression was never invisible in CI; the gap was in the methodology, not the coverage. Darling 3626 passed / 194 gated-live skipped, 0 warnings 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReviewWent through the full diff ( Correctness
Lite/Darling parity
Security
Minor observations (not asking for changes)
Test coverage is thorough and appropriately paired ( |
…tring last_note and last_error were read as MAX(error_message) over the trailing window, which returns the lexicographically greatest string rather than the most recent one. Harmless while every message was a stable failure-mode sentence; erikdarlingdata#1837 shipped the first note carrying a NUMBER, and text does not sort like a count, so "12 item(s) failed their enumeration probe" sorts below "9 item(s)" and the grid could show an arbitrary earlier cycle's number. Both columns now rank each class of message newest-first in a subquery and read the top row. The ordering key is whether the class's status-gated CASE came back empty, so every row carrying such a message sorts ahead of every row that does not: rank 1 is the newest row that has text, and a later clean run no longer blanks a note the window still holds. The status re-check on the rank-1 row is load-bearing rather than belt-and-braces - with no failing run carrying text the error rank falls through to the newest row of any class, and without it a SUCCESS row's note would be reported as the collector's last error. One byte-identical query shape on both stores despite there being no portable argmax (DuckDB has arg_max, PostgreSQL has no max_by), verified by executing the same string against a real DuckDB and a real PostgreSQL 18.4. It also settles an exact-timestamp tie identically on each, which MAX did not: DuckDB compares text by bytes and PostgreSQL by the database collation, so the two grids could disagree on identical data. The Viewer's FLEET rollup deliberately projects NULL for both message columns instead of the old lexicographic MAX. Its only caller bands and counts for the status bar, no surface renders a fleet row's message, and ranking one per group turns a parallel hash aggregate into a serial sort of the whole window - 0.84s to 13.9s over 200 servers / 4M rows on PG 18.4. Recorded as erikdarlingdata#1863; the order-independent note COUNT stays real on both reads. Tests: paired behavioral suites in both apps, run against a real DuckDB and a real Postgres because a source pin cannot tell a lexicographic MAX from a newest-first rank. Watched red against the shipped SQL: 3 red in Lite, 3 in Darling. erikdarlingdata#1854's pins are re-pointed at the fixed expressions and keep their claims - the note gate is still SUCCESS-only, no read keys on message presence alone, the banding is untouched, and the fleet read's ordinal parity now also pins the deliberate NULLs. Closes erikdarlingdata#1855 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ture out (erikdarlingdata#1865) The object resolution's two CATCHes left `resource_object_id` NULL and the row carried the collector's `Unresolved: key lock, database: Foo` label. The failure was visible; the reason never was, and the two reasons want opposite responses. The screen is the design. These cursors run per contended database on every cycle, so routing the permission cause through erikdarlingdata#1851's channel unscreened would be a permanent note plus a five-minute warning burst for a posture that is not changing -- what erikdarlingdata#1854 added HAS_DBACCESS to query_store's enumeration to stop. So the posture is screened before it is attempted and answers on the row's own label, reaching the channel zero times; only the transient cause rides it. Verified on SQL 2022 that the existing guard was never a screen: for a login with no user in the target database, DB_NAME() still returns the name and DATABASEPROPERTYEX still reports ONLINE, so the cross-database lookup was attempted and raised 916 every cycle. Also verified that with VIEW SERVER STATE but no user in the database, sys.dm_db_page_info raises 916 rather than a permission-class denial -- which is why both sites take the SAME screen, and why there is deliberately no server-level one: the sys.dm_xe_session_targets read this batch opens with needs that permission too, so a login without it fails the first statement with 297 and never reaches the page loop. Both handlers moved out of their nested dynamic SQL: a TRY/CATCH inside a dynamic batch cannot catch that batch's own compile-time failure, which is how both of these fail. The page lookup stays dynamic for the opposite reason -- sys.dm_db_page_info does not exist before 2019, and an outer-batch reference would fail to compile the whole query there. Live on SQL 2022 (healthy, resolvable, screened denial, reallocated page, missing database) and SQL 2016 (the pre-2019 reason). Paired suites in both apps; six watched-red mutations each broke exactly their own test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #1837.
#1843 shipped the minimal core: a fixed message on the collection_log row of an enumerated collector that enumerated nothing. That row is only reachable from the raw collection-log detail grid, so "collected nothing for weeks" still read as healthy in the surface people actually check. This finishes both halves of the issue's scope.
1. Probe-failure contract
An enumeration's SQL may now return an optional second result set of
(item_name, error_text)for items it could not probe. It has to be a second result set - the first one IS the item list both runners consume as database names, so anything added to it would be collected FROM. That constraint is what #1836's implementation ran into and what got folded into this issue.EnumeratedCollectorDriver.ReadEnumerationAsyncreads the item list, callsNextResult, reads the failures, and composes the collection-log note. Both hosts route their entire enumeration read through it, so the item read, the failure read, and the note wording cannot drift.query_store's on-prem cursor records(database, ERROR_MESSAGE())there instead of discarding it in an emptyCATCH. "0 items" and "0 items, and here is the access error on all fourteen of them" are finally different rows.2. Collection Health visibility
Lite's grid, the Viewer's grid, the web dashboard's table, and both
get_collection_healthMCP tools gained a Note column (last_note/note_count, ordinals 11/12 on all three health projections plus the viewer's fleet variant, which sharesMapHealthRow).SUCCESSrows specifically, not the complement oflast_error- the looser filter would drag Darling'sSESSION_MISSINGrows (a real capture fault with its own self-alert) into a column whose tooltip promises it is not an error.CollectorHealthClassifier.FormatCollectionNote:"<note> (all 96 runs)"when the whole window came back that way,"(3 of 96 runs)"otherwise. Always-empty is the signal; sometimes-empty is normal.Classifyreads the new columns. Pinned in both suites by a test asserting two collectors identical except for the note band identically.Two defects found while building this, fixed here
query_store's enumeration was the only one without [QUESTION] Regarding installing the windows service and integrated security #1823'sHAS_DBACCESSscreen. A least-privilege login took a 916 per inaccessible database per cycle - harmless while theCATCHswallowed it, but recording those failures would have made it a permanent probe-failure note plus a warning burst every 5 minutes for a permission posture that is not changing._lastSqlMs/_lastDuckDbMs/ the note were plain instance fields, so server B's reset could blank server A's timings between A's write and A's read - and with a note attached, A's "enumeration yielded 0 items" could land on B's row for a collector that does not enumerate. They now live in one per-server telemetry slot. Darling was never exposed; its note is a return value.Filed rather than shipped (per the standing rule)
database_size_statshas the same empty-CATCHshape, but it is not an enumerating collector: its one result set IS the payload, so giving the plain path a failure channel means changingReadAsync's contract or inventing a sentinel row. Design decision, not a mirror of this PR.Test plan
EnumerationProbeFailureTests/DarlingEnumerationProbeFailureTests, plus extensions to the twoEmptyEnumerationNoteTests), pinned independently so editing one app's copy alone fails a build. ADataSetsupplies a real multi-result-setDbDataReader.CATCHand flattening the qualifier (3 red), removing theHAS_DBACCESSscreen and the web column (2 red). Each was reverted after.dotnet testLite 1684 passed / 0 failed; Darling 3623 passed / 0 failed / 193 skipped (gated-live).dotnet build PerformanceMonitor.sln -t:Rebuild: 0 warnings, 0 errors. Installer.Tests not run.Note for the reviewer
Two source pins from #1843 that asserted a specific note assignment line are replaced with stronger ones asserting both runners route through the shared read. A literal pin could no longer prove the hosts cannot drift now that the note has three possible shapes; routing proves it, because there is then no host-side text at all. Every behavioral pin from #1843 - health-neutrality of a message on a SUCCESS row, the
last_errorstatus gating, andDoesNotContain("error_message IS NOT NULL")in all three readers - is unchanged and still green.🤖 Generated with Claude Code