Skip to content

Plan analysis: run the benefit scorer at every entry point and show each finding's benefit (#4546) - #4552

Merged
erikdarlingdata merged 3 commits into
devfrom
fix/4546-run-benefit-scorer
Sep 28, 2026
Merged

erikdarlingdata merged 3 commits into
devfrom
fix/4546-run-benefit-scorer

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4546
Part of #4511

Why

BenefitScorer.Score existed but no product code called it: every entry point ran only PlanAnalyzer.Analyze. So no finding had a benefit value, and nothing the scorer computes was ever used.

What changes

Impact on stored advisories and alerts

This was checked before merge.

  • The trace: plan findings reach Darling's analysis only as one PLAN_WARNING fact. FactScorer scores it presence-only (0.4), which is below the 0.75 Warning band and the 1.5 notification threshold. critical_count is read only by the advice headline text. Nothing reads MaxBenefitPercent outside the scorer, the MCP output and the viewer.
  • Measured on 60 real showplans: the per-severity finding counts and finding types are identical with and without this change, because the scorer annotates existing findings and adds none itself. This change raises no new alert and moves no health band.
  • Deep plans: the scorer's recursion was measured safe on a 1 MB thread at the parser's 1,000-level limit (about 15× margin; see Plan analysis: a deeply nested plan no longer crashes the process that parses it (#4512) #4551).

Test plan

  • BenefitScorerWiringTests:
    • a census pin: no product file outside PlanAnalysisPipeline calls PlanAnalyzer.Analyze( directly;
    • through McpPlanAnalysisFormatter, a Serial Plan finding carries a non-null max_benefit_percent. Runtime RED on dev: KeyNotFoundException on max_benefit_percent, since dev's JSON has no such key;
    • findings are ordered by benefit, with null last;
    • an empty plan passes through untouched (Run_EmptyPlan_DoesNotThrow_AndLeavesPlanUnchanged).
  • PlanViewerBenefitDisplayTests: the header with a benefit (⚠ Serial Plan — up to 67.5% benefit, plus [SQL Server] for engine warnings), the header without one, and the ordering 80, 20, null. These are compile-only RED on dev, because PlanWarningDisplay is new.
  • Mutation: removing the Score call from PlanAnalysisPipeline.Run fails the MCP benefit fact.
  • Run: Total: 239, Failed: 0, Skipped: 2 (the live-Postgres plan-tool class runs in CI). Lite.Tests and PerformanceMonitor.Ui build.

Also in this PR (test-only): PgWaitSamplerLiveTests.OneCycleAgainstAStockTarget_… now reads query_id and picks its own lock row (the one with the most samples) instead of asserting there is exactly one (Lock, relation) row. The sampler polls every session on the test server, so a concurrent test class's own relation-lock wait can add a second row; this PR's new test classes shifted the CI shard layout enough to expose it.

CHANGELOG

SECTION: Fixed
ENTRY: - Plan analysis now scores each finding's benefit, and the plan viewer and MCP plan tools show it ([#4552]) - The benefit scorer was never run, so no finding had a benefit value. Findings are now ordered by their estimated benefit. The viewer's headers read "— up to X% benefit", and the MCP plan tools report max_benefit_percent. This turns on the Serial Plan and Bare Scan benefit estimates.
REF: [#4552]: #4552

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 04:09
@erikdarlingdata
erikdarlingdata merged commit 586b27a into dev Sep 28, 2026
27 of 30 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4546-run-benefit-scorer branch September 28, 2026 04:09
erikdarlingdata added a commit that referenced this pull request Sep 28, 2026
…e waits get a real benefit (#4555)

Plan analysis turns a plan's wait statistics into findings, and gives external and preemptive waits a real benefit. Fixes #4516, fixes #4517, part of #4511.

- BenefitScorer.ScoreWaitStats, matching PerformanceStudio, adds a "Wait: <type>" finding for each significant wait type in an actual plan, with a severity tier from its estimated benefit and a description from WaitStats.json. The file ships as an embedded resource of the shared plan-analysis assembly, so Darling, Lite and the viewer read the same copy.
- External and preemptive waits get PerformanceStudio's separate benefit formula instead of being folded into operator time.
- GetOperatorMaxThreadOwnCpuMs matches PerformanceStudio: it skips thread 0 and looks through batch mode zones and Compute Scalar pass-throughs.
- The findings appear now that the scorer runs at every entry point (#4552). They reach only the plan warning fact and the advice headline's critical count, not any alert or health band.
- Tests: PlanSync4516Tests, PlanSync4517Tests and PlanSync4517ProbeTests, failing without the change; two mutations fail them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant