Repository navigation
Alert-history legacy metric name + empty-enumeration collection-log note (both SKUs) - #1843
Conversation
Fix 1 - legacy "TempDB Space" alert-history rows render without their unit. Lowercasing the tempdb token across both apps' UI (c0109f3) also changed this alert's metric_name KEY to "tempdb Space", and that commit knowingly left stored history rows on the old name. The Value/Threshold formatter matches ordinally, so those rows missed the percent case and fell through to the bare :F2 default - 87.30 where the row beside it reads 87.3%. Both formatters (Lite's AlertHistoryRow and the Darling Viewer's ViewerAlertRow) now accept the old spelling. Verified by git archaeology that "TempDB Space" is the ONLY legacy spelling that ever existed, so an added case is exact rather than a case-insensitive widening. Fix 2 (#1837 minimal core) - an enumerated collector whose enumeration yields zero items returned early and logged a bare SUCCESS/0 rows, byte-identical to a healthy collector whose databases were quiet. It now records the same SUCCESS/0 rows but attaches a fixed, greppable message to the collection-log column SUCCESS rows otherwise leave null. Status is unchanged and no health banding is added - that larger design stays on #1837. The message lives once, on the shared EnumeratedCollectorDriver, so the two runners cannot drift on the wording; Lite carries it out on a _lastCollectionNote field beside the existing _lastSqlMs convention, Darling on a defaulted CollectorRunResult.Note member. Verified before shipping that a message on a SUCCESS row is inert: health tracking ignores errorMessage when status is SUCCESS, and every band/count/self-alert read keys on status, with last_error gated on status IN ('ERROR','PERMISSIONS'). The note surfaces only in the raw collection-log detail grid, which is the point. Adjacent defect found and fixed at the same call site: Lite cleared _lastSqlMs / _lastDuckDbMs only inside the definition runner, but a collector can throw before the runner is entered - the XE session ensure for deadlocks and blocked_process_report runs first - and every catch falls through to the same LogCollectionAsync, so those ERROR rows were written with the PREVIOUS collector's timings. Now cleared once per run at the top. Tests: legacy-name format pins in both apps' formatter suites; new paired EmptyEnumerationNoteTests / DarlingEmptyEnumerationNoteTests pinning the shared message, both runners' wiring (source pins - the zero-items branch is the tail of a live enumeration read, the #1805 LockTimeoutYieldTests idiom), the record's null-by-default carrying behavior, and real assertions that an annotated SUCCESS is neither an erroring collector nor a fake last_error. Lite.Tests 1634 passed, Darling.Tests 3590 passed / 191 live-gated skips, both projects 0 warnings 0 errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| listed zero databases) onto the collection_log row without touching the SUCCESS status. | ||
| Health tracking and every band/count read key on status, never on error_message, so the | ||
| note is visible in the Collection Log detail grid and inert everywhere else. */ | ||
| errorMessage = _lastCollectionNote; |
There was a problem hiding this comment.
Cross-server data race on _lastCollectionNote (and pre-existing _lastSqlMs/_lastDuckDbMs).
RemoteCollectorService is a single instance shared across all monitored servers (constructed once in MainWindow.xaml.cs). The scheduled sweep runs one Task.Run per online server and awaits them together (RunAllDueCollectorsAsync → Task.WhenAll(serverTasks)), so RunCollectorAsync for server A and server B can execute concurrently on separate thread-pool threads against the same instance.
_lastSqlMs / _lastDuckDbMs / _lastCollectionNote are plain instance fields with no lock, and each collector's actual work (RunCollectorDefinitionAsync) awaits real I/O (SQL reads, DuckDB writes) between resetting these fields and writing their final values. That gives a real window where server A's in-flight collector can be clobbered by server B resetting/writing the same fields, so the value read back here (errorMessage = _lastCollectionNote, and the _lastSqlMs/_lastDuckDbMs read a few lines below at the LogCollectionAsync call) is not reliably "this server's own run."
Before this PR the only symptom was a mislabeled timing number. This PR adds a third field to the exact same unsynchronized pattern, so the failure mode it enables is worse: an unrelated server's healthy SUCCESS collection_log row can end up stamped with "enumeration yielded 0 items" from a totally different server/collector that happens to race it — exactly the kind of misleading-diagnostic-log problem fix 2 is trying to eliminate.
Darling avoids this entirely because CollectorRunResult is a return value, not shared mutable state (as the PR description notes) — that's the safer pattern here too, e.g. threading the note back as part of RunCollectorAsync's own local state / return rather than an instance field.
Review summaryTwo small, well-scoped fixes, both applied to Lite and Darling with parity confirmed by inspection. Fix 1 — legacy
|
…enumeration-polish # Conflicts: # CHANGELOG.md
ReviewBoth fixes are narrowly scoped, well-verified, and land symmetrically in Lite and Darling. No parity drift, no security concerns (no new input handling, file/network/process use, or secrets touched), and no missing-index-DMV suggestions here (not applicable to this diff anyway). Fix 1 — legacy
|
…y why (erikdarlingdata#1837) erikdarlingdata#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 erikdarlingdata#1852; the same empty-CATCH shape in database_size_stats is not an enumeration and is filed as erikdarlingdata#1851. Two defects found while building this, fixed here. query_store's enumeration was the only one without erikdarlingdata#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 erikdarlingdata#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 erikdarlingdata#1843, including health-neutrality and the last_error gating, is unchanged. Closes erikdarlingdata#1837 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Two small verified fixes, both applied to Lite and Darling.
Fix 1 - legacy "TempDB Space" alert-history rows render without their unit
Lowercasing the tempdb token across both apps' UI (
c0109f34) also changed this alert'smetric_nameKEY fromTempDB Spacetotempdb Space. That commit's own message accepted the consequence: "historical alert-history rows keep the old name." The Value/Threshold formatter matches the name ordinally, so those stored rows miss the percent case and fall through to the bare:F2default - an archived tempdb alert shows87.30where the row beside it shows87.3%. Both the Value and Threshold columns are affected, since both run the same formatter.Both copies now accept the old spelling:
Lite/Services/LocalDataService.AlertHistory.csandDarling/PerformanceMonitor.Darling.Viewer/ViewerDataService.AlertHistory.cs.Checked with
git log -Sacross every case variant thatTempDB Spaceis the only legacy spelling that ever existed, so an added case is exact - no case-insensitive widening was needed, and the files' documented "keyed on the exact metric_name strings" contract stays intact. Nothing writes the old name any more; it is kept solely so rows already on disk format like new ones.No issue ref - field find during #1830's triage.
Fix 2 - enumerated collectors that enumerate NOTHING log an indistinguishable SUCCESS (#1837 minimal core)
query_store,index_object_stats, anddatabase_scoped_configlist their databases first, then collect per database. When that list comes back empty the cycle returned early and logged a bareSUCCESSwith 0 rows - byte-identical to a healthy collector whose databases were simply quiet. That is what let #1833's Azure defect sit behind a green Collection Health: Query Store enabled nowhere, a database-scoped collector filtered to nothing, and an ordinary idle cycle all logged the same row.The cycle still records
SUCCESSwith 0 rows (nothing failed), but now attaches a fixed, greppable message to the collection-log column thatSUCCESSrows otherwise leave null:Deliberately not in scope: the status value is unchanged and no health-banding logic is added - that larger design remains #1837. The per-database branch is untouched (it already has its all-failed rethrow guard).
The message lives once, as
EnumeratedCollectorDriver.EmptyEnumerationMessageon the shared driver that already owns the enumerated path, so the two runners cannot drift on the wording an operator greps for. Lite carries it out on a_lastCollectionNotefield beside the existing_lastSqlMs/_lastDuckDbMsconvention; Darling on a defaultedCollectorRunResult.Notemember (null default, so every other run's row is unchanged).Verified inert before shipping
A message on a
SUCCESSrow is new, so I checked every consumer first:RecordCollectorResultignoreserrorMessageentirely when status isSUCCESS(it resets the streak and returns).status, never onerror_message IS NOT NULL.last_erroris gatedMAX(CASE WHEN status IN ('ERROR','PERMISSIONS') THEN error_message END)in all four readers.So the note surfaces only in the raw collection-log detail grid, which is the point. Tests pin both the neutrality and those read-side predicate shapes, so a future broadening cannot quietly turn every quiet enumeration cycle into a fake last-error.
Adjacent defect found and fixed
At the same call site: Lite cleared
_lastSqlMs/_lastDuckDbMsonly inside the definition runner, but a collector can throw before the runner is entered - the Extended Events session ensure fordeadlocksandblocked_process_reportruns first - and every catch falls through to the sameLogCollectionAsync. ThoseERRORrows were written with whatever the previously-run collector had left in those fields. Now cleared once per run at the top, where every path reaching the log write passes through. Darling has no equivalent (its result is a return value, so there is no shared mutable state to go stale).Test plan
AlertHistoryValueFormatTests,ViewerWave3DisplayTests), asserting the old spelling renders87.3%and formats identically to the new one.Lite.Tests/EmptyEnumerationNoteTests.cs+Darling/Darling.Tests/DarlingEmptyEnumerationNoteTests.cs. The zero-items branch is the tail of a live enumeration read, so its wiring is pinned at source - the same idiomLockTimeoutYieldTestsuses for Query-snapshots 1s lock-timeout yields (error 1222) are counted as collection errors, so one benign yield paints a Critical health day #1805's catch - including an assertion that neither runner inlines the literal instead of using the shared constant. Everything reachable is pinned for real: the shared message value (independently in both suites, so editing one app's expectation alone fails the other),CollectorRunResult's null-by-default and round-trip behavior, and the two neutrality assertions above.Lite.Tests1634 passed, 0 failed.Darling.Tests3590 passed, 0 failed, 191 skipped (live-Postgres / live-SQL gated).-t:Rebuild: 0 warnings, 0 errors.Installer.Testsnot run.CHANGELOG
[Unreleased]gets three Fixed entries; the[#1837]link-ref was already present.🤖 Generated with Claude Code