diff --git a/.github/scripts/New-DependencySnapshot.ps1 b/.github/scripts/New-DependencySnapshot.ps1 new file mode 100644 index 00000000..3e6ec418 --- /dev/null +++ b/.github/scripts/New-DependencySnapshot.ps1 @@ -0,0 +1,209 @@ +#Requires -Version 7.0 +<# +.SYNOPSIS + Builds a GitHub dependency snapshot from the obj/project.assets.json files NuGet wrote. + +.DESCRIPTION + Reads every obj/project.assets.json under the repository and writes the JSON body for + POST /repos/{owner}/{repo}/dependency-graph/snapshots. No third-party tool is involved: the + assets file already holds the exact package versions NuGet resolved, transitive packages + included, and which of them the project references directly. + + The resolved map is keyed by package URL and dependencies list package URLs, as in the + snapshots GitHub accepted from the component-detection action. Packages in tests/ projects + get scope 'development'; every other project's packages get 'runtime'. The assets file does + not mark build-time packages in product projects (ILLink, the WASM DevServer, + VSSDK.BuildTools), so they are listed as 'runtime' too. That only affects alert triage. + + One manifest per project per target framework. The manifest is named after the project file + (relative to the repository root, forward slashes), with the target framework appended in + parentheses only when a project has more than one target. + + Fails when it finds no assets file or resolves no package at all, so a broken restore cannot + replace the dependency graph with an empty snapshot. + +.PARAMETER Sha + Commit SHA the snapshot describes (github.sha). + +.PARAMETER Ref + Fully qualified ref, e.g. refs/heads/main (github.ref). + +.PARAMETER RunId + Workflow run id (github.run_id). + +.PARAMETER Correlator + Stable string that makes a new snapshot replace the previous one from the same source. + +.PARAMETER OutputPath + Where to write the snapshot JSON. + +.PARAMETER RepoRoot + Repository root. Defaults to two levels above this script. +#> +[CmdletBinding()] +param( + [Parameter(Mandatory)][string]$Sha, + [Parameter(Mandatory)][string]$Ref, + [Parameter(Mandatory)][string]$RunId, + [Parameter(Mandatory)][string]$Correlator, + [string]$OutputPath = 'snapshot.json', + [string]$RepoRoot = (Join-Path $PSScriptRoot '..\..') +) + +$ErrorActionPreference = 'Stop' + +$root = (Resolve-Path -LiteralPath $RepoRoot).Path.TrimEnd('\', '/') +$rootPrefix = $root.Replace('\', '/') + '/' + +function Get-RelativePath([string]$fullPath) { + $normalized = $fullPath.Replace('\', '/') + if ($normalized.StartsWith($rootPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { + return $normalized.Substring($rootPrefix.Length) + } + return $normalized +} + +# Assets files under .claude/, tmp-*/ and bin/ are scratch copies, not part of the product. +$assetsFiles = Get-ChildItem -LiteralPath $root -Recurse -Filter 'project.assets.json' -File | + Where-Object { $_.Directory.Name -eq 'obj' } | + Where-Object { + $rel = Get-RelativePath $_.FullName + $rel -notmatch '(^|/)(\.claude|bin|tmp-[^/]*)/' + } | + Sort-Object FullName + +if (-not $assetsFiles) { + throw "No obj/project.assets.json found under $root. Did the restore steps run?" +} + +# package_url values: percent-encode the name and version (a version can carry '+' build metadata). +function Get-PackageUrl([string]$name, [string]$version) { + 'pkg:nuget/{0}@{1}' -f [uri]::EscapeDataString($name), [uri]::EscapeDataString($version) +} + +# Split a "Name/Version" key at the last slash. +function Split-LibraryKey([string]$key) { + $i = $key.LastIndexOf('/') + if ($i -lt 1) { return $null } + [pscustomobject]@{ Name = $key.Substring(0, $i); Version = $key.Substring($i + 1) } +} + +$manifests = [ordered]@{} +$totalResolved = 0 + +foreach ($file in $assetsFiles) { + $assets = Get-Content -LiteralPath $file.FullName -Raw | ConvertFrom-Json + + # NuGet writes the assets file even when the restore failed, with the packages it could + # resolve and an Error entry in logs. A partial graph must not replace the full one. + $restoreErrors = @($assets.logs | Where-Object { $_.level -eq 'Error' }) + if ($restoreErrors.Count -gt 0) { + throw "$($file.FullName): restore logged $($restoreErrors.Count) error(s), first $($restoreErrors[0].code). The snapshot would be incomplete." + } + + $projectPath = $assets.project.restore.projectPath + if ([string]::IsNullOrWhiteSpace($projectPath)) { + Write-Warning "Skipping $($file.FullName): no project.restore.projectPath." + continue + } + $projectFile = Get-RelativePath $projectPath + # Packages in tests/ projects are development; everything else is reported as runtime, + # build-only tools in product projects included (the assets file does not mark them). + $scope = $(if ($projectFile -like 'tests/*') { 'development' } else { 'runtime' }) + + # The direct references: names listed for the target framework, plus the framework-less group. + $directByFramework = @{} + foreach ($group in $assets.projectFileDependencyGroups.PSObject.Properties) { + $names = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($line in @($group.Value)) { + if ([string]::IsNullOrWhiteSpace($line)) { continue } + [void]$names.Add(($line.Trim() -split '\s+', 2)[0]) + } + $directByFramework[$group.Name] = $names + } + + # A restore with runtime identifiers adds "tfm/rid" targets that repeat the plain "tfm" target's + # packages; keep one manifest per framework and drop the runtime-specific copies. + $allTargets = @($assets.targets.PSObject.Properties) + $plainNames = @($allTargets | Where-Object { $_.Name -notlike '*/*' } | ForEach-Object { $_.Name }) + $targets = @($allTargets | Where-Object { $_.Name -notlike '*/*' -or ($plainNames -notcontains ($_.Name -split '/', 2)[0]) }) + foreach ($target in $targets) { + # A runtime-specific target is keyed "tfm/rid"; its direct references sit under "tfm". + $framework = ($target.Name -split '/', 2)[0] + $direct = [System.Collections.Generic.HashSet[string]]::new([System.StringComparer]::OrdinalIgnoreCase) + foreach ($key in @($framework, $target.Name, '')) { + if ($directByFramework.ContainsKey($key)) { $direct.UnionWith($directByFramework[$key]) } + } + + # Pass 1: every package NuGet resolved for this target. + $packages = [ordered]@{} + foreach ($entry in $target.Value.PSObject.Properties) { + if ($entry.Value.type -ne 'package') { continue } + $id = Split-LibraryKey $entry.Name + if ($null -eq $id) { continue } + $packages[$id.Name] = [pscustomobject]@{ Version = $id.Version; Entry = $entry.Value } + } + + # Pass 2: build the resolved map, keyed by package URL (the shape GitHub accepted from the + # previous action). Dependencies are package URLs of packages in this same map. + $purlByName = @{} + foreach ($name in $packages.Keys) { $purlByName[$name] = Get-PackageUrl $name $packages[$name].Version } + + $resolved = [ordered]@{} + foreach ($name in $packages.Keys) { + $pkg = $packages[$name] + $purl = $purlByName[$name] + $dependsOn = [System.Collections.Generic.List[string]]::new() + if ($pkg.Entry.PSObject.Properties['dependencies']) { + foreach ($dep in $pkg.Entry.dependencies.PSObject.Properties) { + # $purlByName is case-insensitive, like NuGet package ids. + if ($purlByName.ContainsKey($dep.Name) -and $purlByName[$dep.Name] -ne $purl) { + if (-not $dependsOn.Contains($purlByName[$dep.Name])) { $dependsOn.Add($purlByName[$dep.Name]) } + } + } + } + $resolved[$purl] = [ordered]@{ + package_url = $purl + relationship = $(if ($direct.Contains($name)) { 'direct' } else { 'indirect' }) + scope = $scope + dependencies = [string[]]$dependsOn.ToArray() + } + } + + if ($resolved.Count -eq 0) { continue } + + $manifestName = $projectFile + if ($targets.Count -gt 1) { $manifestName = "$projectFile ($($target.Name))" } + + $manifests[$manifestName] = [ordered]@{ + name = $manifestName + file = [ordered]@{ source_location = $projectFile } + resolved = $resolved + } + $totalResolved += $resolved.Count + Write-Host ("{0}: {1} packages" -f $manifestName, $resolved.Count) + } +} + +if ($manifests.Count -eq 0 -or $totalResolved -eq 0) { + throw "Found $($assetsFiles.Count) assets file(s) but no resolved packages; refusing to write an empty snapshot." +} + +$snapshot = [ordered]@{ + version = 0 + sha = $Sha + ref = $Ref + job = [ordered]@{ correlator = $Correlator; id = $RunId } + detector = [ordered]@{ + name = 'performancestudio-assets-snapshot' + version = '1.0.0' + url = 'https://github.com/erikdarlingdata/PerformanceStudio' + } + scanned = (Get-Date).ToUniversalTime().ToString('yyyy-MM-ddTHH:mm:ssZ') + manifests = $manifests +} + +$json = $snapshot | ConvertTo-Json -Depth 10 +[System.IO.File]::WriteAllText([System.IO.Path]::GetFullPath($OutputPath, (Get-Location).Path), $json, [System.Text.UTF8Encoding]::new($false)) + +Write-Host ("Wrote {0}: {1} manifests, {2} resolved packages from {3} assets files." -f $OutputPath, $manifests.Count, $totalResolved, $assetsFiles.Count) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b1ba26d..00249a59 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,6 +40,7 @@ jobs: - 'src/PlanViewer.Core/**' - 'src/PlanViewer.Web/**' - 'server/PlanShare/**' + - 'Directory.Build.props' - 'src/Directory.Build.props' - 'Directory.Packages.props' - 'tests/**' diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 56cbf429..23558ade 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -56,7 +56,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 + - uses: anthropics/claude-code-action@f27be6b65a4652f08a2ba3cff79114829b588967 # v1.0.252 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} prompt: | diff --git a/.github/workflows/claude.yml b/.github/workflows/claude.yml index ccd3be23..9b183873 100644 --- a/.github/workflows/claude.yml +++ b/.github/workflows/claude.yml @@ -36,7 +36,7 @@ jobs: with: fetch-depth: 1 - - uses: anthropics/claude-code-action@1d6de8cb0c237e7c15e9e1bdf973826ebae490cc # v1.0.248 + - uses: anthropics/claude-code-action@f27be6b65a4652f08a2ba3cff79114829b588967 # v1.0.252 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} claude_args: | diff --git a/.github/workflows/dependency-submission.yml b/.github/workflows/dependency-submission.yml new file mode 100644 index 00000000..59858e1f --- /dev/null +++ b/.github/workflows/dependency-submission.yml @@ -0,0 +1,138 @@ +name: Dependency submission + +# Dependabot's dependency graph cannot read versions from Directory.Packages.props (central +# package management) and no project has a packages.lock.json, so the graph lists direct +# packages with no version and no transitive packages at all. That means no Dependabot +# alerts for a vulnerable transitive package (issue #650). This workflow restores the projects +# and submits what NuGet actually resolved, transitive packages included. +# +# Two jobs, so the token that can write to the repository never sits next to build-time code: +# detect contents: read. Restores the projects (package and MSBuild code runs here) and turns +# the obj/project.assets.json files into a snapshot with +# .github/scripts/New-DependencySnapshot.ps1. +# submit contents: write. Checks out nothing and runs no repository code. It only downloads the +# snapshot artifact and posts it with the gh CLI that ships on the runner (issue #655). +# +# Dependabot alerts read the default branch (main), so transitive alerts start after the next +# dev-to-main release merge. Snapshots from dev feed no alerts; the dev trigger exists to +# exercise this workflow before a release merge. + +on: + push: + branches: [main, dev] + workflow_dispatch: + +permissions: {} + +# A newer push cancels an older run still in progress, so the latest commit's graph is the last +# one submitted and an older sha cannot overwrite it. +concurrency: + group: dependency-submission-${{ github.ref }} + cancel-in-progress: true + +jobs: + detect: + # Windows, so the old-format SSMS add-in project can be restored with MSBuild too. + runs-on: windows-latest + permissions: + contents: read + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Setup .NET 10.0 + uses: actions/setup-dotnet@a98b56852c35b8e3190ac28c8c2271da59106c68 # v6.0.0 + with: + dotnet-version: 10.0.x + + # ci.yml does the same before restoring the solution (Blazor WASM project). + - name: Install WASM workload + run: dotnet workload install wasm-tools + + # The snapshot script reads project.assets.json, which only exists after a restore. Every + # restore here passes -p:NuGetAudit=false: with the audit gate on, a new high advisory fails + # the restore, the snapshot is never submitted, and the graph goes stale exactly when an + # alert is needed. Resolution is unchanged, and GitHub matches advisories on the submitted graph. + - name: Restore solution + run: dotnet restore PlanViewer.sln -p:NuGetAudit=false + + # server/PlanShare is not in PlanViewer.sln and is the internet-facing component. + - name: Restore PlanShare + run: dotnet restore server/PlanShare/PlanShare.csproj -p:NuGetAudit=false + + - name: Add MSBuild to PATH + uses: microsoft/setup-msbuild@30375c66a4eea26614e0d39710365f22f8b0af57 # v3.0.0 + + # pwsh, because the msbuild folder that setup-msbuild adds to PATH reaches pwsh steps but not + # bash steps on the Windows runner (run 38660830104 failed with "msbuild: command not found"). + # pwsh only reports the last command's exit code, so each command checks its own. + - name: Restore SSMS projects + shell: pwsh + run: | + dotnet restore src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } + dotnet restore tests/PlanViewer.Ssms.Installer.Tests/PlanViewer.Ssms.Installer.Tests.csproj -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } + msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -t:Restore -p:Configuration=Release -p:NuGetAudit=false + if ($LASTEXITCODE) { exit $LASTEXITCODE } + + # Fails when there is no assets file or no resolved package, so a broken restore cannot + # replace the graph with an empty snapshot, and when an assets file records a restore error. + # GitHub keeps the latest snapshot per correlator plus detector name. The correlator stays + # "submit" so it is stable for this workflow, but the detector name differs from the old + # action's, so this snapshot does not replace that one: the old action's snapshot exists only + # on dev and stays beside this one (main never received one). + # Values reach the script through env vars, never spliced into the script text. + - name: Build dependency snapshot + shell: pwsh + env: + SNAPSHOT_SHA: ${{ github.sha }} + SNAPSHOT_REF: ${{ github.ref }} + SNAPSHOT_RUN_ID: ${{ github.run_id }} + run: ./.github/scripts/New-DependencySnapshot.ps1 -Sha $env:SNAPSHOT_SHA -Ref $env:SNAPSHOT_REF -RunId $env:SNAPSHOT_RUN_ID -Correlator 'submit' -OutputPath snapshot.json + + - name: Upload snapshot + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: dependency-snapshot + path: snapshot.json + if-no-files-found: error + retention-days: 7 + + submit: + needs: detect + runs-on: ubuntu-latest + permissions: + contents: write + + steps: + - name: Download snapshot + uses: actions/download-artifact@9000827ccba6bdab643e8b6fd33ac0654aef8333 # v8.0.2 + with: + name: dependency-snapshot + + - name: Submit dependency snapshot + env: + GH_TOKEN: ${{ github.token }} + run: | + # sha, ref and run id come from this run, not from the artifact. + jq --arg sha "$GITHUB_SHA" --arg ref "$GITHUB_REF" --arg id "$GITHUB_RUN_ID" \ + '.sha = $sha | .ref = $ref | .job.id = $id' snapshot.json > body.json + # On an HTTP error gh writes the error body to stdout, so print it. + if ! gh api --method POST -H "Accept: application/vnd.github+json" \ + "repos/$GITHUB_REPOSITORY/dependency-graph/snapshots" \ + --input body.json > response.json; then + cat response.json + exit 1 + fi + cat response.json + echo + result=$(jq -r '.result // empty' response.json) + echo "result: $result" + echo "message: $(jq -r '.message // empty' response.json)" + if [ "$result" != "SUCCESS" ] && [ "$result" != "ACCEPTED" ]; then + echo "unexpected result: '$result'" + exit 1 + fi diff --git a/.github/workflows/deploy-planshare.yml b/.github/workflows/deploy-planshare.yml index 2d3876db..2dc3b704 100644 --- a/.github/workflows/deploy-planshare.yml +++ b/.github/workflows/deploy-planshare.yml @@ -8,6 +8,7 @@ on: branches: [main] paths: - 'server/PlanShare/**' + - 'Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/deploy-planshare.yml' workflow_dispatch: diff --git a/.github/workflows/deploy-web.yml b/.github/workflows/deploy-web.yml index 682d9324..271b8595 100644 --- a/.github/workflows/deploy-web.yml +++ b/.github/workflows/deploy-web.yml @@ -6,6 +6,7 @@ on: paths: - 'src/PlanViewer.Core/**' - 'src/PlanViewer.Web/**' + - 'Directory.Build.props' - 'src/Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/deploy-web.yml' diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b95c5a1b..0d9da06f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -118,7 +118,12 @@ jobs: # Those props redirect VSToolsPath into the Microsoft.VSSDK.BuildTools # package, which is how the VSSDK targets resolve on a runner that # lacks the Visual Studio extension-development workload. - msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -restore -t:Build -p:Configuration=Release -p:DeployExtension=false + $msbuildLog = Join-Path $env:RUNNER_TEMP 'ssms-msbuild.log' + msbuild src/PlanViewer.Ssms/PlanViewer.Ssms.csproj -restore -t:Build -p:Configuration=Release -p:DeployExtension=false | Tee-Object -FilePath $msbuildLog + $auditHit = Select-String -Path $msbuildLog -Pattern 'NU1903|NU1904' | Select-Object -First 1 + if ($auditHit) { + Write-Host "::warning::SSMS restore failed the NuGet audit: $($auditHit.Line.Trim())" + } dotnet build src/PlanViewer.Ssms.Installer/PlanViewer.Ssms.Installer.csproj -c Release $vsix = 'src/PlanViewer.Ssms/bin/Release/PlanViewer.Ssms.vsix' diff --git a/.github/workflows/ssms-installer-tests.yml b/.github/workflows/ssms-installer-tests.yml index d0860018..87004f60 100644 --- a/.github/workflows/ssms-installer-tests.yml +++ b/.github/workflows/ssms-installer-tests.yml @@ -13,6 +13,8 @@ on: paths: - 'src/PlanViewer.Ssms.Installer/**' - 'tests/PlanViewer.Ssms.Installer.Tests/**' + - 'Directory.Build.props' + - 'src/Directory.Build.props' - 'Directory.Packages.props' - '.github/workflows/ssms-installer-tests.yml' workflow_dispatch: diff --git a/CITATION.cff b/CITATION.cff index 69569b05..3885e023 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -9,8 +9,8 @@ authors: website: "https://erikdarling.com" repository-code: "https://github.com/erikdarlingdata/PerformanceStudio" license: MIT -version: "1.30.0" -date-released: "2026-10-13" +version: "1.31.0" +date-released: "2026-10-20" keywords: - sql-server - execution-plan diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 00000000..359ebc37 --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,19 @@ + + + + + + all + $(WarningsAsErrors);NU1903;NU1904 + + diff --git a/Directory.Packages.props b/Directory.Packages.props index 94a5a51a..a2f19ebb 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -5,31 +5,36 @@ - + - - - - + + + + - - - - + + + + - - + + - - + + + + @@ -38,9 +43,17 @@ - + + + - + diff --git a/README.md b/README.md index f271cfe9..d822a35d 100644 --- a/README.md +++ b/README.md @@ -512,6 +512,7 @@ Options: --timeout Query timeout (default: 60) --login SQL Server login (bypasses credential store) --password SQL Server password (bypasses credential store) + --config Path to a .planview.json config file (overrides auto-discovery) ``` ### `planview credential` @@ -524,6 +525,62 @@ planview credential remove On Linux, `planview credential` exits with an error. See [Platform Support](#platform-support). +### Configuration file (.planview.json) + +A `.planview.json` file changes what the analyzer reports. The CLI commands `analyze`, `query-store` and `repl` read it. The desktop app reads it each time it opens a plan. The MCP server does not read it and always uses the defaults. + +The file is looked for in this order, and the first one found wins: + +1. The path given to `--config`. If that file does not exist, the command stops with an error. +2. `.planview.json` in the current directory. +3. `.planview.json` in your home directory. + +If none exists, the analyzer uses its defaults. The file is JSON. Comments and trailing commas are allowed. + +```jsonc +{ + "rules": { + "disabled": [3], + "severity_overrides": { "10": "Info" } + }, + "suppressions": [ + { + "query_hash": "0x00E1987CD1C28B4C", + "rule": 10, + "operator": "eventuous.Messages", + "reason": "Library procedure. The library owns the clustered index." + }, + { + "query_hash": "00e1987cd1c28b4c", + "type": "Implicit Conversion", + "reason": "Accepted. A vendor fixes the column type." + } + ] +} +``` + +The `rules.disabled` list holds rule numbers. A disabled rule never runs. + +The `rules.severity_overrides` map gives a rule number a new severity. The allowed values are `Info`, `Warning` and `Critical`. + +The `suppressions` list marks single findings as accepted. Use it when a finding is correct but you cannot or will not act on it. A suppressed finding is not removed. It stays in the output with `"suppressed": true` and its reason. The text output tags it as `[suppressed: ]` and the HTML output shows a `suppressed: ` badge. A suppressed finding keeps its severity, including one set by `severity_overrides`. + +A suppressed finding does not count toward `total_warnings`, `critical_warnings` or `warning_types`. The summary reports it in `suppressed_warnings` instead. The same rule applies to the counts in the desktop app. + +Each suppression has these fields: + +| Field | Required | Meaning | +|-------|----------|---------| +| `query_hash` | Yes | The query hash of the statement. Leading `0x` is optional and case does not matter, so `0x00E1987CD1C28B4C` and `00e1987cd1c28b4c` are the same hash. The suppression applies to that statement only, including statements inside a stored procedure body. | +| `rule` | One of `rule` or `type` | A rule number. It matches findings from that rule. It never matches a warning that SQL Server wrote into the plan, because no rule produced it. | +| `type` | One of `rule` or `type` | A warning type, such as `Implicit Conversion`. The whole type must match and case does not matter. Use it for warnings that have no rule number. If you give both `rule` and `type`, both must match. | +| `operator` | No | Text that must appear in the operator label. Case does not matter. Use the label from the `operator` field of the JSON output, such as `Key Lookup on eventuous.Messages (Node 1)`. A partial match works, so `eventuous.Messages` is enough. A finding on the statement as a whole has no operator, so a suppression that sets `operator` never matches it. | +| `reason` | Yes | Why the finding is accepted. It must not be empty. | + +A suppression that is missing a field is an error. The command prints one line that names the file, for example `Suppression 2 in C:\work\team.json has no "reason".`, and exits with code 1. The number is the zero-based position in the list. The desktop app does not stop. It opens the plan with the defaults and shows the message in the toolbar. + +To find the `query_hash` of a statement, run `planview analyze` with JSON output and read `statements[].query_hash`. Each warning in the JSON also shows its `rule` number when a rule produced it, and its `operator` label when it belongs to an operator. + ## Authentication There are three ways to authenticate, in order of precedence: @@ -582,7 +639,7 @@ The analyzer runs 30 rules against each plan, covering: | Objects | Table variables, table-valued functions, CTE multiple references, eager index spools, lazy spools, row count spools | | Operators | Operator self-time calculation with per-thread awareness for parallel plans | -Rules can be disabled or have their severity overridden via a `.planview.json` config file. See the `--config` option. +Rules can be disabled, have their severity overridden, or have single findings suppressed per statement via a `.planview.json` config file. See [Configuration file](#configuration-file-planviewjson). ## Sponsors diff --git a/src/Directory.Build.props b/src/Directory.Build.props index d2f380d2..60a83bfd 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -1,4 +1,7 @@ + + + - 1.30.0 + 1.31.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index d9904311..4def70d1 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -859,7 +859,7 @@ private void ShowPropertiesPanel(PlanNode node) foreach (var w in sortedPlanWarnings) { var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; - var legacyTag = w.IsLegacy ? " [legacy]" : ""; + var legacyTag = (w.IsLegacy ? " [legacy]" : "") + SuppressedWarningTag(w); var sourceTag = WarningSourceTag(w); var planWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" @@ -945,7 +945,7 @@ murky origin - but until now the only way to see one was to already have clicked ? $" \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : ""; var opHeaderText = - $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{opBenefit}"; + $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{SuppressedWarningTag(w)}{opBenefit}"; var opHeader = new TextBlock { Text = opHeaderText, @@ -1021,7 +1021,7 @@ murky origin - but until now the only way to see one was to already have clicked foreach (var w in sortedNodeWarnings) { var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; - var nodeLegacyTag = w.IsLegacy ? " [legacy]" : ""; + var nodeLegacyTag = (w.IsLegacy ? " [legacy]" : "") + SuppressedWarningTag(w); var nodeSourceTag = WarningSourceTag(w); var nodeWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{nodeSourceTag}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index 2c525959..bae6c6a3 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -19,8 +19,9 @@ public partial class PlanViewerControl : UserControl { private static void CountNodeWarnings(PlanNode node, ref int total, ref int critical) { - total += node.Warnings.Count; - critical += node.Warnings.Count(w => w.Severity == PlanWarningSeverity.Critical); + // Suppressed findings (#651) stay visible but do not count, matching the analysis summary. + total += node.Warnings.Count(w => !w.IsSuppressed); + critical += node.Warnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Critical); foreach (var child in node.Children) CountNodeWarnings(child, ref total, ref critical); } @@ -48,7 +49,7 @@ private void RenderStatement(PlanStatement statement) // Render nodes — pass total warning count to root node for badge var allWarnings = new List(); CollectWarnings(statement.RootNode, allWarnings); - RenderNodes(statement.RootNode, divergenceLimit, allWarnings.Count); + RenderNodes(statement.RootNode, divergenceLimit, allWarnings.Count(w => !w.IsSuppressed)); // Update banners ShowMissingIndexes(statement.MissingIndexes); @@ -569,6 +570,11 @@ cursor and the click below would die in the gaps between words. This is a naviga private static string WarningSourceTag(PlanWarning warning) => warning.Source == PlanWarningSource.SqlServer ? " [SQL Server]" : ""; + private static string SuppressedWarningTag(PlanWarning warning) => + !warning.IsSuppressed ? "" + : string.IsNullOrWhiteSpace(warning.SuppressionReason) ? " [suppressed]" + : $" [suppressed: {warning.SuppressionReason}]"; + private static bool HasSpillInPlanTree(PlanNode node) { foreach (var w in node.Warnings) diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs index 6e6c61ef..03221d38 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs @@ -171,8 +171,8 @@ row.Statement.StatementText and hand out the statement exactly as the plan ElapsedMs = stmt.QueryTimeStats?.ElapsedTimeMs ?? 0, UdfMs = stmt.QueryUdfElapsedTimeMs, EstCost = stmt.StatementSubTreeCost, - Critical = allWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical), - Warnings = allWarnings.Count(w => w.Severity == PlanWarningSeverity.Warning), + Critical = allWarnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Critical), + Warnings = allWarnings.Count(w => !w.IsSuppressed && w.Severity == PlanWarningSeverity.Warning), Statement = stmt }); } diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml b/src/PlanViewer.App/Controls/PlanViewerControl.axaml index f7b1b85c..5fceff86 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml @@ -95,6 +95,12 @@ + + + diff --git a/src/PlanViewer.Core/Services/ConfigLoader.cs b/src/PlanViewer.Core/Services/ConfigLoader.cs index 8eff3b78..f9fadabe 100644 --- a/src/PlanViewer.Core/Services/ConfigLoader.cs +++ b/src/PlanViewer.Core/Services/ConfigLoader.cs @@ -53,8 +53,66 @@ public static AnalyzerConfig Load(string? explicitPath = null) if (configPath == null) return AnalyzerConfig.Default; - var json = File.ReadAllText(configPath); - var config = JsonSerializer.Deserialize(json, JsonOptions); - return config ?? AnalyzerConfig.Default; + /* A file that exists but cannot be read (locked, no permission) is the same kind of + failure as one that will not parse: one line naming the file, caught by every + caller that already catches InvalidDataException. */ + string json; + try + { + json = File.ReadAllText(configPath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + throw new InvalidDataException($"Could not read {configPath}: {ex.Message}", ex); + } + AnalyzerConfig? config; + try + { + config = JsonSerializer.Deserialize(json, JsonOptions); + } + catch (JsonException ex) + { + throw new InvalidDataException($"Could not read {configPath}: {ex.Message}", ex); + } + + if (config == null) + return AnalyzerConfig.Default; + + ValidateSuppressions(config, configPath); + return config; + } + + /// + /// A suppression that cannot match anything, or that hides a finding without saying why, + /// is a mistake worth stopping for. Throws naming the + /// entry by its zero-based index (#651). + /// + private static void ValidateSuppressions(AnalyzerConfig config, string configPath) + { + var entries = config.Suppressions; + if (entries == null) + return; + + for (var i = 0; i < entries.Count; i++) + { + var entry = entries[i]; + var prefix = $"Suppression {i} in {configPath}"; + + if (entry == null) + throw new InvalidDataException($"{prefix} is empty."); + + if (SuppressionEntry.NormalizeHash(entry.QueryHash).Length == 0) + throw new InvalidDataException($"{prefix} has an empty \"query_hash\"."); + + var hasType = !string.IsNullOrWhiteSpace(entry.Type); + if (entry.Rule == null && !hasType) + throw new InvalidDataException($"{prefix} needs \"rule\" or \"type\"."); + + if (entry.Operator != null && string.IsNullOrWhiteSpace(entry.Operator)) + throw new InvalidDataException($"{prefix} has an empty \"operator\"."); + + if (string.IsNullOrWhiteSpace(entry.Reason)) + throw new InvalidDataException($"{prefix} has no \"reason\"."); + } } } diff --git a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs index 67f28216..9408c88b 100644 --- a/src/PlanViewer.Core/Services/NodeTimeAttribution.cs +++ b/src/PlanViewer.Core/Services/NodeTimeAttribution.cs @@ -15,8 +15,9 @@ namespace PlanViewer.Core.Services; /// /// This is the display-grade attribution used for node coloring and the /// properties panel. The analysis pipeline uses the more elaborate per-thread -/// calculation in (GetOperatorOwnElapsedMs / -/// GetOperatorMaxThreadOwnCpuMs); the two are intentionally separate. +/// calculation in (GetOperatorOwnElapsedMs, +/// GetOperatorOwnCpuMs, GetOperatorMaxThreadOwnCpuMs); the two are intentionally +/// separate. /// public static class NodeTimeAttribution { @@ -78,7 +79,9 @@ private static long GetChildCpuMsSum(PlanNode node) long sum = 0; foreach (var child in node.Children) { - if (child.ActualCPUMs > 0) + if ((child.ActualExecutionMode ?? child.ExecutionMode) == "Batch") + sum += SumBatchSubtreeCpuMs(child); // standalone CPU: the zone adds up + else if (child.ActualCPUMs > 0) sum += child.ActualCPUMs; else sum += GetChildCpuMsSum(child); // skip through transparent operators @@ -86,6 +89,27 @@ private static long GetChildCpuMsSum(PlanNode node) return sum; } + /// + /// Sums CPU across a contiguous batch-mode zone, stopping at exchange boundaries. + /// The CPU twin of . + /// + private static long SumBatchSubtreeCpuMs(PlanNode node) + { + long sum = node.ActualCPUMs; + foreach (var child in node.Children) + { + if (child.PhysicalOp == "Parallelism") continue; // zone boundary + + if ((child.ActualExecutionMode ?? child.ExecutionMode) == "Batch") + sum += SumBatchSubtreeCpuMs(child); + else if (child.ActualCPUMs > 0) + sum += child.ActualCPUMs; // row mode: already cumulative + else + sum += GetChildCpuMsSum(child); // pass-through, no stats + } + return sum; + } + private static long GetChildElapsedMsSum(PlanNode node) { long sum = 0; diff --git a/src/PlanViewer.Core/Services/OperatorLabel.cs b/src/PlanViewer.Core/Services/OperatorLabel.cs new file mode 100644 index 00000000..ae57c5fb --- /dev/null +++ b/src/PlanViewer.Core/Services/OperatorLabel.cs @@ -0,0 +1,28 @@ +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// The one place that spells an operator label. The JSON output prints it in the Operator field +/// of a warning, and a suppression's "operator" text is matched against it (#651), so the two +/// share this method and can never drift. +/// +internal static class OperatorLabel +{ + /// + /// Includes the object name for data access operators (scans, seeks, lookups) where it helps + /// identify which table or index is involved. + /// + internal static string Format(PlanNode node) + { + if (!string.IsNullOrEmpty(node.ObjectName)) + { + var objRef = !string.IsNullOrEmpty(node.DatabaseName) + ? $"{node.DatabaseName}.{node.ObjectName}" + : node.ObjectName; + return $"{node.PhysicalOp} on {objRef} (Node {node.NodeId})"; + } + + return $"{node.PhysicalOp} (Node {node.NodeId})"; + } +} diff --git a/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs b/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs index f13b80d5..bfc2a5f4 100644 --- a/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs +++ b/src/PlanViewer.Core/Services/PlanAnalysisPipeline.cs @@ -71,6 +71,8 @@ internal static ParsedPlan AnalyzeParsed( cancellationToken.ThrowIfCancellationRequested(); BenefitScorer.ScoreCancellable(plan, cancellationToken); cancellationToken.ThrowIfCancellationRequested(); + // Last, so the scorer's wait-stats findings can be suppressed too (#651). + SuppressionApplier.Apply(plan, config); return plan; } } diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs index 4ce169fd..8f41ad54 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Timing.cs @@ -53,7 +53,7 @@ private static void FindMemoryConsumersRecursive(PlanNode node, List<(string Lab /// internal static long GetOperatorOwnElapsedMs(PlanNode node) { - if (node.ActualExecutionMode == "Batch") + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") return node.ActualElapsedMs; // Parallel plan with per-thread data: calculate self-time per thread @@ -87,27 +87,32 @@ private static long GetPerThreadOwnElapsed(PlanNode node) => GetPerThreadOwn(node, t => t.ActualElapsedMs, n => n.ActualElapsedMs); private static long GetPerThreadOwn( + PlanNode node, + Func pick, + Func nodeTotal) => + GetPerThreadOwnByThread(node, pick, nodeTotal).Values.DefaultIfEmpty(0).Max(); + + /// + /// Self time on each worker thread: the thread's time minus the same thread's time + /// in the children. Elapsed takes the max of these; total CPU takes the sum. + /// + private static Dictionary GetPerThreadOwnByThread( PlanNode node, Func pick, Func nodeTotal) { - var parentByThread = new Dictionary(); - foreach (var ts in WorkThreads(node)) - parentByThread[ts.ThreadId] = pick(ts); - var childSumByThread = new Dictionary(); foreach (var child in node.Children) AddEffectiveChildByThread(child, pick, nodeTotal, childSumByThread); - var maxSelf = 0L; - foreach (var (threadId, parentMs) in parentByThread) + var selfByThread = new Dictionary(); + foreach (var ts in WorkThreads(node)) { - childSumByThread.TryGetValue(threadId, out var childMs); - var self = Math.Max(0, parentMs - childMs); - if (self > maxSelf) maxSelf = self; + childSumByThread.TryGetValue(ts.ThreadId, out var childMs); + selfByThread[ts.ThreadId] = Math.Max(0, pick(ts) - childMs); } - return maxSelf; + return selfByThread; } /// @@ -197,6 +202,12 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) { if (!node.HasActualStats || node.ActualCPUMs <= 0) return 0; + // Batch mode reports each thread's own CPU directly. Subtracting the + // children would take their standalone CPU off a number that never held it. + // A node with no per-thread rows (built in code, not parsed) keeps its total. + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") + return WorkThreads(node).Select(t => t.ActualCPUMs).DefaultIfEmpty(node.ActualCPUMs).Max(); + if (node.PerThreadStats.Count > 1) return GetPerThreadOwn(node, t => t.ActualCPUMs, n => n.ActualCPUMs); @@ -207,10 +218,43 @@ internal static long GetOperatorMaxThreadOwnCpuMs(PlanNode node) return Math.Max(0, node.ActualCPUMs - totalChildCpu); } + /// + /// The operator's own CPU, children subtracted, summed across threads. CPU sums + /// because every thread burned its own, so this is the figure to set beside + /// QueryTimeStats CpuTime, and self CPU above self elapsed means parallelism. + /// Serial and batch operators get the same value as + /// ; a parallel row-mode operator gets + /// all of its threads instead of the busiest one. + /// + internal static long GetOperatorOwnCpuMs(PlanNode node) + { + if (!node.HasActualStats || node.ActualCPUMs <= 0) return 0; + + if ((node.ActualExecutionMode ?? node.ExecutionMode) == "Batch") + return node.ActualCPUMs; + + if (node.PerThreadStats.Count > 1) + return GetPerThreadOwnByThread(node, t => t.ActualCPUMs, n => n.ActualCPUMs).Values.Sum(); + + var totalChildCpu = 0L; + foreach (var child in node.Children) + totalChildCpu += GetEffectiveChildCpuMs(child); + return Math.Max(0, node.ActualCPUMs - totalChildCpu); + } + private static long GetEffectiveChildCpuMs(PlanNode child) { if (child.PhysicalOp == "Parallelism" && child.Children.Count > 0) return child.Children.Max(GetEffectiveChildCpuMs); + + // Batch CPU is standalone, exactly like batch elapsed, so subtract the whole + // zone. Taking only the child's own value left the rest of the zone in a + // row-mode parent: a serial Stream Aggregate over a batch Sort reported + // 14,771 ms of self CPU next to 3,059 ms of self elapsed. The zone gives 3,112. + var mode = child.ActualExecutionMode ?? child.ExecutionMode; + if (mode == "Batch" && child.HasActualStats) + return SumBatchSubtreeCpuMs(child); + if (child.ActualCPUMs > 0) return child.ActualCPUMs; if (child.Children.Count == 0) @@ -293,6 +337,26 @@ private static long SumBatchSubtreeElapsedMs(PlanNode node) return sum; } + /// + /// The CPU twin of : batch operators report + /// standalone CPU, so a zone's CPU adds up. Stops at exchange zone boundaries. + /// + private static long SumBatchSubtreeCpuMs(PlanNode node) + { + long sum = node.ActualCPUMs; + foreach (var child in node.Children) + { + if (child.PhysicalOp == "Parallelism") continue; + + var childMode = child.ActualExecutionMode ?? child.ExecutionMode; + if (childMode == "Batch" && child.HasActualStats) + sum += SumBatchSubtreeCpuMs(child); + else + sum += GetEffectiveChildCpuMs(child); + } + return sum; + } + /// /// Calculates a Parallelism (exchange) operator's own elapsed time. /// Exchange times are unreliable — they accumulate wait time caused by diff --git a/src/PlanViewer.Core/Services/PlanOperations.cs b/src/PlanViewer.Core/Services/PlanOperations.cs index f7406647..e16a5a82 100644 --- a/src/PlanViewer.Core/Services/PlanOperations.cs +++ b/src/PlanViewer.Core/Services/PlanOperations.cs @@ -312,6 +312,7 @@ public PlanSummaryResult GetSummary(PlanSession session) TotalStatements = analysis.Summary.TotalStatements, TotalWarnings = analysis.Summary.TotalWarnings, CriticalWarnings = analysis.Summary.CriticalWarnings, + SuppressedWarnings = analysis.Summary.SuppressedWarnings, MissingIndexes = analysis.Summary.MissingIndexes, HasActualStats = analysis.Summary.HasActualStats, MaxEstimatedCost = analysis.Summary.MaxEstimatedCost, @@ -427,7 +428,8 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( { var candidate = new RankedOperator(node, statementText); hasActuals |= node.ActualElapsedMs > 0; - AddRanked(topByActual, candidate, top, rankByActuals: true); + if (IsEligibleForOwnTimeRanking(node)) + AddRanked(topByActual, candidate, top, rankByActuals: true); AddRanked(topByCost, candidate, top, rankByActuals: false); }); } @@ -436,7 +438,7 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( return new ExpensiveOperatorsResult { SessionId = session.SessionId, - RankedBy = hasActuals ? "actual_elapsed_ms" : "cost_percent", + RankedBy = hasActuals ? "own_time_ms" : "cost_percent", Operators = ranked.Select(item => new ExpensiveOperatorItem { NodeId = item.Node.NodeId, @@ -447,6 +449,8 @@ internal ExpensiveOperatorsResult GetExpensiveOperators( ActualRows = item.Node.ActualRows ?? 0, ActualElapsedMs = item.Node.ActualElapsedMs ?? 0, ActualCpuMs = item.Node.ActualCpuMs ?? 0, + OwnElapsedMs = item.Node.OwnElapsedMs ?? 0, + OwnCpuMs = item.Node.OwnCpuMs ?? 0, LogicalReads = item.Node.ActualLogicalReads ?? 0, PhysicalReads = item.Node.ActualPhysicalReads ?? 0, ObjectName = useBareObjectNames ? item.Node.BareObjectName : item.Node.ObjectName, @@ -651,6 +655,20 @@ private static void VisitOperators( } } + // Same inclusion rule as the "Expensive operators" list in TextFormatter.CollectNodeTimings: + // exchanges are skipped; with CPU data only nodes with own CPU count, otherwise own elapsed. + private static bool IsEligibleForOwnTimeRanking(OperatorResult node) + { + if (node.PhysicalOp == "Parallelism") + return false; + return node.ActualCpuMs.HasValue + ? (node.OwnCpuMs ?? 0) > 0 + : (node.OwnElapsedMs ?? 0) > 0; + } + + private static long OwnTimeScore(OperatorResult node) => + Math.Max(node.OwnCpuMs ?? 0, node.OwnElapsedMs ?? 0); + private static void AddRanked( List ranked, RankedOperator candidate, @@ -658,13 +676,13 @@ private static void AddRanked( bool rankByActuals) { var candidateScore = rankByActuals - ? candidate.Node.ActualElapsedMs ?? 0 + ? OwnTimeScore(candidate.Node) : candidate.Node.CostPercent; var insertAt = 0; while (insertAt < ranked.Count) { var item = ranked[insertAt]; - var score = rankByActuals ? item.Node.ActualElapsedMs ?? 0 : item.Node.CostPercent; + var score = rankByActuals ? OwnTimeScore(item.Node) : item.Node.CostPercent; if (candidateScore > score) { break; @@ -685,7 +703,12 @@ private static void AddRanked( Message = Truncate(warning.Message, DefaultMaxResponseTextLength), NodeId = warning.NodeId, Operator = warning.Operator is null ? null : Truncate(warning.Operator, 512), - Statement = statement + Statement = statement, + Rule = warning.Rule, + IsSuppressed = warning.IsSuppressed, + SuppressionReason = warning.SuppressionReason is null + ? null + : Truncate(warning.SuppressionReason, DefaultMaxResponseTextLength) }; private static BoundedColumnList BoundColumns(IEnumerable columns) diff --git a/src/PlanViewer.Core/Services/SuppressionApplier.cs b/src/PlanViewer.Core/Services/SuppressionApplier.cs new file mode 100644 index 00000000..294e38a6 --- /dev/null +++ b/src/PlanViewer.Core/Services/SuppressionApplier.cs @@ -0,0 +1,93 @@ +using System; +using System.Collections.Generic; +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// Flags findings that a suppression in .planview.json matches (#651). It runs after the +/// scorer so the scorer's wait-stats findings can be suppressed too. It only sets +/// and : +/// no warning is removed and no severity changes. +/// +internal static class SuppressionApplier +{ + internal static void Apply(ParsedPlan plan, AnalyzerConfig config) + { + var entries = config.Suppressions; + if (entries == null || entries.Count == 0) + return; + + foreach (var stmt in PlanStatements.EnumerateAll(plan)) + { + var hash = SuppressionEntry.NormalizeHash(stmt.QueryHash); + if (hash.Length == 0) + continue; + + var forStatement = new List(); + foreach (var entry in entries) + { + if (entry != null && + string.Equals(SuppressionEntry.NormalizeHash(entry.QueryHash), hash, StringComparison.OrdinalIgnoreCase)) + forStatement.Add(entry); + } + if (forStatement.Count == 0) + continue; + + foreach (var w in stmt.PlanWarnings) + TryApply(w, null, forStatement); + + if (stmt.RootNode != null) + ApplyToTree(stmt.RootNode, forStatement); + } + } + + private static void ApplyToTree(PlanNode root, List entries) + { + var pending = new Stack(); + pending.Push(root); + while (pending.TryPop(out var node)) + { + if (node.Warnings.Count > 0) + { + var label = OperatorLabel.Format(node); + foreach (var w in node.Warnings) + TryApply(w, label, entries); + } + foreach (var child in node.Children) + pending.Push(child); + } + } + + private static void TryApply(PlanWarning warning, string? operatorLabel, List entries) + { + foreach (var entry in entries) + { + if (!Matches(warning, operatorLabel, entry)) + continue; + warning.IsSuppressed = true; + warning.SuppressionReason = entry.Reason; + return; + } + } + + private static bool Matches(PlanWarning warning, string? operatorLabel, SuppressionEntry entry) + { + if (entry.Rule is int rule && warning.RuleNumber != rule) + return false; + + if (!string.IsNullOrWhiteSpace(entry.Type) && + !string.Equals(warning.WarningType, entry.Type.Trim(), StringComparison.OrdinalIgnoreCase)) + return false; + + if (!string.IsNullOrWhiteSpace(entry.Operator)) + { + if (operatorLabel == null) + return false; + if (operatorLabel.IndexOf(entry.Operator.Trim(), StringComparison.OrdinalIgnoreCase) < 0) + return false; + } + + return true; + } +} diff --git a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj index 5d65d330..3da60357 100644 --- a/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj +++ b/src/PlanViewer.Ssms/PlanViewer.Ssms.csproj @@ -83,6 +83,10 @@ + + diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index 1ea67ed3..093173eb 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.30.0.0")] -[assembly: AssemblyFileVersion("1.30.0.0")] +[assembly: AssemblyVersion("1.31.0.0")] +[assembly: AssemblyFileVersion("1.31.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index 6f127183..0718f2d8 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS diff --git a/src/PlanViewer.Web/PlanViewer.Web.csproj b/src/PlanViewer.Web/PlanViewer.Web.csproj index ca4441a3..01ad7981 100644 --- a/src/PlanViewer.Web/PlanViewer.Web.csproj +++ b/src/PlanViewer.Web/PlanViewer.Web.csproj @@ -25,6 +25,7 @@ + diff --git a/tests/PlanViewer.Core.Tests/AdviceCardsTests.cs b/tests/PlanViewer.Core.Tests/AdviceCardsTests.cs index 885adc05..6515b43d 100644 --- a/tests/PlanViewer.Core.Tests/AdviceCardsTests.cs +++ b/tests/PlanViewer.Core.Tests/AdviceCardsTests.cs @@ -183,6 +183,35 @@ public void CardsOpenExpandedUntilThereAreTooManyToRead() }); } + /// + /// #651: a count excludes suppressed warnings, so the statement's "N critical" chip must not + /// count them. The warnings themselves still appear as cards. + /// + [Fact] + public void TheStatementChipsDoNotCountSuppressedWarnings() + { + var result = Analyze(GoldenPlan); + foreach (var type in new[] { "Suppressed Alpha", "Suppressed Beta" }) + { + result.Statements[0].Warnings.Add(new WarningResult + { + Type = type, + Severity = "Critical", + Message = "message", + IsSuppressed = true, + SuppressionReason = "accepted" + }); + } + + HeadlessUi.Run(() => + { + var text = AllText(Show(AdviceContentBuilder.Build("", result))); + + Assert.DoesNotContain("2 critical", text); + Assert.Contains("Suppressed Alpha", text); + }); + } + /// /// The summary counts, as chips. Checked through the pane's text rather than by counting /// Borders: what matters is that a reader sees "1 critical", not how many containers it took. diff --git a/tests/PlanViewer.Core.Tests/ComparisonModelTests.cs b/tests/PlanViewer.Core.Tests/ComparisonModelTests.cs index 087a5a70..8b50a040 100644 --- a/tests/PlanViewer.Core.Tests/ComparisonModelTests.cs +++ b/tests/PlanViewer.Core.Tests/ComparisonModelTests.cs @@ -120,6 +120,19 @@ public void DopIsShownRawWithNoDeltaAndNoVerdict() Assert.Equal(ComparisonDirection.Neutral, metric.Direction); } + /// #651: the Warnings metric is a count, so suppressed warnings are left out of it. + [Fact] + public void TheWarningsMetricDoesNotCountSuppressedWarnings() + { + var withSuppressed = Statement(warnings: 3); + withSuppressed.Warnings[0].IsSuppressed = true; + withSuppressed.Warnings[1].IsSuppressed = true; + + var metric = Metric(Compare(Plan(Statement(warnings: 3)), Plan(withSuppressed)), "Warnings"); + + Assert.Equal("2 resolved", metric.DeltaLabel); + } + [Fact] public void FewerWarningsIsBetterAndMoreIsWorse_WithoutInventingAPercentage() { diff --git a/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs b/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs new file mode 100644 index 00000000..ac962b30 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ConfigLoaderTests.cs @@ -0,0 +1,145 @@ +using System; +using System.IO; +using PlanViewer.Core.Models; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #651: the .planview.json loader. A suppression that cannot do its job, or that hides a finding +/// without saying why, stops the load with one line naming the entry by its zero-based index. +/// +public class ConfigLoaderTests : IDisposable +{ + private readonly string _dir = Path.Combine(Path.GetTempPath(), "planview-config-" + Guid.NewGuid().ToString("N")); + + public ConfigLoaderTests() => Directory.CreateDirectory(_dir); + + public void Dispose() + { + try { Directory.Delete(_dir, recursive: true); } catch (IOException) { } + } + + private string Write(string json) + { + var path = Path.Combine(_dir, ".planview.json"); + File.WriteAllText(path, json); + return path; + } + + [Fact] + public void AValidSuppressionsListRoundTrips() + { + var config = ConfigLoader.Load(Write(""" + { + // comments and trailing commas are allowed + "rules": { "disabled": [3], "severity_overrides": { "10": "Info" } }, + "suppressions": [ + { "query_hash": "0x00E1987CD1C28B4C", "rule": 10, "operator": "eventuous.Messages", "reason": "Library owns the index." }, + { "query_hash": "00e1987cd1c28b4c", "type": "Implicit Conversion", "reason": "Vendor column type." }, + ] + } + """)); + + Assert.True(config.IsRuleDisabled(3)); + Assert.Equal("Info", config.GetSeverityOverride(10)); + + var suppressions = Assert.IsType>(config.Suppressions); + Assert.Equal(2, suppressions.Count); + Assert.Equal("0x00E1987CD1C28B4C", suppressions[0].QueryHash); + Assert.Equal(10, suppressions[0].Rule); + Assert.Equal("eventuous.Messages", suppressions[0].Operator); + Assert.Equal("Library owns the index.", suppressions[0].Reason); + Assert.Null(suppressions[0].Type); + Assert.Equal("Implicit Conversion", suppressions[1].Type); + Assert.Null(suppressions[1].Rule); + Assert.Null(suppressions[1].Operator); + } + + [Fact] + public void AFileWithNoSuppressionsKeyStillLoads() + { + var config = ConfigLoader.Load(Write("""{ "rules": { "disabled": [5] } }""")); + + Assert.True(config.IsRuleDisabled(5)); + Assert.Null(config.Suppressions); + } + + [Fact] + public void AnEmptySuppressionsListLoads() + { + var config = ConfigLoader.Load(Write("""{ "suppressions": [] }""")); + Assert.Empty(config.Suppressions!); + } + + [Theory] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": "ok" }, { "query_hash": "0xAB", "rule": 4, "reason": "ok" }, { "query_hash": "0xAB", "rule": 5 } ] }""", + "Suppression 2 in .planview.json has no \"reason\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": " " } ] }""", + "Suppression 0 in .planview.json has no \"reason\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "reason": "ok" }, { "query_hash": "", "rule": 3, "reason": "ok" } ] }""", + "Suppression 1 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "rule": 3, "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0x", "rule": 3, "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"query_hash\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json needs \"rule\" or \"type\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "type": " ", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json needs \"rule\" or \"type\".")] + [InlineData("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3, "operator": "", "reason": "ok" } ] }""", + "Suppression 0 in .planview.json has an empty \"operator\".")] + [InlineData("""{ "suppressions": [ null ] }""", + "Suppression 0 in .planview.json is empty.")] + public void AnInvalidSuppressionFailsWithOneLineNamingTheEntry(string json, string expected) + { + // The message names the file that was actually read, not the default file name. + var path = Write(json); + var ex = Assert.Throws(() => ConfigLoader.Load(path)); + Assert.Equal(expected.Replace("in .planview.json", $"in {path}"), ex.Message); + } + + [Fact] + public void AnExplicitConfigNameAppearsInTheMessage() + { + var path = Path.Combine(_dir, "team.json"); + File.WriteAllText(path, """{ "suppressions": [ { "query_hash": "0xAB", "rule": 3 } ] }"""); + + var ex = Assert.Throws(() => ConfigLoader.Load(path)); + + Assert.Equal($"Suppression 0 in {path} has no \"reason\".", ex.Message); + } + + [Fact] + public void MalformedJsonFailsWithAMessageNotAStackTrace() + { + var path = Write("""{ "suppressions": [ { "query_hash": """); + var ex = Assert.Throws(() => ConfigLoader.Load(path)); + Assert.Contains(path, ex.Message); + Assert.DoesNotContain(Environment.NewLine, ex.Message); + } + + [Fact] + public void AnUnreadableFileFailsWithAMessageNotAStackTrace() + { + var path = Write("""{ "rules": { "disabled": [] } }"""); + // Hold the file open with no sharing, so the loader's read is refused. + using var _ = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.None); + var ex = Assert.Throws(() => ConfigLoader.Load(path)); + Assert.Contains(path, ex.Message); + Assert.DoesNotContain(Environment.NewLine, ex.Message); + } + + [Fact] + public void ARuleThatIsNotANumberFailsAsAnInvalidFile() + { + Assert.Throws(() => ConfigLoader.Load(Write( + """{ "suppressions": [ { "query_hash": "0xAB", "rule": "ten", "reason": "ok" } ] }"""))); + } + + [Fact] + public void AMissingExplicitPathStillThrowsFileNotFound() + { + Assert.Throws(() => ConfigLoader.Load(Path.Combine(_dir, "nope.json"))); + } +} diff --git a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs index d0d809ac..9510ba54 100644 --- a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs +++ b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs @@ -12,12 +12,83 @@ every warning so a consumer can tell SQL Server's own warnings from Performance operator that produced it. Nothing has been removed or renamed either time, so a consumer reading fields by name is unaffected — but anything diffing or hashing whole output sees different bytes, which is exactly what this constant exists to make somebody decide on rather - than discover. Verified for #440 that the only new key on a warning is origin_node_ids. */ + than discover. Verified for #440 that the only new key on a warning is origin_node_ids. + + Rolled a third time for #651, again additively: every warning gains "suppressed" (always + written) and "rule" (written when a numbered rule produced it), and the summary gains + "suppressed_warnings". Verified by removing exactly those keys from the new output, which + gives back the previous hash, 779a85036b74dc7cc7d26d4bad3d7e2b327e4ff89a5ed79b00b78cfae3717cab. */ private const string ExpectedCompactOutputSha256 = - "779a85036b74dc7cc7d26d4bad3d7e2b327e4ff89a5ed79b00b78cfae3717cab"; + "0eeb5da0bc69ceb162100f4326eee446e07234b4cc1b6660dda19221f951f01a"; [Fact] public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() + { + // An empty config pins the run to the defaults, so a developer's own ~/.planview.json + // cannot change the bytes or fail the test. + var configPath = WriteTempConfig("{}"); + try + { + var run = await RunCliAsync( + "analyze", + "tests/PlanViewer.Core.Tests/Plans/row_goal_plan.sqlplan", + "--compact", + "--config", + configPath); + + Assert.True( + run.ExitCode == 0, + $"Historical analyze command exited with {run.ExitCode}: {run.StandardError}"); + // Normalize line endings before hashing. Console.WriteLine emits CRLF on + // Windows and LF elsewhere, so hashing raw stdout pins the contract to + // whichever OS produced the baseline — this hash was generated on the + // Linux CI runner and therefore failed on every Windows machine. + var text = run.StandardOutput.Replace("\r\n", "\n"); + var actualHash = Convert.ToHexString( + SHA256.HashData(Encoding.UTF8.GetBytes(text))).ToLowerInvariant(); + Assert.Equal(ExpectedCompactOutputSha256, actualHash); + } + finally + { + File.Delete(configPath); + } + } + + /// + /// #651: a suppression with no reason stops the command with one line on stderr that names + /// the file, and exit code 1. No stack trace, and nothing on stdout. + /// + [Fact] + public async Task Analyze_WithABadSuppression_PrintsOneLineAndExits1() + { + var configPath = WriteTempConfig("""{ "suppressions": [ { "query_hash": "0xAB", "rule": 3 } ] }"""); + try + { + var run = await RunCliAsync( + "analyze", + "tests/PlanViewer.Core.Tests/Plans/row_goal_plan.sqlplan", + "--config", + configPath); + + Assert.Equal(1, run.ExitCode); + Assert.Equal($"Suppression 0 in {configPath} has no \"reason\".", run.StandardError.Trim()); + Assert.Equal("", run.StandardOutput.Trim()); + } + finally + { + File.Delete(configPath); + } + } + + private static string WriteTempConfig(string json) + { + var path = Path.Combine(Path.GetTempPath(), $"planview-cli-{Guid.NewGuid():N}.json"); + File.WriteAllText(path, json); + return path; + } + + private static async Task<(int ExitCode, string StandardOutput, string StandardError)> RunCliAsync( + params string[] arguments) { var solutionRoot = FindSolutionRoot(); var configuration = new DirectoryInfo(AppContext.BaseDirectory).Parent?.Name ?? "Release"; @@ -39,9 +110,8 @@ public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() UseShellExecute = false }; startInfo.ArgumentList.Add(cliAssembly); - startInfo.ArgumentList.Add("analyze"); - startInfo.ArgumentList.Add("tests/PlanViewer.Core.Tests/Plans/row_goal_plan.sqlplan"); - startInfo.ArgumentList.Add("--compact"); + foreach (var argument in arguments) + startInfo.ArgumentList.Add(argument); using var timeout = CancellationTokenSource.CreateLinkedTokenSource( TestContext.Current.CancellationToken); @@ -58,17 +128,7 @@ public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() await copyOutput; var standardError = await readError; - Assert.True( - process.ExitCode == 0, - $"Historical analyze command exited with {process.ExitCode}: {standardError}"); - // Normalize line endings before hashing. Console.WriteLine emits CRLF on - // Windows and LF elsewhere, so hashing raw stdout pins the contract to - // whichever OS produced the baseline — this hash was generated on the - // Linux CI runner and therefore failed on every Windows machine. - var text = Encoding.UTF8.GetString(stdout.ToArray()).Replace("\r\n", "\n"); - var actualHash = Convert.ToHexString( - SHA256.HashData(Encoding.UTF8.GetBytes(text))).ToLowerInvariant(); - Assert.Equal(ExpectedCompactOutputSha256, actualHash); + return (process.ExitCode, Encoding.UTF8.GetString(stdout.ToArray()), standardError); } finally { diff --git a/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs b/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs index 2fd184e3..ce7f355c 100644 --- a/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs +++ b/tests/PlanViewer.Core.Tests/OperatorSelfTimeTests.cs @@ -1,5 +1,6 @@ using System.Linq; using PlanViewer.Core.Models; +using PlanViewer.Core.Output; using PlanViewer.Core.Services; namespace PlanViewer.Core.Tests; @@ -131,4 +132,61 @@ public void SerialPlanStillUsesItsOnlyThread() Assert.True(withStats.ActualElapsedMs >= 0); } } + + [Fact] + public void SerialRowModeParentAboveBatchZoneDoesNotAbsorbItsCpu() + { + // A serial row-mode Stream Aggregate (node 4) sits above a batch Sort (node 5) + // over a row-mode Nested Loops (node 6). Batch CPU is standalone, so the zone + // under the aggregate used 14,142 + 11,659 ms. Subtracting only the Sort's + // 14,142 left 14,771 ms of "self" CPU beside 3,059 ms of self elapsed. + var root = RootOf("param-sniffing-posttypeid2.sqlplan"); + var aggregate = TryFindNode(root, 4)!; + + Assert.InRange(PlanAnalyzer.GetOperatorOwnElapsedMs(aggregate), 3_000, 3_100); + Assert.InRange(PlanAnalyzer.GetOperatorOwnCpuMs(aggregate), 3_050, 3_200); + Assert.InRange(PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(aggregate), 3_050, 3_200); + Assert.InRange(NodeTimeAttribution.GetOwnCpuMs(aggregate), 3_050, 3_200); + } + + [Fact] + public void ParallelOwnCpuSumsThreadsAndMaxThreadKeepsTheBusiestOne() + { + // A DOP 8 Clustered Index Scan (node 1) burned 624 ms across its workers in a + // 629 ms query. Its own CPU is the sum. The busiest thread alone had 79 ms, + // which is what the external-wait formula wants from the max-thread figure. + var root = RootOf("case_predicate_plan.sqlplan"); + var scan = TryFindNode(root, 1)!; + + Assert.InRange(PlanAnalyzer.GetOperatorOwnCpuMs(scan), 600, 640); + Assert.InRange(PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(scan), 70, 90); + } + + [Fact] + public void BatchOperatorMaxThreadCpuIsNotReducedByItsChildren() + { + // Batch mode reports each thread's own CPU. The max-thread figure used to + // subtract the children's CPU from it, though the batch number never held it. + var root = RootOf("parallel_row_over_batch_plan.sqlplan"); + var hashMatch = TryFindNode(root, 2)!; + var busiest = hashMatch.PerThreadStats.Where(t => t.ThreadId > 0).Max(t => t.ActualCPUMs); + + Assert.Equal(busiest, PlanAnalyzer.GetOperatorMaxThreadOwnCpuMs(hashMatch)); + } + + [Fact] + public void ExpensiveOperatorsTextUsesTheAnalyzersOwnTimes() + { + // The CLI's "Expensive operators" list did its own row-mode subtraction and + // charged the Stream Aggregate with 14,771 ms CPU and 14,720 ms elapsed. Its + // CPU shares added up to 159%. + var plan = PlanTestHelper.LoadAndAnalyze("param-sniffing-posttypeid2.sqlplan"); + var text = TextFormatter.Format(ResultMapper.Map(plan, "param-sniffing-posttypeid2.sqlplan")); + var lines = text.Split('\n'); + var at = System.Array.FindIndex(lines, l => l.Contains("Stream Aggregate (Node 4)")); + + Assert.True(at >= 0, "node 4 is no longer listed as an expensive operator"); + Assert.Contains("3,112ms CPU", lines[at + 1]); + Assert.Contains("3,059ms elapsed", lines[at + 1]); + } } diff --git a/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs b/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs index 0d43d2c9..a8ba6bad 100644 --- a/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanOperationsTests.cs @@ -183,13 +183,14 @@ public async Task GetExpensiveOperators_RanksEstimatedPlansByCostAndHonorsTop() [Fact] - public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() + public void GetExpensiveOperators_UsesOwnTimeAndKeepsStableTies() { var firstTie = new PlanViewer.Core.Output.OperatorResult { NodeId = 2, PhysicalOp = "First tie", ActualElapsedMs = 10, + OwnElapsedMs = 10, CostPercent = 1 }; var secondTie = new PlanViewer.Core.Output.OperatorResult @@ -197,6 +198,7 @@ public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() NodeId = 3, PhysicalOp = "Second tie", ActualElapsedMs = 10, + OwnElapsedMs = 10, CostPercent = 99 }; var root = new PlanViewer.Core.Output.OperatorResult @@ -229,10 +231,82 @@ public void GetExpensiveOperators_UsesActualElapsedTimeAndKeepsStableTies() var result = operations.GetExpensiveOperators(session, top: 2, TestContext.Current.CancellationToken); - Assert.Equal("actual_elapsed_ms", result.RankedBy); + Assert.Equal("own_time_ms", result.RankedBy); Assert.Equal([2, 3], result.Operators.Select(item => item.NodeId)); } + + private static async Task<(PlanOperations Operations, PlanViewer.Core.Models.PlanSession Session)> OpenFixtureAsync(string planFile) + { + var catalog = new InMemoryPlanCatalog(); + var operations = new PlanOperations(catalog); + var opened = await operations.OpenAsync( + Path.GetFullPath(Path.Combine("Plans", planFile)), TestContext.Current.CancellationToken); + var session = Assert.IsType(catalog.GetSession(opened.SessionId)); + return (operations, session); + } + + private static List ExpensiveOperatorNodeIdsFromText(string text) + { + var ids = new List(); + var lines = text.Split('\n'); + var at = Array.FindIndex(lines, l => l.TrimEnd() == "Expensive operators:"); + Assert.True(at >= 0, "no Expensive operators section"); + for (var i = at + 1; i < lines.Length && lines[i].StartsWith(" "); i++) + { + var match = System.Text.RegularExpressions.Regex.Match(lines[i], @"^ \S.*\(Node (\d+)\):\s*$"); + if (match.Success) + ids.Add(int.Parse(match.Groups[1].Value)); + } + return ids; + } + + [Theory] + [InlineData("param-sniffing-posttypeid2.sqlplan")] + [InlineData("case_predicate_plan.sqlplan")] + public async Task GetExpensiveOperators_MatchesTheTextSummaryExpensiveOperators(string planFile) + { + var (operations, session) = await OpenFixtureAsync(planFile); + var plan = PlanViewer.Core.Tests.PlanTestHelper.LoadAndAnalyze(planFile); + var text = PlanViewer.Core.Output.TextFormatter.Format(PlanViewer.Core.Output.ResultMapper.Map(plan, planFile)); + + var result = operations.GetExpensiveOperators(session, top: 5, TestContext.Current.CancellationToken); + + var expected = ExpensiveOperatorNodeIdsFromText(text); + Assert.NotEmpty(expected); + Assert.Equal("own_time_ms", result.RankedBy); + Assert.Equal(expected, result.Operators.Select(item => item.NodeId)); + Assert.DoesNotContain(result.Operators, item => item.PhysicalOp == "Parallelism"); + } + + [Fact] + public async Task GetExpensiveOperators_WorkerOutranksItsCumulativeParents() + { + var (operations, session) = await OpenFixtureAsync("case_predicate_plan.sqlplan"); + + var result = operations.GetExpensiveOperators(session, top: 5, TestContext.Current.CancellationToken); + + var first = result.Operators[0]; + Assert.Equal(1, first.NodeId); + Assert.Contains("Clustered Index Scan", first.PhysicalOp); + Assert.True(first.OwnCpuMs > 0); + Assert.DoesNotContain(result.Operators, item => item.PhysicalOp == "Parallelism"); + } + + [Fact] + public async Task GetExpensiveOperators_RanksActualPlansByOwnTimeAndEstimatedPlansByCost() + { + var (operations, actual) = await OpenFixtureAsync("case_predicate_plan.sqlplan"); + Assert.Equal("own_time_ms", + operations.GetExpensiveOperators(actual, top: 3, TestContext.Current.CancellationToken).RankedBy); + + var estimatedCatalog = new InMemoryPlanCatalog(); + var estimatedOps = new PlanOperations(estimatedCatalog); + var opened = await estimatedOps.OpenAsync( + Path.GetFullPath(Path.Combine("Plans", "row_goal_plan.sqlplan")), TestContext.Current.CancellationToken); + Assert.Equal("cost_percent", + estimatedOps.GetExpensiveOperators(opened.SessionId, 3, TestContext.Current.CancellationToken).RankedBy); + } [Fact] public async Task GetMissingIndexes_BoundsTheReturnedItemsAndReportsTruncation() { diff --git a/tests/PlanViewer.Core.Tests/SuppressionTests.cs b/tests/PlanViewer.Core.Tests/SuppressionTests.cs new file mode 100644 index 00000000..10fd554c --- /dev/null +++ b/tests/PlanViewer.Core.Tests/SuppressionTests.cs @@ -0,0 +1,469 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using PlanViewer.App.Mcp; +using AnalyzerConfig = PlanViewer.Core.Models.AnalyzerConfig; +using CorePlanSession = PlanViewer.Core.Models.PlanSession; +using PlanViewer.Core.Models; +using PlanViewer.Core.Output; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #651: a suppression in .planview.json marks one finding on one statement as accepted. The +/// finding stays in the output, flagged with its reason, and drops out of the summary counts. +/// +public class SuppressionTests +{ + private const string Hash = "0x00E1987CD1C28B4C"; + + private static PlanWarning Warn( + string type, + int? rule, + PlanWarningSeverity severity = PlanWarningSeverity.Warning, + PlanWarningSource source = PlanWarningSource.PerformanceStudio) => + new() + { + WarningType = type, + Message = type + " message", + Severity = severity, + RuleNumber = rule, + Source = source + }; + + private static PlanNode Node(int id, string physicalOp, string? objectName, params PlanWarning[] warnings) => + new() + { + NodeId = id, + PhysicalOp = physicalOp, + ObjectName = objectName, + Warnings = warnings.ToList() + }; + + /// + /// One statement. The statement carries a rule finding and an engine warning. The tree is a + /// Nested Loops (node 0) over two Key Lookups of the same type, on different tables (nodes 1 + /// and 2). Node 2 also carries the engine warning. + /// + private static ParsedPlan SamplePlan(string hash = Hash) + { + var root = Node(0, "Nested Loops", null); + root.Children.Add(Node(1, "Key Lookup", "eventuous.Messages", Warn("Key Lookup", 10))); + root.Children.Add(Node(2, "Key Lookup", "dbo.Orders", + Warn("Key Lookup", 10, PlanWarningSeverity.Critical), + Warn("Implicit Conversion", null, source: PlanWarningSource.SqlServer))); + + var stmt = new PlanStatement + { + QueryHash = hash, + RootNode = root, + PlanWarnings = + [ + Warn("Dynamic Cursor", 36), + Warn("Implicit Conversion", null, source: PlanWarningSource.SqlServer) + ] + }; + return new ParsedPlan { Batches = [new PlanBatch { Statements = [stmt] }] }; + } + + private static AnalyzerConfig Cfg(params SuppressionEntry[] entries) => + new() { Suppressions = entries.ToList() }; + + private static SuppressionEntry Entry( + string hash = Hash, int? rule = null, string? type = null, string? op = null, string reason = "accepted") => + new() { QueryHash = hash, Rule = rule, Type = type, Operator = op, Reason = reason }; + + private static ParsedPlan Suppress(ParsedPlan plan, params SuppressionEntry[] entries) + { + SuppressionApplier.Apply(plan, Cfg(entries)); + return plan; + } + + private static List Suppressed(ParsedPlan plan) => + PlanTestHelper.AllWarnings(plan).Where(w => w.IsSuppressed).ToList(); + + [Fact] + public void RuleMatchesEveryFindingOfThatRuleOnTheStatement() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10)); + + var suppressed = Suppressed(plan); + Assert.Equal(2, suppressed.Count); + Assert.All(suppressed, w => Assert.Equal(10, w.RuleNumber)); + Assert.All(suppressed, w => Assert.Equal("accepted", w.SuppressionReason)); + } + + [Fact] + public void RuleNeverMatchesAnEngineWarning() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10)); + + var engine = PlanTestHelper.AllWarnings(plan).Where(w => w.Source == PlanWarningSource.SqlServer); + Assert.All(engine, w => Assert.False(w.IsSuppressed)); + } + + [Fact] + public void TypeMatchesAWarningThatHasNoRuleNumber() + { + var plan = Suppress(SamplePlan(), Entry(type: "implicit conversion")); + + var suppressed = Suppressed(plan); + Assert.Equal(2, suppressed.Count); + Assert.All(suppressed, w => Assert.Null(w.RuleNumber)); + Assert.All(suppressed, w => Assert.Equal("Implicit Conversion", w.WarningType)); + } + + [Fact] + public void RuleAndTypeBothHaveToMatch() + { + var wrongType = Suppress(SamplePlan(), Entry(rule: 10, type: "Dynamic Cursor")); + Assert.Empty(Suppressed(wrongType)); + + var rightType = Suppress(SamplePlan(), Entry(rule: 10, type: "Key Lookup")); + Assert.Equal(2, Suppressed(rightType).Count); + } + + [Theory] + [InlineData("0x00E1987CD1C28B4C")] + [InlineData("0X00E1987CD1C28B4C")] + [InlineData("00E1987CD1C28B4C")] + [InlineData("0x00e1987cd1c28b4c")] + [InlineData("00e1987cd1c28b4c")] + [InlineData(" 0x00e1987cd1c28b4c ")] + public void HashMatchesWithOrWithoutThePrefixInEitherCase(string configured) + { + var plan = Suppress(SamplePlan("0x00E1987CD1C28B4C"), Entry(hash: configured, rule: 36)); + Assert.Single(Suppressed(plan)); + } + + [Theory] + [InlineData("00e1987cd1c28b4c")] + [InlineData("0X00E1987CD1C28B4C")] + public void HashMatchesWhenThePlanSideDiffersInShape(string planHash) + { + var plan = Suppress(SamplePlan(planHash), Entry(hash: "0x00E1987CD1C28B4C", rule: 36)); + Assert.Single(Suppressed(plan)); + } + + [Fact] + public void AnotherStatementsHashIsLeftAlone() + { + var plan = Suppress(SamplePlan("0x1111111111111111"), Entry(rule: 10)); + Assert.Empty(Suppressed(plan)); + } + + [Fact] + public void AStatementWithNoHashIsNeverMatched() + { + var plan = Suppress(SamplePlan(hash: null!), Entry(rule: 10)); + Assert.Empty(Suppressed(plan)); + } + + [Fact] + public void OperatorMatchesOnlyTheNodeWhoseLabelContainsIt() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10, op: "eventuous.Messages")); + + var suppressed = Assert.Single(Suppressed(plan)); + var root = plan.Batches[0].Statements[0].RootNode!; + Assert.Same(suppressed, root.Children[0].Warnings.Single()); + // Node 2 holds the same type of finding and is untouched. + Assert.False(root.Children[1].Warnings.First(w => w.RuleNumber == 10).IsSuppressed); + } + + [Fact] + public void OperatorIsMatchedAgainstTheLabelTheJsonOutputPrints() + { + var plan = SamplePlan(); + var result = ResultMapper.Map(plan, "x"); + var printed = result.Statements[0].OperatorTree!.Children[0].Warnings.Single().Operator!; + + // Pasting the printed label into a suppression has to hit that node and no other. + Suppress(plan, Entry(rule: 10, op: printed)); + + Assert.Single(Suppressed(plan)); + Assert.Equal("Key Lookup on eventuous.Messages (Node 1)", printed); + } + + [Fact] + public void OperatorIsCaseInsensitive() + { + var plan = Suppress(SamplePlan(), Entry(rule: 10, op: "DBO.ORDERS")); + Assert.Single(Suppressed(plan)); + } + + [Fact] + public void OperatorNeverMatchesAStatementLevelWarning() + { + var plan = Suppress(SamplePlan(), Entry(rule: 36, op: "Nested Loops")); + Assert.Empty(Suppressed(plan)); + + var withoutOperator = Suppress(SamplePlan(), Entry(rule: 36)); + Assert.Single(Suppressed(withoutOperator)); + } + + [Fact] + public void ASuppressionKeepsTheWarningAndItsSeverity() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 10, op: "dbo.Orders")); + + var warning = plan.Batches[0].Statements[0].RootNode!.Children[1].Warnings.First(w => w.RuleNumber == 10); + Assert.True(warning.IsSuppressed); + Assert.Equal(PlanWarningSeverity.Critical, warning.Severity); + Assert.Equal(5, PlanTestHelper.AllWarnings(plan).Count); + } + + [Fact] + public void ASeverityOverrideAndASuppressionBothApplyToOneWarning() + { + var stmt = new PlanStatement + { + QueryHash = Hash, + CursorActualType = "Dynamic" + }; + var plan = new ParsedPlan { Batches = [new PlanBatch { Statements = [stmt] }] }; + var config = new AnalyzerConfig + { + Rules = new RulesConfig { SeverityOverrides = { [36] = "Info" } }, + Suppressions = [Entry(rule: 36, reason: "vendor cursor")] + }; + + PlanAnalysisPipeline.AnalyzeParsed(plan, config, cancellationToken: TestContext.Current.CancellationToken); + + var warning = Assert.Single(stmt.PlanWarnings, w => w.RuleNumber == 36); + Assert.Equal(PlanWarningSeverity.Info, warning.Severity); + Assert.True(warning.IsSuppressed); + Assert.Equal("vendor cursor", warning.SuppressionReason); + } + + [Fact] + public void ASuppressionReachesAStatementInsideAProcedureBody() + { + var baseline = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan"); + var topLevel = baseline.Batches.SelectMany(b => b.Statements).ToHashSet(); + var nested = PlanStatements.EnumerateAll(baseline) + .Where(s => !topLevel.Contains(s) && !string.IsNullOrEmpty(s.QueryHash)) + .Select(s => (Statement: s, Warning: AllOf(s).FirstOrDefault(w => w.RuleNumber != null))) + .First(x => x.Warning != null); + + var hash = nested.Statement.QueryHash!; + var rule = nested.Warning!.RuleNumber!.Value; + + var xml = File.ReadAllText(Path.Combine("Plans", "exec_stored_procedure_plan.sqlplan")) + .Replace("encoding=\"utf-16\"", "encoding=\"utf-8\""); + var plan = ShowPlanParser.Parse(xml); + PlanAnalysisPipeline.AnalyzeParsed(plan, Cfg(Entry(hash: hash, rule: rule, reason: "library owns it")), + cancellationToken: TestContext.Current.CancellationToken); + + var suppressedStatements = PlanStatements.EnumerateAll(plan) + .Where(s => AllOf(s).Any(w => w.IsSuppressed)) + .ToList(); + Assert.NotEmpty(suppressedStatements); + Assert.All(suppressedStatements, s => Assert.Equal(hash, s.QueryHash)); + Assert.DoesNotContain(plan.Batches.SelectMany(b => b.Statements), + s => AllOf(s).Any(w => w.IsSuppressed) && s.QueryHash != hash); + Assert.All(PlanTestHelper.AllWarnings(plan).Where(w => w.IsSuppressed), + w => Assert.Equal(rule, w.RuleNumber)); + } + + private static List AllOf(PlanStatement stmt) + { + var all = new List(stmt.PlanWarnings); + all.AddRange(PlanTestHelper.AllNodeWarnings(stmt)); + return all; + } + + [Fact] + public void AScorerWaitStatsFindingCanBeSuppressedByType() + { + // The scorer adds its wait-stats findings after the analyzer, so the pass has to run last. + static ParsedPlan Run(AnalyzerConfig config) + { + var xml = File.ReadAllText(Path.Combine("Plans", "case_predicate_plan.sqlplan")) + .Replace("encoding=\"utf-16\"", "encoding=\"utf-8\""); + var parsed = ShowPlanParser.Parse(xml); + PlanAnalysisPipeline.AnalyzeParsed(parsed, config, cancellationToken: TestContext.Current.CancellationToken); + return parsed; + } + + var baseline = Run(new AnalyzerConfig()); + var wait = PlanStatements.EnumerateAll(baseline) + .SelectMany(s => s.PlanWarnings.Select(w => (Statement: s, Warning: w))) + .First(x => x.Warning.WarningType.StartsWith("Wait: ", StringComparison.Ordinal) + && !string.IsNullOrEmpty(x.Statement.QueryHash)); + Assert.Null(wait.Warning.RuleNumber); + + var plan = Run(Cfg(Entry(hash: wait.Statement.QueryHash!, type: wait.Warning.WarningType))); + + Assert.Contains(PlanTestHelper.AllWarnings(plan), + w => w.IsSuppressed && w.WarningType == wait.Warning.WarningType); + } + + // ---- Output ------------------------------------------------------------------------------- + + [Fact] + public void SuppressedWarningsLeaveTheSummaryCountsButStayInTheOutput() + { + var plan = SamplePlan(); + var before = ResultMapper.Map(plan, "x").Summary; + Assert.Equal(5, before.TotalWarnings); + Assert.Equal(1, before.CriticalWarnings); + Assert.Equal(0, before.SuppressedWarnings); + + // The Critical lookup on dbo.Orders, and the statement-level Dynamic Cursor. + Suppress(plan, Entry(rule: 10, op: "dbo.Orders", reason: "r1"), Entry(rule: 36, reason: "r2")); + var result = ResultMapper.Map(plan, "x"); + + Assert.Equal(3, result.Summary.TotalWarnings); + Assert.Equal(0, result.Summary.CriticalWarnings); + Assert.Equal(2, result.Summary.SuppressedWarnings); + Assert.DoesNotContain("Dynamic Cursor", result.Summary.WarningTypes); + Assert.Contains("Key Lookup", result.Summary.WarningTypes); // node 1 is still active + + var statement = result.Statements[0]; + var cursor = Assert.Single(statement.Warnings, w => w.Type == "Dynamic Cursor"); + Assert.True(cursor.IsSuppressed); + Assert.Equal("r2", cursor.SuppressionReason); + + var lookup = statement.OperatorTree!.Children[1].Warnings.First(w => w.Type == "Key Lookup"); + Assert.True(lookup.IsSuppressed); + Assert.Equal("r1", lookup.SuppressionReason); + Assert.Equal("Critical", lookup.Severity); + } + + [Fact] + public void WarningTypesDropsATypeOnlyWhenEveryFindingOfItIsSuppressed() + { + var plan = SamplePlan(); + Suppress(plan, Entry(type: "Key Lookup")); + + var summary = ResultMapper.Map(plan, "x").Summary; + + Assert.DoesNotContain("Key Lookup", summary.WarningTypes); + Assert.Equal(3, summary.TotalWarnings); + Assert.Equal(2, summary.SuppressedWarnings); + } + + [Fact] + public void TheSerializedJsonCarriesRuleSuppressedAndReasonAndOmitsRuleForEngineWarnings() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 10, op: "eventuous.Messages", reason: "library owns it")); + + var json = JsonSerializer.Serialize(ResultMapper.Map(plan, "x"), AnalysisJson.IndentedWithoutNulls); + using var doc = JsonDocument.Parse(json); + var statement = doc.RootElement.GetProperty("statements")[0]; + + Assert.Equal(1, doc.RootElement.GetProperty("summary").GetProperty("suppressed_warnings").GetInt32()); + + var cursor = statement.GetProperty("warnings").EnumerateArray().Single(w => w.GetProperty("type").GetString() == "Dynamic Cursor"); + Assert.Equal(36, cursor.GetProperty("rule").GetInt32()); + Assert.False(cursor.GetProperty("suppressed").GetBoolean()); + Assert.False(cursor.TryGetProperty("suppression_reason", out _)); + + var engine = statement.GetProperty("warnings").EnumerateArray().Single(w => w.GetProperty("type").GetString() == "Implicit Conversion"); + Assert.False(engine.TryGetProperty("rule", out _)); + Assert.False(engine.GetProperty("suppressed").GetBoolean()); + + var lookup = statement.GetProperty("operator_tree").GetProperty("children")[0].GetProperty("warnings")[0]; + Assert.Equal(10, lookup.GetProperty("rule").GetInt32()); + Assert.True(lookup.GetProperty("suppressed").GetBoolean()); + Assert.Equal("library owns it", lookup.GetProperty("suppression_reason").GetString()); + } + + [Fact] + public void TheTextOutputTagsASuppressedWarningWithItsReasonAndReportsTheCount() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor cursor")); + + var text = TextFormatter.Format(ResultMapper.Map(plan, "x")); + + Assert.Contains("[suppressed: vendor cursor]", text); + Assert.Contains("Suppressed warnings: 1", text); + Assert.Contains("Warnings: 4 (1 critical)", text); + } + + [Fact] + public void TheTextOutputTagsSuppressedOperatorWarningsOnTheGroupedAndTheSinglePath() + { + // Two warnings that share a severity and a harm sentence are written as a group; the + // third has no harm sentence and is written on its own. Both paths build their own tag. + static PlanWarning Estimate(string data) => new() + { + WarningType = "Row Estimate Mismatch", + Message = data + ". The underestimate caused a spill.", + Severity = PlanWarningSeverity.Warning, + RuleNumber = 5 + }; + + var root = Node(0, "Nested Loops", null); + root.Children.Add(Node(1, "Index Scan", "dbo.A", Estimate("Estimated 1 row, got 900"))); + root.Children.Add(Node(2, "Index Scan", "dbo.B", Estimate("Estimated 2 rows, got 800"))); + root.Children.Add(Node(3, "Index Scan", "dbo.C", + new PlanWarning { WarningType = "Scan With Predicate", Message = "Scan reads every row", Severity = PlanWarningSeverity.Info, RuleNumber = 6 })); + var plan = new ParsedPlan + { + Batches = [new PlanBatch { Statements = [new PlanStatement { QueryHash = Hash, RootNode = root }] }] + }; + Suppress(plan, + Entry(rule: 5, op: "dbo.A", reason: "grouped reason"), + Entry(rule: 6, reason: "single reason")); + + var lines = TextFormatter.Format(ResultMapper.Map(plan, "x")).Split('\n'); + + var groupedA = Assert.Single(lines, l => l.Contains("dbo.A")); + var groupedB = Assert.Single(lines, l => l.Contains("dbo.B")); + var single = Assert.Single(lines, l => l.Contains("dbo.C")); + Assert.Contains("[suppressed: grouped reason]", groupedA); + Assert.DoesNotContain("[suppressed", groupedB); + Assert.Contains("[suppressed: single reason]", single); + } + + [Fact] + public void TheHtmlOutputTagsASuppressedWarningWithItsReason() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor ")); + + var html = HtmlExporter.Export(ResultMapper.Map(plan, "x"), ""); + + Assert.Contains("suppressed: vendor <cursor>", html); + } + + [Fact] + public void TheMcpSummaryAndWarningListCarryTheSuppression() + { + var plan = SamplePlan(); + Suppress(plan, Entry(rule: 36, reason: "vendor cursor")); + + var manager = new PlanSessionManager(); + var sessionId = $"mcp-{Guid.NewGuid():N}"; + manager.Register(new CorePlanSession + { + SessionId = sessionId, + Label = "sample", + Source = "file", + Plan = plan + }); + var operations = new PlanOperations(manager, AnalyzerConfig.Default, enforceQueryAdmission: false); + + var summary = operations.GetSummary(sessionId); + Assert.Equal(1, summary.SuppressedWarnings); + Assert.Equal(4, summary.TotalWarnings); + + var warnings = operations.GetWarnings( + sessionId, null, TestContext.Current.CancellationToken); + var cursor = Assert.Single(warnings.Warnings, w => w.Type == "Dynamic Cursor"); + Assert.True(cursor.IsSuppressed); + Assert.Equal("vendor cursor", cursor.SuppressionReason); + Assert.Equal(36, cursor.Rule); + var engine = warnings.Warnings.First(w => w.Type == "Implicit Conversion"); + Assert.Null(engine.Rule); + Assert.False(engine.IsSuppressed); + } +}