query_stats and procedure_stats rank before they render (#1959)

[erikdarlingdata]

The field investigation's validated rewrite, applied to both collectors and both variants each. Full mechanism, measurements, and caveats in #1959 and the CHANGELOG entry. Shape summary: derived table ranks on cheap DMV columns (query_stats inner TOP 300 for post-ranking self-filter headroom; procedure_stats exact), applies run against survivors only, procedure_stats' single outer render round-trips the handle via CONVERT(varbinary(64), ..., 1) so no raw column threads through the three branches and the stored payload is untouched. Survivors-only ordering pinned structurally in both definition suites (watched-red names the violation); both plan-on variants executed VERBATIM on SQL Server 2025 via the reflection-extracted emitted text. The SQL matrix on this PR is the four-engine proof.

🤖 Generated with Claude Code

[claude]

Nit: this test name (...SplicesWholeModuleTextPlanIntoEveryBranch) is now stale. Per the updated body/comment, the whole point of #1959 is that the render happens once, outside the ranked derived table — no longer "into every branch". Worth renaming (e.g. ...SplicesWholeModuleTextPlanOnceAfterRank) so the name doesn't contradict the assertions right below it.

[claude]

Test-coverage gap: AssertAppliesRunAgainstSurvivorsOnly is only invoked from the capturePlanXml: true (Darling) tests (here and in the Azure plan-on test). But the property it pins — dm_exec_sql_text and the NOT LIKE self-filter running after the ranked derived table closes — applies unconditionally, including Lite's default plan-off path (BuildQuery_Standard_JoinsPlanAttributes_WithExclusions / BuildQuery_Azure_SkipsPlanAttributes_RunsPerDatabase above). Since StandardQueryText/AzureSqlDbQueryText are shared verbatim between both flag states this isn't a live bug today, but a regression that broke the ordering only under CapturePlanXml = false would slip through untested. Worth calling the helper from those tests too.

[claude]

Review: query_stats / procedure_stats rank-before-render (#1959)

Overview

This applies the same structural fix to both QueryStatsCollector and ProcedureStatsCollector in the shared PerformanceMonitor.Collectors project: rank on cheap DMV columns inside a derived table first, and run the expensive applies (dm_exec_sql_text, dm_exec_text_query_plan, the self-filter) against the survivors only, instead of letting them execute for every row the TOP later discards.

Lite/Darling parity

No drift — PerformanceMonitor.Collectors is the single shared implementation referenced by both Lite/PerformanceMonitorLite.csproj and Darling's storage/service/analysis projects, so this fix lands in both apps atomically. I didn't find any leftover duplicate copy of this query in Lite/Services/RemoteCollectorService.* (that logic was already extracted, per the class doc comments).

Correctness

• query_stats: the derived table alias is deliberately kept as qs so SelectColumnsText and the plan fragments splice unchanged — verified the column set survives via qs.* (real DMV column names), and that the self-filter (st.text NOT LIKE) and dm_exec_sql_text/dm_exec_text_query_plan applies now sit after ) AS qs closes, confirmed against both the Standard and Azure variants.
• procedure_stats: the plan-handle round-trip via CONVERT(varbinary(64), ranked.plan_handle, 1) from the varchar(130) hex string is correct — style 1 is a lossless, symmetric hex round-trip, and the value is never truncated (130 chars exactly fits 0x + 128 hex digits for a varbinary(64)).
• Dynamic-SQL quote-escaping is untouched by the new lines (no single quotes introduced into the nested @sql string), and the plan fragments themselves contain no quotes, so they splice safely into both the escaped dynamic-SQL body and the plain Azure variant.
• The one behavior change worth naming explicitly: query_stats' inner TOP (300) headroom (up from an implicit "no cap" pre-PR) means self-filtered rows now compete for a bounded ranked set rather than being excluded before ranking. The PR body already discloses and measures this (99.2% row-identity parity, residual attributed to cache churn) — flagging only so reviewers know it's a deliberate, measured trade-off and not an oversight.

Style

Matches the T-SQL conventions in CONTRIBUTING.md (uppercase keywords, lowercase non-abbreviated types, column = expr aliasing, /* */ block comments, aligned WHERE/AND).

Minor findings (posted inline)

1. ProcedureStatsCollectorDefinitionTests.BuildQuery_PlanCaptureOn_Standard_SplicesWholeModuleTextPlanIntoEveryBranch — the test name is now stale; the fix's entire point is that the render happens once, not "into every branch."
2. The new AssertAppliesRunAgainstSurvivorsOnly structural check in QueryStatsCollectorDefinitionTests is only exercised from the capturePlanXml: true (Darling) tests. Since the underlying query template is shared verbatim with the plan-off (Lite default) path, this isn't a live bug, but it does mean the ordering guarantee for Lite's actual default path isn't independently pinned.

No security, injection, or missing-index concerns. Solid, well-tested change — the field-measurement rigor described in the PR body is reflected in the structural test assertions added here.

[claude]

Minor/optional: the inner TOP (300) headroom over the outer TOP (200) is a fixed constant sized for the self-filter (st.text NOT LIKE '%PerformanceMonitorLite%') to remove enough rows post-rank without starving the final 200. Since total_elapsed_time is a cumulative counter, a monitoring query polling very frequently (e.g. sub-10s interval against a fleet with many databases) could rank unusually high purely from execution volume, and if more than ~100 of the top-300-by-elapsed rows turn out to be self-noise, the final result silently comes back short of 200 rather than backfilling from the next-ranked candidates — no error, just fewer/different rows than before this change.

The CHANGELOG documents this tradeoff and cites 99.2% row-identity parity from field validation, so this reads as an accepted, measured risk rather than an oversight — flagging only in case a wider headroom (or a TOP (200) outer count no longer aiming for exactness) is worth a comment noting the failure mode is "quietly fewer rows," not a crash.

[claude]

Review summary

What this does: Rewrites QueryStatsCollector and ProcedureStatsCollector (shared code in PerformanceMonitor.Collectors, consumed by both Lite and Darling) so the expensive per-row applies — dm_exec_sql_text, the self-filter, and the Darling-only dm_exec_text_query_plan render — run after an inner derived table ranks candidates on cheap DMV columns and applies TOP, instead of being spliced below the TOP where the optimizer wasn't deferring them. Both the standard and Azure SQL DB variants of both collectors get the same treatment. CHANGELOG entry and #1959 link included.

Parity: Because the fix lives entirely in the shared PerformanceMonitor.Collectors project (not duplicated per-app), Lite and Darling get it identically by construction — no drift risk here. The CapturePlanXml flag continues to gate the Darling-only render via the same /*PLAN_SELECT*///*PLAN_APPLY*/ splice points, and BuildQuery_PlanCaptureOffByDefault_NoPlanClauses_LiteParity still pins Lite's SQL as byte-identical to the no-plan form.

Correctness — traced in detail, no bugs found:

• QueryStatsCollector: the inner derived table selects qs.* (raw types straight from sys.dm_exec_query_stats), so the existing PlanApplyFragment (qs.plan_handle, unconverted) still binds correctly against the derived table aliased back to qs — no type mismatch. The database_name alias added in the inner SELECT doesn't collide with any real DMV column. The re-aliasing of the derived table back to qs (shadowing the base-table alias used inside the subquery) is valid T-SQL scoping, not a bug, and is deliberate (per the code comment) so SelectColumnsText and the plan fragments splice unchanged.
• ProcedureStatsCollector: plan_handle is already CONVERTed to varchar(130) inside the ranked derived table (unlike query_stats), so the new PlanApplyFragment correctly round-trips it back via CONVERT(varbinary(64), ranked.plan_handle, 1) — style-1 hex round-trip verified correct (produces/consumes the 0x + 128 hex-char form). sql_handle is untouched, matching the stated "no raw column threads through" design. Column ordinals in ReadAsync are unaffected in both files since the outer SELECT lists are unchanged in order/count.
• Ordinal/column-count tests (PayloadColumns_MatchSchema*, ReadAsync/WritePayload tests) weren't touched and don't need to be — confirms the shape change is purely about query structure, not payload shape.

Tests: The new structural pinning assertions (AssertAppliesRunAgainstSurvivorsOnly, the )ASranked/)ASqs ordering checks) are well-targeted — they test the actual defect class (apply position relative to the TOP) rather than just presence/absence of a fragment, which is exactly what would have caught the original bug. Good use of the existing Collapse() helper pattern already used elsewhere in these test files.

One thing worth a second look (posted inline): query_stats' inner TOP (300) headroom over the outer TOP (200) is a fixed constant. total_elapsed_time is cumulative, so a monitoring query polling very frequently could rank surprisingly high by volume; if self-noise ever exceeds the 100-row headroom in the top 300, the final result comes back short of 200 rather than backfilling — a quiet behavior change, not a crash. The CHANGELOG documents this as a measured, accepted tradeoff (99.2% field parity), so this is a flag for awareness rather than a blocker.

Out of scope but related (not required for this PR): install/08_collect_query_stats.sql and install/10_collect_procedure_stats.sql (the separate full-Dashboard product, referenced in this PR's own code comments as what the plan-render logic "mirrors") still render dm_exec_text_query_plan/apply text per-row with no ranking-before-render structure — a related but architecturally different exposure (unbounded by cutoff time rather than TOP-then-discard) that this PR correctly doesn't touch since it's neither Lite nor Darling. Might be worth a follow-up issue if that product is still maintained.

No security, injection, or secrets concerns — all changes are static SQL text templates; existing parameterization (DatabaseExclusionFilter) is untouched. No missing-index recommendations offered, per repo convention.