Report cpu_window on the top-CPU reads: the attribution denominator (#2320)

[erikdarlingdata]

What

get_top_queries_by_cpu and get_top_procedures_by_cpu (both apps) gain a cpu_window object: measured_sql_cpu_seconds (the collected utilization series' average × core count × window), attributed_cpu_seconds (the returned rows' windowed total), attributed_ratio, and a conditional note. Twice earned on #2235: pre-#2290 the reads explained ~10% of one box and nothing said so; and the Datadog disagreement died the moment its worker_time sum was divided by the box's consumed CPU-seconds and produced 137% — this field is that division as a first-class output.

How

• Shared math (PerformanceMonitor.Common.CpuAttribution): the denominator, the ratio, and two notes — under-half explains where the remainder lives (below the ranking cut, evicted plans, uncached work); past 1.1 is the impossibility/skew detector. Decision table pinned identically in both suites, with Per-query CPU attribution unusable on a plan-churning instance: reads see 18 execs where Datadog sees 43% of the box; literal fragmentation defeats top-N #2235's field numbers (18% × 8 cores × 2h → 10,368 core-seconds; the 14,229s claim → ratio 1.37 → skew note) as the load-bearing cases.
• Omitted, never fabricated: the whole object is null when the denominator can't be trusted — no samples, coverage under half of one-sample-per-minute (MinSampleCoverage), unknown core count, hard-zero average. A missing ratio is honest; a wrong one sends someone chasing CPU that doesn't exist.
• Darling: two small reader helpers (window average+count over cpu_utilization_stats on collection_time; latest COALESCE(vcore_count, cpu_count) from server_properties — the Azure-aware read the FinOps CTE already uses). Lite: mirrors via LocalDataService against v_cpu_utilization_stats on server-local sample_time (that table's clock, same as its existing CPU read) and v_server_properties.
• The attributed sum is over the RETURNED rows (post-filter, post-top) — the ratio answers "what does this ranking explain", not "what does the plan cache hold".
• Tool descriptions updated in both apps.

Verification

Local harness 10/10 over the shared math including both field cases; both SKUs and both test suites build clean. The mirrored CpuAttributionTests run in CI's Windows build.

🤖 Generated with Claude Code

[claude]

Correctness: a failure fetching cpu_window now fails the whole tool call, not just the new field.

rows (the query-stats data) has already been fetched successfully at this point, and the "unavailable" empty-result case is handled above. But GetCpuWindowAverageAsync/GetLatestCpuCountAsync run inside the same try with no local guard — if either throws (transient connection blip, timeout, a lock on cpu_utilization_stats/server_properties, etc.), the outer catch at the bottom of this method turns what would have been a perfectly good get_top_queries_by_cpu response into a hard error, even though none of the actual query-stats data was at fault.

That's the opposite of this feature's stated design ("omitted, never fabricated" — cpu_window: null on any data problem). An exception while gathering the denominator should degrade the same way a legitimately-missing denominator does (attribution = null), not take down rows that were already successfully retrieved. Same pattern recurs at the get_top_procedures_by_cpu call site below (~line 590), and mirrored in Lite's McpQueryTools.cs.

Suggest wrapping just the attribution fetch/compute in its own try/catch (log-and-continue with attribution = null) rather than letting it share fate with the row fetch.

[erikdarlingdata]

Both catches taken in 663638f, as one helper per SKU (TryComputeCpuWindowAsync): the two reads run under Task.WhenAll (they're independent — one round-trip of latency, not two) inside a try/catch that collapses any failure to a null cpu_window — the rows are already fetched, and a data problem in the DENOMINATOR is precisely the omitted-never-fabricated case this feature documents. Lite gets the identical shape (its reads each open their own DuckDB connection, so the concurrency is safe). Covers the Lite twin comment and the Task.WhenAll note in the same commit.

[claude]

Correctness: same issue as the Darling counterpart — GetCpuWindowAverageAsync/GetLatestCpuCountAsync run unguarded inside the outer try, sharing fate with the already-successful rows fetch above. An exception here (e.g. a transient DuckDB read error on v_cpu_utilization_stats/v_server_properties) fails the entire get_top_queries_by_cpu response instead of just omitting cpu_window, which contradicts the "omitted, never fabricated" contract this feature documents for data problems — an exception isn't a data problem the caller should see as a fetch failure. Same pattern at the procedures call site (~line 146-147).

[claude]

Minor perf: these two reads (GetCpuWindowAverageAsync, GetLatestCpuCountAsync) are independent of each other and could run concurrently via Task.WhenAll instead of two sequential awaits — saves one Postgres round-trip of latency on every get_top_queries_by_cpu/get_top_procedures_by_cpu call. Same opportunity in Lite's McpQueryTools.cs. Not a big deal at this scale, just a free win since neither query depends on the other's result.

[claude]

Reviewed the cpu_window attribution feature (#2320). Overall this is well-built: CpuAttribution.Compute is pure and side-effect-free, the null-vs-fabricate contract is enforced correctly at every boundary (no samples, sub-floor coverage, unknown/zero core count, zero-average, degenerate window all return null), the coverage-floor and note-threshold boundaries are tested precisely (including the exact-floor case), all new SQL is properly parameterized (no injection surface), and the two SKUs are in lock-step — CpuAttributionTests.cs is byte-identical between Darling.Tests and Lite.Tests apart from the namespace, the tool description strings match word-for-word, the JSON shape matches field-for-field, and the ms/µs conversions line up.

Left two inline notes:

• Correctness (both apps, 4 call sites): the two new attribution queries run unguarded inside the same try as the already-successful row fetch, so a transient error fetching CPU utilization or core count (not a data-availability problem, an actual exception) now fails the whole get_top_queries_by_cpu/get_top_procedures_by_cpu response instead of just omitting cpu_window. That's a real regression in robustness for a tool that previously didn't depend on those tables at all, and it cuts against the feature's own "omitted, never fabricated" design intent.
• Minor perf (both apps): the average-CPU and core-count reads are independent and currently run as two sequential awaits; Task.WhenAll would save a round-trip.

Nothing else stood out — window math (windowStart/hours_back alignment), the hours_back validation bounds, and the post-filter/post-top attribution semantics (explicitly called out in the PR description) all check out.

[claude]

catch (Exception) here swallows everything — a bad connection string, a typo in the SQL, a schema mismatch, an OOM — with no log line anywhere. The rest of these tools surface exceptions to the caller via McpHelpers.FormatError, so this is the one path in the file where an actual bug becomes permanently invisible.

That cuts against the feature's own stated contract: "a missing ratio is honest; a wrong one sends someone chasing CPU that doesn't exist." A cpu_window: null caused by a real defect is not honest — it's indistinguishable from the legitimate "coverage too thin" case, and nothing in logs or the response will ever hint that the two reads are silently failing every time. Worth at least a debug-level log of the exception before returning null, so a systemic failure (e.g. server_properties never populated, or a Postgres permission issue) doesn't look identical to a healthy "not enough data yet" server forever.

Same pattern exists in Lite/Mcp/McpQueryTools.cs (line 29) — both should probably log, for parity.

[erikdarlingdata]

Agreed — a null born of a throw masquerading as thin coverage breaks the honesty the contract claims. e89268a: the catch now logs a warning with the exception ("if this repeats, the attribution reads have a defect"). Plumbing detail worth recording: the MCP host clears its logging providers, so an injected ILogger<T> from ITS factory would be a black hole — the host now registers the SERVICE's logger instance into the tool DI, and the two tool methods take it as an injected parameter. The web dispatch's call sites pass NullLogger.Instance with a comment saying why (that host's providers are cleared too, and both paths run the same reads, so a systemic failure still surfaces via the MCP path's logger).

[claude]

Same broad catch (Exception) { return null; } as the Darling twin (DarlingMcpDataTools.cs around line 450) — no log, no trace, nothing to distinguish a real bug in GetCpuWindowAverageAsync/GetLatestCpuCountAsync from a legitimately untrustworthy denominator. See the parallel comment on the Darling side for the detail; flagging here too so a fix keeps both SKUs in parity.

[erikdarlingdata]

Same commit — Lite's catch logs through AppLogger.Warn (the app's rotating file logger), same wording, parity kept.

[claude]

Review

Reviewed the cpu_window addition to get_top_queries_by_cpu / get_top_procedures_by_cpu. Overall this is solid — went through the shared CpuAttribution math, both readers, and both MCP tool call sites in detail.

What checks out:

• CpuAttribution.Compute unit math is correct (verified the Per-query CPU attribution unusable on a plan-churning instance: reads see 18 execs where Datadog sees 43% of the box; literal fragmentation defeats top-N #2235 field numbers by hand: 18% × 8 cores × 2h × 3600 = 10,368 measured core-seconds; 14,229s attributed → ratio 1.372, matches the skew-note test).
• Lite.Tests/CpuAttributionTests.cs and Darling/Darling.Tests/CpuAttributionTests.cs are byte-identical apart from the namespace — good parity discipline on the pinned decision table.
• The core-count read (COALESCE(vcore_count, cpu_count)) matches the existing FinOps utilization CTE in both LocalDataService.FinOps.Utilization.cs and ViewerDataService.FinOps.Utilization.cs exactly — no drift from the established Azure-aware convention.
• Lite's new GetCpuWindowAverageAsync correctly windows on sample_time via GetTimeRangeServerLocal (matching the existing GetCpuUtilizationAsync for the same table), while the row queries window on collection_time (UTC) via GetTimeRange — different clocks, but each is the right one for its own table, so the two windows still cover the same real time span.
• attributed_cpu_seconds is summed over the same post-filter, post-top row set that's actually returned (kept/rows), not the full unfiltered result — matches the PR's stated intent ("what does this ranking explain", not "what does the plan cache hold").
• Unit conversions are consistent: Darling's TotalCpuUs / 1000.0 (→ ms) then /1000.0 again inside Compute (→ s) nets to Us/1e6; Lite's TotalCpuMs already-computed property lines up the same way.
• No SQL injection surface — all new queries are parameterized ($1/$2/$3 via Npgsql, $1/$2/$3 via DuckDBParameter).

One finding posted inline (both files, for parity): TryComputeCpuWindowAsync in both DarlingMcpDataTools.cs and Lite/Mcp/McpQueryTools.cs catches bare Exception and returns null with no logging anywhere. That means a real bug in the two attribution reads (bad connection, schema drift, a typo) becomes permanently indistinguishable from a legitimately untrustworthy denominator — which undercuts the feature's own "omitted, never fabricated" honesty claim, since an omission caused by a defect isn't actually honest. Suggest at least a debug-level log of the caught exception before returning null.

Nothing else stood out — no missing-index suggestions (per instructions), no secrets/network/process concerns, and I didn't find other Lite/Darling drift beyond the one item above.

[claude]

NullLogger.Instance silently swallows every warning TryComputeCpuWindowAsync logs when a cpu_window computation throws — which is exactly the "review find" this PR's third commit set out to fix ("Log the degrade-to-null path so a defect can't hide as thin coverage"). That fix only reaches the MCP path.

The comment justifies this by saying a DI-resolved ILogger here "would be a black hole anyway" because DarlingMcpHostService/the web builder calls builder.Logging.ClearProviders(). That's true for a logger resolved from the web host's own ASP.NET DI container, but DarlingWebHostService (the class that owns this web host) has its own working ILogger<DarlingWebHostService> _logger field that's used throughout the file for LogWarning/LogError/LogCritical and is unaffected by ClearProviders(). The MCP host solves this identically for its own path — DarlingMcpHostService.cs registers builder.Services.AddSingleton<ILogger>(_logger) so the service's real logger flows into TryComputeCpuWindowAsync. The web host never threads _logger through MapAll → BuildReadDispatch, so the exact same fix wasn't applied here.

Net effect: a systemic defect in the attribution reads (the scenario this logging was added to catch) is fully invisible when triggered via GET /api/read/get_top_queries_by_cpu or get_top_procedures_by_cpu — it degrades to cpu_window: null with no trace anywhere, forever. Suggest passing _logger into DarlingWebEndpoints.MapAll and threading it to these two dispatch entries instead of NullLogger.Instance.

[erikdarlingdata]

The NullLogger call sites are gone entirely — e1f35d1 replaced parameter injection with a host-set static ambient (DegradeLogger), so the web dispatch reverted to its original shape and logs through the same sink as everything else.

[claude]

This uses AppLogger.Warn(source, message), which only takes a string — there's no overload to attach ex itself, so this call captures only ex.Message and discards the stack trace and any inner exceptions. That undercuts the comment's own stated goal ("if this repeats, the attribution reads have a defect") — a repeating failure with no stack trace is much harder to root-cause than one with it.

AppLogger already has Error(string source, string message, Exception? ex = null) specifically for this — it logs the message, the full stack trace, and recursively unwraps inner/aggregate exceptions. It's the only other exception-logging call site in Lite/Mcp/ (McpHostService.cs:143), so Warn with a message-only interpolation is a departure from the established pattern here, and also drops fidelity relative to Darling's mirror (logger.LogWarning(ex, "...", serverId), which preserves the full exception object for whatever sink is attached).

Suggest AppLogger.Error("McpQueryTools", "cpu_window computation failed — omitting the field; if this repeats, the attribution reads have a defect.", ex) instead.

[erikdarlingdata]

Taken in 4733c03 — AppLogger gains Warn(source, message, Exception) sharing Error's full emission (stack, recursive inners, aggregates) via an extracted level-parameterized helper, so the severity stays truthful AND the diagnosis keeps its stack.

[claude]

Review: cpu_window attribution denominator (#2320)

Reviewed the shared CpuAttribution math, both apps' reader helpers, and the tool wiring. The core formula checks out — verified by hand against the pinned #2235 field numbers (18% × 8 cores × 2h → 10,368 measured core-seconds; 14,229s attributed → 1.372 ratio → skew note), and the omit-vs-fabricate contract (Compute returning null on missing average / thin coverage / unknown cores / degenerate window / hard-zero average) is correct and identically pinned in both CpuAttributionTests.cs copies. SQL access throughout (Npgsql $1/$2/$3 params, DuckDB $1/$2/$3 params) is properly parameterized — no injection surface. The windowHours passed into Compute matches the actual query window in both apps, and hours_back is validated (McpHelpers.ValidateHoursBack) before it reaches the window math, so the negative/zero-window edge cases can't occur. Concurrency (Task.WhenAll over the two independent reads) is safe on both sides — Npgsql's NpgsqlDataSource pools per-command connections, and Lite's DuckDB reads go through a ReaderWriterLockSlim-backed read lock that already permits concurrent readers.

Two inline findings, both about the round-2 review fix ("log the degrade-to-null path so a defect can't hide as thin coverage") not fully reaching both entry points:

1. Darling — DarlingWebEndpoints.cs: the /api/read/get_top_{queries,procedures}_by_cpu REST mirrors pass NullLogger.Instance instead of the service's real logger, so any exception in TryComputeCpuWindowAsync is now silently discarded when triggered via the web dashboard's REST surface — even though the MCP path fixed exactly this by wiring the service's _logger through DI. The web host has an equally-working _logger field sitting unused one file over.
2. Lite — McpQueryTools.cs: the equivalent catch logs via AppLogger.Warn(source, message) with only ex.Message interpolated, losing the stack trace AppLogger.Error(source, message, ex) would have preserved — and Error is the pattern the only other exception-logging call site in Lite/Mcp/ already uses.

Neither is a data-correctness bug (the field itself still degrades to null correctly either way), but both quietly undercut the exact diagnostic the PR's own commit history describes adding.

No Lite/Darling parity drift found beyond the above — field names, rounding, and JSON shape (measured_sql_cpu_seconds, attributed_cpu_seconds, attributed_ratio, note) match exactly between the two cpu_window projections, and both tool descriptions were updated identically.

[claude]

The "systemic attribution failure still surfaces there [via MCP]" premise doesn't hold: mcp.enabled is OFF by default (see DarlingMcpHostService.cs's class doc: "Gated by darling.json's mcp.enabled (default OFF...)"). On a default install, the web dashboard's /api/read/* mirror is the only path that ever calls GetTopQueriesByCpu/GetTopProceduresByCpu, and NullLogger.Instance here means a systemic defect in TryComputeCpuWindowAsync degrades to cpu_window: null with zero log trace anywhere, on every call, forever — exactly the failure mode the "Logged, not silent (the round-2 review catch)" comment in DarlingMcpDataTools.cs was added to prevent.

Also, the "a resolved ILogger here would be a black hole anyway" reasoning conflates two different loggers: DarlingWebHostService has its own working _logger field (ILogger<DarlingWebHostService>, injected from the outer host, not the inner builder whose providers get cleared) — the exact same shape as DarlingMcpHostService._logger, which is threaded through via AddSingleton<ILogger>(_logger) a few lines away in DarlingMcpHostService.cs. The same fix is available here: thread _logger from DarlingWebHostService through MapAll(app, postgres, logger) → BuildReadDispatch(logger) instead of hardcoding NullLogger.Instance.

[erikdarlingdata]

Sharp catch, and it SURVIVED the redesign in a mutated form: with the ambient logger set only by the MCP host, a default install (mcp.enabled OFF) would still have been silent on the web-only path. 4733c03 sets DegradeLogger in DarlingWorker's constructor — the one component that always runs — with the mcp-off rationale in the comment; the MCP host's assignment stays as a harmless second writer of the same sink family.

[claude]

Review

Focused on the new cpu_window attribution feature (shared PerformanceMonitor.Common/CpuAttribution.cs, both apps' get_top_queries_by_cpu/get_top_procedures_by_cpu).

Verified as correct:

• CpuAttribution.Compute's decision table (coverage floor, zero-denominator guard, note thresholds) matches its pinned test suite exactly, and the two CpuAttributionTests.cs files are byte-identical decision tables aside from namespace, confirming no drift between Darling and Lite.
• Unit conversions check out: Darling's TotalCpuUs / 1000.0 (µs→ms) feeding Compute's internal /1000.0 (ms→s) nets to µs→s correctly; Lite's already-ms TotalCpuMs skips the first conversion consistently.
• The attributed-CPU sum is taken over the returned (post-filter, post-top) row set in both apps (kept/rows), matching the "what does this ranking explain" framing in the PR description.
• The 1-sample-per-minute cadence assumption behind MinSampleCoverage is confirmed against both apps' actual collector schedules (Lite/config/collection_schedule.json, install/04_create_schedule_table.sql, and Darling's CollectorHealthClassifierTests.cs comment).
• TryComputeCpuWindowAsync's catch-all-and-degrade-to-null is intentional and matches the "omitted, never fabricated" contract; the two reads run concurrently via Task.WhenAll, and Lite's DuckDB reader/writer lock is a ReaderWriterLockSlim that permits concurrent readers, so no contention there.
• DI wiring for the new ILogger tool parameter in Darling follows the same established pattern as the existing NpgsqlDataSource/DarlingAnalysisService DI parameters.
• No SQL injection risk — all new reader queries are fully parameterized ($1/$2/$3 / DuckDBParameter).
• No missing-index DMV suggestions here, nothing security-sensitive introduced (no new file/process/network surface).

One issue flagged inline: in DarlingWebEndpoints.cs, the /api/read/get_top_queries_by_cpu and /api/read/get_top_procedures_by_cpu handlers pass NullLogger.Instance, reasoning that the MCP path's logger covers systemic failures — but mcp.enabled is OFF by default, so on a default Darling install the web dashboard mirror is the only caller, and a real defect in the attribution reads would silently and permanently degrade to cpu_window: null with no log trace anywhere. DarlingWebHostService already has a working _logger (same shape as DarlingMcpHostService._logger, which is threaded through for the MCP path) — threading it through MapAll/BuildReadDispatch would close the gap cheaply.

No other Lite/Darling parity drift found — the two SKUs' tool bodies, JSON envelope shapes, and error-handling/logging idioms (static AppLogger vs ILogger) mirror each other correctly for this change.

[claude]

Reviewed this PR (correctness, Lite/Darling parity, security, performance).

Summary: no blocking issues found.

Things I specifically checked and verified correct:

• Unit consistency: attributedCpuMs (worker_time, already an aggregate core-ms across parallel workers) and measuredSeconds = avgPct/100 × cores × hours × 3600 (also aggregate core-seconds) are the same physical quantity — the ratio math is dimensionally sound, and the field-case numbers in CpuAttributionTests (10,368 measured / 3,000 attributed → 0.289; 14,229 attributed → ratio > 1.3) check out by hand.
• Coverage floor semantics: MinSampleCoverage = 0.5 assumes ~1 sample/min. Confirmed both collectors backfill against a watermark from the SQL Server ring buffer (not just "1 poll = 1 row"), so this holds even under Lite's "Low-Impact" 5-minute preset or a widened Darling frequency_minutes — as long as the polling gap stays under ring-buffer retention. Only breaks down under an admin-set interval so wide it exceeds ring-buffer retention, in which case nulling the ratio is the correct behavior (real gaps → real distrust of the denominator), not a bug.
• Time-window alignment: Lite's new GetCpuWindowAverageAsync uses GetTimeRangeServerLocal (server-local sample_time) while the row query it's attributing against uses GetTimeRange (UTC collection_time) — at first glance a mismatch, but both resolve to the same real-world instant just expressed in each table's native clock, matching the pre-existing GetCpuUtilizationAsync convention. Not a bug.
• Parity: CpuAttribution math, decision thresholds, and the two new reader methods are mirrored exactly between Lite and Darling (same SQL shape modulo store dialect, same TryComputeCpuWindowAsync catch-and-degrade wrapper, same JSON shape/field names, identical test files). No drift found.
• Security: all new SQL is fully parameterized ($1/$2/$3 in Darling, DuckDBParameter in Lite), no string concatenation into query text.
• Failure isolation: the attribution reads are wrapped in try/catch so a denominator-side failure can't fail the whole tool call (rows are already fetched); this was called out in the diff as a "review catch," and it's implemented consistently in both apps, including logging so a systemic failure doesn't silently look identical to legitimate thin coverage.

No missing-index suggestions per the repo's convention (none applicable here anyway — no new SQL Server DDL/queries against user tables).

[claude]

expectedSamples hardcodes a 1-sample-per-minute cadence (windowHours * 60.0), but cpu_utilization's collection frequency is user-configurable per server — Lite's ScheduleManager (Lite/Services/ScheduleManager.cs:146, :578) lets a user set FrequencyMinutes for any collector including cpu_utilization (default 1, but changeable), and Darling's StoreConfigProvider.ResolveSchedule reads a config_collector_schedules override for the same purpose.

If a server's cpu_utilization cadence is slowed to, say, 5 minutes, samplesInWindow will permanently sit at ~1/5 of expectedSamples, so MinSampleCoverage (0.5) is never met and cpu_window silently and permanently disappears for that server — even though collection is perfectly healthy at its configured cadence. That's a real usability regression for exactly the case ScheduleManager exists to support (reducing collection overhead on busy servers), and it fails silently (per the "omit, never fabricate" design) so nothing will surface it to the user.

Worth deriving the expected-sample baseline from the server's actual configured cadence for cpu_utilization (threaded through from ScheduleManager/config_collector_schedules) rather than a hardcoded 60/hour, or at minimum noting the limitation in the tool description so a customized cadence doesn't read as "attribution unsupported."

[erikdarlingdata]

Right — any assumed frequency bakes today's default into the gate and permanently disqualifies a legitimately slowed server. f63893a makes the gate cadence-agnostic: the readers return the sampled SPAN (MAX minus MIN sample time) alongside avg+count, and Compute now requires the span to cover ≥ half the window (MinSpanCoverage) plus a bare MinSamples = 3 floor so two lonely points can't bracket a wide span. A 5-minute-cadence server (24 samples spanning a full 2h window) now qualifies exactly like a 1-minute one — pinned in both suites, with the slow-cadence case named as the review catch.

[claude]

Review summary

Reviewed the diff (shared CpuAttribution math + both apps' MCP wiring + the new decision-table test pins). This is a well-scoped change and the Lite/Darling parity discipline is genuinely good here: the ratio math lives once in PerformanceMonitor.Common, the decision table is pinned identically in both test suites, the tool-description text is byte-identical, the JSON shape (measured_sql_cpu_seconds / attributed_cpu_seconds / attributed_ratio / note) matches field-for-field, and both readers use the Azure-aware COALESCE(vcore_count, cpu_count) consistently. The degrade-to-null exception handling and logging (round-2/round-4 catches already baked in) look correct in both SKUs, the SQL is properly parameterized in both the Postgres and DuckDB readers (no injection surface), and hours_back/top are validated before use in every path I traced.

One correctness edge case worth a look, left as an inline comment on PerformanceMonitor.Common/CpuAttribution.cs:57: the coverage floor (MinSampleCoverage) assumes a fixed 1-sample-per-minute cpu_utilization cadence (windowHours * 60.0), but that cadence is user-configurable in both apps (Lite's ScheduleManager, Darling's config_collector_schedules override). A server whose cpu_utilization collector has been slowed down will never clear the coverage floor and will silently lose cpu_window forever, even though collection is healthy at its configured rate — a quiet usability regression for exactly the customization path ScheduleManager exists to support.

Everything else checked out: no missing-index DMV suggestions applicable (no T-SQL changed in this PR — it's all C#), no secrets/file/process/network additions, and the two independent reads per tool call run concurrently via Task.WhenAll against connection-pooled sources (Npgsql NpgsqlDataSource, DuckDB read-locked connections), which is safe as implemented.

🤖 Generated with Claude Code

[claude]

Correctness / Lite-Darling parity drift: cpu_window's two halves can be windowed over different real-world time spans in Lite.

GetCpuWindowAverageAsync windows sample_time via GetTimeRangeServerLocal(hoursBack, null, null), which resolves serverNow from the global ambient ServerTimeHelper.UtcOffsetMinutes — set only when a WPF tab is manually selected (MainWindow.xaml.cs ServerTabControl_SelectionChanged), defaulting to the desktop machine's own local timezone if no tab has ever been switched. It is not derived from the serverId parameter this method receives.

Meanwhile the numerator, GetTopQueriesByCpuAsync/GetTopProceduresByCpuAsync, windows collection_time via GetTimeRange — true UTC, independent of any ambient state (ServerManager.GetConnectionStatus(serverId).UtcOffsetMinutes is the per-server value that actually exists but isn't used here).

ServerResolver.Resolve (Lite/Mcp/ServerResolver.cs) picks the target server purely by server_name/single-enabled-server, with zero connection to which UI tab happens to be active. So for any multi-server install where the MCP-queried server's timezone differs from whichever tab is currently selected (or the app hasn't had a tab manually clicked since launch), measured_sql_cpu_seconds is computed over a sample_time window shifted by the wrong UTC offset relative to the attributed_cpu_seconds window — producing a cpu_window/attributed_ratio that silently compares two different real-world time periods. No exception is thrown, so the TryComputeCpuWindowAsync try/catch safety net doesn't catch it; it just produces a plausible-looking but wrong ratio, which is precisely the failure mode (#2235-style) this whole feature was built to eliminate.

Darling doesn't have this problem — DarlingDataReader.GetCpuWindowAverageAsync windows on collection_time (true UTC) for both series consistently, so the two apps' correctness guarantees for cpu_window diverge here.

Worth resolving the server's own UTC offset (e.g., via ServerManager.GetConnectionStatus(serverId)) instead of reading the ambient ServerTimeHelper.UtcOffsetMinutes, the same way the numerator's query avoids the ambient state entirely by using true UTC.

[erikdarlingdata]

Confirmed and fixed in 0ac0625, by removing the offset dance rather than parameterizing it: v_cpu_utilization_stats also carries collection_time (Lite's own AnomalyDetector already windows it against DateTime.UtcNow), so the denominator read now windows collection_time in TRUE UTC — the same clock the stats numerator uses — and the span is measured on the same column. No ambient state, no per-server offset plumbing, and both halves of the ratio cover the same real-world period by construction. The charting read above keeps its server-local sample_time window untouched — that one is per-tab by design.

[claude]

Review summary

Reviewed the cpu_window attribution feature (#2320) — shared CpuAttribution math in PerformanceMonitor.Common, both apps' MCP tool wiring, and the mirrored test suites.

What's solid:

• CpuAttribution.Compute's omit-vs-fabricate contract is correctly implemented and thoroughly pinned — the sample-count floor, span-coverage gate (deliberately cadence-agnostic per the "review catch" comments), zero-average guard, and the two note thresholds all check out against the Per-query CPU attribution unusable on a plan-churning instance: reads see 18 execs where Datadog sees 43% of the box; literal fragmentation defeats top-N #2235 field numbers.
• Darling.Tests/Lite.Tests CpuAttributionTests.cs are byte-identical, satisfying the "same decision table in both suites" requirement.
• The exception-swallowing TryComputeCpuWindowAsync wrappers (never fail the tool call, log instead of silently degrading) are mirrored correctly between apps, including the DegradeLogger static-ambient reasoning in Darling and the equivalent AppLogger.Warn path in Lite.
• SQL parameterization is clean throughout ($1/$2... placeholders, no string concatenation) — no injection surface.
• AppLogger.cs's LogWithException extraction is a clean, behavior-preserving refactor.
• Unit math checks out: Darling's r.TotalCpuUs / 1000.0 and Lite's r.TotalCpuMs (TotalCpuUs / 1000.0) both feed attributedCpuMs correctly; measured_sql_cpu_seconds and attributed_cpu_seconds land in the same units before the ratio.

One correctness/parity issue found, posted inline on Lite/Services/LocalDataService.Cpu.cs:70: Lite's GetCpuWindowAverageAsync windows the CPU-utilization series (the ratio's denominator) using the global ambient ServerTimeHelper.UtcOffsetMinutes — which tracks whichever WPF tab a human last clicked, not the serverId the MCP tool was actually called with — while the numerator windows on true UTC collection_time. For a multi-server install (or before any tab has been manually selected), the two halves of the ratio can cover different real-world time spans, silently producing a wrong-but-plausible attributed_ratio for exactly the scenario this feature exists to prevent. Darling doesn't have this issue since it windows both series on true UTC collection_time consistently — so this is also a Lite/Darling correctness-guarantee divergence, not just a Lite-only bug.

No missing-index DMV suggestions, no other Lite/Darling drift, no security concerns beyond the above.

[claude]

Nit: this entry's coverage description is stale relative to the shipped gate. It says the object is omitted on "coverage under half of one-per-minute", but PerformanceMonitor.Common/CpuAttribution.cs no longer uses a count-per-minute expectation at all — that was replaced (per the code's own "round-6 review catch" comments) with a cadence-agnostic span check: samples must span at least half the window (MinSpanCoverage = 0.5) and there must be at least MinSamples = 3 of them. A server whose cpu_utilization cadence was slowed to e.g. 5 minutes now correctly still qualifies, which is the opposite of what this changelog line implies. Worth updating the wording so the changelog doesn't describe a gate the code doesn't have.

[erikdarlingdata]

0b535b4 — the entry now describes the shipped gate (three-sample floor + span under half the window, explicitly cadence-agnostic).

[claude]

Reviewed the final diff (base dev → head). This PR has clearly already been through several rounds of self-review (visible in the 8 commits' "review finds"/"review catch" messages), and it shows — the shared CpuAttribution.Compute math checks out against its test table (verified the #2235 field numbers by hand: 18% × 8 cores × 2h → 10,368 core-seconds; 3,000s attributed → 0.289 ratio; 14,229s attributed → 1.372 ratio → skew note), the trust-gate boundary tests (span-at-floor, sample-floor, hard-zero average) all match the implementation, and Lite/Darling parity looks solid:

• Both SKUs window the denominator on the exact same UTC clock the numerator query uses (collection_time in Postgres, collection_time in DuckDB — not the server-local sample_time the CPU chart uses), which the code comments call out was itself a review catch (round 6) to prevent a multi-server UTC-offset mismatch.
• Both SKUs sum the attribution numerator over the same post-filter, post-top row set, never fail the tool call on a denominator read error (degrade to cpu_window: null, logged), and run the two independent reads concurrently.
• Both SKUs' CpuAttributionTests.cs are effectively identical decision tables, as intended.
• No SQL injection surface — all reads are parameterized ($1/$2/$3 in Postgres, $1 positional in DuckDB). No new secrets/file/network/process handling.
• Confirmed the DuckDB concurrent-read pattern in Lite's new TryComputeCpuWindowAsync (two connections opened via Task.WhenAll) is safe under the existing ReaderWriterLockSlim-based read lock, which already has same-thread-recursion handling.

One documentation nit posted inline: CHANGELOG.md's new entry still describes the old "coverage under half of one-per-minute" sample-count gate, but the shipped CpuAttribution.cs replaced that with a cadence-agnostic span check during this same PR's review — worth a wording pass so the changelog doesn't describe a rule the code no longer enforces.

Nothing else rose to the level of a blocking finding.

[claude]

COUNT(*) counts every row in the window regardless of whether sqlserver_cpu_utilization is NULL — but AVG(sqlserver_cpu_utilization) silently skips NULLs. The column is nullable in the schema (cpu_utilization_stats.sqlserver_cpu_utilization integer, no NOT NULL), so Samples here can overstate how many actual readings back the average.

That matters because Samples is exactly what CpuAttribution.MinSamples (and, indirectly, the caller's trust in the average) is gating on. If a stretch of collection produced rows with a non-null collection_time but a null sqlserver_cpu_utilization (e.g. a partial collector failure that still logged a heartbeat row), this reports a "well-supported" sample count while the average is actually backed by far fewer real readings — exactly the "omit rather than fabricate" failure mode this whole feature exists to prevent.

Today's single collector (CpuUtilizationCollector) happens to always populate a non-null value, so this is dormant rather than live — but the schema allows it, and the coverage gate should count what it claims to count. Suggest COUNT(sqlserver_cpu_utilization) instead of COUNT(*). Same issue in the Lite mirror (Lite/Services/LocalDataService.Cpu.cs, GetCpuWindowAverageAsync).

[erikdarlingdata]

f652660 — fixed with one predicate instead of a COUNT-column swap: AND sqlserver_cpu_utilization IS NOT NULL in the WHERE, so COUNT, the SPAN, and the AVG all rest on exactly the same rows (COUNT(column) alone would still have let NULL-valued rows stretch the span).

[claude]

Same COUNT(*) vs COUNT(sqlserver_cpu_utilization) issue as the Darling mirror (DarlingDataReader.GetCpuWindowAverageAsync) — sqlserver_cpu_utilization is nullable, and COUNT(*) doesn't reflect how many rows actually contributed to the AVG. See the note on the Darling side for the full scenario; flagging here too since both readers would need the fix to stay in parity.

[erikdarlingdata]

Same commit, same predicate on the Lite mirror.

[claude]

This pins the pure CpuAttribution.Compute() math well, but nothing in either suite exercises the actual wiring that produces cpu_window through the real tool call — grep -rn "cpu_window" across the repo only matches the two production files (DarlingMcpDataTools.cs, McpQueryTools.cs), never a test. GetCpuWindowAverageAsync/GetLatestCpuCountAsync (both apps) are likewise never referenced from a test.

Concretely, DarlingMcpDataToolsTests' full-lifecycle test plants only one cpu_utilization_stats row (PlantCpuAsync), which is below CpuAttribution.MinSamples (3) — so cpu_window is always null on that path, and the test can't tell whether the SQL, unit conversion (µs→ms→s), or JSON field wiring is actually correct. A bug in any of those (wrong column, wrong table, a units slip) would ship silently since TryComputeCpuWindowAsync swallows all exceptions into "omit the field."

Worth adding at least one test per app that plants ≥3 CPU samples + a server_properties/v_server_properties row and asserts the populated cpu_window object's actual values (measured/attributed/ratio) round-trip through get_top_queries_by_cpu or get_top_procedures_by_cpu.

[erikdarlingdata]

Fair — and the vcore COALESCE was exactly the kind of thing that would have hidden behind the degrade catch if the column hadn't existed. f652660 wires it: the gated live-PG round-trip now plants three CPU samples spanning 23h (meeting the gate's floors for the default window) and asserts through the REAL tool call that cpu_window is non-null, measured_sql_cpu_seconds equals the planted 40% × cpu_count 16 × 24h exactly, the ratio is below half, and the remainder note fires. A reader-SQL typo now fails the suite instead of nulling silently.

[claude]

Review summary

Reviewed the cpu_window attribution feature (#2320). Overall this is a well-built, carefully cross-checked change — shared math in PerformanceMonitor.Common/CpuAttribution.cs with an identical decision table pinned in both Darling.Tests and Lite.Tests, consistent unit handling (µs→ms→s) between the two readers, parameterized SQL throughout (no injection risk), the "omit rather than fabricate" contract enforced consistently, and good attention to prior review rounds (UTC-vs-server-local window, cadence-agnostic span coverage, logger wiring for the mcp.enabled=false path). No missing-index recommendations, no secrets, no Lite/Darling drift in the tool descriptions, JSON shape, or rounding.

Two things worth a look, left as inline comments:

1. COUNT(*) vs COUNT(sqlserver_cpu_utilization) in both DarlingDataReader.GetCpuWindowAverageAsync and LocalDataService.GetCpuWindowAverageAsync — the sample-count feeding CpuAttribution.MinSamples counts all rows in the window, not rows with a non-null CPU reading. sqlserver_cpu_utilization is nullable in the schema, so this can overstate confidence in a thin/partial average. Dormant today (the current collector never writes a null), but the coverage gate should count what it claims to count given the schema allows it.
2. Test coverage gap: nothing exercises the actual DB-backed cpu_window wiring end-to-end — CpuAttributionTests only pins the pure Compute() math, and the existing integration test plants just one CPU sample (below MinSamples), so cpu_window is always null there. A bug in the SQL/unit conversion/JSON wiring wouldn't be caught by anything in either suite.

Neither blocks correctness of what's shipped today, but both are worth addressing given how much rigor the rest of the PR puts into exactly this kind of edge case.

[claude]

Parity gap: no end-to-end test for these new reads on the Lite side.

Darling added an integration test (DarlingMcpDataToolsTests.cs, the GetTopQueriesByCpu assertions around cpu_window) that exercises GetCpuWindowAverageAsync/GetLatestCpuCountAsync against the real Postgres schema, with the comment explicitly noting why: "a column typo would hide behind the degrade-to-null catch forever, which is why this exists."

That failure mode applies identically here — GetCpuWindowAverageAsync/GetLatestCpuCountAsync query v_cpu_utilization_stats/v_server_properties directly, and any mismatch (column rename, view change) would be swallowed by TryComputeCpuWindowAsync's catch block in McpQueryTools.cs and silently degrade to cpu_window: null, logged only. Lite already has the fixture infrastructure to catch this (SharedDuckDbFixture, used by McpTopQueriesDistinctTextsTests.cs to call McpQueryTools.GetTopQueriesByCpu end-to-end), but no equivalent cpu_window assertion was added anywhere in Lite.Tests. Only the pure-math CpuAttributionTests.cs (identical in both suites) covers this feature on the Lite side — the wiring itself is untested.

Worth adding a planted-CPU-samples case to an existing DuckDB-fixture-backed test (or a new one) that asserts cpu_window actually computes through the real tool call, mirroring what Darling did.

[claude]

Review summary

Went through the shared CpuAttribution math, both apps' reader methods (DarlingDataReader/LocalDataService.Cpu.cs), the MCP tool wiring in DarlingMcpDataTools.cs/McpQueryTools.cs, the logger-degrade plumbing, and both test suites.

Overall: solid. The core design (omit-never-fabricate, span-based coverage gate that's cadence-agnostic, identical decision table pinned in both CpuAttributionTests.cs files, concurrent independent reads via Task.WhenAll, exception isolation so a denominator failure never fails the tool call) is sound and the Lite/Darling implementations mirror each other closely — units line up (TotalCpuMs in Lite is already ms-denominated via TotalCpuUs / 1000.0; Darling divides explicitly), the windowStart/now anchor is shared between the row fetch and the attribution read in both apps, and the attribution sum is correctly computed over post-filter returned rows for get_top_queries_by_cpu (after parallel_only/min_dop filtering) in both apps.

One gap flagged inline: Darling added an end-to-end integration test asserting cpu_window computes correctly through the real tool call against the live Postgres schema (explicitly to catch a column typo that the degrade-to-null catch would otherwise hide forever). Lite has the equivalent fixture infrastructure (SharedDuckDbFixture) already in use elsewhere for this exact tool, but no matching cpu_window end-to-end assertion was added — only the shared pure-math unit tests cover Lite's side of this feature. See inline comment on Lite/Services/LocalDataService.Cpu.cs.

No correctness bugs, security issues, or other parity drift found.