[erikdarlingdata]

Addresses the attribution half of #2235. The literal-fragmentation half already shipped (get_top_queries_by_cpu(group_by: 'host_object')).

The defect

query_stats keys its deltas on the full dm_exec_query_stats row identity — which includes plan_handle, and plan_handle changes on every recompile:

var deltaKey = $"{row.SqlHandle}:{row.StatementStartOffset}:{row.StatementEndOffset}:{row.PlanHandle}";

So a statement whose dynamic SQL mints a new plan constantly presents a new key on nearly every sighting, and a first sighting reports 0. Measured on a plan-churning readable secondary:

Datadog sqlserver.queries.worker_time, one hash	43.1% / 43.9% / 42.5% of an 8-vCPU box, three windows
The same hash through these collectors, 168 h	18 executions, 2,824 ms
Instance-wide: top-25 procedures	~49M ms against roughly 498M core-ms available

The worse half: it was invisible

The calculator already had an honest path for cache churn. The counter-reset branch reports interval = 0 on purpose, so a reader can tell a fabricated zero from an idle one — the invariant #2234 rests on:

/* Counter reset (plan cache eviction/re-entry): the work between the two readings
   is unknowable, not zero. Report no interval either, so the pair stays honest [...] */

But it needs the same key to reappear with a lower value, and a recompile never does — it arrives under a new key and takes the baseline path instead. So the loss was indistinguishable from a query that simply had not run. Same class of harm as the 300-second gap policy #2233 replaced: it did not merely lose data, it invented quiet.

The fix, using a discriminator already being collected

creation_time has been in this collector's SELECT all along (QueryStatsCollector.cs:91). The row now also carries how long ago its plan was compiled, and when a series demonstrably began since the previous pass, its whole counter accrued inside that window — so the delta is the full value rather than 0, reported with a real interval because it is knowable.

No new collection and no schema change. PayloadColumns is unchanged and the age is not stored; the 51-column pin stays green.

Sent as an AGE, not a timestamp. A DMV creation_time is in the monitored server's local time while collection times are UTC, so comparing them client-side is a timezone bug on every server that is not UTC. DATEDIFF(SECOND, qs.creation_time, GETDATE()) is evaluated where both clocks are the same one.

Bounded by the existing gap policy. A plan compiled during an hour-long outage is not an hour of work in the next minute, so the credit is refused past DefaultMaxGapSeconds — the same guard the reset branch uses.

Two things stated rather than left to be discovered

The ceiling. A plan compiled and evicted between two passes never appears in dm_exec_query_stats at all, so no keying scheme can recover it. This fixes the recoverable half only.

Why plan_handle was not simply dropped from the key, since that is the obvious first move and it is wrong: parameter-sensitive variants of one statement coexist in cache, so a statement-only key would collide across several live rows in a single pass and each delta would be computed against whichever row happened to be processed last. That is the multi-statement cross-contamination #2012 fixed, reintroduced from the other direction. Narrowing the key would require aggregating per statement before the delta, which changes what a row means — a defensible design, but a different and larger one.

Blast radius

The new entry point is default-implemented on the interface, delegating to the existing method and ignoring the hint. So the other forty-odd delta call sites (perfmon, wait stats, file IO, …) and every existing implementer — including test doubles — are byte-identical until they opt in. All eight of the row's counters take the rule together, because crediting only some would make one row's metrics disagree about how much work it did.

Also fixed: ClearServer now drops the pass window along with the baselines. Left behind, a re-added server's first pass would measure a series age against a look from before it was removed and credit a whole counter to an interval that never happened.

Verification

Darling.Tests is net10.0-windows and cannot run on this machine, so I Compile-Include'd the new test file into a plain net10.0 console harness and executed the facts rather than assuming they transcribe: 11/11 pass, covering the fix itself, the gap-policy bound, a series older than the gap, the cold start, ordinary same-key subtraction, a reset on the same key still reporting the honest (0, 0), ClearServer, per-server and per-collector isolation, and a pre-#2235 implementer still compiling and behaving as before.

One of those deserves calling out because it is the trap that would have made this look like it worked: every row of one pass must measure against the same previous pass. A collector calls in once per row, so if the first row rolled the pass window forward, every later row in that sweep would compare against its own pass, see a zero gap, and be credited nothing — the fix would then work for exactly one row per cycle. EveryRowOfOnePassSeesTheSamePreviousPass pins it.

The test fake also records the age now, overriding the default rather than inheriting it: the default forwards and drops the age, so a collector that stopped passing it would otherwise keep every existing pin green.

Not claimed here

That this closes the 43%-vs-18-executions gap. It removes the mechanism that provably discards a churning plan's work, and the direction and scale are right, but the confirming measurement needs a live window on the affected instance — which is what remains open on #2235.

🤖 Generated with Claude Code

[claude]

interval is set to the full inter-pass gap, not the counter's actual accrual window — so worker_time_per_second stays diluted for exactly the churning-plan case this PR targets.

The credit here is correct (delta = currentValue), but a recompiled plan's counter only accrued over seriesAgeSeconds (compile time → now), which is guaranteed <= gap by the condition on line 202. Reporting interval = gap instead of interval = seriesAgeSeconds breaks the implicit "delta accrued over exactly interval seconds" contract that every rate consumer relies on (ViewerDataService.QueryStats.cs: worker_time_per_second = delta_worker_time / sample_interval_seconds / 1000.0).

Concretely: a plan compiles 5s before a collection pass that lands 300s after the previous one (a delayed sweep, or just ordinary jitter on top of a churn-prone workload). total_worker_time = 900 (all of it earned in those 5s). Delta is correctly credited as 900, but interval is reported as 300 instead of 5 — so worker_time_per_second computes to 900/300/1000 = 0.003 instead of the true 900/5/1000 = 0.18, a 60x under-report of the peak rate. At the boundary (age ≈ 0, gap ≈ maxGapSeconds = 3600), the dilution factor is unbounded.

This doesn't affect the SUM-based totals (avg_cpu_ms divides by delta_execution_count, not the interval) — the "43%-of-the-box discarded to 0" scenario from the PR description is genuinely fixed for those. But the peak/spike metric this same fix is meant to make visible stays quietly suppressed for every row that takes this new credit path.

Suggest interval = (int)seriesAgeSeconds.Value; here instead of interval = (int)gap; (age is already bounded by gap via the seriesAgeSeconds.Value <= gap check on line 202, so this can't report an interval longer than the real gap).

[claude]

Review

Reviewed the delta-calculator and query_stats changes for #2235. The core fix — passing compile_age_seconds so a recompiled plan's counter is credited instead of silently reporting 0 on first sighting — is sound: PreviousPass correctly tracks the (server, collector) pass boundary once per pass (stable across every row via the "advance only when collectionTime changes" guard), the credit path is properly gated by the existing gap policy, ClearServer drops the pass window with the baselines, and the default-interface-method forwarding keeps every other of the ~40 call sites and every existing ICollectorDeltaCalculator implementer untouched. Lite and Darling both derive from the shared CollectorDeltaCalculator without overriding the new method, and QueryStatsCollector.cs is a single file shared by both SKUs, so there's no parity drift here.

One correctness issue found, left as an inline comment on CollectorDeltaCalculator.cs: in the new credit branch, interval is set to the full inter-pass gap rather than the counter's actual accrual window (seriesAgeSeconds, which is already proven <= gap by the guarding condition). This doesn't affect the delta itself or the SUM-based CPU totals, but it dilutes worker_time_per_second (delta_worker_time / sample_interval_seconds) for exactly the newly-credited, plan-churning rows — the peak-rate metric that would otherwise make this class of spike visible stays under-reported, sometimes by orders of magnitude near the gap-policy boundary.

Everything else — the SQL (compile_age_seconds = DATEDIFF(SECOND, qs.creation_time, GETDATE()), correctly comparing two server-local-clock values rather than mixing in UTC), the ordinal bookkeeping in ReadAsync (age fixed at 43 in both capture modes, plan XML correctly pushed to 44), the unchanged 51-column PayloadColumns pin, and the test coverage in DeltaSeriesAgeTests.cs / QueryStatsCollectorDefinitionTests.cs — looks correct and thorough.

(Per repo convention, not flagging anything about missing-index DMV usage — out of scope here anyway.)