MCP budget: get_collection_health compacts healthy collectors under the size budget (#4198)

Darling/Darling.Tests/CollectionHealthPayloadBudgetLiveTests.cs

@@ -0,0 +1,229 @@
+/*
+ * 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.Linq;
+using System.Text;
+using System.Text.Json;
+using System.Threading.Tasks;
+using Npgsql;
+using PerformanceMonitor.Collectors;
+using PerformanceMonitor.Common;
+using PerformanceMonitor.Darling.Service.Mcp;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #4198: get_collection_health has no row to drop (every collector on the server is one row, and a health
+/// read must never hide a failing/stale/disabled/erroring one by leaving it off the page), so its default-size
+/// cut is per-field instead. This seeds every SQL Server catalog collector - the realistic per-server shape -
+/// mostly HEALTHY-and-boring, plus four deliberately NOT-boring rows that must never compact even though three
+/// of the four band HEALTHY, and measures the tool method's own UTF-8 byte count. Its own file/seeding per the
+/// #4198 common brief: not shared with any other lane's tonight.
+/// </summary>
+[Collection("live-postgres")]
+public sealed class CollectionHealthPayloadBudgetLiveTests
+{
+    private const string ServerName = "darling-collection-health-budget-e2e";
+    private static readonly int ServerId = ServerIdHelper.GetDeterministicHashCode(ServerName);
+
+    private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+
+    private readonly ITestOutputHelper _output;
+    public CollectionHealthPayloadBudgetLiveTests(ITestOutputHelper output) => _output = output;
+
+    /// <summary>No-rig pin: the web viewer's /api/read row must keep today's full per-collector payload, not
+    /// the new compact default. Source-scanned rather than rig-driven so it runs everywhere, including CI legs
+    /// with no Postgres rig.</summary>
+    [Fact]
+    public void WebViewerRow_PassesFullDetailTrue()
+    {
+        var path = FindRepoFile("Darling", "PerformanceMonitor.Darling.Service", "DarlingWebEndpoints.cs");
+        var source = System.IO.File.ReadAllText(path);
+        Assert.Contains(
+            "[\"get_collection_health\"] = (c, pg, an) => DarlingMcpDataTools.GetCollectionHealth(pg, Server(c), full_detail: true)",
+            source, StringComparison.Ordinal);
+    }
+
+    private static string FindRepoFile(params string[] relativeParts)
+    {
+        var dir = AppContext.BaseDirectory;
+        for (var i = 0; i < 8; i++)
+        {
+            var candidate = System.IO.Path.Combine(new[] { dir }.Concat(relativeParts).ToArray());
+            if (System.IO.File.Exists(candidate)) return candidate;
+            dir = System.IO.Path.GetDirectoryName(dir) ?? dir;
+        }
+        throw new System.IO.FileNotFoundException(System.IO.Path.Combine(relativeParts));
+    }
+
+    [Fact]
+    public async Task DefaultCall_StaysUnderBudget_AndNeverCompactsANonBoringCollector()
+    {
+        var cs = ConnectionString;
+        Assert.SkipWhen(string.IsNullOrEmpty(cs), "Set DARLING_TEST_PG to run the live get_collection_health budget test.");
+        var ct = TestContext.Current.CancellationToken;
+
+        using var connection = new NpgsqlConnection(cs);
+        await connection.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(connection, ct);
+        await DeleteRowsAsync(connection, ct);
+
+        await using var postgres = NpgsqlDataSource.Create(cs!);
+        var bodySucceeded = false;
+        try
+        {
+            await RegisterServerAsync(connection, ct);
+
+            var now = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified);
+            var sqlServerCollectors = CollectorCatalog.All
+                .Where(d => d.TargetEngine == CollectorTargetEngine.SqlServer)
+                .Select(d => d.Name)
+                .ToArray();
+            Assert.True(sqlServerCollectors.Length > 30, "expected a realistic SQL Server catalog width");
+
+            /* four collectors that must NEVER compact, three of them despite banding HEALTHY: */
+            var neverCompact = new[] { "wait_stats", "memory_grant_stats", "query_store_health", "database_scoped_config" };
+            Assert.All(neverCompact, name => Assert.Contains(name, sqlServerCollectors));
+
+            foreach (var name in sqlServerCollectors)
+            {
+                switch (name)
+                {
+                    case "wait_stats":
+                        /* FAILING band: recent ERROR runs, never a success, so HealthStatus itself excludes it. */
+                        for (var i = 0; i < 5; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 6), "ERROR", 120, null,
+                                "Login failed for user 'darling_monitor'.", ct);
+                        break;
+
+                    case "memory_grant_stats":
+                        /* WARNING band via a 30% error rate - HealthStatus alone would already exclude this
+                           one, but it also exercises errors > 0 beside a fresh fresh success. */
+                        for (var i = 0; i < 7; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 20 - 1), "SUCCESS", 80, 40, null, ct);
+                        for (var i = 0; i < 3; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 30 - 2), "ERROR", 90, null, "Timeout expired.", ct);
+                        break;
+
+                    case "query_store_health":
+                        /* HEALTHY band (a fresh success, 0 current errors) but PermissionDeniedCount > 0 from
+                           an OLDER denial this window - the case HealthStatus alone would miss and the reason
+                           IsCollectionHealthCompactEligible checks PermissionDeniedCount directly. */
+                        await InsertLogRowAsync(connection, name, now.AddDays(-6), "PERMISSIONS", 50, null,
+                            "permission denied for function pg_read_file", ct);
+                        await InsertLogRowAsync(connection, name, now.AddDays(-6).AddHours(-1), "PERMISSIONS", 50, null,
+                            "permission denied for function pg_read_file", ct);
+                        for (var i = 0; i < 4; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 12), "SUCCESS", 60, 12, null, ct);
+                        break;
+
+                    case "database_scoped_config":
+                        /* HEALTHY band, RowsStored = 0, and NOT an event collector - FormatOutputFinding's
+                           "needs a look" reading, which a compact row must never hide. */
+                        for (var i = 0; i < 8; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 18), "SUCCESS", 30, 0, null, ct);
+                        break;
+
+                    case "deadlocks":
+                        /* Event collector at rest: RowsStored = 0 but IsEventCollector is true, so this one
+                           SHOULD compact - the boring-empty case #1852/#3754 protect deliberately as healthy. */
+                        for (var i = 0; i < 8; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 18), "SUCCESS", 15, 0, null, ct);
+                        break;
+
+                    default:
+                        /* The realistic majority: plainly healthy and productive. */
+                        for (var i = 0; i < 6; i++)
+                            await InsertLogRowAsync(connection, name, now.AddHours(-i * 24 - 1), "SUCCESS", 100 + i * 15, 50 + i * 5, null, ct);
+                        break;
+                }
+            }
+
+            var defaultJson = await DarlingMcpDataTools.GetCollectionHealth(postgres, ServerName);
+            var defaultBytes = Encoding.UTF8.GetByteCount(defaultJson);
+            var fullJson = await DarlingMcpDataTools.GetCollectionHealth(postgres, ServerName, full_detail: true);
+            var fullBytes = Encoding.UTF8.GetByteCount(fullJson);
+            _output.WriteLine($"get_collection_health: default {defaultBytes:N0} bytes, full_detail=true {fullBytes:N0} bytes, budget {McpResponseBudget.DefaultBytes:N0}.");
+
+            Assert.True(defaultBytes <= McpResponseBudget.DefaultBytes,
+                $"default get_collection_health is {defaultBytes:N0} bytes, over the {McpResponseBudget.DefaultBytes:N0}-byte budget.");
+            Assert.True(defaultBytes < fullBytes, "the default call should be smaller than full_detail=true.");
+
+            using var defaultDoc = JsonDocument.Parse(defaultJson);
+            var defaultRows = defaultDoc.RootElement.GetProperty("collectors").EnumerateArray()
+                .ToDictionary(r => r.GetProperty("collector").GetString()!, r => r);
+            Assert.Equal(sqlServerCollectors.Length, defaultRows.Count);
+
+            foreach (var name in neverCompact)
+            {
+                Assert.True(defaultRows[name].TryGetProperty("errors", out _), $"{name} must keep full detail by default (it is not boring-healthy).");
+                Assert.False(defaultRows[name].TryGetProperty("compact", out _), $"{name} must not be marked compact.");
+            }
+            Assert.True(defaultRows["deadlocks"].TryGetProperty("compact", out var deadlocksCompact) && deadlocksCompact.GetBoolean(),
+                "an event collector resting at zero rows should compact.");
+            Assert.True(defaultRows.Values.Count(r => r.TryGetProperty("compact", out _)) >= sqlServerCollectors.Length - neverCompact.Length,
+                "every boring-healthy collector should compact.");
+
+            using var fullDoc = JsonDocument.Parse(fullJson);
+            Assert.All(fullDoc.RootElement.GetProperty("collectors").EnumerateArray(),
+                r => Assert.False(r.TryGetProperty("compact", out _), "full_detail=true must serve every field on every row."));
+
+            var note = defaultDoc.RootElement.GetProperty("collector_detail_note").GetString();
+            Assert.Contains($"of {sqlServerCollectors.Length} collector", note, StringComparison.Ordinal);
+
+            bodySucceeded = true;
+        }
+        finally
+        {
+            await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, async (cleanup, cleanupCt) => await DeleteRowsAsync(cleanup, cleanupCt));
+        }
+    }
+
+    private static async Task RegisterServerAsync(NpgsqlConnection connection, System.Threading.CancellationToken ct)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO servers (server_id, server_name, display_name, is_enabled, sql_major_version, created_date, modified_date)
+VALUES ($1, $2, $3, TRUE, 15, $4, $4)
+ON CONFLICT (server_id) DO UPDATE SET is_enabled = TRUE, sql_major_version = 15;", connection);
+        command.Parameters.AddWithValue(ServerId);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified));
+        await command.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task InsertLogRowAsync(
+        NpgsqlConnection connection, string collectorName, DateTime collectionTime, string status,
+        int durationMs, int? rowsCollected, string? errorMessage, System.Threading.CancellationToken ct)
+    {
+        using var command = new NpgsqlCommand(@"
+INSERT INTO collection_log (log_id, collection_time, server_id, server_name, collector_name, status, duration_ms, rows_collected, error_message)
+VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)", connection);
+        command.Parameters.AddWithValue(CollectionIdGenerator.Next());
+        command.Parameters.AddWithValue(DateTime.SpecifyKind(collectionTime, DateTimeKind.Unspecified));
+        command.Parameters.AddWithValue(ServerId);
+        command.Parameters.AddWithValue(ServerName);
+        command.Parameters.AddWithValue(collectorName);
+        command.Parameters.AddWithValue(status);
+        command.Parameters.AddWithValue(durationMs);
+        command.Parameters.AddWithValue((object?)rowsCollected ?? DBNull.Value);
+        command.Parameters.AddWithValue((object?)errorMessage ?? DBNull.Value);
+        await command.ExecuteNonQueryAsync(ct);
+    }
+
+    private static async Task DeleteRowsAsync(NpgsqlConnection connection, System.Threading.CancellationToken ct)
+    {
+        using var cleanup = new NpgsqlCommand(
+            $"DELETE FROM collection_log WHERE server_id = {ServerId}; DELETE FROM servers WHERE server_id = {ServerId};",
+            connection);
+        await cleanup.ExecuteNonQueryAsync(ct);
+    }
+}

Darling/Darling.Tests/McpToolsListBudget/DarlingMcpDataTools.txt

@@ -5,6 +5,7 @@ param get_blocking_stats.hours_back 191
 param get_blocking_stats.server_name 28
 
 tool get_collection_health 583
+param get_collection_health.full_detail 191
 param get_collection_health.server_name 28
 
 tool get_collection_log 606

Darling/Darling.Tests/McpToolsListBudgetTests.cs

@@ -105,6 +105,10 @@ so neither counts here. */
     /* #4198 (lane TB): +364 bytes for get_deadlock_detail's default-preview note in its served description
        and its new full_graph opt-in parameter (deadlock_graph_xml, the wide field, is now a 2000-char
        preview by default). */
+    /* #4198 (lane TK): +240 bytes for get_collection_health's new full_detail opt-in parameter (the head is
+       unchanged; the compaction rule lives in the tail get_tool_guide serves, not the served head). Default
+       calls now compact HEALTHY collectors with nothing to report, which took the default response from
+       41,669 bytes (measured, every field on every collector) to 20,707 bytes, under the shared 32 KB budget. */
 
     /* #4198 (lane TH): +193 bytes for get_active_queries' new full_query_text opt-in parameter and its
        limit description's #4198 note (query_text, the wide field, is now a 500-char preview by default;
@@ -140,7 +144,9 @@ combined total with blocking (#4267) changes on top. */
        combined total with qs-regressions (#4264) changes on top. */
     /* #4198 (analysis-findings, merge): re-measured after merging origin/dev (dev now includes #4261+#4258+#4265+#4267+#4264);
        combined total with analysis-findings (#4266) changes on top. */
-    private const int TotalCeilingBytes = 173_522;
+    /* #4198 (collection-health, merge): re-measured after merging origin/dev (dev now includes #4261+#4258+#4265+#4267+#4264+#4266);
+       combined total with collection-health (#4268) changes on top. */
+    private const int TotalCeilingBytes = 173_763;
 
     private const int ConvertedHeadCap = 1_000;
     private const int ConvertedParameterCap = 200;

Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs

@@ -2682,12 +2682,16 @@ budget cut does not silently shrink what the viewer renders. */
             ["get_trace_flag_changes"] = (c, pg, an) => DarlingMcpConfigHistoryTools.GetTraceFlagChanges(pg, Server(c), Hours(c, 168), as_of: AsOf(c)),
 
             /* ── core data reads ── */
-            ["get_collection_health"] = (c, pg, an) => DarlingMcpDataTools.GetCollectionHealth(pg, Server(c)),
+            /* #4198: full_detail=true keeps the web viewer's payload exactly what it was before the default
+               cut — every field on every collector row, never the compact shape a boring-healthy row gets
+               by default. */
+            ["get_collection_health"] = (c, pg, an) => DarlingMcpDataTools.GetCollectionHealth(pg, Server(c), full_detail: true),
             /* #4198: full_text: true, because error_message carried no preview cap before this PR — the
                web viewer keeps that behavior (an operator reading the Collection Log grid gets the whole
                error, the same way get_deadlock_detail's row passes TrendBudget.Chart-style overrides to
                hold its OWN pre-existing behavior steady). limit stays explicit at the pre-#4198 200, also
                unaffected by the new lower MCP default. */
+
             ["get_collection_log"] = (c, pg, an) => OptionalDouble(c, "min_duration_ms", out var minDurationMs)
                 ? DarlingMcpDataTools.GetCollectionLog(pg, Server(c), Hours(c, 24), Rows(c, "limit", 200), AsOf(c), Str(c, "collector_name"), minDurationMs, full_text: true)
                 : UnparseableParam("min_duration_ms"),

Darling/PerformanceMonitor.Darling.Service/wwwroot/js/view-templates.js

@@ -110,7 +110,9 @@ export const DASHBOARD_TEMPLATES = [
           {
             title: "Collection Health",
             read: "get_collection_health",
-            params: { server },
+            /* #4198: full_detail keeps every column below populated - errors, avg_duration_ms etc. are
+               omitted by default on a boring-healthy collector row, and this template reads them. */
+            params: { server, full_detail: true },
             viz: "table",
             span: 2,
             rowsKey: "collectors",