diff --git a/CHANGELOG.md b/CHANGELOG.md index c76de25efb..11ac311449 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Monitored-server connections use 32 KB TDS packets instead of the 8 KB default** ([#2164]) - the heavy collectors read rows carrying query text and execution-plan XML, and the client reads each row whole, so per-packet overhead is paid against payloads hundreds of KB wide. New per-database timing (open vs drain) measured on a cross-region fleet showed effective throughput tracking LOB size rather than total bytes: a database averaging ~180 KB per row moved 256 KB/s while one averaging ~41 KB per row moved 1,080 KB/s over the same link with the same code. Fewer, larger packets attack exactly that. A server that dislikes the larger size negotiates down rather than failing. + - **Force-plan findings carry a machine-first verdict for MCP consumers** ([#2138]; Erik: agents will read these more than people) - analyze_server and get_analysis_findings (both apps) now emit structured_remediation alongside the copy-paste command: per target, eligible plus NAMED blockers (parameter_sensitivity_cofired, secondary_replica_evidence), the evidence numbers, join keys, and split force_sql / unforce_sql / verify_sql artifacts (verify asks both post-force questions: did the force stick, and the per-interval cost since). The blocker list comes from FactRemediation.ForcePlanBlockers - the ONE policy gate a future auto-force feature would consult, so what agents inspect today is what any later automation enforces, as a testable data contract. Computed at read time from the persisted targets (never persisted itself - one source of truth, no DTO mirror to forget). - **Plan-regression detection scores CPU as the primary signal and gates on absolute spend** ([#2138] Phase 0, the advise-only foundation for the auto-force-plan bot) - the old score was GREATEST(cpu ratio, duration ratio), so a plan whose CPU never moved could fire on duration alone - but duration is confounded by blocking, IO waits and machine contention that no plan choice caused, exactly the false positive a plan-forcing bot must never act on. Now a CPU regression scores at its own ratio; a duration-dominant one fires only when EXTREME (>= 4x) AND corroborated by at least mild CPU worsening (>= 1.25x), scored at HALF the duration ratio so it competes honestly with CPU-detected rows. A new noise floor drops offenders whose latest plan burned under 10 CPU-seconds across the whole 14-day comparison window - a 12x ratio on a query costing 12ms per run is sampling jitter, not a finding. The regressed-queries drill-down previously kept its OWN copy of the old score and could surface rows the fact never counted; it now runs the same scoring, in both SKUs, pinned by split-signal tests that move CPU and duration independently. - **The force-plan recommendation now warns when the regressed query is parameter-sensitive** ([#2138] gap 3) - each regressed_queries row carries a `parameter_sensitivity_cofired` flag, computed inside the drill-down with the PARAMETER_SENSITIVITY detector's own thresholds (one cached plan whose per-execution cost varies >= 10x across parameter values, same floors, same window) joined by query hash - so the flag can never claim evidence the detector would not report. A flagged target's force-plan preview gains a caution block naming the risk (forcing pins ONE shape for every parameter value; the population that preferred the other plan inherits the wrong one permanently, quietly, because a forced plan no longer recompiles away) and the gentler first levers (statistics updates; PSP optimization / Query Store hints on 2022+), and the copy-paste surface gets a compact two-line version of the same warning. Unflagged targets render byte-identically to before. This flag is also the standing gate for the future auto-force bot: a flagged target is never auto-forced. Both SKUs, pinned by live tests in both stores. diff --git a/Darling/Darling.Tests/PacketSizeTuningTests.cs b/Darling/Darling.Tests/PacketSizeTuningTests.cs new file mode 100644 index 0000000000..0942e0ca09 --- /dev/null +++ b/Darling/Darling.Tests/PacketSizeTuningTests.cs @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using Microsoft.Data.SqlClient; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Service; +using Xunit; + +namespace Darling.Tests; + +/// +/// Pins the TDS packet size both apps apply to monitored-server connections (#2164). It exists because +/// the setting is invisible at runtime — a connection with the wrong packet size behaves correctly and +/// merely runs slowly, so nothing but a pin would notice it being dropped in a builder refactor. +/// +public sealed class PacketSizeTuningTests +{ + [Fact] + public void DarlingBuildsMonitoredConnectionsWithTheTunedPacketSize() + { + var cs = MonitoredServerConnection.BuildConnectionString(new MonitoredServer + { + Name = "s", + Host = "example.database.windows.net", + Auth = "integrated", + }); + + var parsed = new SqlConnectionStringBuilder(cs); + Assert.Equal(CollectorTdsTuning.MonitoredServerPacketSize, parsed.PacketSize); + } + + [Fact] + public void TunedPacketSizeIsWithinWhatTheDriverAndProtocolAccept() + { + /* The driver accepts 512-32768 and SQL Server negotiates down to the smaller of the two ends, so a + value in range can never fail a connection — it can only fail to help. A value OUTSIDE the range + throws at connect time, which on a monitoring fleet means every server going dark at once, so + the bound matters more than the tuning does. */ + Assert.InRange(CollectorTdsTuning.MonitoredServerPacketSize, 512, 32768); + + /* Not the 8 KB default — if someone "cleans up" the constant back to the default, this fails + rather than silently reverting the measurement this was built on. */ + Assert.NotEqual(8000, CollectorTdsTuning.MonitoredServerPacketSize); + Assert.NotEqual(8192, CollectorTdsTuning.MonitoredServerPacketSize); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs b/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs index 05dc33009b..df22cd530b 100644 --- a/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs +++ b/Darling/PerformanceMonitor.Darling.Service/MonitoredServerConnection.cs @@ -8,6 +8,7 @@ using System; using Microsoft.Data.SqlClient; +using PerformanceMonitor.Common; namespace PerformanceMonitor.Darling.Service; @@ -37,6 +38,9 @@ public static string BuildConnectionString(MonitoredServer server, string? resol MultipleActiveResultSets = true, ApplicationIntent = server.ReadOnlyIntent ? ApplicationIntent.ReadOnly : ApplicationIntent.ReadWrite, MultiSubnetFailover = server.MultiSubnetFailover, + /* #2164: fewer, larger TDS packets for the plan-XML/query-text payloads the heavy + collectors read. See CollectorTdsTuning for the measurement that motivated it. */ + PacketSize = CollectorTdsTuning.MonitoredServerPacketSize, }; /* Encrypt fail-closed: unknown/blank modes get Mandatory, matching Lite. */ diff --git a/Lite/Models/ServerConnection.cs b/Lite/Models/ServerConnection.cs index 6f19726ee8..c695de376c 100644 --- a/Lite/Models/ServerConnection.cs +++ b/Lite/Models/ServerConnection.cs @@ -391,7 +391,10 @@ private string BuildConnectionString(ResolvedAuth auth) TrustServerCertificate = TrustServerCertificate, MultipleActiveResultSets = true, ApplicationIntent = ReadOnlyIntent ? ApplicationIntent.ReadOnly : ApplicationIntent.ReadWrite, - MultiSubnetFailover = MultiSubnetFailover + MultiSubnetFailover = MultiSubnetFailover, + /* #2164: fewer, larger TDS packets for the plan-XML/query-text payloads the heavy + collectors read. Shared constant so Lite and Darling cannot drift. */ + PacketSize = PerformanceMonitor.Common.CollectorTdsTuning.MonitoredServerPacketSize }; // Set encryption mode diff --git a/PerformanceMonitor.Common/CollectorTdsTuning.cs b/PerformanceMonitor.Common/CollectorTdsTuning.cs new file mode 100644 index 0000000000..e425099445 --- /dev/null +++ b/PerformanceMonitor.Common/CollectorTdsTuning.cs @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +namespace PerformanceMonitor.Common; + +/// +/// TDS-level transport settings both apps apply when they build a monitored-server connection string +/// (#2164). One definition so Lite and Darling cannot drift apart on how they talk to a monitored +/// instance — the two builders are otherwise independent code. +/// +public static class CollectorTdsTuning +{ + /// + /// The TDS packet size, in bytes, for monitored-server connections. Neither app used to set this, so + /// every connection ran at the driver's 8 KB default. + /// + /// Why it matters here. The heavy collectors read rows carrying two + /// nvarchar(max) columns — query text and execution-plan XML — and the client reads each row + /// whole before the loop sees it. Measured on a cross-region production fleet, effective drain + /// throughput tracked LOB size rather than total bytes: a database averaging ~180 KB per row moved + /// 256 KB/s while one averaging ~41 KB per row moved 1,080 KB/s over the same link, same code. That + /// inverse relationship is per-packet overhead, and 8 KB packets mean a 180 KB plan costs ~22 packets + /// where 32 KB packets cost ~6. + /// + /// 32 KB is the largest value SQL Server negotiates (the protocol maximum is 32,767; the driver + /// accepts 512–32,768 and both ends agree down to the smaller). Chosen over an intermediate value + /// because the payloads at issue are hundreds of KB — the whole point is fewer, larger packets — and + /// because a server that dislikes it negotiates down rather than failing. + /// + /// Cost of being wrong: a larger packet size raises per-connection network-buffer memory + /// on both ends. At the fleet's connection count (one per server per sweep, bounded by the sweep + /// width) that is kilobytes, not megabytes — cheap enough that it is not worth a knob until someone + /// shows it hurts. This is measured against a fleet where it should help most; if the numbers say + /// otherwise it comes back out, and the comment records what evidence would justify that. + /// + public const int MonitoredServerPacketSize = 32768; +}