Repository navigation
The server-scoped watermark read is discarded entirely for collectors that declare a per-database watermark #2797
Description
Follow-up from the review on #2796, filed rather than folded in because it needs verification across four collectors and three dispatch paths.
The claim
For query_store, the value GetLastCollectedTimeAsync returns is never consumed. QueryStoreCollector declares PerDatabaseWatermarkColumn, and every execution path overwrites context.Watermark before a query is built:
- Azure SQL DB (
RunsPerDatabase):context.Watermark = await GetLastCollectedTimeForDatabaseAsync(...)in the per-database connection loop (DarlingCollectorRunner.cs:581) - on-prem / RDS / MI enumeration:
context.Watermark = clampedinside theperItemWatermarkdelegate (:923)
The local is otherwise read only to compute hasCollectedBefore.
What I verified
HasCollectedBefore is consulted by exactly two collectors — DefaultTraceEventsCollector (:319) and JobHistoryCollector (:220) — and neither declares PerDatabaseWatermarkColumn. So for the four collectors that declare both watermark columns, hasCollectedBefore is computed from this read and then never consulted.
The four: QueryStoreCollector, DeadlocksCollector, BlockedProcessReportCollector, LongQueryCompletionsCollector.
What I did NOT verify, and why this is filed rather than fixed
The second half of the claim — that every dispatch path overwrites context.Watermark for all four of those collectors — is the load-bearing part, and it spans three paths (per-database Azure loop, enumeration, plain). If any path for deadlocks, blocked_process_report or long_query_completions does not overwrite, skipping the read silently destroys that collector's watermark and it re-collects its whole window forever — the same failure #2795 just fixed, arrived at from the other direction.
That is not a check to do by reading; it wants the live rig.
The prize
After #2796 the read costs ~2.9-5.9 s per cycle per server (down from 40-50 s and a cancellation). Skipping it where its answer is discarded takes that to zero, and makes the new 60 s CommandTimeout unreachable for a call nothing uses.
Gate would be definition.PerDatabaseWatermarkColumn is not null — the same signal the per-item paths already key on.
Suggested shape
- Assert, per collector and per dispatch path on the live rig, that
context.Watermarkis written beforeBuildQuery. - Only then skip the server-scoped read when
PerDatabaseWatermarkColumnis non-null. - Pin it the way Bound the server-scoped watermark read on the partitioning column #2796 did — over the family, derived from the definitions rather than enumerating the four names, so a fifth collector declaring both columns is covered automatically.
Activity
erikdarlingdata commentedon Sep 4, 2026
The proposed gate would ship a data-correctness bug. Not implemented.
The issue's own warning was right, and the load-bearing half is false: the suggested gate
(
definition.PerDatabaseWatermarkColumn is not null) skips a read that three of the four namedcollectors actually consume, on every non-Azure target — i.e. the entire production fleet.
Derived from the built assembly, not grep
Reflecting over
CollectorCatalog.Alland invoking the realRunsPerDatabase/BuildEnumerationQueryagainst a synthetic on-prem target:
query_storelast_execution_timedatabase_namedeadlocksdeadlock_timedatabase_nameblocked_process_reportevent_timedatabase_namelong_query_completionsevent_timedatabase_nameQueryStoreCollectoris the only one of the four that overridesBuildEnumerationQuery; the other threeinherit
CollectorDefinitionBase's=> null. All four declareRunsPerDatabase => target.IsAzureSqlDb, sooff Azure they fall through to the plain path at
DarlingCollectorRunner.cs:1424, wheredefinition.BuildQuery(context)reads the server-scopedcontext.Watermarkthat was never overwritten.Skipping the read there sets it null on every cycle — the collector re-collects its whole first-run window
forever. That is #2795's failure arrived at from the other direction, exactly as this issue predicted.
Live confirmation on 42 production servers (90-minute window)
The V108 phase columns are populated only on the server-scoped path, so they identify dispatch directly:
sql_open_mspopulatedblocked_process_reportdeadlocksquery_storeReading and measurement agree.
The correct gate
The read is discarded iff the run will take path 1 or 2 — which is a dispatch-path predicate, not a
watermark-column one:
On this fleet that reduces to
query_storealone. Note the awkward part:BuildEnumerationQueryneeds aCollectorContext, which today is constructed after the watermark read, so implementing this needs eithera definition-level "enumerates" signal on
ICollectorSchemaInfoor a reordering — a design decision, not aone-line gate. That is why this is filed rather than fixed.
The prize is real but ~70x smaller than stated
The issue quotes ~2.9-5.9 s per cycle per server. Measured from
wm:in the phase line over ~2.6 h on thelarge store:
query_storedeadlocksblocked_process_reportSo #2796's bound already took the common case to 41 ms. The remaining prize for
query_storeis~198 s per 2.6 h (~76 s/hour, ~30 min/day of store time) that is genuinely discarded — worth having, driven
by the tail rather than the median, and the 22.1 s max shows the tail is still live.
The
deadlocks/blocked_process_reportfigures are legitimate work, not waste.Instrumentation gap found on the way
watermark_ms(V108) is NULL on all 636query_storeruns, because it is persisted only on theserver-scoped branch. The one collector whose watermark read is worth optimising is the one whose read cost
the store cannot report — it is only visible in the app-log
wm:field. Same shape as #2860. Worth foldinginto that decision.
Skip the server-scoped watermark read only where dispatch discards it (…
Skip the server-scoped watermark read only where dispatch discards it (…
erikdarlingdata commentedon Sep 4, 2026
Fixed by #2890, merged to
dev.The gate this issue proposed was not the one built, and that is deliberate.
PerDatabaseWatermarkColumn is not nullwould have skipped the read for all four collectors that declare both watermark columns — but onlyquery_storeoverridesBuildEnumerationQuery; the other three inherit=> null. Off Azure, i.e. the entire on-prem and RDS fleet, those three fall through to the plain server-scoped path and genuinely consume the value, so the proposed gate would have handed them a null watermark every cycle and made them re-collect their whole fallback window forever — #2795's defect from the other side. The shipped predicate is therefore a dispatch-path one:On the current catalog that skips 7 of 168 (definition, target) pairs:
query_storeon every target, plus the three XE collectors on Azure SQL DB only, where they really do run per database. Verified by invoking the shipped predicate overCollectorCatalog.Allin the built assembly, not by reading source.The enumeration half calls the definition's own
BuildEnumerationQueryrather than mirroring it into a second declared flag, so the two cannot drift apart. That needs a probe context, becauseHasCollectedBeforeis computed from the gated read and it,NumericWatermarkandStateare init-only — so the alternative was widening a deliberately-immutable contract shared by both SKUs to buy an optimisation. The probe is sound only while enumeration null-ness depends onTargetalone, and a pin asserts that across 5,376 (definition, target, context) combinations rather than assuming it.Pinned over the family derived from
CollectorCatalog.Allrather than by naming collectors, so a fifth collector declaring both columns and enumerating is covered the day it appears. The load-bearing assertion is the negative one — that the three server-scoped members still receive a non-null watermark off Azure — and it was proven red against this issue's own proposed gate before being left green.One correction to the record, because it is easy to misattribute. This issue estimated ~2.9–5.9 s per cycle per server; a later comment revised that down to ~76 s/hour on one collector. Both readings of the fleet data were wrong, in opposite directions, and the ~76 s/hour figure was measuring the wrong read.
ServerWatermarkMsreaches the phase log only underServerPhasesMeasured, which the enumerated path leaves false (ServerScopePhaseSplitTestspins exactly that), and V108'swatermark_msis persisted on the same branch — so forquery_storethis read's cost is recorded in neither the log nor the store. Thewm:field that is populated forquery_storebelongs to the per-item line and measures the per-database read (p50 41 ms over 822 samples) — different read, legitimate incremental work, untouched here.So the only direct measurement of the read actually removed is #2796's, taken as it was being bounded: 2.9–5.9 s warm-to-cold against a 62.5 GB table (40.7 s and 50.6 s unbounded). Directionally that must exceed the per-database read, since it is a
MAXover the whole read-floor window across all databases rather than one. The true value is therefore plausibly larger than this issue first claimed, not smaller — but it cannot be confirmed from the fleet until the instrumentation gap is closed, which is #2860's decision. That gap is stated in the changelog entry rather than papered over with either number.The
deadlocksandblocked_process_reportserver-scoped reads (p50 9 ms and 5 ms) are legitimate and deliberately stay.Merge pull request #2890 from erikdarlingdata/fix/2797-dispatch-path-…
The CPU collector's watermark and dedup key are the UTC twin where th…
The CPU collector's watermark and dedup key are the UTC twin where th…
Move the #2797 dispatch gate into ResolveServerWatermarkAsync (#4197 …