diff --git a/CHANGELOG.md b/CHANGELOG.md index 430161dc3f..7081f0a6cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Server Inventory's `Last Updated` was a config-snapshot time wearing a freshness label** ([#2359]) - `server_properties` ships with `FrequencyMinutes 0`, which the schedule table defines as "collect once on server load only", so the column showed the last SERVICE START and every actively-monitored server on a long-running install looked days stale. Renamed to `Inventory As Of`, and a new `Last Collected` column carries the real heartbeat from `v_collection_log` - the same signal `list_servers` uses. Darling only: Lite reads live and stamps the value at read time, so its column was never misleading. Reported by @ghauan, who correctly pushed back when the first fix did not explain their case. +- **get_query_store_top reported a window it could not serve** ([#2364]) - it reads raw `query_store_stats`, which is dropped at 4 days on a store with the rollups armed, so a 30-day request returned at most four days with `hours_back: 720` echoed back unchanged. It cannot be routed to a rollup the way #2353 was: the corrected CAGGs carry no `query_id` or `plan_id`, and plan identity is the whole point of the tool. So the response now carries `effective_start`, `effective_hours_back`, `truncated` and a note naming why, and the empty path no longer blames Query Store configuration for a window it never read. Found while scoping #2357 with @carlei1978. - **Server Inventory's Last Updated looked broken on decommissioned servers** ([#2359]) - the grid lists every REGISTERED server and never read `servers.is_enabled`, so a server removed from monitoring kept the `collection_time` it had when collection stopped. Accurate, and read by everyone as a stale freshness column. Measured on a 61-server fleet the split was total: 19 disabled servers last collected within five minutes of each other on the day they were removed, 42 enabled servers all fresh within a minute. A new Monitoring column now says Active or Stopped. The disabled rows are KEPT, not filtered - this is the FinOps tab and a decommissioned server's cost history is what it exists to show. Reported by @ghauan. - **Extended-length paths no longer slip past the install-location guards** ([#2348]) - `\\?\UNC\server\share` is the long spelling of a REAL share and `\\?\C:\Users\bob` of a REAL profile, but the wholesale `\\?\` exclusion waved both through undiagnosed, because skipping a check is not the same as passing it. Both implementations now strip the prefix BEFORE classifying, so the long spelling gets the same verdict as the short one, and `\\?\C:\PerformanceMonitorDarling` - an ordinary local root written the long way - is still correctly left alone. The shared decision table and the cross-language parity test hold the service and `install-darling.ps1` to it together. - **get_query_trend silently truncated any window past 4 days** ([#2353]) - it read only the raw `query_stats` table, whose raw tier is dropped at 4 days independently of the collector's much longer advertised retention, while echoing `hours_back: 168` back unchanged. Worse than a short array: the empty path asserted "No history found ... within the last 168 hours" over a span it never read, which for a query whose history had aged out is a false statement rather than an incomplete one. It now routes to the hourly rollup by the age of the window's OLDEST point measured from wall clock, and reports `source`, `effective_hours_back`, `bucket` and `truncated` so the response describes the data instead of the request. Columns the rollup does not carry come back NULL, never zero - on an aggregate row a zero would read as "none observed". Reported by @carlei1978. @@ -2822,6 +2823,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#2340]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2340 [#2344]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2344 [#2362]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2362 +[#2364]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2364 [#2347]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2347 [#2359]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2359 [#2348]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2348 diff --git a/Darling/Darling.Tests/QueryStoreTopWindowTests.cs b/Darling/Darling.Tests/QueryStoreTopWindowTests.cs new file mode 100644 index 0000000000..58dd0eab93 --- /dev/null +++ b/Darling/Darling.Tests/QueryStoreTopWindowTests.cs @@ -0,0 +1,135 @@ +/* + * 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 System; +using System.IO; +using System.Runtime.CompilerServices; +using PerformanceMonitor.Darling.Service.Mcp; +using Xunit; + +namespace Darling.Tests; + +/// +/// #2364: get_query_store_top reports the window it actually served. +/// +/// The defect. It reads raw query_store_stats, and on a store with the rollups armed that +/// table is dropped at four days — measured: a 4d 13h span over 17,162,516 rows under a +/// policy_retention {"drop_after": "4 days"}. A caller asking for 30 days therefore got at most four, +/// with hours_back: 720 echoed back unchanged and nothing marking the difference. +/// +/// Why this could not be fixed the way #2353 was. That one routed to the hourly rollup. Here there +/// is no route: QueryStoreTopSql groups by database_name, query_id, plan_id, query_hash, +/// replica_role and the corrected CAGG groups by database_name, module_name, query_hash — no +/// query_id, no plan_id. Plan identity is the entire purpose of this tool, and a rollup grained to +/// it would approach the size of the raw data. So the fix is honesty, not routing. +/// +public class QueryStoreTopWindowTests +{ + private static string ReaderSource => ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingDataReader.cs")); + + private static string ToolSource => ReadRepoFile(Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpDataTools.cs")); + + /// + /// The floor probe is bounded on BOTH sides of the window. Bounding the partitioning column is what lets + /// TimescaleDB exclude chunks; an unbounded MIN would read the retention window to answer a question + /// about it. + /// + [Fact] + public void TheWindowFloorProbe_IsBoundedOnBothSides() + { + var sql = DarlingDataReader.QueryStoreWindowFloorSql; + + Assert.Contains("MIN(collection_time)", sql, StringComparison.Ordinal); + Assert.Contains("FROM query_store_stats", sql, StringComparison.Ordinal); + Assert.Contains("collection_time >=", sql, StringComparison.Ordinal); + Assert.Contains("collection_time <=", sql, StringComparison.Ordinal); + } + + /// + /// The floor must come from a probe, never from the returned rows. The result set is the top N by + /// COST, so its timestamps say nothing about how far back the read reached — the most expensive query in a + /// month may have run this morning. Deriving the window from the rows would produce a confident, wrong + /// answer, which is worse than the silence it replaces. + /// + [Fact] + public void TheEffectiveWindow_ComesFromTheProbe_NotTheRows() + { + Assert.Contains("GetQueryStoreWindowFloorAsync", ToolSource, StringComparison.Ordinal); + + var at = ToolSource.IndexOf("var effectiveStart =", StringComparison.Ordinal); + Assert.True(at >= 0, "the tool no longer computes an effective window (#2364)"); + + var line = ToolSource[at..ToolSource.IndexOf(';', at)]; + Assert.Contains("floor", line, StringComparison.Ordinal); + + /* Not from the projection: rows are ordered by cost, and LastExecutionTime is a per-query fact. */ + Assert.DoesNotContain("rows.Min(", ToolSource, StringComparison.Ordinal); + Assert.DoesNotContain("rows.Max(", ToolSource, StringComparison.Ordinal); + } + + /// The payload describes the data, not just the request. + [Fact] + public void ThePayload_CarriesTheServedWindow() + { + Assert.Contains("effective_start", ToolSource, StringComparison.Ordinal); + Assert.Contains("effective_hours_back", ToolSource, StringComparison.Ordinal); + Assert.Contains("truncated", ToolSource, StringComparison.Ordinal); + + /* hours_back is still echoed — the caller needs to see what it asked for beside what it got. */ + Assert.Contains("hours_back,", ToolSource, StringComparison.Ordinal); + } + + /// + /// The empty path must not assert absence over a span it never read. The old message said Query Store "may + /// not be enabled", which for a window reaching past raw retention is a confident wrong diagnosis — an agent + /// acts on it by going to look at a Query Store configuration that is fine. + /// + [Fact] + public void TheEmptyPath_NamesTheWindowSearched_AndDoesNotOnlyBlameConfiguration() + { + var at = ToolSource.IndexOf("No Query Store rows for this server", StringComparison.Ordinal); + Assert.True(at >= 0, "the empty message no longer names the window searched (#2364)"); + + var message = ToolSource[at..Math.Min(ToolSource.Length, at + 700)]; + + Assert.Contains("hours_back", message, StringComparison.Ordinal); + Assert.Contains("4 days", message, StringComparison.Ordinal); + Assert.Contains("shorter window", message, StringComparison.Ordinal); + } + + /// + /// The tool still reads RAW, and that is deliberate rather than an oversight — pinned so a well-meaning + /// "route it to the rollup like #2353" edit has to confront why it cannot. + /// + [Fact] + public void TheTool_StillReadsRaw_BecauseTheRollupCannotCarryPlanIdentity() + { + Assert.Contains("FROM query_store_stats", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal); + Assert.DoesNotContain("query_store_stats_corrected", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal); + Assert.DoesNotContain("query_store_stats_hourly", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal); + + /* plan_id is the reason: it is a grouping key here and absent from every rollup. */ + Assert.Contains("plan_id", DarlingDataReader.QueryStoreTopSql, StringComparison.Ordinal); + } + + private static string ReadRepoFile(string relative, [CallerFilePath] string thisFile = "") + { + for (var dir = new DirectoryInfo(Path.GetDirectoryName(thisFile)!); dir is not null; dir = dir.Parent) + { + var candidate = Path.Combine(dir.FullName, relative); + if (File.Exists(candidate)) + { + return File.ReadAllText(candidate); + } + } + + throw new FileNotFoundException($"Could not locate {relative} walking up from {thisFile}"); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs index fd265bee70..432d4f53c6 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs @@ -879,6 +879,41 @@ public static async Task> GetTopProceduresByCpuAsync( /* ─────────────────────────── query store ─────────────────────────── */ + /// + /// The oldest collection_time this server actually has inside the requested window (#2364). + /// + /// Why a separate probe rather than reading the returned rows. + /// returns the top N by COST, not by time, so the timestamps on those rows say nothing about how far back + /// the window reaches — the most expensive query in a month might have run this morning. The window floor is + /// a property of the tier, not of the result set, and has to be asked for separately. + /// + /// Bounded on both sides, so it prunes chunks and answers from an ordered scan that stops at the first + /// row rather than reading the window. $1 server_id, $2/$3 window (naive UTC). + /// + public const string QueryStoreWindowFloorSql = """ + SELECT MIN(collection_time) + FROM query_store_stats + WHERE server_id = $1 + AND collection_time >= $2 + AND collection_time <= $3 + """; + + /// + /// Reads . Null when the window holds nothing at all, which the caller + /// reports as "nothing was read" rather than as an absence of activity. + /// + public static async Task GetQueryStoreWindowFloorAsync( + NpgsqlDataSource postgres, int serverId, DateTime startUtc, DateTime endUtc, + CancellationToken cancellationToken = default) + { + await using var command = postgres.CreateCommand(QueryStoreWindowFloorSql); + DarlingMcpReadParameters.AddInt(command, serverId); + DarlingMcpReadParameters.AddTimestamp(command, startUtc); + DarlingMcpReadParameters.AddTimestamp(command, endUtc); + var value = await command.ExecuteScalarAsync(cancellationToken); + return value is DateTime dt ? dt : null; + } + /// /// Top Query Store groups over the window — a focused projection of the viewer's /// QueryStoreTopSql (the columns Lite's get_query_store_top returns): group by diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs index 3a3f89b5bd..202840f3da 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs @@ -663,9 +663,27 @@ public static async Task GetQueryStoreTop( try { var now = DateTime.UtcNow; - var rows = await DarlingDataReader.GetQueryStoreTopAsync(postgres, resolved.ServerId, now.AddHours(-hours_back), now, top, database_name); + var requestedStart = now.AddHours(-hours_back); + var rows = await DarlingDataReader.GetQueryStoreTopAsync(postgres, resolved.ServerId, requestedStart, now, top, database_name); + + /* #2364: what the window ACTUALLY holds. The rows above are the top N by COST, so their timestamps + say nothing about how far back the read reached -- the most expensive query in a month may have + run this morning. raw query_store_stats is dropped at 4 days on a store with the rollups armed, + and this tool has no rollup to fall back to (the corrected CAGGs carry no query_id or plan_id, + and plan identity is the whole point of this tool). So the honest move is to report the window + that was served rather than echo the one that was asked for. */ + var floor = await DarlingDataReader.GetQueryStoreWindowFloorAsync(postgres, resolved.ServerId, requestedStart, now); + var effectiveStart = floor ?? requestedStart; + var truncated = floor is DateTime f && f > requestedStart.AddMinutes(90); + if (rows.Count == 0) - return McpHelpers.Status("unavailable", "No Query Store data available. Query Store may not be enabled on target databases."); + return McpHelpers.Status( + "unavailable", + $"No Query Store rows for this server in the {hours_back}-hour window searched. Query Store " + + "may not be enabled on the target databases -- or the window reaches past what the raw tier " + + "retains (query_store_stats is dropped at 4 days when the rollups are armed), in which case " + + "nothing was read for the older part of it. Try a shorter window before concluding the " + + "queries did not run."); var result = rows.Select(r => new { @@ -693,6 +711,16 @@ to tell them apart. NULL when the server did not attribute the row. */ { server = resolved.ServerName, hours_back, + /* #2364: what was served, beside what was asked for. hours_back alone was a request echoed + back as though it described the data. */ + effective_start = effectiveStart.ToString("o"), + effective_hours_back = Math.Round((now - effectiveStart).TotalHours, 1), + truncated, + truncation_note = truncated + ? "The window reaches further back than this server's raw query_store_stats retains, so the " + + "older part of it was not read. This tool reads the raw tier only: the corrected rollups " + + "carry no query_id or plan_id, and plan identity is what it exists to return." + : null, queries = result }, McpHelpers.JsonOptions); }