cpu_attribution on the top-CPU rankings: what fraction of the box the ranking explains (#2320)

[erikdarlingdata]

Closes #2320 — the last unshipped item from #2235's wishlist.

What

get_top_queries_by_cpu and get_top_procedures_by_cpu (both SKUs) gain a top-level cpu_attribution object:

• ranked_cpu_seconds — the RETURNED rows' summed windowed CPU (post top-N, post filters): what the caller-visible ranking claims.
• sql_cpu_seconds_in_window — measured, not theoretical: avg cpu_utilization % over the window × core count (server_properties) × window seconds. Both stores already collect every piece.
• attributed_cpu_ratio + note — under half, the note explains where unattributable CPU goes (evictions between snapshots, rows outside the top-N, zero-cost rows, non-query CPU); above the process's own measured consumption (>1.1) it flags the impossible claim — the 137% marker from the issue — and says to distrust the absolute numbers.

Degrade rule (the issue's explicit requirement): missing CPU series, missing core count, or a series covering under 90% of the window ⇒ ratio omitted with a reason, never invented.

How

• One computation in PerformanceMonitor.Common (CpuAttribution) so the SKUs cannot disagree — same placement as QueryStatExtremes, which these same tools already share.
• Denominator reads are cheap aggregates windowed on collection_time with the SAME bounds as the rankings, so numerator and denominator share collection gaps (and Lite's server-local sample_time skew stays irrelevant).
• No parameter changes — the disclosure is automatic; tool descriptions and both instructions docs updated.

Tests

• The decision table is pinned identically in Darling.Tests and Lite.Tests (CpuAttributionTests): healthy/low/impossible/slightly-over, no-samples, no-cores, partial-coverage, edge-clamping, zero-CPU, empty-window, and rounding.
• CpuWindowAggregateSql pinned (collection_time both edges, aggregate shape) and added to the PG-dialect theory.
• The full table was additionally executed against the built PerformanceMonitor.Common in a throwaway net10.0 harness — all cases pass (the Windows test projects only build on this Mac).

🤖 Generated with Claude Code

[claude]

Minor perf: GetCpuWindowAggregateAsync and GetLatestServerPropertiesAsync are independent reads (different tables, no data dependency) but run sequentially, adding two full round-trips to every get_top_queries_by_cpu call (and the same pattern repeats below in GetTopProceduresByCpu, plus the Lite twin). Since NpgsqlDataSource pools connections, these can run concurrently:

var cpuAggregateTask = DarlingDataReader.GetCpuWindowAggregateAsync(postgres, resolved.ServerId, now.AddHours(-hours_back), now);
var propertiesTask = DarlingDataReader.GetLatestServerPropertiesAsync(postgres, resolved.ServerId);
await Task.WhenAll(cpuAggregateTask, propertiesTask);
var cpuAggregate = cpuAggregateTask.Result;
var properties = propertiesTask.Result;

Not a correctness issue, just avoids doubling the added latency for a disclosure block that's now on every call to these two tools.

[erikdarlingdata]

All three points taken in 958dec3: both SKUs' tools now run the two independent reads under Task.WhenAll; Lite's GetCpuWindowAggregateAsync takes the window explicitly and one nowUtc backs both the aggregate read and the ratio math (the ranking read keeps its internal clock — its shared signature isn't worth threading a timestamp through for sub-millisecond drift, and the aggregate+Compute pair is where the invariant matters); and the Result doc now states the measured-zero exception instead of claiming both-null-together.

[claude]

Two things here, both minor:

1. Same sequential-await perf point as the Darling twin — GetCpuWindowAggregateAsync and GetLatestServerPropertiesAsync are independent and could run via Task.WhenAll instead of two back-to-back awaits.

2. Window-drift vs. Darling's design: Darling captures a single now up front and threads it through the ranking query, the CPU aggregate query, and CpuAttribution.Compute's window bounds (see DarlingMcpDataTools.GetTopQueriesByCpu), so numerator and denominator are guaranteed to share the exact same window — which is the invariant CpuAttribution.cs's doc comment calls out explicitly ("numerator and denominator share collection gaps"). Here, nowUtc is a fresh DateTime.UtcNow call taken after dataService.GetTopQueriesByCpuAsync and GetCpuWindowAggregateAsync have already each independently called DateTime.UtcNow internally (via GetTimeRange). So three separately-sampled timestamps back the ranking rows, the CPU aggregate, and the ratio math instead of one shared value. At hours_back >= 1 the drift is sub-millisecond against an hour-plus window, so it's not user-visible today — but it's a real inconsistency between the two SKUs implementing the same disclosure, and worth threading one now through here too for the same guarantee Darling has by construction.

[claude]

This branch returns a non-null SqlCpuSecondsInWindow (0) alongside a null AttributedCpuRatio, which contradicts the Result record's doc comment above (line 46-49): "SqlCpuSecondsInWindow and AttributedCpuRatio are null together". It's intentional — ZeroMeasuredCpu_OmitsRatio_ReportsTheZero in both test suites pins exactly this (measured zero is reported, ratio omitted) — but the doc comment states an invariant that isn't actually true. Worth updating the summary so a future caller/maintainer doesn't rely on "both null or both present" as a real contract.

[claude]

Same sequential-await pattern as GetTopQueriesByCpu above — cpuAggregate/properties could run concurrently via Task.WhenAll here too.

[claude]

Same two points as GetTopQueriesByCpu above (sequential awaits that could be Task.WhenAll'd, and nowUtc sampled independently from the two GetTimeRange-driven DateTime.UtcNow calls already made by GetTopProceduresByCpuAsync and GetCpuWindowAggregateAsync) — repeating here rather than re-explaining.

[claude]

Review summary

Reviewed the cpu_attribution denominator work (PerformanceMonitor.Common/CpuAttribution.cs) and its wiring into get_top_queries_by_cpu / get_top_procedures_by_cpu on both SKUs.

Overall: solid. The degrade-rather-than-invent logic in CpuAttribution.Compute is correctly implemented (empty window → no samples → no core count → thin coverage → zero measured CPU, in that priority order), the two SKUs' test suites are byte-identical decision tables, the SQL pin tests were extended for the new CpuWindowAggregateSql, and the instructions docs / CHANGELOG were updated in lockstep for both apps. Numerator (microseconds → seconds) and denominator (avg % × cores × window seconds) conversions check out arithmetically, and I didn't find a SQL injection, null-handling, or missing-index-recommendation issue anywhere in the diff.

Left 5 inline nits, none blocking:

• Perf (Darling DarlingMcpDataTools.cs, Lite McpQueryTools.cs, both tools in both SKUs): the new GetCpuWindowAggregateAsync + GetLatestServerPropertiesAsync reads are independent but awaited sequentially, adding two full round-trips per call. Task.WhenAll would halve the added latency.
• Parity/design drift (Lite McpQueryTools.cs): Darling threads a single captured now through the ranking query, the CPU-aggregate query, and CpuAttribution.Compute's window bounds, guaranteeing numerator/denominator see byte-identical bounds. Lite instead lets three independent DateTime.UtcNow calls (two inside GetTimeRange, one explicit nowUtc) each sample the clock separately. At hours_back >= 1 the drift is sub-millisecond and not user-visible, but it's a real inconsistency between the two SKUs' implementations of the same invariant the CpuAttribution.cs doc comment calls out.
• Doc/contract nit (CpuAttribution.cs): the Result doc comment says SqlCpuSecondsInWindow and AttributedCpuRatio are "null together," but the zero-measured-CPU branch returns SqlCpuSecondsInWindow = 0 with a null ratio — intentional (pinned by ZeroMeasuredCpu_OmitsRatio_ReportsTheZero) but the doc states an invariant that isn't actually true.

[claude]

Lite/Darling drift: attribution window isn't actually the same window as the ranking.

nowUtc here is captured after rows was already fetched via dataService.GetTopQueriesByCpuAsync(resolved.ServerId, hours_back, top, ...) (line 34). That call resolves its own window internally through GetTimeRange(hoursBack, null, null), which does its own independent DateTime.UtcNow calls. So the window that produced rows/filtered (the numerator: ranked_cpu_seconds) and the nowUtc window used for the CPU aggregate query + CpuAttribution.Compute (the denominator) are two different windows — offset by however long the ranking query took. The identical pattern repeats in GetTopProceduresByCpu at line 152.

Darling avoids this: it captures a single now = DateTime.UtcNow once at the top of the method and reuses it for both the ranking query (DarlingDataReader.GetTopQueriesByCpuAsync(postgres, resolved.ServerId, now.AddHours(-hours_back), now, ...)) and the attribution reads. Lite doesn't have an equivalent explicit-window overload of GetTopQueriesByCpuAsync/GetTopProceduresByCpuAsync to do the same.

Practically the drift is one DB round-trip against an hours-long window, so it won't visibly move the ratio in most cases. But it does contradict the PR's stated invariant ("the denominator read windows on collection_time with the SAME bounds as the rankings") for Lite specifically, and it's the exact class of bug the comment two lines below warns about ("three independently-sampled UtcNow calls backing one disclosure is drift by construction") — the fix only unified 2 of the 3 UtcNow calls, not all of them.

[claude]

Stray double percent sign: CPU%% should be CPU%. The same %% doubling appears in several other new doc comments in this PR — DarlingDataReader.cs:181 ("CPU%% over the window"), and both CpuAttributionTests.cs files (Darling and Lite) at lines 21/29/44/57 ("137%%-of-the-box claim", "25%% of 8 cores", "~10%% of the box", "The 137%% case"). No existing comment in the codebase uses this pattern, so it looks like a leftover escaping artifact (e.g. from a printf-style template) rather than an intentional convention. Doesn't affect runtime behavior, but it'll render as a literal double percent sign in IntelliSense/generated docs — worth a find/replace across the new files before merge.

[claude]

Review summary

Reviewed the cpu_attribution disclosure added to get_top_queries_by_cpu / get_top_procedures_by_cpu (both SKUs), the shared CpuAttribution.Compute decision table, and the new CpuWindowAggregate reads.

Overall: solid design — one shared computation in PerformanceMonitor.Common so the SKUs can't disagree on the math, an explicit and well-justified degrade rule (omit rather than invent), and the decision table is thoroughly pinned identically in both test projects (healthy/low/impossible/slightly-over/no-samples/no-cores/partial-coverage/edge-clamping/zero-CPU/empty-window/rounding — I hand-checked the arithmetic against each assertion and it's all correct). Parameterized SQL throughout, no injection surface. No new missing-index-DMV suggestions needed here since none of this touches indexing.

Two things worth a look, left as inline comments:

1. Lite/Darling drift (Lite/Mcp/McpQueryTools.cs): Darling captures a single now once and reuses it for both the ranking query and the attribution reads, so numerator and denominator are windowed on the exact same instant. Lite instead reuses the already-fetched rows (windowed by an earlier, independent DateTime.UtcNow inside GetTimeRange) as the numerator, but captures a fresh nowUtc for the denominator/ratio math — two different windows, off by however long the ranking query took. Low practical impact (small drift against an hours-long window), but it breaks the PR's stated "same bounds as the rankings" invariant for Lite specifically, and it's exactly the failure mode a comment two lines away calls out.

2. Doc comment typo: %% instead of % in several new XML doc comments (CpuAttribution.cs, DarlingDataReader.cs, both CpuAttributionTests.cs) — cosmetic only, looks like a leftover template-escaping artifact.

Nothing else stood out — instructions docs, CHANGELOG, and MCP tool descriptions are consistently updated across both SKUs.

[erikdarlingdata]

Both round-two points addressed in f1ba666: Lite's nowUtc is now captured BEFORE the ranking read in both tools, shrinking the numerator/denominator window skew from the ranking query's full duration to call-entry overhead (zeroing it entirely would mean threading an instant into the shared ranking read's signature — sub-microsecond drift against an hours-long window doesn't buy that churn); and the %% doc-comment artifacts are fixed to % in all four files.

[claude]

Reviewed the diff (CpuAttribution.cs, both SKUs' DataReader/McpDataTools/McpQueryTools, both instructions docs, both test projects).

Correctness

• CpuAttribution.Compute decision table checks out: window≤0, no samples, no core count, <90% coverage, zero measured CPU, and the low/impossible ratio thresholds are all handled and match the pinned tests. Rounding order (round the emitted numerator, but divide the raw value for the ratio) is correct and deliberately tested (RankedSecondsRoundToOneDecimal_RatioToThree).
• Numerator/denominator window alignment: Darling's ranking read takes explicit start/end so it's exact by construction; Lite's ranking read only takes hoursBack and resolves its own UtcNow internally, so the PR hoists a nowUtc before the read and reuses it for both the ranking call and the CPU aggregate — a small, deliberately-documented skew (call-entry overhead) rather than a real bug.
• sqlserver_cpu_utilization is confirmed to be a 0–100 percentage elsewhere in both codebases, consistent with the /100.0 in Compute.
• Units: Darling sums TotalCpuUs (÷1e6 → seconds), Lite sums TotalCpuMs (÷1e3 → seconds) — both correct for their store's native unit.
• ranked_cpu_seconds numerator is computed post top-N (the SQL reads already apply top/LIMIT) and post parallel_only/min_dop filtering on the queries tool — matches the "RETURNED rows" claim in the PR description and tool descriptions.

Lite/Darling parity

• CpuAttribution is shared in PerformanceMonitor.Common, so both SKUs literally cannot disagree on the math.
• Tool descriptions, McpInstructions/DarlingMcpInstructions tables, and the CpuAttributionTests decision table are updated/pinned identically in both apps (diffed the two test files — only namespace/copyright text differs).
• Both new GetCpuWindowAggregateAsync reads window on collection_time with the same bounds as their respective rankings, and both run concurrently with the (pre-existing) GetLatestServerPropertiesAsync read via Task.WhenAll.

Security / boundaries

• New SQL (CpuWindowAggregateSql in Darling, inline query in Lite) is fully parameterized ($1/$2/$3 via NpgsqlParameter<T>/DuckDBParameter) — no injection surface.

Nothing to flag — this is clean, well-tested, parity-preserving work. No missing-index-DMV suggestions here either way.