update some nuget packages and fix many warnings#346
Conversation
MisterZeus
changed the title
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thanks for tackling the analyzer warnings and NuGet updates — this is useful housekeeping work. A few things to address before merging:
1. Incomplete Lite PlanAnalyzer conversion
The Dashboard PlanAnalyzer.cs converts all four regex fields to [GeneratedRegex], but the Lite copy only converts FunctionInPredicateRegex and leaves LeadingWildcardLikeRegex, CaseInPredicateRegex, and CteDefinitionRegex unconverted. These two files should stay in sync.
2. Remove "en-GB" culture from GeneratedRegex attributes
All the [GeneratedRegex] attributes specify "en-GB" as the culture parameter. These are all ASCII patterns that don't do any culture-sensitive matching, so the culture parameter can be dropped entirely.
3. RegexOptions.Compiled is redundant in GeneratedRegex
[GeneratedRegex] already source-generates the compiled regex at build time, so including RegexOptions.Compiled in the attribute is redundant. It won't cause issues, but it should be removed for clarity.
4. StringConstantToFormat naming
The rename from QuerySnapshotsBase to StringConstantToFormat is very generic. If you're keeping the CompositeFormat change, please use a more descriptive name (e.g., QuerySnapshotsBaseFormat or similar).
5. MCP 0.7-preview → 1.0.0
This is a major version jump from a preview release to stable. Could you confirm there are no breaking API changes? If the MCP update is independent of the warning fixes, feel free to split it into a separate PR so neither blocks the other.
That goes for the NuGet updates generally — if you want to separate the package updates from the warning fixes into two PRs, that's totally fine and might make review easier.
Thanks again for the contribution!
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
PR #346 Review — NuGet Updates + Warning Fixes
Good work cleaning these up. The GeneratedRegex conversions and CA warning fixes are solid improvements.
Notes
1. MCP 0.7.0-preview → 1.0.0 (Lite)
This is a significant jump from preview to GA. Since the build succeeds and you tested it, I'll trust it works — but worth calling out for awareness. If there are breaking API changes they'd show up at build time anyway.
2. Partial PlanAnalyzer conversion in Lite
Dashboard PlanAnalyzer got 5 regex fields converted to GeneratedRegex (FunctionInPredicateRegex, LeadingWildcardLikeRegex, CaseInPredicateRegex, CteDefinitionRegex, IsNullCoalesceRegExp), but Lite PlanAnalyzer only got 1 (FunctionInPredicateRegex). The other 4 are still runtime new Regex(...) in Lite. We try to keep these two files in sync — not blocking, but worth completing in a follow-up.
3. "en-GB" culture on GeneratedRegex (cosmetic)
All the [GeneratedRegex] attributes specify "en-GB" as the culture parameter. For ASCII SQL keyword patterns this is functionally harmless, but it's a bit unusual to hardcode a locale. Most projects omit it or use "" for invariant culture. Not blocking.
4. RegexOptions.Compiled on GeneratedRegex (cosmetic)
Several [GeneratedRegex] attributes include RegexOptions.Compiled. With source generators, Compiled is ignored — the generator produces the compiled code at build time. Harmless but redundant.
Verdict
Approve — the NuGet updates and regex improvements are all positive. The PlanAnalyzer sync gap in Lite is minor and we can clean it up ourselves.
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Approving — good cleanup work. We'll handle the Lite PlanAnalyzer sync gap in a follow-up.
|
Approved, but there are merge conflicts from recent dev changes (PlanAnalyzer updates, etc.). Could you rebase on dev and resolve? Then we can merge. Thanks! |
erikdarlingdata
dismissed
their stale review
Dismissing — there are unresolved items from the prior changes-requested review (incomplete Lite PlanAnalyzer sync, en-GB culture, Compiled flag, StringConstantToFormat naming, MCP version jump). Those need to be addressed first.
MisterZeus
force-pushed
the
fix-the-warnings
branch
from
34371e9 to
8be254e
Compare
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Nice cleanup — GeneratedRegex conversions, char overloads, and NuGet updates all look good mechanically. A few notes:
1. MCP 0.7-preview → 1.0.0 is a major version bump
This is a preview-to-GA jump for ModelContextProtocol. Has anyone verified the MCP server still compiles and works with the 1.0 API? Could have breaking changes.
2. Lite PlanAnalyzer only partially converted
Dashboard PlanAnalyzer got all 4 regex fields converted to GeneratedRegex plus the inline IsNullCoalesceRegExp, but Lite only got FunctionInPredicateRegex. The other 3 (LeadingWildcardLikeRegex, CaseInPredicateRegex, CteDefinitionRegex) and the inline ISNULL/COALESCE pattern are still old-style in Lite. This creates an inconsistency between the two copies.
3. RegexOptions.Compiled in [GeneratedRegex]
Several generated regex attributes still include RegexOptions.Compiled. The source generator ignores this flag (it's always compiled), so it's harmless but unnecessary.
4. QuerySnapshotsBase → StringConstantToFormat
The rename is less descriptive — QuerySnapshotsBase was a better name for what the constant actually is.
… time instead of at runtime: warning CA1847: Use 'string.Contains(char)' instead of 'string.Contains(string)' when searching for a single character (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1847) warning CA1875: Use 'Regex.Count' instead of 'Regex.Matches(...).Count' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1875) warning CA1863: Cache a 'CompositeFormat' for repeated use in this formatting operation (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1863)
MisterZeus
force-pushed
the
fix-the-warnings
branch
from
8be254e to
f380d55
Compare
PR #346 left Lite PlanAnalyzer with 3 old-style regex fields and inline Regex.IsMatch calls, and added unnecessary RegexOptions.Compiled to all GeneratedRegex attributes (source generator always compiles, flag is ignored). - Convert remaining Lite regex fields to use GeneratedRegex source generator - Convert all inline Regex.IsMatch calls to GeneratedRegex in both apps - Remove RegexOptions.Compiled from all [GeneratedRegex] attributes - Keep both PlanAnalyzer copies in sync with identical regex methods Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR #346 left Lite PlanAnalyzer with 3 old-style regex fields and inline Regex.IsMatch calls, and added unnecessary RegexOptions.Compiled to all GeneratedRegex attributes (source generator always compiles, flag is ignored). - Convert remaining Lite regex fields to use GeneratedRegex source generator - Convert all inline Regex.IsMatch calls to GeneratedRegex in both apps - Remove RegexOptions.Compiled from all [GeneratedRegex] attributes - Keep both PlanAnalyzer copies in sync with identical regex methods Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Fix #410 (#411) * Fix #412 (#413) * GUI installer: log installation history to config.installation_history (#414) The GUI installer read from installation_history for version detection but never wrote to it, causing subsequent upgrades to fail to detect the prior install. Adds LogInstallationHistoryAsync mirroring the CLI installer's existing LogInstallationHistory method. Fixes #409 Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Feature/long running queries config settings (#415) * Added exclusions in GetLongRunningQueriesAsync() method for SP_SERVER_DIAGNOSTIC wait types, and WAITFOR wait types. * Added TOP parameter for query in GetLongRunningQueriesAsync() method to allow future configurability on the number of long-running queries returned. Added optional parameter to control display of WAITFOR types in future. * Replaced waitForFilter string constructor with C# string interpolation instead of janky string addition. * Added exclusion for backup-related waits to GetLongRunningQueriesAsync() method. Removed System.Collections.Generic using statement as it is unnecessary. * Reverted Controls/LandingPage.xaml.cs * Added BROKER_RECEIVE_WAITFOR wait type to waitforFilter exclusions. Added miscWaitsFilter to exclude XE_LIVE_TARGET_TVF waits. Removed unused parameters. Corrected minimum value for maxLongRunningQueryCount (minimum 1 instead of 5). * Added filtering for SP_SERVER_DIAGNOSTICS, WAITFOR, BROKER_RECEIVE_WAITFOR, BACKUPTHREAD, BACKUPIO, and XE_LIVE_TARGET_TVF wait types in Lite/Services/LocalDataService.WaitStats.cs * Add configurable max results setting for long-running queries Adds LongRunningQueryMaxResults to UserPreferences (default 5) and exposes it in the Settings UI alongside the existing duration threshold. Threads the value through GetAlertHealthAsync and GetLongRunningQueriesAsync to replace the hardcoded TOP(5) in the DMV query. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Apply max results validation and Lite parity for long-running queries Adds Math.Clamp(1, int.MaxValue) guard to GetLongRunningQueriesAsync in both Dashboard and Lite, and updates the Dashboard settings validation to use an explicit range check with a descriptive error message. Mirrors the LongRunningQueryMaxResults setting across the Lite project: adds the App property, loads/saves it to settings.json, exposes it in the Lite Settings UI, and passes it through to GetLongRunningQueriesAsync to replace the hardcoded LIMIT 5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Use GetInt64() when loading long-running query max results from JSON Prevents an OverflowException if the value in settings.json is outside the int32 range. The value is read as long, clamped to [1, int.MaxValue], then cast back to int. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add configurable long-running query filter toggles Replaces hardcoded wait type exclusions in GetLongRunningQueriesAsync with user-configurable booleans for SP_SERVER_DIAGNOSTICS, WAITFOR / BROKER_RECEIVE_WAITFOR, backup waits, and miscellaneous waits. All four filters default to true (existing behavior preserved). Settings are exposed in the Notifications section of both Dashboard and Lite Settings UIs and persisted to UserPreferences / settings.json. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * merged with incoming dev branch * Parameterized TOP/LIMIT value in Dashboard/Services/DatabaseService.NocHealth.cs and Lite/Services/LocalDataService.WaitStats.cs, and clamped the upper bound of the value to 1000 to avoid foot shooting. Removed blank lines as per Erik's request. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Sync plan viewer fixes from plan-b: spool labels, unmatched index detail (#416) - Spool operators now show Eager/Lazy prefix (e.g., "Eager Index Spool" instead of just "Index Spool") by prepending from LogicalOp - PlanIconMapper entries added for all Eager/Lazy spool variants - UnmatchedIndexes warning now parses child Parameterization elements to show specific database.schema.table.index names Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * fixes many warnings, and pre-calculates the RegEx patterns at compile time instead of at runtime: warning CA1847: Use 'string.Contains(char)' instead of 'string.Contains(string)' when searching for a single character (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1847) warning CA1875: Use 'Regex.Count' instead of 'Regex.Matches(...).Count' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1875) warning CA1863: Cache a 'CompositeFormat' for repeated use in this formatting operation (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1863) (#346) Co-authored-by: Orestes Zoupanos <orestes.zoupanos@tpr.gov.uk> * Complete GeneratedRegex conversion and remove Compiled flags (#420) PR #346 left Lite PlanAnalyzer with 3 old-style regex fields and inline Regex.IsMatch calls, and added unnecessary RegexOptions.Compiled to all GeneratedRegex attributes (source generator always compiles, flag is ignored). - Convert remaining Lite regex fields to use GeneratedRegex source generator - Convert all inline Regex.IsMatch calls to GeneratedRegex in both apps - Remove RegexOptions.Compiled from all [GeneratedRegex] attributes - Keep both PlanAnalyzer copies in sync with identical regex methods Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Add permissions section to README with least-privilege setup (#421) Documents required permissions for all platforms: on-prem Full/Lite, Azure SQL Database (contained user), Azure SQL MI, and AWS RDS. Includes copy-paste SQL scripts. Updates comparison table and troubleshooting to link to the new section. Prompted by user question in issue #418. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Replace custom TrayToolTip with plain ToolTipText to fix crash The custom visual TrayToolTip (Border + TextBlock) triggers a known race condition in Hardcodet.NotifyIcon.Wpf where Popup.CreateWindow throws "The root Visual of a VisualTarget cannot have a parent." This crash poisons the ReaderWriterLockSlim on the UI thread, breaking Overview queries permanently until restart. Plain string ToolTipText avoids the WPF Popup infrastructure entirely. Fixes #422 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add resilience to DuckDB read lock acquisition If an unhandled exception leaks a read lock on the UI thread, subsequent EnterReadLock() calls throw LockRecursionException permanently. Catch this and return a no-op disposable instead, since the thread already has read protection in place. This makes the lock self-healing: even if a crash leaks the lock, subsequent operations recover gracefully rather than failing forever. Fixes #423 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Restore custom TrayToolTip and silently handle Hardcodet crash (issue #422) Reverts the ToolTipText approach which caused context menu positioning and theme regressions. Instead, keeps the original custom TrayToolTip for proper dark theme styling and popup anchoring, and adds IsTrayToolTipCrash() detection in both apps' DispatcherUnhandledException handlers to silently swallow the rare Hardcodet VisualTarget race condition without showing error dialogs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix incorrect table name in Data Retention section README referenced config.retention_settings which doesn't exist. Retention is configured via the retention_days column in config.collection_schedule. Same bug reported in #223 but the fix never actually landed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix RID Lookup analyzer rule to match new PhysicalOp label (#429) The Key Lookup parser fix (PR #413) changes PhysicalOp from "RID Lookup" to "RID Lookup (Heap)". The analyzer rule used exact equality which no longer matched. Changed to StartsWith. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Add uninstall option to CLI and GUI installers (#431) Adds complete uninstall capability that removes all server-level objects: - 3 SQL Agent jobs (Collection, Data Retention, Hung Job Monitor) - 2 Extended Events sessions (BlockedProcess, Deadlock) - Server-side traces - PerformanceMonitor database Changes: - New install/00_uninstall.sql standalone script for SSMS users - CLI: --uninstall flag with interactive confirmation - GUI: Uninstall button (red, enabled when installed version detected) - Fix: existing clean install now removes all 3 jobs + XE sessions (previously missed Hung Job Monitor and both XE sessions) blocked process threshold (s) is intentionally NOT reset during uninstall as other monitoring tools may depend on it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * LOB compression + deduplication for query stats tables (#419) Databases were growing to 150-200 GB in under a week on busy servers. LOB columns (query_text, query_plan_text, query_sql_text, compilation_metrics) were 92-94% of storage. COMPRESS()/DECOMPRESS() achieves 90-91% reduction. Schema changes: - query_stats, query_store_data, procedure_stats: LOB columns changed from nvarchar(max)/xml to varbinary(max) with COMPRESS() on write - Dropped unused query_plan xml columns - Added row_hash binary(32) for deduplication - Added tracking tables (query_stats_latest_hash, procedure_stats_latest_hash, query_store_data_latest_hash) for efficient hash lookups Collector changes (08, 09, 10): - COMPRESS() on all text/plan INSERT expressions - collect_query/collect_plan flag support added to query_stats and procedure_stats - HASHBYTES('SHA2_256', binary_concat) dedup: only INSERT rows where metrics changed - MERGE source deduped with ROW_NUMBER() to prevent duplicate key errors Read-side changes: - All reporting views (46, 47) wrap compressed columns in DECOMPRESS() - Dashboard C# queries updated with DECOMPRESS() in SQL strings Upgrade path: - Batched DELETE WITH OUTPUT migration compresses existing data in place - IDENTITY reseed preserves ID continuity - Old tables renamed to _old for safety (manual DROP later) Version bump: 2.1.0 → 2.2.0 across all four projects. Tested: clean install (sql2016), CLI upgrade (sql2017, sql2019, sql2022), GUI upgrade (sql2025). All collectors healthy, dedup validated, all views and MCP tools return readable decompressed data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add RESTORING database filter to waiting_tasks collector (#430) The waiting_tasks collector joins sys.databases without filtering d.state = 0 (ONLINE), which means sessions running in the context of a RESTORING database (mirroring passive/AG secondary) can pass through to sys.dm_exec_sql_text and sys.dm_exec_text_query_plan. While the dumps in #430 turned out to be an internal SQL Server 2016 background thread crash (not our collector), this hardens the waiting_tasks collector to match the pattern already used in query_stats, procedure_stats, query_store, and file_io_stats collectors (PR #385). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add CI check to require version bump on PRs to main Ensures the Dashboard.csproj version has been bumped before merging dev into main. Only runs on PRs from dev to main. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Restore commercial support tiers to README The procurement/compliance support tiers were accidentally removed during the README restructure in PR #377. Restores the Supported ($500/yr) and Priority ($2,500/yr) tiers alongside the existing sponsorship and consulting sections. Reported in #436. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add wait stats query drill-down (#372) Right-click any wait type in the chart legend to see queries causing it. Classifies waits into categories (correlated, chain, uncapturable, filtered) and shows the appropriate data: correlated metrics for brief waits like SOS_SCHEDULER_YIELD, head blockers for lock waits (LCK_M_*), and direct wait-type filtering for everything else. Chain mode shows the same full column set with blocking path prepended — no jarring layout changes. Both Dashboard and Lite. No new collectors needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix poison wait false positives and alert log parsing (#445) The Lite poison wait query had no time filter, so stale data from days/weeks ago with high avg waits kept triggering alerts indefinitely. Added a 10-minute window matching Dashboard's existing filter. Also fixed alert history logging: non-numeric display strings (poison wait, LRQ, TempDB, job alerts) failed double.TryParse and logged as 0/0. Added optional numeric parameters to TrySendAlertEmailAsync so call sites can pass actual values for the DuckDB alert log while keeping display strings for emails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Cláudio Silva <claudiosil100@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Hannah Vernon <hannah@mvct.com> Co-authored-by: Orestes <MisterZeus@users.noreply.github.com> Co-authored-by: Orestes Zoupanos <orestes.zoupanos@tpr.gov.uk>
3 participants
What does this PR do?
Updates the following nuget packages:
Fixes many warnings, and pre-calculates the RegEx patterns at compile time instead of at runtime:
warning CA1847: Use 'string.Contains(char)' instead of 'string.Contains(string)' when searching for a single character (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1847)
warning CA1875: Use 'Regex.Count' instead of 'Regex.Matches(...).Count' (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1875)
warning CA1863: Cache a 'CompositeFormat' for repeated use in this formatting operation (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1863)
Which component(s) does this affect?
How was this tested?
Describe the testing you've done. Include:
Checklist
dotnet build -c Debug): we still get 71 instances of the CA1873 warnings, I'm just ignoring them to be honest :-/ Fixing any others I see though, see separate PR in a bit.