From 061d2c1969daff4ba5842e933b0ebe4be098f9d0 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:17:03 -0400 Subject: [PATCH 1/9] Recommendations skip databases that are a secondary copy on this node: shared rule, fact table, Lite engine (#5558) --- Lite.Tests/LatestValueLookbackTests.cs | 4 +- Lite.Tests/SecondaryReplicaScopeTests.cs | 432 ++++++++++++++++++ Lite/Analysis/AnalysisService.cs | 9 + Lite/Analysis/AnomalyDetector.cs | 4 + Lite/Analysis/DrillDownCollector.Config.cs | 3 + Lite/Analysis/DrillDownCollector.Queries.cs | 4 + Lite/Analysis/DrillDownCollector.Storage.cs | 3 + Lite/Analysis/DuckDbFactCollector.Config.cs | 4 + .../Analysis/DuckDbFactCollector.QueryPerf.cs | 4 + Lite/Analysis/DuckDbFactCollector.Storage.cs | 5 +- Lite/Analysis/SecondaryReplicaScope.cs | 138 ++++++ .../AnalysisContext.cs | 10 + .../FactReplicaScope.cs | 126 +++++ PerformanceMonitor.Common/AgReplicaScope.cs | 122 +++++ 14 files changed, 865 insertions(+), 3 deletions(-) create mode 100644 Lite.Tests/SecondaryReplicaScopeTests.cs create mode 100644 Lite/Analysis/SecondaryReplicaScope.cs create mode 100644 PerformanceMonitor.Analysis/FactReplicaScope.cs create mode 100644 PerformanceMonitor.Common/AgReplicaScope.cs diff --git a/Lite.Tests/LatestValueLookbackTests.cs b/Lite.Tests/LatestValueLookbackTests.cs index d579747adb..be097947df 100644 --- a/Lite.Tests/LatestValueLookbackTests.cs +++ b/Lite.Tests/LatestValueLookbackTests.cs @@ -530,7 +530,7 @@ INSERT INTO file_io_stats VALUES ($1, $2, $3, 'latest-value-lookback', $4, $5, 'ROWS', $6)", Interlocked.Decrement(ref s_nextId), at, serverId, database, file, sizeMb); - private static Task InsertSizeAsync(DuckDBConnection connection, int serverId, DateTime at, + public static Task InsertSizeAsync(DuckDBConnection connection, int serverId, DateTime at, string database, int fileId, string fileType, string file, decimal totalMb, bool percentGrowth, string volume, decimal volumeTotalMb, decimal volumeFreeMb) => InsertAsync(connection, @" @@ -563,7 +563,7 @@ INSERT INTO memory_stats VALUES ($1, $2, $3, 'latest-value-lookback', $4, $5, $6)", Interlocked.Decrement(ref s_nextId), at, serverId, physicalMb, physicalMb / 2, physicalMb * 3 / 4); - private static Task InsertDatabaseConfigAsync(DuckDBConnection connection, int serverId, DateTime capturedAt, + public static Task InsertDatabaseConfigAsync(DuckDBConnection connection, int serverId, DateTime capturedAt, string database, bool autoShrink, bool rcsiOn) => InsertAsync(connection, @" INSERT INTO database_config diff --git a/Lite.Tests/SecondaryReplicaScopeTests.cs b/Lite.Tests/SecondaryReplicaScopeTests.cs new file mode 100644 index 0000000000..1cfde86a6f --- /dev/null +++ b/Lite.Tests/SecondaryReplicaScopeTests.cs @@ -0,0 +1,432 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; +using PerformanceMonitorLite.Analysis; +using PerformanceMonitorLite.Database; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #5558: the pure rule that names the databases this node holds only as a secondary copy. Every fail-open case +/// is pinned: skipping a database wrongly hides a finding, so anything unknown skips nothing. +/// +public sealed class AgReplicaScopeTests +{ + private static readonly DateTime End = new(2026, 10, 8, 12, 0, 0, DateTimeKind.Utc); + + private static AgReplicaReading Replica(string ag, string role, bool? local = true, string server = "NODE1") => + new(ag, server, role, "CONNECTED", local); + + private static AgDatabaseMembership Db(string ag, string db, bool? local = true) => new(ag, db, local); + + [Fact] + public void ThePrimary_SkipsNothing() + { + var set = AgReplicaScope.SecondaryDatabases([Replica("AG1", "PRIMARY")], [Db("AG1", "Sales")]); + Assert.Empty(set); + } + + [Fact] + public void TheSecondary_SkipsItsDatabases_AndOnlyTheLocalRows() + { + var set = AgReplicaScope.SecondaryDatabases( + [Replica("AG1", "SECONDARY"), Replica("AG1", "PRIMARY", local: false, server: "NODE2")], + [Db("AG1", "Sales"), Db("AG1", "Orders"), Db("AG1", "RemoteOnly", local: false)]); + Assert.Equal(["Orders", "Sales"], set.OrderBy(n => n).ToArray()); + } + + [Fact] + public void TwoGroupsWithDifferentPrimaries_AreJudgedPerGroup() + { + var set = AgReplicaScope.SecondaryDatabases( + [Replica("AG1", "PRIMARY"), Replica("AG2", "SECONDARY")], + [Db("AG1", "OnPrimary"), Db("AG2", "OnSecondary")]); + Assert.Equal(["OnSecondary"], set.ToArray()); + } + + [Theory] + [InlineData("RESOLVING")] + [InlineData("")] + [InlineData(null)] + [InlineData("UNKNOWN")] + public void ARoleThatIsNotExplicitlySecondary_SkipsNothing(string? role) + { + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", role!)], [Db("AG1", "Sales")])); + } + + [Fact] + public void ANullIsLocal_OnEitherGrain_SkipsNothing() + { + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", "SECONDARY", local: null)], [Db("AG1", "Sales")])); + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", "SECONDARY")], [Db("AG1", "Sales", local: null)])); + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", "SECONDARY", local: false)], [Db("AG1", "Sales")])); + } + + [Fact] + public void NoRows_OrAStandaloneDatabase_SkipsNothing() + { + Assert.Empty(AgReplicaScope.SecondaryDatabases(null, null)); + Assert.Empty(AgReplicaScope.SecondaryDatabases([], [])); + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", "SECONDARY")], [])); + Assert.Empty(AgReplicaScope.SecondaryDatabases([], [Db("AG1", "Sales")])); + /* A database in a group the replica snapshot does not know about. */ + Assert.Empty(AgReplicaScope.SecondaryDatabases([Replica("AG1", "SECONDARY")], [Db("AG9", "Sales")])); + } + + [Fact] + public void Names_AreMatchedWithoutRegardToCase_AndRoleCaseDoesNotMatter() + { + var set = AgReplicaScope.SecondaryDatabases([Replica("ag1", "secondary")], [Db("AG1", "Sales")]); + Assert.Contains("SALES", set); + Assert.Contains("sales", set); + } + + [Fact] + public void TwoLocalRowsThatDisagree_AreATornSnapshot_AndSkipNothing() + { + Assert.Empty(AgReplicaScope.SecondaryDatabases( + [Replica("AG1", "SECONDARY"), Replica("AG1", "PRIMARY")], [Db("AG1", "Sales")])); + } + + [Fact] + public void AStaleOrMissingSnapshot_SkipsNothing_AndAFutureOneIsNotUsed() + { + var replicas = new[] { Replica("AG1", "SECONDARY") }; + var databases = new[] { Db("AG1", "Sales") }; + + Assert.Single(AgReplicaScope.SecondaryDatabases(replicas, End.AddMinutes(-1), databases, End.AddMinutes(-2), End)); + Assert.Empty(AgReplicaScope.SecondaryDatabases(replicas, End.AddHours(-2), databases, End.AddMinutes(-1), End)); + Assert.Empty(AgReplicaScope.SecondaryDatabases(replicas, End.AddMinutes(-1), databases, End.AddHours(-2), End)); + Assert.Empty(AgReplicaScope.SecondaryDatabases(replicas, null, databases, End.AddMinutes(-1), End)); + Assert.Empty(AgReplicaScope.SecondaryDatabases(replicas, End.AddMinutes(-1), databases, End.AddMinutes(1), End)); + } + + [Fact] + public void TheNote_IsOneSentence_ForOneAndMany_AndNullForNone() + { + Assert.Null(AgReplicaScope.SkippedNote(0)); + Assert.Contains("1 database skipped", AgReplicaScope.SkippedNote(1)); + var many = AgReplicaScope.SkippedNote(3); + Assert.StartsWith("3 databases skipped: this server holds a secondary copy of them in an availability group.", many); + Assert.EndsWith("Check the primary replica for their findings.", many); + } +} + +/// #5558: the table that classifies facts, and the census that keeps it complete. +public sealed class FactReplicaScopeCoverageTests +{ + [Theory] + [InlineData("DB_CONFIG")] + [InlineData("DB_CONFIG_RCSI")] + [InlineData("FILE_AUTOGROWTH_PERCENT")] + [InlineData("PLAN_REGRESSION")] + [InlineData("ANOMALY_OBJECT_GROWTH")] + public void TheReplicatedFacts_AreFiltered(string key) => Assert.Equal(FactReplicaKind.Replicated, FactReplicaScope.Of(key)); + + [Theory] + [InlineData("MISSING_INDEX")] + [InlineData("PLAN_WARNING")] + [InlineData("BAD_ACTOR_0xF00D")] + [InlineData("PARAMETER_SENSITIVITY")] + [InlineData("QUERY_SPILLS")] + [InlineData("QUERY_HIGH_DOP")] + [InlineData("PROCEDURE_STATS")] + [InlineData("IO_READ_LATENCY_MS")] + [InlineData("DATABASE_TOTAL_SIZE_MB")] + [InlineData("DISK_SPACE")] + [InlineData("TEMPDB_USAGE")] + [InlineData("DEADLOCKS")] + [InlineData("ANOMALY_OBJECT_CONTENTION")] + [InlineData("CONFIG_MAXDOP")] + [InlineData("CXPACKET")] + public void TheNodeLocalFacts_AreKept(string key) => Assert.Equal(FactReplicaKind.NodeLocal, FactReplicaScope.Of(key)); + + [Fact] + public void AReplicatedKey_YieldsNames_OnlyWhenTheContextHasASecondarySet() + { + var empty = new AnalysisContext(); + Assert.Empty(FactReplicaScope.SecondariesFor(empty, "DB_CONFIG")); + + var context = new AnalysisContext { SecondaryReplicaDatabases = new HashSet(StringComparer.OrdinalIgnoreCase) { "SecDb" } }; + Assert.Equal(["secdb"], FactReplicaScope.SecondariesFor(context, "DB_CONFIG")); + Assert.Empty(FactReplicaScope.SecondariesFor(context, "MISSING_INDEX")); + } + + /// Every fact key a collector writes as a literal must be in the table, so a new fact is classified on + /// purpose. Both products' collectors are scanned: Darling's port emits the same keys as Lite's. + [Fact] + public void EveryLiteralFactKeyTheCollectorsEmit_IsInTheTable() + { + var root = FindRepoRoot(); + var sources = new List(); + sources.AddRange(Directory.GetFiles(Path.Combine(root, "Lite", "Analysis"), "*.cs")); + var darling = Path.Combine(root, "Darling", "PerformanceMonitor.Darling.Analysis"); + sources.AddRange(Directory.GetFiles(darling, "PgFactCollector*.cs")); + sources.Add(Path.Combine(darling, "PgAnomalyDetector.cs")); + + var emitted = new SortedSet(StringComparer.Ordinal); + var pattern = new Regex("\\bKey = \"([A-Z][A-Z0-9_]+)\"", RegexOptions.CultureInvariant); + foreach (var file in sources) + foreach (Match m in pattern.Matches(File.ReadAllText(file))) + emitted.Add(m.Groups[1].Value); + + Assert.NotEmpty(emitted); + var missing = emitted.Where(k => !FactReplicaScope.IsKnown(k)).ToArray(); + Assert.True(missing.Length == 0, "fact keys missing from FactReplicaScope: " + string.Join(", ", missing)); + } + + private static string FindRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !Directory.Exists(Path.Combine(dir.FullName, "Lite", "Analysis"))) dir = dir.Parent; + return dir?.FullName ?? throw new InvalidOperationException("repo root not found"); + } +} + +/// +/// #5558 on a seeded DuckDB store: the replicated facts drop the database this node holds as a secondary copy and +/// the node-local ones keep it. Three databases: SecDb (local secondary), PrimDb (local primary of a +/// second group) and StandDb (not in a group). +/// +public sealed class SecondaryReplicaFactTests : IClassFixture +{ + private const int ServerId = -558_001; + private static long s_id = -55_800_000; + + private readonly DuckDbInitializer _duckDb; + + public SecondaryReplicaFactTests(SharedDuckDbFixture fixture) + { + fixture.ResetData(); + _duckDb = fixture.DuckDb; + } + + private static DateTime Now() => LatestValueSeed.TruncateToSeconds(DateTime.UtcNow); + + private async Task SeedAsync(Func seed) + { + using var readLock = _duckDb.AcquireReadLock(); + using var connection = _duckDb.CreateConnection(); + await connection.OpenAsync(TestContext.Current.CancellationToken); + await seed(connection); + } + + private static async Task ExecAsync(DuckDBConnection connection, string sql, params object[] values) + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = sql; + foreach (var v in values) cmd.Parameters.Add(new DuckDBParameter { Value = v }); + await cmd.ExecuteNonQueryAsync(TestContext.Current.CancellationToken); + } + + private static async Task SeedAgAsync(DuckDBConnection c, DateTime at, string secondaryRole = "SECONDARY") + { + const string Replica = "INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,'n',$4,$5,$6,$7)"; + await ExecAsync(c, Replica, Interlocked.Decrement(ref s_id), at, ServerId, "AG1", "NODE1", secondaryRole, true); + await ExecAsync(c, Replica, Interlocked.Decrement(ref s_id), at, ServerId, "AG1", "NODE2", "PRIMARY", false); + await ExecAsync(c, Replica, Interlocked.Decrement(ref s_id), at, ServerId, "AG2", "NODE1", "PRIMARY", true); + const string Db = "INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,'n',$4,$5,$6,$7)"; + await ExecAsync(c, Db, Interlocked.Decrement(ref s_id), at, ServerId, "AG1", "SecDb", "NODE1", true); + await ExecAsync(c, Db, Interlocked.Decrement(ref s_id), at, ServerId, "AG1", "SecDb", "NODE2", false); + await ExecAsync(c, Db, Interlocked.Decrement(ref s_id), at, ServerId, "AG2", "PrimDb", "NODE1", true); + } + + private static async Task SeedDatabasesAsync(DuckDBConnection c, DateTime end) + { + foreach (var db in new[] { "SecDb", "PrimDb", "StandDb" }) + { + await LatestValueSeed.InsertDatabaseConfigAsync(c, ServerId, end.AddDays(-1), db, autoShrink: true, rcsiOn: false); + await LatestValueSeed.InsertSizeAsync(c, ServerId, end.AddMinutes(-30), db, 1, "ROWS", db + "_data", 20_480, true, "D:\\", 1_000_000, 500_000); + await LatestValueSeed.InsertFileIoAsync(c, ServerId, end.AddMinutes(-5), db, db + "_data", 1000); + } + } + + private static AnalysisContext Context(DateTime end) => new() + { + ServerId = ServerId, + ServerName = "ag-scope", + TimeRangeStart = end.AddHours(-4), + TimeRangeEnd = end, + }; + + private async Task<(Dictionary Facts, AnalysisContext Context)> FactsAsync(DateTime end) + { + var context = Context(end); + await SecondaryReplicaScope.EnsureAsync(_duckDb, context); + var facts = (await new DuckDbFactCollector(_duckDb).CollectFactsAsync(context)).ToDictionary(f => f.Key); + return (facts, context); + } + + [Fact] + public async Task OnASecondary_DbConfigAndAutogrowthDropTheSecondaryDatabase_AndNodeLocalFactsKeepIt() + { + var end = Now(); + await SeedAsync(async c => { await SeedDatabasesAsync(c, end); await SeedAgAsync(c, end.AddMinutes(-1)); }); + + var (facts, context) = await FactsAsync(end); + + Assert.Equal(["SecDb"], context.SecondaryReplicaDatabases!.ToArray()); + var config = facts["DB_CONFIG"]; + Assert.Equal(2, config.Metadata["database_count"]); + Assert.Equal(2, config.Metadata["auto_shrink_on_count"]); + Assert.Equal(2, config.Metadata["rcsi_off_count"]); + + var autogrowth = facts["FILE_AUTOGROWTH_PERCENT"]; + Assert.Equal(2, autogrowth.Value); + Assert.Equal(2, autogrowth.Metadata["database_count"]); + + /* Node-local: the secondary database's own I/O footprint is still this node's. */ + Assert.Equal(3000.0, facts["DATABASE_TOTAL_SIZE_MB"].Value, precision: 6); + + var drill = new DrillDownCollector(_duckDb); + var files = await LatestValueSeed.AutogrowthFilesAsync(drill, context); + Assert.Equal(["PrimDb", "StandDb"], files.Select(f => f.GetProperty("database").GetString()!).OrderBy(n => n).ToArray()); + } + + [Fact] + public async Task TheDbConfigDrillDown_ListsTheSameDatabasesTheFactCounted() + { + var end = Now(); + await SeedAsync(async c => { await SeedDatabasesAsync(c, end); await SeedAgAsync(c, end.AddMinutes(-1)); }); + var (_, context) = await FactsAsync(end); + + var finding = new AnalysisFinding + { + ServerId = ServerId, RootFactKey = "DB_CONFIG", StoryPath = "DB_CONFIG", PathKeys = ["DB_CONFIG"], Severity = 0.3, + }; + await new DrillDownCollector(_duckDb).EnrichFindingsAsync([finding], context); + + var json = System.Text.Json.JsonSerializer.Serialize(finding.DrillDown); + Assert.Contains("PrimDb", json); + Assert.Contains("StandDb", json); + Assert.DoesNotContain("SecDb", json); + } + + [Fact] + public async Task OnThePrimary_OrWithNoOrStaleAgRows_NothingIsSkipped() + { + var end = Now(); + + /* The local role is PRIMARY for the group. */ + await SeedAsync(async c => { await SeedDatabasesAsync(c, end); await SeedAgAsync(c, end.AddMinutes(-1), secondaryRole: "PRIMARY"); }); + var (primary, primaryContext) = await FactsAsync(end); + Assert.Empty(primaryContext.SecondaryReplicaDatabases!); + Assert.Equal(3, primary["DB_CONFIG"].Metadata["database_count"]); + } + + [Fact] + public async Task WithNoAgRows_OrStaleOnes_NothingIsSkipped() + { + var end = Now(); + await SeedAsync(c => SeedDatabasesAsync(c, end)); + var (noRows, noRowsContext) = await FactsAsync(end); + Assert.Empty(noRowsContext.SecondaryReplicaDatabases!); + Assert.Equal(3, noRows["DB_CONFIG"].Metadata["database_count"]); + + /* Rows exist, but the newest is three hours old: stale, so the role vouches for nothing. */ + await SeedAsync(c => SeedAgAsync(c, end.AddHours(-3))); + var (stale, staleContext) = await FactsAsync(end); + Assert.Empty(staleContext.SecondaryReplicaDatabases!); + Assert.Equal(3, stale["DB_CONFIG"].Metadata["database_count"]); + } + + [Fact] + public async Task AnAsOfPass_UsesTheRoleAtItsOwnEnd_NotTheCurrentOne() + { + var now = Now(); + /* Primary three hours ago, secondary a minute ago (a failover in between). */ + await SeedAsync(async c => + { + await SeedDatabasesAsync(c, now); + await SeedAgAsync(c, now.AddHours(-3).AddMinutes(-1), secondaryRole: "PRIMARY"); + await SeedAgAsync(c, now.AddMinutes(-1)); + }); + + var current = await SecondaryReplicaScope.ReadAsync(_duckDb, ServerId, now, CancellationToken.None); + Assert.Equal(["SecDb"], current.ToArray()); + + var earlier = await SecondaryReplicaScope.ReadAsync(_duckDb, ServerId, now.AddHours(-3), CancellationToken.None); + Assert.Empty(earlier); + } + + [Fact] + public async Task PlanRegression_DropsTheSecondaryDatabase() + { + var end = Now(); + await SeedAsync(async c => + { + await SeedAgAsync(c, end.AddMinutes(-1)); + foreach (var db in new[] { "SecDb", "StandDb" }) + { + await SeedRegressionAsync(c, end, db, planId: 1, hash: "0xGOOD", avgCpu: 100_000, lastExec: end.AddDays(-5), firstExec: end.AddDays(-6)); + await SeedRegressionAsync(c, end, db, planId: 2, hash: "0xBAD", avgCpu: 1_200_000, lastExec: end, firstExec: end.AddDays(-1)); + } + }); + + var (facts, context) = await FactsAsync(end); + + Assert.Equal(1, facts["PLAN_REGRESSION"].Metadata["offender_count"]); + Assert.All(context.PlanRegressionOffenders!, o => Assert.Equal("StandDb", o.DatabaseName)); + } + + [Fact] + public async Task ObjectGrowth_DropsTheSecondaryDatabase_ButTheContentionAnomalyIsNotTouched() + { + var end = Now(); + await SeedAsync(async c => + { + await SeedAgAsync(c, end.AddMinutes(-1)); + foreach (var (db, grownMb) in new[] { ("SecDb", 900m), ("StandDb", 500m) }) + { + await SeedObjectAsync(c, end.AddDays(-1), db, 100m); + await SeedObjectAsync(c, end, db, 100m + grownMb); + } + for (var d = 1; d <= 5; d++) + await ExecAsync(c, "INSERT INTO cpu_utilization_stats (collection_id, collection_time, server_id, server_name, sample_time, sqlserver_cpu_utilization, other_process_cpu_utilization) VALUES ($1,$2,$3,'n',$2,10,0)", + Interlocked.Decrement(ref s_id), end.AddDays(-d), ServerId); + }); + + var context = Context(end); + await SecondaryReplicaScope.EnsureAsync(_duckDb, context); + var detector = new AnomalyDetector(_duckDb, new BaselineProvider(_duckDb)); + var anomalies = await detector.DetectAnomaliesAsync(context); + + var growth = Assert.Single(anomalies, f => f.Key == "ANOMALY_OBJECT_GROWTH"); + Assert.Equal("StandDb", growth.DatabaseName); + } + + private static Task SeedRegressionAsync(DuckDBConnection c, DateTime end, string db, long planId, string hash, long avgCpu, + DateTime lastExec, DateTime firstExec) => + ExecAsync(c, @" +INSERT INTO query_store_stats + (collection_id, collection_time, server_id, server_name, database_name, query_id, plan_id, execution_type_desc, + first_execution_time, last_execution_time, query_text, query_hash, execution_count, avg_cpu_time_us, + avg_duration_us, query_plan_hash, is_forced_plan, force_failure_count) +VALUES ($1,$2,$3,'n',$4,101,$5,'Regular',$6,$7,'SELECT 1','0xQH',100,$8,$8,$9,false,0)", + Interlocked.Decrement(ref s_id), end, ServerId, db, planId, firstExec, lastExec, avgCpu, hash); + + private static Task SeedObjectAsync(DuckDBConnection c, DateTime at, string db, decimal reservedMb) => + ExecAsync(c, @" +INSERT INTO index_object_stats + (collection_id, collection_time, server_id, server_name, sqlserver_start_time, database_name, database_id, + schema_name, object_id, table_name, index_id, index_name, index_type_desc, reserved_mb, used_mb, total_rows, + user_seeks, user_scans, user_lookups, user_updates, row_lock_wait_in_ms, index_lock_promotion_count) +VALUES ($1,$2,$3,'n',$4,$5,7,'dbo',100,'Big',1,'PK_Big','CLUSTERED',$6,$6,1000,0,0,0,0,0,0)", + Interlocked.Decrement(ref s_id), at, ServerId, at.AddDays(-10), db, reservedMb); +} diff --git a/Lite/Analysis/AnalysisService.cs b/Lite/Analysis/AnalysisService.cs index d567df690d..e4bbba9b46 100644 --- a/Lite/Analysis/AnalysisService.cs +++ b/Lite/Analysis/AnalysisService.cs @@ -218,6 +218,10 @@ public async Task> AnalyzeAsync(AnalysisContext context) /* Filled once per pass at the one door every path goes through. */ context.SeparatelyMonitoredDatabases ??= ResolveSeparatelyMonitoredDatabases(context.ServerId); + /* #5558: the databases this node holds only as a secondary copy, at the window's end (so an AsOf pass + uses the role at that time). Fails open: any unknown leaves it empty. */ + await SecondaryReplicaScope.EnsureAsync(_duckDb, context); + IsAnalyzing = true; InsufficientDataMessage = null; WindowEmptyMessage = null; @@ -572,6 +576,7 @@ cancellation really produces — narrowed to the one shape that arises here. */ CancellationToken = cancellationToken }; context.SeparatelyMonitoredDatabases ??= ResolveSeparatelyMonitoredDatabases(serverId); + await SecondaryReplicaScope.EnsureAsync(_duckDb, context); /* #5558 */ try { @@ -673,6 +678,10 @@ not be swallowed into an empty result and logged as a fault. */ }; baselineContext.SeparatelyMonitoredDatabases ??= ResolveSeparatelyMonitoredDatabases(serverId); comparisonContext.SeparatelyMonitoredDatabases = baselineContext.SeparatelyMonitoredDatabases; + /* #5558: NOT copied across like the list above: the two windows can straddle a failover, and each window + uses the role at its own end. */ + await SecondaryReplicaScope.EnsureAsync(_duckDb, baselineContext); + await SecondaryReplicaScope.EnsureAsync(_duckDb, comparisonContext); try { diff --git a/Lite/Analysis/AnomalyDetector.cs b/Lite/Analysis/AnomalyDetector.cs index 069511acba..4189c32b5f 100644 --- a/Lite/Analysis/AnomalyDetector.cs +++ b/Lite/Analysis/AnomalyDetector.cs @@ -176,10 +176,14 @@ prv AS (SELECT database_name, object_id, SUM(reserved_mb) mb WHERE (SELECT t FROM latest) <> (SELECT t FROM prior) AND cur.mb - prv.mb >= $2 AND (CASE WHEN prv.mb > 0 THEN (cur.mb - prv.mb) * 100.0 / prv.mb ELSE 0 END) >= $3 +/*SEC*/ ORDER BY growth_mb DESC LIMIT 1"; cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); cmd.Parameters.Add(new DuckDBParameter { Value = ObjectGrowthMbThreshold }); cmd.Parameters.Add(new DuckDBParameter { Value = ObjectGrowthPctThreshold }); + /* #5558: object sizes (reserved_mb) mirror the primary, so a secondary copy's growth is the primary's + to report. Only this growth query is filtered; the contention anomaly stays node-local. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "ANOMALY_OBJECT_GROWTH", "cur.database_name", 4); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (await reader.ReadAsync(context.CancellationToken)) diff --git a/Lite/Analysis/DrillDownCollector.Config.cs b/Lite/Analysis/DrillDownCollector.Config.cs index 6fe49f3d93..49f038754f 100644 --- a/Lite/Analysis/DrillDownCollector.Config.cs +++ b/Lite/Analysis/DrillDownCollector.Config.cs @@ -29,11 +29,14 @@ private async Task CollectConfigIssues(AnalysisFinding finding, AnalysisContext FROM v_database_config WHERE server_id = $1 AND capture_time = (SELECT MAX(capture_time) FROM v_database_config WHERE server_id = $1) +/*SEC*/ AND (is_auto_shrink_on = true OR is_auto_close_on = true OR is_read_committed_snapshot_on = false OR page_verify_option != 'CHECKSUM') ORDER BY database_name"; cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); + /* #5558: the same databases the fact skipped. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "DB_CONFIG", "database_name", 2); var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Lite/Analysis/DrillDownCollector.Queries.cs b/Lite/Analysis/DrillDownCollector.Queries.cs index 9f86498f90..8dd497e322 100644 --- a/Lite/Analysis/DrillDownCollector.Queries.cs +++ b/Lite/Analysis/DrillDownCollector.Queries.cs @@ -632,6 +632,7 @@ FROM v_query_store_stats -- subset. AND ($5::VARCHAR[] IS NULL OR (list_contains($5::VARCHAR[], database_name) AND list_contains($6::BIGINT[], query_id))) + /*SEC*/ ), plan_agg AS ( @@ -784,6 +785,9 @@ ORDER BY regression_factor DESC /* $7 (#4821): the psp_signature filter's first-pass bound, opened by an hour. The exact compiled-before-the-window test for the co-fired flag is made below, per row. */ cmd.Parameters.Add(new DuckDBParameter { Value = PlanCreationClock.RoughBound(context.TimeRangeStart) }); + /* $8 and up (#5558): the same databases the PLAN_REGRESSION fact skipped, for the pass where the fact did not + run and the offender list above is NULL. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "PLAN_REGRESSION", "database_name", 8); var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Lite/Analysis/DrillDownCollector.Storage.cs b/Lite/Analysis/DrillDownCollector.Storage.cs index f0eefc968a..4971835976 100644 --- a/Lite/Analysis/DrillDownCollector.Storage.cs +++ b/Lite/Analysis/DrillDownCollector.Storage.cs @@ -99,12 +99,15 @@ FROM latest AND is_percent_growth = true AND total_size_mb >= 10240 AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/ ORDER BY total_size_mb DESC LIMIT 50"; cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); cmd.Parameters.Add(new DuckDBParameter { Value = context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name) }); cmd.Parameters.Add(new DuckDBParameter { Value = context.TimeRangeEnd }); + /* #5558: the same databases the fact skipped. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name", 4); var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Lite/Analysis/DuckDbFactCollector.Config.cs b/Lite/Analysis/DuckDbFactCollector.Config.cs index 1bfa7f3d5c..64905dd344 100644 --- a/Lite/Analysis/DuckDbFactCollector.Config.cs +++ b/Lite/Analysis/DuckDbFactCollector.Config.cs @@ -173,6 +173,7 @@ WITH latest AS ( FROM v_database_config WHERE server_id = $1 AND capture_time = (SELECT MAX(capture_time) FROM v_database_config WHERE server_id = $1) + /*SEC*/ ) SELECT COUNT(*) AS database_count, @@ -189,6 +190,9 @@ FROM v_database_config AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb')"; cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); + /* #5558: a database this node holds only as a secondary copy is the primary's to report (and drops out + of every count here, database_count included). */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "DB_CONFIG", "database_name", 2); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Lite/Analysis/DuckDbFactCollector.QueryPerf.cs b/Lite/Analysis/DuckDbFactCollector.QueryPerf.cs index 426e0cd7cf..5618ad3b2e 100644 --- a/Lite/Analysis/DuckDbFactCollector.QueryPerf.cs +++ b/Lite/Analysis/DuckDbFactCollector.QueryPerf.cs @@ -349,6 +349,7 @@ FROM v_query_store_stats WHERE server_id = $1 AND execution_type_desc = 'Regular' AND last_execution_time >= $2 + /*SEC*/ ), plan_agg AS ( @@ -470,6 +471,9 @@ ORDER BY regression_factor DESC cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); cmd.Parameters.Add(new DuckDBParameter { Value = context.TimeRangeStart.AddDays(-14) }); + /* #5558: Query Store on a secondary copy is the primary's content, and rows from the days this node was + primary stay in the window, so the read skips those databases. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "PLAN_REGRESSION", "database_name", 3); var offenderCount = 0; var worstFactor = 0.0; diff --git a/Lite/Analysis/DuckDbFactCollector.Storage.cs b/Lite/Analysis/DuckDbFactCollector.Storage.cs index 7658491148..81faaa7f94 100644 --- a/Lite/Analysis/DuckDbFactCollector.Storage.cs +++ b/Lite/Analysis/DuckDbFactCollector.Storage.cs @@ -272,11 +272,14 @@ FROM latest WHERE rn = 1 AND is_percent_growth = true AND total_size_mb >= 10240 -AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb')"; +AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/"; cmd.Parameters.Add(new DuckDBParameter { Value = context.ServerId }); cmd.Parameters.Add(new DuckDBParameter { Value = context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name) }); cmd.Parameters.Add(new DuckDBParameter { Value = context.TimeRangeEnd }); + /* #5558: the file growth setting replicates from the primary; a secondary copy is the primary's to report. */ + cmd.CommandText = SecondaryReplicaScope.Apply(cmd.CommandText, cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name", 4); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Lite/Analysis/SecondaryReplicaScope.cs b/Lite/Analysis/SecondaryReplicaScope.cs new file mode 100644 index 0000000000..2f878e525d --- /dev/null +++ b/Lite/Analysis/SecondaryReplicaScope.cs @@ -0,0 +1,138 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor Lite. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using DuckDB.NET.Data; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; +using PerformanceMonitorLite.Database; +using PerformanceMonitorLite.Services; + +namespace PerformanceMonitorLite.Analysis; + +/// +/// #5558: resolves from the store's own +/// ag_replica_states / ag_database_replica_states snapshots, at or before the pass's window end +/// (so an AsOf pass and each compare_analysis window use the role at THAT time, not the current one). The decision +/// is 's; this class only reads the rows. Every failure is "skip nothing". +/// +internal static class SecondaryReplicaScope +{ + /// Fills the set once per context. Never throws except an abandonment (#2443): a read fault logs and + /// leaves the pass unfiltered. + internal static async Task EnsureAsync(DuckDbInitializer duckDb, AnalysisContext context) + { + if (context.SecondaryReplicaDatabases is not null) return; + context.SecondaryReplicaDatabases = await ReadAsync(duckDb, context.ServerId, context.TimeRangeEnd, context.CancellationToken); + } + + /// The secondary databases as of ; empty on every failure or unknown. + internal static async Task> ReadAsync( + DuckDbInitializer duckDb, int serverId, DateTime asOfUtc, CancellationToken cancellationToken) + { + try + { + using var readLock = duckDb.AcquireReadLock(cancellationToken); + using var connection = duckDb.CreateConnection(); + await connection.OpenAsync(cancellationToken); + + var replicaTime = await NewestAsync(connection, "ag_replica_states", serverId, asOfUtc, cancellationToken); + var databaseTime = await NewestAsync(connection, "ag_database_replica_states", serverId, asOfUtc, cancellationToken); + if (!AgReplicaScope.IsFresh(replicaTime, asOfUtc) || !AgReplicaScope.IsFresh(databaseTime, asOfUtc)) + return new HashSet(StringComparer.OrdinalIgnoreCase); + + var replicas = new List(); + using (var cmd = connection.CreateCommand()) + { + cmd.CommandText = @" +SELECT ag_name, replica_server_name, role_desc, is_local +FROM ag_replica_states +WHERE server_id = $1 +AND collection_time = $2"; + cmd.Parameters.Add(new DuckDBParameter { Value = serverId }); + cmd.Parameters.Add(new DuckDBParameter { Value = DateTime.SpecifyKind(replicaTime!.Value, DateTimeKind.Unspecified) }); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0)) continue; + replicas.Add(new AgReplicaReading( + AgName: reader.GetString(0), + ReplicaServerName: reader.IsDBNull(1) ? "" : reader.GetString(1), + RoleDesc: reader.IsDBNull(2) ? null : reader.GetString(2), + ConnectedStateDesc: null, + IsLocal: reader.IsDBNull(3) ? null : reader.GetBoolean(3))); + } + } + + var databases = new List(); + using (var cmd = connection.CreateCommand()) + { + cmd.CommandText = @" +SELECT ag_name, database_name, is_local +FROM ag_database_replica_states +WHERE server_id = $1 +AND collection_time = $2"; + cmd.Parameters.Add(new DuckDBParameter { Value = serverId }); + cmd.Parameters.Add(new DuckDBParameter { Value = DateTime.SpecifyKind(databaseTime!.Value, DateTimeKind.Unspecified) }); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0) || reader.IsDBNull(1)) continue; + databases.Add(new AgDatabaseMembership( + reader.GetString(0), reader.GetString(1), reader.IsDBNull(2) ? null : reader.GetBoolean(2))); + } + } + + return AgReplicaScope.SecondaryDatabases(replicas, databases); + } + catch (Exception ex) when (!AnalysisAbandon.IsExpected(ex, cancellationToken)) + { + AppLogger.Warn("SecondaryReplicaScope", + $"Availability Group role lookup failed for server {serverId}; no database is skipped this pass: {ex.Message}"); + return new HashSet(StringComparer.OrdinalIgnoreCase); + } + } + + private static async Task NewestAsync( + DuckDBConnection connection, string table, int serverId, DateTime asOfUtc, CancellationToken cancellationToken) + { + using var cmd = connection.CreateCommand(); + /* The table name is a constant of this class, never input. */ + cmd.CommandText = "SELECT MAX(collection_time) FROM " + table + " WHERE server_id = $1 AND collection_time <= $2"; + cmd.Parameters.Add(new DuckDBParameter { Value = serverId }); + cmd.Parameters.Add(new DuckDBParameter { Value = DateTime.SpecifyKind(asOfUtc, DateTimeKind.Unspecified) }); + var value = await cmd.ExecuteScalarAsync(cancellationToken); + return value is DateTime at ? DateTime.SpecifyKind(at, DateTimeKind.Utc) : null; + } + + /// The marker a replicated read puts in its SQL where the secondary-database predicate goes. + internal const string Marker = "/*SEC*/"; + + /// Replaces in with the predicate that drops the skipped + /// databases of () and adds the parameters, numbered from + /// . With nothing to skip the marker just disappears, so the SQL is the same as + /// before and no parameter is added. + internal static string Apply(string sql, System.Data.Common.DbCommand cmd, AnalysisContext context, string factKey, + string column, int firstParameter) + { + var names = FactReplicaScope.SecondariesFor(context, factKey); + foreach (var name in names) cmd.Parameters.Add(new DuckDBParameter { Value = name }); + return sql.Replace(Marker, FactReplicaScope.PositionalFilter(names, column, firstParameter)); + } + + /// The note for the tabs (Recommendations, FinOps): the shared sentence when this node holds a secondary + /// copy of any database right now, else null. Reads the current role, like the tab it sits on. + internal static async Task NoteAsync(DuckDbInitializer duckDb, int serverId, CancellationToken cancellationToken = default) + { + var set = await ReadAsync(duckDb, serverId, DateTime.UtcNow, cancellationToken); + return AgReplicaScope.SkippedNote(set); + } +} diff --git a/PerformanceMonitor.Analysis/AnalysisContext.cs b/PerformanceMonitor.Analysis/AnalysisContext.cs index 82f2af8685..08fc128302 100644 --- a/PerformanceMonitor.Analysis/AnalysisContext.cs +++ b/PerformanceMonitor.Analysis/AnalysisContext.cs @@ -25,6 +25,16 @@ public class AnalysisContext /// findings come from the databases' own targets. Null or empty changes nothing. /// public IReadOnlyList? SeparatelyMonitoredDatabases { get; set; } + + /// + /// The databases this node holds only as a SECONDARY copy in an Availability Group (#5558), resolved once per + /// pass at the same door as from the store's own AG snapshots at or + /// before . The facts whose data replicates from the primary + /// () skip these databases, so the primary reports each finding once. Null or + /// empty changes nothing: every unknown (collector off, stale rows, standalone, Azure SQL Database) leaves it + /// null or empty, so the filter fails open. + /// + public IReadOnlySet? SecondaryReplicaDatabases { get; set; } public string ServerName { get; set; } = string.Empty; public DateTime TimeRangeStart { get; set; } public DateTime TimeRangeEnd { get; set; } diff --git a/PerformanceMonitor.Analysis/FactReplicaScope.cs b/PerformanceMonitor.Analysis/FactReplicaScope.cs new file mode 100644 index 0000000000..e7703c0d02 --- /dev/null +++ b/PerformanceMonitor.Analysis/FactReplicaScope.cs @@ -0,0 +1,126 @@ +/* + * 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.Collections.Generic; +using System.Linq; + +namespace PerformanceMonitor.Analysis; + +/// Whether a fact's data is the same on every replica of an Availability Group, or belongs to this node. +public enum FactReplicaKind +{ + /// Real work or state of THIS node (a query that ran here, a wait, blocking, a disk). Kept on a + /// secondary: it happened here and no other node reports it. + NodeLocal, + + /// Data that replicates from the primary (database settings, Query Store content, the shape of the + /// objects), so every node would report the same finding for the same database. Skipped for the databases + /// this node holds only as a secondary copy (). + Replicated, +} + +/// +/// The one table that decides which per-database facts are filtered on a secondary replica (#5558). A wrong call +/// is a one-line flip here; the collectors ask and never hard-code a fact key. +/// A key that is in neither list, and not in a family, is : an unknown fact +/// is kept, so the filter fails open. FactReplicaScopeCoverageTests fails when a fact key a collector emits +/// is missing from this table, so a new fact is a deliberate choice rather than a silent default. +/// +public static class FactReplicaScope +{ + /// The replicated facts. DB_CONFIG is database settings (sys.databases); FILE_AUTOGROWTH_PERCENT is the + /// file growth setting (sys.master_files, and the ALTER it suggests cannot run on a secondary either); + /// PLAN_REGRESSION is Query Store content, which a secondary shows as the primary's; ANOMALY_OBJECT_GROWTH is + /// computed from reserved_mb, which mirrors the primary. + private static readonly string[] ReplicatedKeys = + [ + "DB_CONFIG", + "FILE_AUTOGROWTH_PERCENT", + "PLAN_REGRESSION", + "ANOMALY_OBJECT_GROWTH", + ]; + + /// The facts that stay on every node: server-level state, and per-database work that ran on this node. + private static readonly string[] NodeLocalKeys = + [ + // Per-database, node-local: real workload on this node (a readable secondary can run reporting). + "MISSING_INDEX", "PLAN_WARNING", "BAD_ACTOR", "PARAMETER_SENSITIVITY", "QUERY_SPILLS", "QUERY_HIGH_DOP", + "PROCEDURE_STATS", "PLAN_CACHE_BLOAT", "IO_READ_LATENCY_MS", "IO_WRITE_LATENCY_MS", + "DATABASE_TOTAL_SIZE_MB", "DISK_SPACE", "TEMPDB_USAGE", "BLOCKING_EVENTS", "BLOCKING_CHAIN", "DEADLOCKS", + "ANOMALY_OBJECT_CONTENTION", + // Server-level. + "TRACE_FLAGS", "SERVER_EDITION", "SERVER_MAJOR_VERSION", "SESSION_STATS", "RUNNING_JOBS", "RUNNABLE_TASKS", + "ACTIVE_QUERIES", "CPU_SQL_PERCENT", "CPU_SPIKE", "MEMORY_TOTAL_PHYSICAL_MB", "MEMORY_TARGET_MB", + "MEMORY_PRESSURE_EVENTS", "MEMORY_GRANT_PENDING", "MEMORY_CLERKS", "MEMORY_BUFFER_POOL_MB", + // Server-level anomalies. + "ANOMALY_WRITE_LATENCY", "ANOMALY_WAIT_PROFILE", "ANOMALY_SESSION_SPIKE", "ANOMALY_READ_LATENCY", + "ANOMALY_QUERY_DURATION", "ANOMALY_MEMORY_PRESSURE", "ANOMALY_DEADLOCK_SPIKE", "ANOMALY_CPU_SPIKE", + "ANOMALY_BLOCKING_SPIKE", "ANOMALY_BATCH_REQUESTS", + ]; + + /// Families whose keys are built at run time: server settings (CONFIG_*, server level), one fact per + /// bad-actor query (BAD_ACTOR_*, a query that ran on this node), and DB_CONFIG sub-keys (replicated). + private static readonly (string Prefix, FactReplicaKind Kind)[] Families = + [ + ("DB_CONFIG_", FactReplicaKind.Replicated), + ("BAD_ACTOR_", FactReplicaKind.NodeLocal), + ("CONFIG_", FactReplicaKind.NodeLocal), + ]; + + private static readonly Dictionary Table = BuildTable(); + + private static Dictionary BuildTable() + { + var table = new Dictionary(StringComparer.Ordinal); + foreach (var key in ReplicatedKeys) table[key] = FactReplicaKind.Replicated; + foreach (var key in NodeLocalKeys) table[key] = FactReplicaKind.NodeLocal; + return table; + } + + /// Every literal key in the table, for the coverage test. + public static IReadOnlyCollection KnownKeys => Table.Keys; + + /// True when the key is in the table or one of its families. Wait-type facts (one key per wait type) + /// are not in the table: they are all node-local and fall to the default. + public static bool IsKnown(string key) => + Table.ContainsKey(key) || Families.Any(f => key.StartsWith(f.Prefix, StringComparison.Ordinal)); + + /// The key's kind. Unknown keys are node-local: kept, never filtered. + public static FactReplicaKind Of(string key) + { + if (Table.TryGetValue(key, out var kind)) return kind; + foreach (var (prefix, familyKind) in Families) + if (key.StartsWith(prefix, StringComparison.Ordinal)) return familyKind; + return FactReplicaKind.NodeLocal; + } + + /// The lower-cased database names a read must skip on this pass: the context's + /// secondary set when the key is , else empty. A read builds its + /// predicate from a non-empty answer and leaves its SQL byte-identical on an empty one. + public static string[] SecondariesFor(AnalysisContext context, string factKey) + { + if (context.SecondaryReplicaDatabases is not { Count: > 0 } set) return []; + if (Of(factKey) != FactReplicaKind.Replicated) return []; + return LowerNames(set); + } + + /// The set's names lower-cased, de-duplicated and ordered, for a SQL lower(database_name) compare. + public static string[] LowerNames(IEnumerable names) => + names.Select(n => n.ToLowerInvariant()).Distinct(StringComparer.Ordinal).OrderBy(n => n, StringComparer.Ordinal).ToArray(); + + /// A DuckDB predicate fragment (leading AND) that drops rows of the skipped databases, with positional + /// parameters starting at ; empty when there is nothing to skip. A NULL + /// database passes. Add the values with in the same order. + public static string PositionalFilter(string[] lowerNames, string column, int firstParameter) + { + if (lowerNames.Length == 0) return string.Empty; + var placeholders = string.Join(", ", lowerNames.Select((_, i) => "$" + (firstParameter + i))); + return " AND (" + column + " IS NULL OR lower(" + column + ") NOT IN (" + placeholders + "))"; + } +} diff --git a/PerformanceMonitor.Common/AgReplicaScope.cs b/PerformanceMonitor.Common/AgReplicaScope.cs new file mode 100644 index 0000000000..493a74c13d --- /dev/null +++ b/PerformanceMonitor.Common/AgReplicaScope.cs @@ -0,0 +1,122 @@ +/* + * 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.Collections.Generic; +using System.Globalization; + +namespace PerformanceMonitor.Common; + +/// One database-on-one-replica row from the ag_database_replica_states snapshot, reduced to the +/// columns the replica scope judges (#5558). Every column but the names is nullable on purpose: a NULL +/// IsLocal (a row from before the column, or WSFC quorum loss) is "cannot tell", never "secondary". +public readonly record struct AgDatabaseMembership(string AgName, string DatabaseName, bool? IsLocal); + +/// +/// Which databases this node holds only as a SECONDARY copy in an Availability Group (#5558). A secondary's +/// database-level settings are the primary's (they replicate), so every node would report the same finding for +/// the same database; the primary reports it once and the Recommendations skip it everywhere else. +/// +/// Read from the monitoring store's own ag_replica_states / ag_database_replica_states +/// snapshots, not from a live sys.fn_hadr_is_primary_replica call: it costs no new query on the monitored +/// server, it works for an AsOf pass (the role at that time, not now), and it fails open where the AG views do +/// not exist (Azure SQL Database). See +/// https://learn.microsoft.com/en-us/sql/relational-databases/system-dynamic-management-views/sys-dm-hadr-database-replica-states-transact-sql +/// and +/// https://learn.microsoft.com/en-us/sql/relational-databases/system-functions/sys-fn-hadr-is-primary-replica-transact-sql +/// for the engine's own definition of primary and secondary. +/// +/// Fail open, always. A database is skipped only when BOTH snapshots exist and are fresh, the database +/// has a LOCAL row in the database snapshot, and the LOCAL replica's role for that database's group is explicitly +/// SECONDARY. No rows, stale rows, a NULL role or is_local, RESOLVING, a standalone database and an engine +/// without the AG views all skip nothing. The role is per group: two groups on one instance can have different +/// primaries, so server_properties.ag_replica_role (one value per instance) is never used. +/// +public static class AgReplicaScope +{ + /// How old a snapshot may be, measured against the pass's window end, before it is "stale" and + /// vouches for nothing. The AG collectors run every minute by default; an hour tolerates a slower custom + /// cadence without letting a stopped collector keep a days-old role in force. + public static readonly TimeSpan SnapshotFreshness = TimeSpan.FromHours(1); + + /// The role string the engine reports for a secondary replica. + public const string SecondaryRole = "SECONDARY"; + + /// True when exists, is no later than the window end, and is within + /// of it. + public static bool IsFresh(DateTime? snapshotUtc, DateTime windowEndUtc) => + snapshotUtc is { } at && at <= windowEndUtc && windowEndUtc - at <= SnapshotFreshness; + + /// The secondary databases from two snapshots read as of , applying + /// the freshness rule. Empty (never null) whenever anything is missing or stale. + public static IReadOnlySet SecondaryDatabases( + IReadOnlyList? replicas, DateTime? replicaSnapshotUtc, + IReadOnlyList? databases, DateTime? databaseSnapshotUtc, + DateTime windowEndUtc) + { + if (!IsFresh(replicaSnapshotUtc, windowEndUtc) || !IsFresh(databaseSnapshotUtc, windowEndUtc)) + return Empty(); + return SecondaryDatabases(replicas, databases); + } + + /// The secondary databases from two snapshots already judged fresh. A database is in the set only + /// when its group has exactly one distinct local role and that role is SECONDARY. + public static IReadOnlySet SecondaryDatabases( + IReadOnlyList? replicas, IReadOnlyList? databases) + { + if (replicas is not { Count: > 0 } || databases is not { Count: > 0 }) return Empty(); + + /* Local role per group. Two local rows for one group that disagree (a torn snapshot) are "cannot tell". */ + var roleByGroup = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var replica in replicas) + { + if (replica.IsLocal != true || string.IsNullOrWhiteSpace(replica.AgName)) continue; + var role = string.IsNullOrWhiteSpace(replica.RoleDesc) ? null : replica.RoleDesc.Trim().ToUpperInvariant(); + if (roleByGroup.TryGetValue(replica.AgName, out var seen)) + { + if (!string.Equals(seen, role, StringComparison.Ordinal)) roleByGroup[replica.AgName] = null; + } + else + { + roleByGroup[replica.AgName] = role; + } + } + + var secondary = new HashSet(StringComparer.OrdinalIgnoreCase); + var notSecondary = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var database in databases) + { + if (database.IsLocal != true || string.IsNullOrWhiteSpace(database.DatabaseName) + || string.IsNullOrWhiteSpace(database.AgName)) continue; + var isSecondary = roleByGroup.TryGetValue(database.AgName, out var localRole) + && string.Equals(localRole, SecondaryRole, StringComparison.Ordinal); + if (isSecondary) secondary.Add(database.DatabaseName); + else notSecondary.Add(database.DatabaseName); + } + + /* A name that also reads as not-secondary (two groups claiming one name, a torn snapshot) is not skipped. */ + secondary.ExceptWith(notSecondary); + return secondary; + } + + /// The one-sentence note shown wherever databases were skipped, or null for none. Plain English; + /// the same sentence on every surface. + public static string? SkippedNote(int count) => count switch + { + <= 0 => null, + 1 => "1 database skipped: this server holds a secondary copy of it in an availability group. Check the primary replica for its findings.", + _ => string.Create(CultureInfo.InvariantCulture, + $"{count} databases skipped: this server holds a secondary copy of them in an availability group. Check the primary replica for their findings."), + }; + + /// The note for a set, or null when it is null or empty. + public static string? SkippedNote(IReadOnlyCollection? secondaryDatabases) => + secondaryDatabases is null ? null : SkippedNote(secondaryDatabases.Count); + + private static HashSet Empty() => new(StringComparer.OrdinalIgnoreCase); +} From 67f69ce315932cccea846b1ac310469efb7b280a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:24:35 -0400 Subject: [PATCH 2/9] Darling: replicated facts skip secondary copies, every analysis context resolves the set (#5558) --- .../DarlingAnalysisService.cs | 15 ++ .../PgAnomalyDetector.cs | 4 + .../PgDrillDownCollector.Config.cs | 3 + .../PgDrillDownCollector.Queries.cs | 4 + .../PgDrillDownCollector.Storage.cs | 3 + .../PgFactCollector.Config.cs | 4 + .../PgFactCollector.QueryPerf.cs | 6 + .../PgFactCollector.Storage.cs | 5 +- .../PgSecondaryReplicaScope.cs | 144 ++++++++++++++++++ .../DarlingWorker.cs | 2 + 10 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs diff --git a/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs b/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs index fd71a934e6..f43edad276 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/DarlingAnalysisService.cs @@ -266,6 +266,14 @@ internal sealed record AnalysisEngineSet( } } + /// + /// #5558: the databases this node holds only as a secondary Availability Group copy, as of the pass's window end, + /// so the replicated facts skip them. Every context this service builds goes through here. Fails open: any unknown + /// returns an empty set. + /// + internal Task> SecondaryScopeForAsync(int serverId, DateTime windowEndUtc, CancellationToken cancellationToken) => + PgSecondaryReplicaScope.ReadAsync(_postgres, serverId, windowEndUtc, _logger, cancellationToken); + /// The store, read as whatever role this data source connects as. /// Optional; the SQL Server drill-down's cached-plan fetch. /// Optional. @@ -342,6 +350,7 @@ public async Task> AnalyzeAsync( { ServerId = serverId, SeparatelyMonitoredDatabases = await ScopeForAsync(serverId, cancellationToken), + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, timeRangeEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = timeRangeStart, TimeRangeEnd = timeRangeEnd, @@ -766,6 +775,7 @@ letting the halves drift. */ { ServerId = serverId, SeparatelyMonitoredDatabases = await ScopeForAsync(serverId, cancellationToken), + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, timeRangeEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = timeRangeStart, TimeRangeEnd = timeRangeEnd, @@ -824,6 +834,8 @@ public async Task> CollectConfigAuditFactsAsync( var context = new AnalysisContext { ServerId = serverId, + /* #5558: audit_config reads server settings only, all node-local, so nothing is skipped and no AG read is paid for. */ + SecondaryReplicaDatabases = PgSecondaryReplicaScope.NoneSkipped, ServerName = serverName, TimeRangeStart = timeRangeEnd.AddHours(-1), TimeRangeEnd = timeRangeEnd, @@ -888,6 +900,8 @@ context.CancellationToken is the caller's own token. */ { ServerId = serverId, SeparatelyMonitoredDatabases = separatelyMonitored, + /* #5558: per window, not shared: the windows can straddle a failover and each uses the role at its own end. */ + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, baselineEnd, cancellationToken), ServerName = serverName, TimeRangeStart = baselineStart, TimeRangeEnd = baselineEnd, @@ -898,6 +912,7 @@ context.CancellationToken is the caller's own token. */ { ServerId = serverId, SeparatelyMonitoredDatabases = separatelyMonitored, + SecondaryReplicaDatabases = await SecondaryScopeForAsync(serverId, comparisonEnd, cancellationToken), /* #5558 */ ServerName = serverName, TimeRangeStart = comparisonStart, TimeRangeEnd = comparisonEnd, diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs index e10dd03ee3..d2f3bba7b9 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgAnomalyDetector.cs @@ -473,6 +473,7 @@ prv AS (SELECT database_name, object_id, SUM(reserved_mb) mb WHERE (SELECT t FROM latest) <> (SELECT t FROM prior) AND cur.mb - prv.mb >= $2 AND (CASE WHEN prv.mb > 0 THEN (cur.mb - prv.mb) * 100.0 / prv.mb ELSE 0 END) >= $3 +/*SEC*/ ORDER BY growth_mb DESC LIMIT 1"; public const string ObjectContentionSql = @" @@ -512,6 +513,9 @@ private async Task DetectObjectStatsAnomalies(AnalysisContext context, List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs index 99492d7197..6ea618299f 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Queries.cs @@ -1045,9 +1045,13 @@ test on each row (#4821). */ ServerClock? clock = null; var items = new List(); + /* #5558: when the fact did not run the offender list above is NULL and every query is read, so the rows of a + database this node holds only as a secondary copy are dropped here, as the fact drops them. */ + var skipped = FactReplicaScope.SecondariesFor(context, "PLAN_REGRESSION"); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); while (await reader.ReadAsync(context.CancellationToken)) { + if (skipped.Length > 0 && !reader.IsDBNull(0) && Array.IndexOf(skipped, reader.GetString(0).ToLowerInvariant()) >= 0) continue; /* #4821: the newest snapshot's zone and offset ride on every row (columns 15 and 14), so the clock is built once. */ clock ??= ServerLocalTimes.ClockFrom( reader.IsDBNull(15) ? null : reader.GetString(15), diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs index f711b3c4c8..e3075d6780 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgDrillDownCollector.Storage.cs @@ -77,6 +77,7 @@ FROM latest AND is_percent_growth = true AND total_size_mb >= 10240 AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/ ORDER BY total_size_mb DESC LIMIT 50"; @@ -99,6 +100,8 @@ private async Task CollectAutogrowthPercentFiles(AnalysisFinding finding, Analys cmd.Parameters.AddWithValue(context.ServerId); cmd.Parameters.AddWithValue(AsNaive(context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name))); cmd.Parameters.AddWithValue(AsNaive(context.TimeRangeEnd)); + /* #5558: the same databases the fact skipped. */ + PgSecondaryReplicaScope.Apply(cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name"); var items = new List(); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs index 740395bc47..ac0da0b6d6 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Config.cs @@ -160,6 +160,7 @@ WITH latest AS ( FROM database_config WHERE server_id = $1 AND capture_time = (SELECT MAX(capture_time) FROM database_config WHERE server_id = $1) + /*SEC*/ ) SELECT COUNT(*) AS database_count, @@ -187,6 +188,9 @@ private async Task CollectDatabaseConfigFactsAsync(AnalysisContext context, List using var cmd = new NpgsqlCommand(DatabaseConfigSql, connection) { CommandTimeout = FactCommandTimeoutSeconds }; cmd.Parameters.AddWithValue(context.ServerId); + /* #5558: a database this node holds only as a secondary copy is the primary's to report (and drops out of + every count here, database_count included). */ + PgSecondaryReplicaScope.Apply(cmd, context, "DB_CONFIG", "database_name"); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs index e65c2ffa26..7bc51543a4 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs @@ -565,6 +565,7 @@ internal static DateTime PlanRegressionLiveFloor(DateTime windowFloor, IReadOnly bool_or(is_forced_plan) AS is_forced_plan, MAX(force_failure_count) AS force_failure_count FROM plan_agg + /*SEC*/ GROUP BY database_name, query_id, replica_role, query_plan_hash HAVING SUM(execs) >= 25 ), @@ -743,6 +744,11 @@ not swallowed. */ } } + /* #5558: Query Store on a secondary copy is the primary's content, and rows from the days this node was + primary stay in the window, so the read skips those databases. The marker sits in the suffix all three + variants share, so they cannot drift; bound after every other parameter. */ + PgSecondaryReplicaScope.Apply(cmd, context, "PLAN_REGRESSION", "database_name", "WHERE"); + var offenderCount = 0; var worstFactor = 0.0; var worstQueryId = 0L; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs index d0243b5dab..da5a723db0 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.Storage.cs @@ -351,7 +351,8 @@ FROM latest WHERE rn = 1 AND is_percent_growth = true AND total_size_mb >= 10240 -AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb')"; +AND database_name NOT IN ('master', 'msdb', 'model', 'tempdb') +/*SEC*/"; /// /// Collects the percent-autogrowth-on-large-files config fact (WS3): data/log files set @@ -372,6 +373,8 @@ private async Task CollectFileAutogrowthFactsAsync(AnalysisContext context, List cmd.Parameters.AddWithValue(context.ServerId); cmd.Parameters.AddWithValue(AsNaive(context.LatestValueStartFor(DatabaseSizeStatsCollector.Instance.Name))); cmd.Parameters.AddWithValue(AsNaive(context.TimeRangeEnd)); + /* #5558: the file growth setting replicates from the primary; a secondary copy is the primary's to report. */ + PgSecondaryReplicaScope.Apply(cmd, context, "FILE_AUTOGROWTH_PERCENT", "database_name"); using var reader = await cmd.ExecuteReaderAsync(context.CancellationToken); if (!await reader.ReadAsync(context.CancellationToken)) return; diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs new file mode 100644 index 0000000000..337bc6b7bd --- /dev/null +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs @@ -0,0 +1,144 @@ +/* + * 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.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; + +namespace PerformanceMonitor.Darling.Analysis; + +/// +/// #5558, Darling's half of Lite's SecondaryReplicaScope: resolves +/// from the store's own ag_replica_states and +/// ag_database_replica_states snapshots at or before the pass's window end, and builds the predicate the +/// replicated facts add. The decision is 's; this class only reads rows. Every failure +/// is "skip nothing". +/// +public static class PgSecondaryReplicaScope +{ + /// The marker a replicated read puts in its SQL where the secondary-database predicate goes. + public const string Marker = "/*SEC*/"; + + /// The set a pass that reads only node-local facts carries: nothing is skipped, and no AG read is paid for. + public static IReadOnlySet NoneSkipped { get; } = new HashSet(StringComparer.OrdinalIgnoreCase); + + /// Fills the set once per context. Never throws except an abandonment: a read fault logs and leaves the pass unfiltered. + public static async Task EnsureAsync(NpgsqlDataSource postgres, AnalysisContext context, Microsoft.Extensions.Logging.ILogger? logger) + { + if (context.SecondaryReplicaDatabases is not null) return; + context.SecondaryReplicaDatabases = await ReadAsync(postgres, context.ServerId, context.TimeRangeEnd, logger, context.CancellationToken); + } + + /// The secondary databases as of ; empty on every failure or unknown. + public static async Task> ReadAsync( + NpgsqlDataSource postgres, int serverId, DateTime asOfUtc, + Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken) + { + try + { + await using var connection = await postgres.OpenConnectionAsync(cancellationToken); + var asOf = DateTime.SpecifyKind(asOfUtc, DateTimeKind.Unspecified); + + var replicaTime = await NewestAsync(connection, "ag_replica_states", serverId, asOf, cancellationToken); + var databaseTime = await NewestAsync(connection, "ag_database_replica_states", serverId, asOf, cancellationToken); + if (!AgReplicaScope.IsFresh(replicaTime, asOfUtc) || !AgReplicaScope.IsFresh(databaseTime, asOfUtc)) + return new HashSet(StringComparer.OrdinalIgnoreCase); + + var replicas = new List(); + using (var cmd = new NpgsqlCommand( + "SELECT ag_name, replica_server_name, role_desc, is_local FROM ag_replica_states WHERE server_id = $1 AND collection_time = $2", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }) + { + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(DateTime.SpecifyKind(replicaTime!.Value, DateTimeKind.Unspecified)); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0)) continue; + replicas.Add(new AgReplicaReading( + AgName: reader.GetString(0), + ReplicaServerName: reader.IsDBNull(1) ? "" : reader.GetString(1), + RoleDesc: reader.IsDBNull(2) ? null : reader.GetString(2), + ConnectedStateDesc: null, + IsLocal: reader.IsDBNull(3) ? null : reader.GetBoolean(3))); + } + } + + var databases = new List(); + using (var cmd = new NpgsqlCommand( + "SELECT ag_name, database_name, is_local FROM ag_database_replica_states WHERE server_id = $1 AND collection_time = $2", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }) + { + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(DateTime.SpecifyKind(databaseTime!.Value, DateTimeKind.Unspecified)); + using var reader = await cmd.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + if (reader.IsDBNull(0) || reader.IsDBNull(1)) continue; + databases.Add(new AgDatabaseMembership( + reader.GetString(0), reader.GetString(1), reader.IsDBNull(2) ? null : reader.GetBoolean(2))); + } + } + + return AgReplicaScope.SecondaryDatabases(replicas, databases); + } + catch (Exception ex) when (!AnalysisShutdown.IsExpectedAbandon(ex, cancellationToken)) + { + logger?.LogWarning(ex, "Availability Group role lookup failed for server {ServerId}; no database is skipped this pass", serverId); + return new HashSet(StringComparer.OrdinalIgnoreCase); + } + } + + private static async Task NewestAsync( + NpgsqlConnection connection, string table, int serverId, DateTime asOf, CancellationToken cancellationToken) + { + /* The table name is a constant of this class, never input. */ + using var cmd = new NpgsqlCommand( + "SELECT MAX(collection_time) FROM " + table + " WHERE server_id = $1 AND collection_time <= $2", connection) + { CommandTimeout = DarlingAnalysisService.AnalysisCommandTimeoutSeconds }; + cmd.Parameters.AddWithValue(serverId); + cmd.Parameters.AddWithValue(asOf); + var value = await cmd.ExecuteScalarAsync(cancellationToken); + return value is DateTime at ? DateTime.SpecifyKind(at, DateTimeKind.Utc) : null; + } + + /// The note for the viewer, web and MCP (current role), or null when nothing is skipped. + public static async Task NoteAsync( + NpgsqlDataSource postgres, int serverId, Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken) + { + var set = await ReadAsync(postgres, serverId, DateTime.UtcNow, logger, cancellationToken); + return AgReplicaScope.SkippedNote(set); + } + + /// + /// Replaces in the command's text with the predicate that drops the skipped databases of + /// (), binding the lower-cased names as one text[] + /// parameter numbered after the parameters already added. is AND for a clause that + /// extends a WHERE and WHERE for one that opens it. With nothing to skip the marker just disappears: the SQL is + /// the same as before and no parameter is added. Call it after every other parameter is bound. + /// + public static void Apply(NpgsqlCommand cmd, AnalysisContext context, string factKey, string column, string keyword = "AND") + { + var names = FactReplicaScope.SecondariesFor(context, factKey); + if (names.Length == 0) + { + cmd.CommandText = cmd.CommandText.Replace(Marker, string.Empty, StringComparison.Ordinal); + return; + } + + var index = cmd.Parameters.Count + 1; + cmd.Parameters.Add(new NpgsqlParameter { NpgsqlDbType = NpgsqlTypes.NpgsqlDbType.Array | NpgsqlTypes.NpgsqlDbType.Text, Value = names }); + cmd.CommandText = cmd.CommandText.Replace( + Marker, $" {keyword} ({column} IS NULL OR lower({column}) <> ALL(${index}::text[]))", StringComparison.Ordinal); + } +} diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index 5104e1b02b..b95cc46f6f 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -10641,6 +10641,8 @@ surviving findings (the STORAGE identity, same as every collected row), the dril ServerName = runtime.StorageName, TimeRangeStart = latest.AddMinutes(-SameStatementPileupDetector.BaselineLookbackMinutes), TimeRangeEnd = latest, + /* #5558: the same-statement pileup is a node-local finding (queries that piled up on THIS node), so there is nothing to skip. */ + SecondaryReplicaDatabases = PerformanceMonitor.Darling.Analysis.PgSecondaryReplicaScope.NoneSkipped, CancellationToken = stoppingToken, ShutdownToken = stoppingToken, }; From d1d37c6a68be8f26eaba2bbe8bec67c2eee9e5cb Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:45:22 -0400 Subject: [PATCH 3/9] Recommendations say when databases were skipped as a secondary copy: Lite tab, Darling viewer and web, both get_analysis_findings tools, Darling twins of the seeded tests (#5558) --- .../Darling.Tests/AnalysisFindingsTabTests.cs | 11 + .../SecondaryReplicaScopeLiveTests.cs | 355 ++++++++++++++++++ .../SecondaryReplicaScopeTests.cs | 213 +++++++++++ .../analysis-findings-harness.mjs | 9 + .../PgSecondaryReplicaScope.cs | 8 +- .../Mcp/DarlingMcpTools.cs | 8 +- .../wwwroot/js/pages/analysis-findings.js | 5 +- .../MainWindow.xaml | 9 +- .../MainWindow.xaml.cs | 10 +- .../RecommendationsViewModel.cs | 14 + .../ViewerDataService.Findings.cs | 9 + Lite.Tests/McpAnalysisFindingsCommandTests.cs | 51 +++ Lite/Analysis/AnalysisService.cs | 8 + .../LiteRecommendationsViewModel.cs | 14 + Lite/Analysis/SecondaryReplicaScope.cs | 11 +- Lite/Controls/RecommendationsTab.xaml | 9 +- Lite/Controls/RecommendationsTab.xaml.cs | 27 +- Lite/Mcp/McpAnalysisTools.cs | 8 +- Lite/Services/CollectionBackgroundService.cs | 2 + 19 files changed, 767 insertions(+), 14 deletions(-) create mode 100644 Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs create mode 100644 Darling/Darling.Tests/SecondaryReplicaScopeTests.cs diff --git a/Darling/Darling.Tests/AnalysisFindingsTabTests.cs b/Darling/Darling.Tests/AnalysisFindingsTabTests.cs index 063c759039..13a858e110 100644 --- a/Darling/Darling.Tests/AnalysisFindingsTabTests.cs +++ b/Darling/Darling.Tests/AnalysisFindingsTabTests.cs @@ -140,6 +140,17 @@ public void TheReadCapNote_AndThePageNote_AreBothShown() Assert.Contains("PAGE-NOTE", text, StringComparison.Ordinal); } + [Fact] + public void TheSecondaryReplicaNote_ShowsWithFindingsAndWithNone_AndIsAbsentWhenTheReadCarriesNone() + { + var r = Run("replicaNote"); + Assert.Contains("REPLICA-NOTE", r.GetProperty("withFindings").GetString(), StringComparison.Ordinal); + Assert.Contains("REPLICA-NOTE", r.GetProperty("noFindings").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("REPLICA-NOTE", r.GetProperty("without").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("REPLICA-NOTE", r.GetProperty("nullNote").GetString(), StringComparison.Ordinal); + Assert.DoesNotContain("null", r.GetProperty("nullNote").GetString(), StringComparison.Ordinal); + } + [Fact] public void TheTabsAbortSignal_ReachesTheRead() { diff --git a/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs b/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs new file mode 100644 index 0000000000..bb843427a8 --- /dev/null +++ b/Darling/Darling.Tests/SecondaryReplicaScopeLiveTests.cs @@ -0,0 +1,355 @@ +/* + * 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.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; +using PerformanceMonitor.Darling.Service.Mcp; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/* #1776 own-store: this fact mints its own scratch database through ScratchPostgres for every test and never touches + another test's rows, so it is deliberately NOT [Collection("live-postgres")]. */ + +/// +/// #5558 on a seeded PostgreSQL store, the twin of Lite's SecondaryReplicaFactTests: the replicated facts drop the +/// database this node holds as a secondary copy and the node-local ones keep it. Three databases: SecDb (the local +/// copy of group AG1 is SECONDARY), PrimDb (the local copy of group AG2 is PRIMARY) and StandDb (in no group). +/// Skipped when DARLING_TEST_PG is not set. +/// +public sealed class SecondaryReplicaScopeLiveTests +{ + private const int ServerId = -558_201; + private const string ServerName = "darling-secondary-replica-scope"; + private static readonly string[] Databases = ["SecDb", "PrimDb", "StandDb"]; + + private static string? BaseConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + + private static DateTime Now() => DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow); + + /// Runs against a fresh migrated scratch store with the test server registered, then + /// drops the store whether the body passed or not. + private static async Task WithStoreAsync(Func body) + { + var baseConnectionString = BaseConnectionString; + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), "Set DARLING_TEST_PG to run the live secondary replica scope tests."); + + var ct = TestContext.Current.CancellationToken; + var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct); + var bodySucceeded = false; + try + { + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await DarlingMcpTestData.RegisterServerAsync(connection, ServerId, ServerName, ct); + + await using var postgres = NpgsqlDataSource.Create(scratch.ConnectionString); + await body(connection, postgres, ct); + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(scratch.ConnectionString, bodySucceeded, async (_, _) => await Task.CompletedTask); + await scratch.DisposeAsync(); + } + } + + private static Task ExecAsync(NpgsqlConnection connection, CancellationToken ct, string sql, params object?[] values) => + DarlingMcpTestData.ExecAsync(connection, ct, sql, values); + + /// AG1: this node (NODE1) is for it and holds SecDb; AG2: this node is PRIMARY and + /// holds PrimDb. NODE2 is the other replica of AG1 and holds SecDb too (not local). + private static async Task SeedAgAsync(NpgsqlConnection c, CancellationToken ct, DateTime at, string localRole = "SECONDARY") + { + const string Replica = "INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,$4,$5,$6,$7,$8)"; + var when = DarlingMcpTestData.Naive(at); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "NODE1", localRole, true); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "NODE2", "PRIMARY", false); + await ExecAsync(c, ct, Replica, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG2", "NODE1", "PRIMARY", true); + const string Db = "INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,$4,$5,$6,$7,$8)"; + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "SecDb", "NODE1", true); + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG1", "SecDb", "NODE2", false); + await ExecAsync(c, ct, Db, CollectionIdGenerator.Next(), when, ServerId, ServerName, "AG2", "PrimDb", "NODE1", true); + } + + /// One sys.databases capture for each of the three databases: auto_shrink on and RCSI off, so every one is a + /// DB_CONFIG offender, plus one 20 GB percent-growth data file each for FILE_AUTOGROWTH_PERCENT. + private static async Task SeedDatabasesAsync(NpgsqlConnection c, CancellationToken ct, DateTime end) + { + foreach (var db in Databases) + { + await ExecAsync(c, ct, @" +INSERT INTO database_config + (config_id, capture_time, server_id, server_name, database_name, + state_desc, compatibility_level, collation_name, recovery_model, is_read_only, + is_auto_close_on, is_auto_shrink_on, is_auto_create_stats_on, is_auto_update_stats_on, + is_auto_update_stats_async_on, is_read_committed_snapshot_on, snapshot_isolation_state, + is_parameterization_forced, is_query_store_on, is_encrypted, is_trustworthy_on, is_db_chaining_on, + is_broker_enabled, is_cdc_enabled, is_mixed_page_allocation_on, log_reuse_wait_desc, page_verify_option, + target_recovery_time_seconds, delayed_durability, is_accelerated_database_recovery_on, + is_memory_optimized_enabled, is_optimized_locking_on) +VALUES ($1, $2, $3, $4, $5, + 'ONLINE', 160, 'SQL_Latin1_General_CP1_CI_AS', 'FULL', FALSE, + FALSE, TRUE, TRUE, TRUE, + FALSE, FALSE, 'OFF', + FALSE, TRUE, FALSE, FALSE, FALSE, + FALSE, FALSE, FALSE, 'NOTHING', 'CHECKSUM', + 60, 'DISABLED', FALSE, + FALSE, FALSE)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddHours(-1)), ServerId, ServerName, db); + + await ExecAsync(c, ct, @" +INSERT INTO database_size_stats + (collection_id, collection_time, server_id, server_name, database_name, database_id, file_id, file_type_desc, file_name, + physical_name, total_size_mb, used_size_mb, is_percent_growth, growth_pct) +VALUES ($1, $2, $3, $4, $5, 7, 1, 'ROWS', $6, 'D:\data.mdf', 20480, 10000, TRUE, 10)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddMinutes(-30)), ServerId, ServerName, db, db + "_data"); + } + } + + private static AnalysisContext Context(DateTime end) => new() + { + ServerId = ServerId, + ServerName = ServerName, + TimeRangeStart = DarlingMcpTestData.Naive(end.AddHours(-4)), + TimeRangeEnd = DarlingMcpTestData.Naive(end), + ServerUtcOffset = TimeSpan.Zero, + }; + + private static async Task<(List Facts, AnalysisContext Context)> FactsAsync(NpgsqlDataSource postgres, DateTime end, CancellationToken ct) + { + var context = Context(end); + context.CancellationToken = ct; + await PgSecondaryReplicaScope.EnsureAsync(postgres, context, logger: null); + var facts = (await new PgFactCollector(postgres).CollectFactsAsync(context)).ToList(); + return (facts, context); + } + + private static Fact One(List facts, string key) => Assert.Single(facts, f => f.Key == key); + + private static async Task DrillDownJsonAsync(NpgsqlDataSource postgres, AnalysisContext context, string rootFactKey) + { + var finding = new AnalysisFinding + { + ServerId = ServerId, RootFactKey = rootFactKey, StoryPath = rootFactKey, PathKeys = [rootFactKey], Severity = 0.3, + }; + await new PgDrillDownCollector(postgres).EnrichFindingsAsync([finding], context); + return JsonSerializer.Serialize(finding.DrillDown); + } + + [Fact] + public async Task OnASecondary_DbConfigAndAutogrowthDropTheSecondaryDatabase_TheDrillDownsAgree_AndNodeLocalFactsKeepIt() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedDatabasesAsync(c, ct, end); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + + var (facts, context) = await FactsAsync(postgres, end, ct); + + Assert.Equal(["SecDb"], context.SecondaryReplicaDatabases!.ToArray()); + + var config = One(facts, "DB_CONFIG"); + Assert.Equal(2.0, config.Value); + Assert.Equal(2.0, config.Metadata["database_count"]); + Assert.Equal(2.0, config.Metadata["auto_shrink_on_count"]); + Assert.Equal(2.0, config.Metadata["rcsi_off_count"]); + + var autogrowth = One(facts, "FILE_AUTOGROWTH_PERCENT"); + Assert.Equal(2.0, autogrowth.Value); + Assert.Equal(2.0, autogrowth.Metadata["database_count"]); + + /* The drill-downs list the same databases the facts counted. */ + var configJson = await DrillDownJsonAsync(postgres, context, "DB_CONFIG"); + Assert.Contains("PrimDb", configJson); + Assert.Contains("StandDb", configJson); + Assert.DoesNotContain("SecDb", configJson); + + var growthJson = await DrillDownJsonAsync(postgres, context, "FILE_AUTOGROWTH_PERCENT"); + Assert.Contains("PrimDb", growthJson); + Assert.Contains("StandDb", growthJson); + Assert.DoesNotContain("SecDb", growthJson); + }); + } + + [Fact] + public async Task OnThePrimary_OrWithNoOrStaleAgRows_NothingIsSkipped() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedDatabasesAsync(c, ct, end); + + /* No AG rows at all. */ + var (noRows, noRowsContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(noRowsContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(noRows, "DB_CONFIG").Metadata["database_count"]); + Assert.Equal(3.0, One(noRows, "FILE_AUTOGROWTH_PERCENT").Metadata["database_count"]); + + /* Rows exist, but the newest is three hours old: stale, so the role vouches for nothing. */ + await SeedAgAsync(c, ct, end.AddHours(-3)); + var (stale, staleContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(staleContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(stale, "DB_CONFIG").Metadata["database_count"]); + + /* A fresh snapshot where the local role for the group is PRIMARY. */ + await SeedAgAsync(c, ct, end.AddMinutes(-1), localRole: "PRIMARY"); + var (primary, primaryContext) = await FactsAsync(postgres, end, ct); + Assert.Empty(primaryContext.SecondaryReplicaDatabases!); + Assert.Equal(3.0, One(primary, "DB_CONFIG").Metadata["database_count"]); + }); + } + + [Fact] + public async Task AnAsOfRead_UsesTheRoleAtItsOwnEnd_NotTheCurrentOne_AndTheNoteFollows() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var now = Now(); + /* Primary three hours ago, secondary a minute ago (a failover in between). */ + await SeedAgAsync(c, ct, now.AddHours(-3).AddMinutes(-1), localRole: "PRIMARY"); + await SeedAgAsync(c, ct, now.AddMinutes(-1)); + + var current = await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now, logger: null, ct); + Assert.Equal(["SecDb"], current.ToArray()); + Assert.Equal(AgReplicaScope.SkippedNote(1), await PgSecondaryReplicaScope.NoteAsync(postgres, ServerId, logger: null, ct)); + + var earlier = await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now.AddHours(-3), logger: null, ct); + Assert.Empty(earlier); + Assert.Null(await PgSecondaryReplicaScope.NoteAsync(postgres, ServerId, logger: null, ct, asOfUtc: now.AddHours(-3))); + + /* An anchor before any snapshot knows nothing, so it skips nothing. */ + Assert.Empty(await PgSecondaryReplicaScope.ReadAsync(postgres, ServerId, now.AddDays(-30), logger: null, ct)); + }); + } + + private static Task SeedRegressionAsync(NpgsqlConnection c, CancellationToken ct, DateTime end, string db, long planId, string hash, + long avgCpu, DateTime firstExec, DateTime lastExec) => + ExecAsync(c, ct, @" +INSERT INTO query_store_stats + (collection_id, collection_time, server_id, server_name, database_name, query_id, plan_id, execution_type_desc, + first_execution_time, last_execution_time, query_text, query_hash, execution_count, avg_cpu_time_us, + avg_duration_us, query_plan_hash, is_forced_plan, force_failure_count, runtime_stats_interval_id) +VALUES ($1,$2,$3,$4,$5,101,$6,'Regular',$7,$8,'SELECT 1','0xQH',100,$9,$9,$10,FALSE,0,$6)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end), ServerId, ServerName, db, planId, + DarlingMcpTestData.Naive(firstExec), DarlingMcpTestData.Naive(lastExec), avgCpu, hash); + + [Fact] + public async Task PlanRegression_DropsTheSecondaryDatabase() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + foreach (var db in new[] { "SecDb", "StandDb" }) + { + await SeedRegressionAsync(c, ct, end, db, planId: 1, hash: "0xGOOD", avgCpu: 100_000, firstExec: end.AddDays(-6), lastExec: end.AddDays(-5)); + await SeedRegressionAsync(c, ct, end, db, planId: 2, hash: "0xBAD", avgCpu: 1_200_000, firstExec: end.AddDays(-1), lastExec: end); + } + + var (facts, context) = await FactsAsync(postgres, end, ct); + + Assert.Equal(1.0, One(facts, "PLAN_REGRESSION").Metadata["offender_count"]); + Assert.NotEmpty(context.PlanRegressionOffenders!); + Assert.All(context.PlanRegressionOffenders!, o => Assert.Equal("StandDb", o.DatabaseName)); + }); + } + + private static Task SeedObjectAsync(NpgsqlConnection c, CancellationToken ct, DateTime at, string db, decimal reservedMb) => + ExecAsync(c, ct, @" +INSERT INTO index_object_stats + (collection_id, collection_time, server_id, server_name, database_name, database_id, schema_name, object_id, table_name, + index_id, index_name, reserved_mb, row_lock_wait_in_ms, index_lock_promotion_count) +VALUES ($1,$2,$3,$4,$5,7,'dbo',100,'Big',1,'PK_Big',$6,0,0)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(at), ServerId, ServerName, db, reservedMb); + + [Fact] + public async Task ObjectGrowth_DropsTheSecondaryDatabase() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var end = Now(); + await SeedAgAsync(c, ct, end.AddMinutes(-1)); + foreach (var (db, grownMb) in new[] { ("SecDb", 900m), ("StandDb", 500m) }) + { + await SeedObjectAsync(c, ct, end.AddDays(-1), db, 100m); + await SeedObjectAsync(c, ct, end.AddHours(-1), db, 100m + grownMb); + } + + /* The baseline gate: anomaly detection sits out a server with no history to compare against. */ + await ExecAsync(c, ct, + "INSERT INTO cpu_utilization_stats (collection_id, collection_time, server_id, server_name, sample_time, sqlserver_cpu_utilization, other_process_cpu_utilization) VALUES ($1, $2, $3, $4, $2, 10, 5)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(end.AddDays(-2)), ServerId, ServerName); + + var context = Context(end); + context.CancellationToken = ct; + await PgSecondaryReplicaScope.EnsureAsync(postgres, context, logger: null); + var detector = new PgAnomalyDetector(postgres, new PgBaselineProvider(postgres)); + var anomalies = await detector.DetectAnomaliesAsync(context); + + var growth = Assert.Single(anomalies, f => f.Key == "ANOMALY_OBJECT_GROWTH"); + Assert.Equal("StandDb", growth.DatabaseName); + }); + } + + /// + /// get_analysis_findings carries the shared one-sentence note beside a non-empty list and inside the empty answer when + /// this node holds a secondary copy. Lite's twin is in McpAnalysisFindingsCommandTests. + /// + [Fact] + public async Task GetAnalysisFindings_CarriesTheSecondaryReplicaNote_OnlyWhenThisNodeHoldsASecondaryCopy() + { + await WithStoreAsync(async (c, postgres, ct) => + { + var now = Now(); + var service = new DarlingAnalysisService(postgres); + + /* Nothing stored, no AG rows: the plain empty answer, no note. */ + using (var plain = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Equal("empty", plain.RootElement.GetProperty("status").GetString()); + Assert.DoesNotContain("secondary copy", plain.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + } + + /* A secondary copy, still no findings: the empty answer names the skipped database. */ + await SeedAgAsync(c, ct, now.AddMinutes(-1)); + using (var empty = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Contains(AgReplicaScope.SkippedNote(1)!, empty.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + } + + /* With a finding stored the field rides on the payload. */ + var t = now.AddMinutes(-10); + await ExecAsync(c, ct, @" +INSERT INTO analysis_findings + (finding_id, analysis_time, server_id, server_name, database_name, time_range_start, time_range_end, severity, confidence, + category, story_path, story_path_hash, story_text, root_fact_key, root_fact_value, leaf_fact_key, leaf_fact_value, fact_count, + incident_id, remediation_action_json, drill_down_json) +VALUES ($1, $2, $3, $4, 'StandDb', $5, $2, 0.72, 0.55, 'config', 'DB_CONFIG', 'AG5558_HASH', '', 'DB_CONFIG', 2, 'DB_CONFIG', 2, 1, + 'AG5558_INCIDENT', NULL, NULL)", + CollectionIdGenerator.Next(), DarlingMcpTestData.Naive(t), ServerId, ServerName, DarlingMcpTestData.Naive(t.AddHours(-4))); + + using (var withNote = JsonDocument.Parse(await DarlingMcpTools.GetAnalysisFindings(service, postgres, ServerName))) + { + Assert.Equal(AgReplicaScope.SkippedNote(1), withNote.RootElement.GetProperty("secondary_replica_note").GetString()); + } + }); + } +} diff --git a/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs b/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs new file mode 100644 index 0000000000..169344078d --- /dev/null +++ b/Darling/Darling.Tests/SecondaryReplicaScopeTests.cs @@ -0,0 +1,213 @@ +/* + * 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.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Analysis; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// #5558: 's predicate builder and its fail-open read, with no database. The seeded +/// twins of Lite's fact tests are in . +/// +public sealed class PgSecondaryReplicaScopeTests +{ + private static AnalysisContext ContextWith(params string[] secondaries) => new() + { + SecondaryReplicaDatabases = new HashSet(secondaries, StringComparer.OrdinalIgnoreCase), + }; + + [Fact] + public void Apply_WithNothingToSkip_JustRemovesTheMarker_AndAddsNoParameter() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/ ORDER BY 1"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith(), "DB_CONFIG", "database_name"); + + Assert.DoesNotContain("/*SEC*/", cmd.CommandText, StringComparison.Ordinal); + Assert.Equal("SELECT 1 FROM t WHERE a = $1 ORDER BY 1", cmd.CommandText); + Assert.Single(cmd.Parameters); + } + + [Fact] + public void Apply_WithASecondary_BindsTheLowerCasedNamesAsOneArray_NumberedAfterTheExistingParameters() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/ ORDER BY 1"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb", "OtherDb"), "DB_CONFIG", "database_name"); + + Assert.Contains("AND (database_name IS NULL OR lower(database_name) <> ALL($2::text[]))", cmd.CommandText, StringComparison.Ordinal); + Assert.Equal(2, cmd.Parameters.Count); + var names = Assert.IsType(cmd.Parameters[1].Value); + Assert.Equal(["otherdb", "secdb"], names.OrderBy(n => n, StringComparer.Ordinal).ToArray()); + } + + [Fact] + public void Apply_UsesTheKeywordItIsGiven_ForAClauseThatOpensTheWhere() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t /*SEC*/ GROUP BY 1"); + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb"), "PLAN_REGRESSION", "database_name", "WHERE"); + Assert.Contains(" WHERE (database_name IS NULL OR lower(database_name) <> ALL($1::text[]))", cmd.CommandText, StringComparison.Ordinal); + } + + [Fact] + public void Apply_LeavesANodeLocalKeyUnfiltered_EvenWithASecondarySet() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, ContextWith("SecDb"), "MISSING_INDEX", "database_name"); + + Assert.DoesNotContain("/*SEC*/", cmd.CommandText, StringComparison.Ordinal); + Assert.DoesNotContain("lower(", cmd.CommandText, StringComparison.Ordinal); + Assert.Single(cmd.Parameters); + } + + [Fact] + public void Apply_WithNoSetOnTheContext_SkipsNothing() + { + using var cmd = new NpgsqlCommand("SELECT 1 FROM t WHERE a = $1 /*SEC*/"); + cmd.Parameters.AddWithValue(1); + + PgSecondaryReplicaScope.Apply(cmd, new AnalysisContext(), "DB_CONFIG", "database_name"); + + Assert.DoesNotContain("lower(", cmd.CommandText, StringComparison.Ordinal); + Assert.Single(cmd.Parameters); + } + + [Fact] + public async Task ReadAsync_FailsOpen_WhenTheStoreCannotBeReached() + { + /* A port nothing listens on: the connection fails at once, and the answer is the empty set, not a throw. */ + await using var unreachable = NpgsqlDataSource.Create( + "Host=127.0.0.1;Port=1;Username=x;Database=x;Timeout=2;Pooling=false"); + + var set = await PgSecondaryReplicaScope.ReadAsync(unreachable, 1, DateTime.UtcNow, logger: null, CancellationToken.None); + Assert.Empty(set); + + Assert.Null(await PgSecondaryReplicaScope.NoteAsync(unreachable, 1, logger: null, CancellationToken.None)); + } + + [Fact] + public async Task EnsureAsync_FillsTheSetOnce_AndLeavesAnAlreadyResolvedSetAlone() + { + await using var unreachable = NpgsqlDataSource.Create( + "Host=127.0.0.1;Port=1;Username=x;Database=x;Timeout=2;Pooling=false"); + + var context = new AnalysisContext { ServerId = 1, TimeRangeEnd = DateTime.UtcNow }; + await PgSecondaryReplicaScope.EnsureAsync(unreachable, context, logger: null); + Assert.NotNull(context.SecondaryReplicaDatabases); + Assert.Empty(context.SecondaryReplicaDatabases!); + + var resolved = new HashSet(StringComparer.OrdinalIgnoreCase) { "SecDb" }; + var already = new AnalysisContext { ServerId = 1, SecondaryReplicaDatabases = resolved }; + await PgSecondaryReplicaScope.EnsureAsync(unreachable, already, logger: null); + Assert.Same(resolved, already.SecondaryReplicaDatabases); + } + + [Fact] + public void NoneSkipped_IsEmpty() + { + Assert.Empty(PgSecondaryReplicaScope.NoneSkipped); + } +} + +/// +/// #5558: every new AnalysisContext in the product code resolves the secondary replica set, either in its own +/// initializer or through EnsureAsync on the variable it is assigned to. A context built without it leaves +/// null, which skips nothing, so the replicated findings of a +/// secondary copy would quietly come back for that one pass. A pass that reads only node-local facts says so by setting the +/// empty set on purpose. Scans Lite and Darling; test projects build contexts freely and are not scanned. +/// +public sealed class AnalysisContextSecondaryScopeSourceScanTests +{ + private static string RepoRoot([CallerFilePath] string thisFile = "") + { + var dir = Path.GetDirectoryName(thisFile)!; + while (dir is not null && !File.Exists(Path.Combine(dir, "Directory.Build.props"))) + dir = Path.GetDirectoryName(dir); + Assert.False(dir is null, "could not locate the repo root from the test source path"); + return dir!; + } + + private static IEnumerable ProductSources(string root) + { + foreach (var top in new[] { "Lite", "Darling" }) + { + foreach (var file in Directory.EnumerateFiles(Path.Combine(root, top), "*.cs", SearchOption.AllDirectories)) + { + var relative = Path.GetRelativePath(root, file).Replace('\\', '/'); + if (relative.Contains("/bin/") || relative.Contains("/obj/") || relative.Contains(".Tests/")) continue; + yield return file; + } + } + } + + [Fact] + public void EveryAnalysisContextInTheProduct_ResolvesTheSecondaryReplicaSet() + { + var root = RepoRoot(); + var construction = new Regex(@"(?:var\s+(?\w+)\s*=\s*)?new\s+AnalysisContext\b", RegexOptions.CultureInvariant); + var missing = new List(); + var sites = 0; + + foreach (var file in ProductSources(root)) + { + var text = File.ReadAllText(file); + foreach (Match m in construction.Matches(text)) + { + sites++; + var close = text.IndexOf("};", m.Index, StringComparison.Ordinal); + var initializer = close < 0 ? text[m.Index..] : text[m.Index..close]; + var name = m.Groups["name"].Value; + var inInitializer = initializer.Contains("SecondaryReplicaDatabases", StringComparison.Ordinal); + var ensured = name.Length > 0 && Regex.IsMatch(text, @"SecondaryReplicaScope\.EnsureAsync\([^)]*\b" + Regex.Escape(name) + @"\b"); + if (!inInitializer && !ensured) + { + var line = text[..m.Index].Count(c => c == '\n') + 1; + missing.Add(Path.GetRelativePath(root, file).Replace('\\', '/') + ":" + line); + } + } + } + + Assert.True(sites >= 10, "the scan found only " + sites + " construction sites, so its pattern no longer matches the code"); + Assert.True(missing.Count == 0, + "an AnalysisContext that never resolves SecondaryReplicaDatabases would bring back a secondary copy's findings: " + string.Join(", ", missing)); + } +} + +/// #5558: the viewer Recommendations view model carries the note only where a list or the all-clear shows. +public sealed class RecommendationsReplicaNoteTests +{ + [Fact] + public void TheNote_IsCarriedOnTheEmptyAndLoadedStates_AndNullWhenNotSet() + { + var empty = RecommendationsViewModel.FromFindings(Array.Empty(), "SQL2022"); + Assert.Null(empty.ReplicaNote); + + var note = AgReplicaScope.SkippedNote(2); + Assert.Equal(note, empty.WithReplicaNote(note).ReplicaNote); + Assert.Same(empty, empty.WithReplicaNote(" ")); + Assert.Null(empty.ReplicaNote); + Assert.Equal(RecommendationsState.Empty, empty.State); + } +} diff --git a/Darling/Darling.Tests/analysis-findings-harness.mjs b/Darling/Darling.Tests/analysis-findings-harness.mjs index ecae30c8f4..d8d98b0dc0 100644 --- a/Darling/Darling.Tests/analysis-findings-harness.mjs +++ b/Darling/Darling.Tests/analysis-findings-harness.mjs @@ -121,6 +121,15 @@ try { const root = await run("srv-a", { findings: sample, truncated: true, truncation_note: "READ-CAP-NOTE", findings_truncated: true, findings_truncated_note: "PAGE-NOTE" }); return { text: root.textContent }; }, + replicaNote: async () => { + /* #5558: the secondary-copy note shows with a list and with an empty chain list, and is absent when the read carries none. */ + const note = "REPLICA-NOTE"; + const withFindings = await run("srv-a", { findings: sample, secondary_replica_note: note }); + const noFindings = await run("srv-a", { findings: [], secondary_replica_note: note }); + const without = await run("srv-a", { findings: sample }); + const nullNote = await run("srv-a", { findings: sample, secondary_replica_note: null }); + return { withFindings: withFindings.textContent, noFindings: noFindings.textContent, without: without.textContent, nullNote: nullNote.textContent }; + }, signal: async () => { const ac = new AbortController(); await run("srv-a", null, { hours: 24, label: "x", signal: ac.signal }); diff --git a/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs index 337bc6b7bd..9c4da7d80f 100644 --- a/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs +++ b/Darling/PerformanceMonitor.Darling.Analysis/PgSecondaryReplicaScope.cs @@ -112,11 +112,13 @@ public static async Task> ReadAsync( return value is DateTime at ? DateTime.SpecifyKind(at, DateTimeKind.Utc) : null; } - /// The note for the viewer, web and MCP (current role), or null when nothing is skipped. + /// The note for the viewer, web and MCP, or null when nothing is skipped. The role is the current one, + /// or the role at for an AsOf read of stored findings. public static async Task NoteAsync( - NpgsqlDataSource postgres, int serverId, Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken) + NpgsqlDataSource postgres, int serverId, Microsoft.Extensions.Logging.ILogger? logger, CancellationToken cancellationToken, + DateTime? asOfUtc = null) { - var set = await ReadAsync(postgres, serverId, DateTime.UtcNow, logger, cancellationToken); + var set = await ReadAsync(postgres, serverId, asOfUtc ?? DateTime.UtcNow, logger, cancellationToken); return AgReplicaScope.SkippedNote(set); } diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs index 04725450d0..af9ac4c59b 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs @@ -1128,11 +1128,16 @@ exactly when the window held an occurrence the newest-first LIMIT dropped. This resolved.ServerId, hours_back, FindingOccurrences.WindowCoveringLimit + 1, asOfUtc: anchor, cancellationToken: cancellationToken); var (findings, truncated) = McpHelpers.BoundPage(fetched, FindingOccurrences.WindowCoveringLimit); + /* #5558: databases skipped because this node holds only a secondary copy of them in an availability group + (their per-database findings are the primary's). Null when none; the same sentence on every surface. */ + var secondaryReplicaNote = await PgSecondaryReplicaScope.NoteAsync(postgres, resolved.ServerId, logger, cancellationToken, anchor); + if (findings.Count == 0) { return McpHelpers.Status( "empty", - "No findings in the requested time range. Run analyze_server to generate new findings."); + "No findings in the requested time range. Run analyze_server to generate new findings." + + (secondaryReplicaNote is null ? "" : " " + secondaryReplicaNote)); } /* #2000: collapse to one entry per (story_path_hash, incident_id). Measured 27.9x @@ -1180,6 +1185,7 @@ than the fleet mean (~15/server/24h after collapse), and unlike the raw-read cap finding_count = groups.Count, total_finding_count = totalFindingCount, total_occurrences = findings.Count, + secondary_replica_note = secondaryReplicaNote, // No silent caps: a read the window-covering limit CUT has had its OLDEST rows dropped by // the store's newest-first LIMIT, so occurrence stats may under-report — say so instead of // letting first_seen quietly lie. truncated is the #3594 flag (observed above); the note diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js index 3469d3caa0..0310aaa089 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/analysis-findings.js @@ -179,9 +179,12 @@ export function renderFindings(server, res) { if (res.kind !== "data") return []; const data = res.data || {}; const groups = groupFindings(data.findings); - if (!groups.length) return [keptWindowStrip(res), emptyStrip("No findings in this window.")]; + /* #5558: databases left out because this node holds only a secondary copy of them in an availability group. */ + const replicaNote = typeof data.secondary_replica_note === "string" && data.secondary_replica_note ? noticeStrip(data.secondary_replica_note) : null; + if (!groups.length) return [keptWindowStrip(res), replicaNote, emptyStrip("No findings in this window.")]; return [ keptWindowStrip(res), + replicaNote, typeof data.truncation_note === "string" && data.truncation_note ? noticeStrip(data.truncation_note) : null, typeof data.findings_truncated_note === "string" && data.findings_truncated_note ? noticeStrip(data.findings_truncated_note) : null, ...groups.map((g) => groupNode(server, g)), diff --git a/Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml b/Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml index 9fcd341ade..af5fcc7860 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml +++ b/Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml @@ -1315,6 +1315,7 @@ + @@ -1349,8 +1350,14 @@ FontStyle="Italic" Foreground="{DynamicResource ForegroundMutedBrush}"/> + + + - + Swaps the visible content region to match the view-model's state (mirrors Lite's ApplyViewModel). private void ApplyRecommendationsViewModel(RecommendationsViewModel vm) { + /* #5558: the availability group note rides with the list or the all-clear, never with a state that replaces them. */ + var replicaNote = vm.State is RecommendationsState.Loaded or RecommendationsState.Empty ? vm.ReplicaNote : null; + RecommendationsReplicaNoteText.Text = replicaNote ?? string.Empty; + RecommendationsReplicaNoteText.Visibility = replicaNote is null ? Visibility.Collapsed : Visibility.Visible; + switch (vm.State) { case RecommendationsState.Loading: diff --git a/Darling/PerformanceMonitor.Darling.Viewer/RecommendationsViewModel.cs b/Darling/PerformanceMonitor.Darling.Viewer/RecommendationsViewModel.cs index dff4a83e4c..6a87e17539 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/RecommendationsViewModel.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/RecommendationsViewModel.cs @@ -404,6 +404,20 @@ public sealed class RecommendationsViewModel /// public string WindowEmptyMessage { get; } + /// + /// #5558: the one-sentence note shown above a Loaded or Empty list when the monitored node holds a secondary copy + /// of some databases in an availability group (their per-database findings are the primary's), or null for none. + /// + public string? ReplicaNote { get; private set; } + + /// Sets and returns this view-model, so a caller can chain it onto a factory. + /// Only the Loaded and Empty states show it; the others replace the whole list area with their own message. + public RecommendationsViewModel WithReplicaNote(string? note) + { + ReplicaNote = string.IsNullOrWhiteSpace(note) ? null : note; + return this; + } + /// Total card count across all sections. public int TotalCount => Sections.Sum(s => s.Count); diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.Findings.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.Findings.cs index 9f5b5cb037..877a79085b 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.Findings.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.Findings.cs @@ -10,6 +10,7 @@ using System.Collections.Generic; using System.Text; using System.Text.Json; +using System.Threading; using System.Threading.Tasks; using Npgsql; using PerformanceMonitor.Analysis; @@ -126,6 +127,14 @@ FROM analysis_state } } + /// + /// #5558: the shared one-sentence note when the monitored node holds only a secondary copy of any database in an + /// availability group right now (their per-database findings are the primary's), else null. Read from the store's own + /// AG snapshots; any read failure is "no note", never an error on the tab. + /// + public Task GetSecondaryReplicaNoteAsync(int serverId, CancellationToken cancellationToken = default) => + PgSecondaryReplicaScope.NoteAsync(_dataSource, serverId, logger: null, cancellationToken); + /// /// The latest analysis run's findings for one server, mapped to display rows. Reads /// through the shared (its GetLatestFindingsSql — diff --git a/Lite.Tests/McpAnalysisFindingsCommandTests.cs b/Lite.Tests/McpAnalysisFindingsCommandTests.cs index 8d3fe75447..be6de2beb4 100644 --- a/Lite.Tests/McpAnalysisFindingsCommandTests.cs +++ b/Lite.Tests/McpAnalysisFindingsCommandTests.cs @@ -13,6 +13,7 @@ using System.Text; using System.Text.Json; using System.Threading.Tasks; +using DuckDB.NET.Data; using PerformanceMonitor.Analysis; using PerformanceMonitor.Common; using PerformanceMonitorLite.Analysis; @@ -430,6 +431,56 @@ public async Task GetAnalysisFindings_Default_StaysUnderResponseBudget_WithThirt } } + /// #5558: rows for one group where this node's copy is SECONDARY (or PRIMARY), written for the tool's server. + private async Task SeedAgAsync(DateTime at, string localRole) + { + using var readLock = _duckDb.AcquireReadLock(); + using var connection = _duckDb.CreateConnection(); + await connection.OpenAsync(TestContext.Current.CancellationToken); + async Task Exec(string sql, params object[] values) + { + using var cmd = connection.CreateCommand(); + cmd.CommandText = sql; + foreach (var v in values) cmd.Parameters.Add(new DuckDBParameter { Value = v }); + await cmd.ExecuteNonQueryAsync(TestContext.Current.CancellationToken); + } + + await Exec("INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,'n',$4,$5,$6,$7)", + -55_810_001L, at, _serverId, "AG1", "NODE1", localRole, true); + await Exec("INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,'n',$4,$5,$6,$7)", + -55_810_002L, at, _serverId, "AG1", "SecDb", "NODE1", true); + } + + /// + /// #5558: get_analysis_findings carries the one-sentence secondary-copy note beside a non-empty list and inside the + /// empty answer, so a short list is not read as a clean bill of health; a primary (or no AG rows) carries none. + /// + [Fact] + public async Task GetAnalysisFindings_CarriesTheSecondaryReplicaNote_OnlyWhenThisNodeHoldsASecondaryCopy() + { + var now = DateTime.UtcNow; + var analysisService = new AnalysisService(_duckDb); + + /* Nothing stored, no AG rows: the plain empty answer, no note. */ + var plainEmpty = JsonDocument.Parse(await McpAnalysisTools.GetAnalysisFindings(analysisService, _serverManager, "TestServer", 24)); + Assert.Equal("empty", plainEmpty.RootElement.GetProperty("status").GetString()); + Assert.DoesNotContain("secondary copy", plainEmpty.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + + /* A secondary copy, still no findings: the empty answer names the skipped database. */ + await SeedAgAsync(now.AddMinutes(-1), "SECONDARY"); + var emptyWithNote = JsonDocument.Parse(await McpAnalysisTools.GetAnalysisFindings(analysisService, _serverManager, "TestServer", 24)); + Assert.Contains(AgReplicaScope.SkippedNote(1)!, emptyWithNote.RootElement.GetProperty("message").GetString(), StringComparison.Ordinal); + + /* With a finding stored the field rides on the payload. */ + var context = new AnalysisContext { ServerId = _serverId, ServerName = "TestServer", TimeRangeStart = now.AddHours(-4), TimeRangeEnd = now }; + await new FindingStore(_duckDb).InsertFindingsAsync(new List + { + MakeFinding(920001, now, 1.0, "SOS_SCHEDULER_YIELD", "ag_note_hash", remediation: null), + }, context); + var withNote = JsonDocument.Parse(await McpAnalysisTools.GetAnalysisFindings(analysisService, _serverManager, "TestServer", 24)); + Assert.Equal(AgReplicaScope.SkippedNote(1), withNote.RootElement.GetProperty("secondary_replica_note").GetString()); + } + private AnalysisFinding MakeFinding( long findingId, DateTime analysisTime, double severity, string rootFactKey, string storyPathHash, RemediationAction? remediation) => diff --git a/Lite/Analysis/AnalysisService.cs b/Lite/Analysis/AnalysisService.cs index e4bbba9b46..8d380f13a5 100644 --- a/Lite/Analysis/AnalysisService.cs +++ b/Lite/Analysis/AnalysisService.cs @@ -1167,6 +1167,14 @@ over OBSERVED windows is the "nothing moved" answer and is banded like any other } } + /// #5558: the shared one-sentence note when this node holds a secondary copy of any database in an + /// availability group as of (now when null), else null. Read-back surfaces that list stored + /// findings (get_analysis_findings) carry it so a short list is not mistaken for a clean bill of health. A failed + /// read is "no note", never an error. + internal Task GetSecondaryReplicaNoteAsync( + int serverId, DateTime? asOfUtc = null, CancellationToken cancellationToken = default) => + SecondaryReplicaScope.NoteAsync(_duckDb, serverId, asOfUtc, cancellationToken); + /// /// The insufficient-history message for , or null when it has enough history /// OR when the span could not be read: a failed read says nothing about history, so it must not be diff --git a/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs b/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs index c9b5294e91..52137177a0 100644 --- a/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs +++ b/Lite/Analysis/Recommendations/LiteRecommendationsViewModel.cs @@ -257,6 +257,20 @@ public sealed class LiteRecommendationsViewModel /// public string WindowEmptyMessage { get; } + /// + /// #5558: the one-sentence note shown above a Loaded or Empty list when this node holds a secondary copy of some + /// databases in an availability group (their per-database findings are the primary's), or null for none. + /// + public string? ReplicaNote { get; private set; } + + /// Sets and returns this view-model, so a caller can chain it onto a factory. + /// Only the Loaded and Empty states show it; the others replace the whole list area with their own message. + public LiteRecommendationsViewModel WithReplicaNote(string? note) + { + ReplicaNote = string.IsNullOrWhiteSpace(note) ? null : note; + return this; + } + /// Total card count across all sections. public int TotalCount => Sections.Sum(s => s.Count); diff --git a/Lite/Analysis/SecondaryReplicaScope.cs b/Lite/Analysis/SecondaryReplicaScope.cs index 2f878e525d..110616a8e0 100644 --- a/Lite/Analysis/SecondaryReplicaScope.cs +++ b/Lite/Analysis/SecondaryReplicaScope.cs @@ -26,6 +26,9 @@ namespace PerformanceMonitorLite.Analysis; /// internal static class SecondaryReplicaScope { + /// The set a pass that reads only node-local facts carries: nothing is skipped, and no AG read is paid for. + internal static IReadOnlySet NoneSkipped { get; } = new HashSet(StringComparer.OrdinalIgnoreCase); + /// Fills the set once per context. Never throws except an abandonment (#2443): a read fault logs and /// leaves the pass unfiltered. internal static async Task EnsureAsync(DuckDbInitializer duckDb, AnalysisContext context) @@ -129,10 +132,12 @@ internal static string Apply(string sql, System.Data.Common.DbCommand cmd, Analy } /// The note for the tabs (Recommendations, FinOps): the shared sentence when this node holds a secondary - /// copy of any database right now, else null. Reads the current role, like the tab it sits on. - internal static async Task NoteAsync(DuckDbInitializer duckDb, int serverId, CancellationToken cancellationToken = default) + /// copy of any database as of (now when null), else null. A tab reads the current role; + /// an AsOf read of stored findings passes its anchor, so the note names the role at that time. + internal static async Task NoteAsync( + DuckDbInitializer duckDb, int serverId, DateTime? asOfUtc = null, CancellationToken cancellationToken = default) { - var set = await ReadAsync(duckDb, serverId, DateTime.UtcNow, cancellationToken); + var set = await ReadAsync(duckDb, serverId, asOfUtc ?? DateTime.UtcNow, cancellationToken); return AgReplicaScope.SkippedNote(set); } } diff --git a/Lite/Controls/RecommendationsTab.xaml b/Lite/Controls/RecommendationsTab.xaml index 21d7099953..d9116b5b99 100644 --- a/Lite/Controls/RecommendationsTab.xaml +++ b/Lite/Controls/RecommendationsTab.xaml @@ -128,6 +128,7 @@ + @@ -162,8 +163,14 @@ + + + - + diff --git a/Lite/Controls/RecommendationsTab.xaml.cs b/Lite/Controls/RecommendationsTab.xaml.cs index 68f0e813d0..e4d3d2a959 100644 --- a/Lite/Controls/RecommendationsTab.xaml.cs +++ b/Lite/Controls/RecommendationsTab.xaml.cs @@ -209,7 +209,8 @@ A server with no collected clock yet keeps the offset its OWN server tab shows ( selected tab's), and with no tab open the machine's, not UTC. */ var serverClock = await ReadCardClockAsync(_dataService, serverId, _openTabClock); - ApplyViewModel(LiteRecommendationsViewModel.FromItems(items, serverClock)); + var replicaNote = await ReadReplicaNoteAsync(serverId); + ApplyViewModel(LiteRecommendationsViewModel.FromItems(items, serverClock).WithReplicaNote(replicaNote)); } while (_reloadRequested); } @@ -322,7 +323,8 @@ schedules is null /* #4766: the selected server's own clock, by the serverId the analysis ran for (see RefreshDataAsync). */ var serverClock = await ReadCardClockAsync(_dataService, serverId, _openTabClock); - ApplyViewModel(LiteRecommendationsViewModel.FromItems(items, serverClock)); + var replicaNote = await ReadReplicaNoteAsync(serverId); + ApplyViewModel(LiteRecommendationsViewModel.FromItems(items, serverClock).WithReplicaNote(replicaNote)); } catch (Exception ex) { @@ -400,12 +402,33 @@ private void AskAi_Click(object sender, RoutedEventArgs e) } } + /// #5558: the shared note when this node holds a secondary copy of any database in an availability group + /// (the findings for those databases are the primary's), else null. A failed read shows no note. + private async Task ReadReplicaNoteAsync(int serverId) + { + if (_duckDb is null) return null; + try + { + return await Task.Run(() => PerformanceMonitorLite.Analysis.SecondaryReplicaScope.NoteAsync(_duckDb, serverId)); + } + catch (Exception ex) + { + AppLogger.Debug("Recommendations", $"Availability group note read failed, showing none: {ex.Message}"); + return null; + } + } + /// /// Swaps the visible region to match the view-model's state and binds the sections. Mirrors the /// Dashboard control's ApplyViewModel. /// private void ApplyViewModel(LiteRecommendationsViewModel vm) { + /* #5558: the availability group note rides with the list or the all-clear, never with a state that replaces them. */ + var replicaNote = vm.State is LiteRecommendationsState.Loaded or LiteRecommendationsState.Empty ? vm.ReplicaNote : null; + ReplicaNoteText.Text = replicaNote ?? string.Empty; + ReplicaNoteText.Visibility = replicaNote is null ? Visibility.Collapsed : Visibility.Visible; + switch (vm.State) { case LiteRecommendationsState.Loading: diff --git a/Lite/Mcp/McpAnalysisTools.cs b/Lite/Mcp/McpAnalysisTools.cs index d16b96a186..34cf6475a9 100644 --- a/Lite/Mcp/McpAnalysisTools.cs +++ b/Lite/Mcp/McpAnalysisTools.cs @@ -842,11 +842,16 @@ exactly when the window held an occurrence the newest-first LIMIT dropped. This resolved.ServerId, hours_back, FindingOccurrences.WindowCoveringLimit + 1, asOfUtc: anchor); var (findings, truncated) = McpHelpers.BoundPage(fetched, FindingOccurrences.WindowCoveringLimit); + /* #5558: databases skipped because this node holds only a secondary copy of them in an availability group + (their per-database findings are the primary's). Null when none; the same sentence on every surface. */ + var secondaryReplicaNote = await analysisService.GetSecondaryReplicaNoteAsync(resolved.ServerId, anchor); + if (findings.Count == 0) { return McpHelpers.Status( "empty", - "No findings in the requested time range. Run analyze_server to generate new findings."); + "No findings in the requested time range. Run analyze_server to generate new findings." + + (secondaryReplicaNote is null ? "" : " " + secondaryReplicaNote)); } /* #2000: collapse to one entry per (story_path_hash, incident_id). Measured 27.9x @@ -888,6 +893,7 @@ stats. The store keeps every row — this shapes the read only. */ finding_count = groups.Count, total_finding_count = totalFindingCount, total_occurrences = findings.Count, + secondary_replica_note = secondaryReplicaNote, // No silent caps: a read the window-covering limit CUT has had its OLDEST rows dropped by // the store's newest-first LIMIT, so occurrence stats may under-report — say so instead of // letting first_seen quietly lie. truncated is the #3594 flag (observed above); the note diff --git a/Lite/Services/CollectionBackgroundService.cs b/Lite/Services/CollectionBackgroundService.cs index 63c12d7b4a..9f0a0266be 100644 --- a/Lite/Services/CollectionBackgroundService.cs +++ b/Lite/Services/CollectionBackgroundService.cs @@ -632,6 +632,8 @@ set or nothing (#2448). */ ServerName = serverName, TimeRangeStart = latest.AddMinutes(-SameStatementPileupDetector.BaselineLookbackMinutes), TimeRangeEnd = latest, + /* #5558: the same-statement pileup is a node-local finding (queries that piled up on THIS node), so there is nothing to skip. */ + SecondaryReplicaDatabases = SecondaryReplicaScope.NoneSkipped, CancellationToken = stoppingToken, ShutdownToken = stoppingToken, }; From 7ee6fcdbfd6a7cab99f0f8f32caba1c4a0a5812f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 8 Oct 2026 09:50:26 -0400 Subject: [PATCH 4/9] FinOps Recommendations skip databases that are a secondary copy on this node: Lite, Darling viewer, web and MCP, with the skipped-databases note (#5558) --- ...rlingMcpFinOpsRecommendationsToolsTests.cs | 18 ++ .../FinOpsRecommendationsReadsLiveTests.cs | 4 +- ...ecommendationsSecondaryReplicaLiveTests.cs | 199 ++++++++++++++++++ .../McpPayloadContractCensusTests.cs | 2 +- .../DarlingMcpFinOpsRecommendationsTools.cs | 34 ++- .../js/pages/finops/recommendations.js | 2 + .../DarlingFinOpsRecommendationsReader.cs | 35 ++- .../FinOpsTab.Recommendations.cs | 5 +- .../FinOpsTab.xaml | 5 +- ...iewerDataService.FinOps.Recommendations.cs | 18 ++ Lite.Tests/FinOpsSecondaryReplicaTests.cs | 159 ++++++++++++++ Lite/Controls/FinOpsTab.xaml | 15 +- Lite/Controls/FinOpsTab.xaml.cs | 5 +- ...LocalDataService.FinOps.Recommendations.cs | 62 ++++-- PerformanceMonitor.Common/AgReplicaScope.cs | 17 ++ 15 files changed, 536 insertions(+), 44 deletions(-) create mode 100644 Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs create mode 100644 Lite.Tests/FinOpsSecondaryReplicaTests.cs diff --git a/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs b/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs index 88a11f0e22..8b148008a2 100644 --- a/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs +++ b/Darling/Darling.Tests/DarlingMcpFinOpsRecommendationsToolsTests.cs @@ -98,6 +98,24 @@ public void Envelope_WithNoMonthlyCost_NullsTheCostAndSaysWhy() Assert.Equal(new[] { "B", "A" }, env.GetProperty("recommendations").EnumerateArray().Select(e => e.GetProperty("category").GetString()).ToArray()); } + [Fact] + public void Envelope_WithASecondaryNote_WritesItBetweenSkippedChecksAndTheRows_AndWithoutOneOmitsTheKey() + { + var note = AgReplicaScope.SkippedNote(2); + var with = Json(DarlingMcpFinOpsRecommendationsTools.Envelope("alpha", 1000m, [], [Rec(1m, "A")], note)); + Assert.Equal( + new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "secondary_databases_note", "recommendations" }, + with.EnumerateObject().Select(p => p.Name).ToArray()); + Assert.Equal(note, with.GetProperty("secondary_databases_note").GetString()); + + /* #5558: a server with no secondary copy keeps its old payload, key for key. */ + var without = Json(DarlingMcpFinOpsRecommendationsTools.Envelope("alpha", 1000m, [], [Rec(1m, "A")], null)); + Assert.False(without.TryGetProperty("secondary_databases_note", out _)); + Assert.Equal( + new[] { "server", "monthly_cost_usd", "cost_reason", "recommendation_count", "skipped_checks", "recommendations" }, + without.EnumerateObject().Select(p => p.Name).ToArray()); + } + [Fact] public void Envelope_WithAMonthlyCost_CarriesItAndNoReason() { diff --git a/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs b/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs index 4054ce1289..79d9f4b3eb 100644 --- a/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs +++ b/Darling/Darling.Tests/FinOpsRecommendationsReadsLiveTests.cs @@ -332,7 +332,7 @@ public async Task Composer_ReportsAFailedCheck_ToTheHook_AndKeepsTheRest() { Assert.NotNull(ex); failed.Add(label); - }, ct); + }, cancellationToken: ct); Assert.Equal(new[] { "Maintenance window" }, failed.ToArray()); Func key = r => $"{r.Severity}|{r.Category}|{r.Finding}|{r.Detail}|{r.EstMonthlySavings}"; @@ -404,7 +404,7 @@ public async Task Composer_DoesNotReportTheVmCpuFallback_WhileTheVmRowsStillEmit var failed = new System.Collections.Generic.List(); var after = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( - dataSource, idA, 1000m, TimeoutSeconds, (label, _) => failed.Add(label), ct); + dataSource, idA, 1000m, TimeoutSeconds, (label, _) => failed.Add(label), cancellationToken: ct); Assert.DoesNotContain("VM right-sizing", failed); Assert.Contains(after, r => r.Category == "Hardware" && r.Finding.StartsWith("CPU:", StringComparison.Ordinal)); diff --git a/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs b/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs new file mode 100644 index 0000000000..9395343850 --- /dev/null +++ b/Darling/Darling.Tests/FinOpsRecommendationsSecondaryReplicaLiveTests.cs @@ -0,0 +1,199 @@ +/* + * 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.Collections.Generic; +using System.Linq; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Service.Mcp; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Storage.FinOps; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/* #1776 own-store: every test seeds its own scratch database, so nothing here shares rows with another test. */ +/// +/// #5558: the FinOps Recommendations list leaves the per-database findings of a database this node holds only as a +/// secondary copy in an Availability Group to the primary. Over the golden store, server A has an idle database +/// (ArchiveA), two dev/test names (app_dev_a, qa1_a), two TDE databases (SalesA, OrdersA), a 2 GB uncompressed index in +/// SalesA, and a low-latency database (OrdersA). Each test seeds the stored Availability Group snapshots for server A, +/// reads through the storage composer, the viewer and the MCP tool, and compares with the same read over no snapshots. +/// +public sealed class FinOpsRecommendationsSecondaryReplicaLiveTests +{ + private const int TimeoutSeconds = 30; + private const string BaseSkip = "Set DARLING_TEST_PG to a Postgres connection string to run the live FinOps secondary replica test."; + + private static int ServerId => FinOpsRecommendationsGoldenLiveTests.ServerIdA; + + private static async Task SeedStoreAsync(string baseConnectionString, CancellationToken ct) + { + var scratch = await ScratchPostgres.CreateAsync(baseConnectionString, ct); + await using var connection = new NpgsqlConnection(scratch.ConnectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await FinOpsRecommendationsGoldenLiveTests.SeedAsync(connection, ct); + return scratch; + } + + /// One AG whose local replica has , holding ; snapshots taken ago. + private static async Task SeedAgAsync(string connectionString, string localRole, TimeSpan age, CancellationToken ct, params string[] databases) + { + await using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(ct); + var at = DateTime.SpecifyKind(DateTime.UtcNow - age, DateTimeKind.Unspecified); + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO ag_replica_states (collection_id, collection_time, server_id, server_name, ag_name, replica_server_name, role_desc, is_local) VALUES ($1,$2,$3,$4,'AG1','NODE1',$5,TRUE)", + CollectionIdGenerator.Next(), at, ServerId, FinOpsRecommendationsGoldenLiveTests.ServerNameA, localRole); + foreach (var database in databases) + await DarlingMcpTestData.ExecAsync(connection, ct, + "INSERT INTO ag_database_replica_states (collection_id, collection_time, server_id, server_name, ag_name, database_name, replica_server_name, is_local) VALUES ($1,$2,$3,$4,'AG1',$5,'NODE1',TRUE)", + CollectionIdGenerator.Next(), at, ServerId, FinOpsRecommendationsGoldenLiveTests.ServerNameA, database); + } + + private static FinOpsRecommendation? Find(IEnumerable rows, string findingContains) => + rows.FirstOrDefault(r => r.Finding.Contains(findingContains, StringComparison.Ordinal)); + + [Fact] + public async Task ASecondaryCopy_IsDroppedFromIdleDevTestTdeAndCompression_ButKeptInLowIoLatency() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var baseline = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync(dataSource, ServerId, 1000m, TimeoutSeconds, cancellationToken: ct); + Assert.NotNull(Find(baseline, "idle database")); + Assert.Contains("2 possible dev/test", Find(baseline, "possible dev/test")!.Finding, StringComparison.Ordinal); + Assert.Contains("SalesA", Find(baseline, "TDE in use")!.Detail, StringComparison.Ordinal); + Assert.Contains("OrdersA", Find(baseline, "TDE in use")!.Detail, StringComparison.Ordinal); + Assert.NotNull(Find(baseline, "uncompressed object")); + Assert.Contains("OrdersA", Find(baseline, "low IO latency")!.Detail, StringComparison.Ordinal); + + /* ArchiveA (idle), app_dev_a (dev/test) and SalesA (TDE and the 2 GB index) are secondary copies; the case of + a name is the engine's, not the store's, so one is written in the wrong case on purpose. */ + var secondary = new HashSet(StringComparer.OrdinalIgnoreCase) { "archivea", "APP_DEV_A", "SalesA" }; + var rows = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: secondary, cancellationToken: ct); + + Assert.Null(Find(rows, "idle database")); + Assert.Contains("1 possible dev/test", Find(rows, "possible dev/test")!.Finding, StringComparison.Ordinal); + Assert.DoesNotContain("app_dev_a", Find(rows, "possible dev/test")!.Detail, StringComparison.Ordinal); + var tde = Find(rows, "TDE in use")!; + Assert.Contains("OrdersA", tde.Detail, StringComparison.Ordinal); + Assert.DoesNotContain("SalesA", tde.Detail, StringComparison.Ordinal); + Assert.Null(Find(rows, "uncompressed object")); + /* Low IO latency is unchanged, and so is every finding that is not about one database. */ + Assert.Equal(Find(baseline, "low IO latency")!.Detail, Find(rows, "low IO latency")!.Detail); + static bool PerDatabase(FinOpsRecommendation r) => + r.Finding.Contains("idle database", StringComparison.Ordinal) || r.Finding.Contains("possible dev/test", StringComparison.Ordinal) + || r.Finding.Contains("TDE in use", StringComparison.Ordinal) || r.Finding.Contains("uncompressed object", StringComparison.Ordinal); + Assert.Equal( + baseline.Where(r => !PerDatabase(r)).Select(r => (r.Finding, r.Detail)).ToArray(), + rows.Where(r => !PerDatabase(r)).Select(r => (r.Finding, r.Detail)).ToArray()); + } + + [Fact] + public async Task EveryTdeDatabaseASecondary_SaysNothingAboutTdeAndNeverClaimsNoEnterpriseFeatures() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var rows = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: new[] { "SalesA", "OrdersA" }, cancellationToken: ct); + + Assert.DoesNotContain(rows, r => r.Category == "Licensing"); + Assert.NotNull(Find(rows, "low IO latency")); + } + + [Fact] + public async Task NullOrEmptySet_ChangesNothing() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + + var baseline = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync(dataSource, ServerId, 1000m, TimeoutSeconds, cancellationToken: ct); + var empty = await DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( + dataSource, ServerId, 1000m, TimeoutSeconds, secondaryDatabases: new HashSet(), cancellationToken: ct); + + Assert.Equal(baseline.Select(r => (r.Finding, r.Detail)).ToArray(), empty.Select(r => (r.Finding, r.Detail)).ToArray()); + } + + [Fact] + public async Task TheViewerAndTheMcpTool_ReadTheStoredSnapshots_AndFailOpenWhenTheyAreStaleOrTheNodeIsPrimary() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var dataSource = NpgsqlDataSource.Create(scratch.ConnectionString); + await using var viewer = new ViewerDataService(scratch.ConnectionString); + + /* No snapshots: no note, and the MCP payload keeps its old shape. */ + var (plainRows, plainNote) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Null(plainNote); + var plainRead = await FinOpsAsync(dataSource, ct); + Assert.False(plainRead.TryGetProperty("secondary_databases_note", out _)); + + /* A stale secondary snapshot (a collector that stopped hours ago) vouches for nothing. */ + await SeedAgAsync(scratch.ConnectionString, "SECONDARY", TimeSpan.FromHours(6), ct, "ArchiveA"); + var (staleRows, staleNote) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Null(staleNote); + Assert.Equal(plainRows.Select(r => r.Finding).ToArray(), staleRows.Select(r => r.Finding).ToArray()); + + /* A fresh secondary snapshot of ArchiveA drops its idle finding on both surfaces and writes the note. */ + await SeedAgAsync(scratch.ConnectionString, "SECONDARY", TimeSpan.FromMinutes(2), ct, "ArchiveA"); + var (rows, note) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + Assert.Equal(AgReplicaScope.SkippedNote(1), note); + Assert.DoesNotContain(rows, r => r.Finding.Contains("idle database", StringComparison.Ordinal)); + Assert.Contains(plainRows, r => r.Finding.Contains("idle database", StringComparison.Ordinal)); + + var payload = await FinOpsAsync(dataSource, ct); + Assert.Equal(AgReplicaScope.SkippedNote(1), payload.GetProperty("secondary_databases_note").GetString()); + Assert.DoesNotContain(payload.GetProperty("recommendations").EnumerateArray(), + r => r.GetProperty("finding").GetString()!.Contains("idle database", StringComparison.Ordinal)); + } + + [Fact] + public async Task OnThePrimary_NothingIsSkipped_AndThereIsNoNote() + { + var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(baseConnectionString), BaseSkip); + var ct = TestContext.Current.CancellationToken; + await using var scratch = await SeedStoreAsync(baseConnectionString!, ct); + await using var viewer = new ViewerDataService(scratch.ConnectionString); + + var (plainRows, _) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + await SeedAgAsync(scratch.ConnectionString, "PRIMARY", TimeSpan.FromMinutes(2), ct, "ArchiveA", "app_dev_a", "SalesA"); + var (rows, note) = await viewer.GetRecommendationsWithNoteAsync(ServerId, 1000m, ct); + + Assert.Null(note); + Assert.Equal(plainRows.Select(r => (r.Finding, r.Detail)).ToArray(), rows.Select(r => (r.Finding, r.Detail)).ToArray()); + } + + private static async Task FinOpsAsync(NpgsqlDataSource dataSource, CancellationToken ct) + { + var json = await DarlingMcpFinOpsRecommendationsTools.GetFinOpsRecommendations( + dataSource, FinOpsRecommendationsGoldenLiveTests.ServerNameA, ct); + return JsonDocument.Parse(json).RootElement.Clone(); + } +} diff --git a/Darling/Darling.Tests/McpPayloadContractCensusTests.cs b/Darling/Darling.Tests/McpPayloadContractCensusTests.cs index ed02846798..f630364f20 100644 --- a/Darling/Darling.Tests/McpPayloadContractCensusTests.cs +++ b/Darling/Darling.Tests/McpPayloadContractCensusTests.cs @@ -2305,7 +2305,7 @@ public static readonly (string File, string Member, int Limit)[] StatementTermin [ ("DarlingFinOpsRecommendationsReader.cs", "(file scope)", 10), ("LocalDataService.Blocking.cs", "GetBlockingPairRowsAsync", 5000), - ("LocalDataService.FinOps.Recommendations.cs", "GetRecommendationsAsync", 10), + ("LocalDataService.FinOps.Recommendations.cs", "GetRecommendationsWithNoteAsync", 10), ("LocalDataService.PlanCache.cs", "GetPlanCacheSnapshotAsync", 30), ("LocalDataService.RunningJobs.cs", "GetAnomalousJobsAsync", 5), ("LocalDataService.WaitStats.cs", "GetAllQuerySnapshotsInRangeAsync", 2000), diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs index 92fbe4a6c3..a7272f537d 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpFinOpsRecommendationsTools.cs @@ -17,6 +17,7 @@ using ModelContextProtocol.Server; using Npgsql; using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Analysis; using PerformanceMonitor.Darling.Storage.FinOps; namespace PerformanceMonitor.Darling.Service.Mcp; @@ -30,7 +31,7 @@ namespace PerformanceMonitor.Darling.Service.Mcp; public sealed class DarlingMcpFinOpsRecommendationsTools { private const string RecommendationsGuide = - " est_savings_usd_month is a rough monthly USD estimate rounded to cents; null when the check gives none or no monthly cost is set (monthly_cost_usd null, cost_reason 'monthly cost not set'). severity and confidence are High, Medium or Low. finding and detail are English text with numbers in invariant format (1,234.5; a percent reads '20 %'). skipped_checks names each check whose read failed: its rows are absent, not clean. An empty recommendations list with no skipped_checks means no check found anything."; + " est_savings_usd_month is a rough monthly USD estimate rounded to cents; null when the check gives none or no monthly cost is set (monthly_cost_usd null, cost_reason 'monthly cost not set'). severity and confidence are High, Medium or Low. finding and detail are English text with numbers in invariant format (1,234.5; a percent reads '20 %'). skipped_checks names each check whose read failed: its rows are absent, not clean. An empty recommendations list with no skipped_checks means no check found anything. secondary_databases_note appears only when this server holds a secondary copy of some database in an availability group: idle, dev/test, TDE and compression findings leave those databases out (the primary reports them), and the low IO latency finding keeps them."; [McpServerTool(Name = "get_finops_recommendations"), Description( "FinOps recommendations for one server: the cost and right-sizing findings the desktop Recommendations tab shows, High severity first. Mixed fixed windows ending now (CPU 24 hours and 7 days; memory, jobs and file I/O 7 days; edition and database facts from the latest snapshot); UTC; no hours_back, limit or as_of. At most about 21 rows. <>" + RecommendationsGuide)] @@ -47,6 +48,11 @@ public static async Task GetFinOpsRecommendations( var monthly = await FinOpsUtilizationFigures.GetMonthlyCostUsdAsync( postgres, resolved.ServerId, McpCommandDeadlines.ReadSeconds, cancellationToken); + /* #5558: databases this node holds only as an Availability Group secondary copy are left to the primary's findings. + Fails open (stale or missing snapshots, standalone, Azure SQL Database skip nothing). */ + var secondary = await PgSecondaryReplicaScope.ReadAsync( + postgres, resolved.ServerId, DateTime.UtcNow, logger: null, cancellationToken); + /* The composer swallows every failed check; the hook names them. A cancelled request is not a failed check. */ var skipped = new List(); var rows = await InInvariantCultureAsync(() => DarlingFinOpsRecommendationsReader.GetRecommendationsAsync( @@ -56,10 +62,11 @@ public static async Task GetFinOpsRecommendations( if (ex is OperationCanceledException && cancellationToken.IsCancellationRequested) return; if (!skipped.Contains(label, StringComparer.Ordinal)) skipped.Add(label); }, - cancellationToken)); + secondaryDatabases: secondary, cancellationToken: cancellationToken)); cancellationToken.ThrowIfCancellationRequested(); - return JsonSerializer.Serialize(Envelope(resolved.ServerName, monthly, skipped, rows), McpHelpers.JsonOptions); + return JsonSerializer.Serialize( + Envelope(resolved.ServerName, monthly, skipped, rows, AgReplicaScope.SkippedNote(secondary)), McpHelpers.JsonOptions); } catch (Exception ex) when (ex is not OperationCanceledException) { @@ -97,17 +104,22 @@ internal static async Task InInvariantCultureAsync(Func> body) }; /// The response envelope. Rows keep the composer's order. - internal static object Envelope(string server, decimal monthly, IReadOnlyList skipped, IReadOnlyList rows) + internal static object Envelope(string server, decimal monthly, IReadOnlyList skipped, IReadOnlyList rows, + string? secondaryNote = null) { var hasCost = monthly > 0m; - return new + /* #5558: secondary_databases_note is written only when the node holds a secondary copy of some database, so every other + server's payload keeps its old shape. It sits after skipped_checks, before the rows. */ + var envelope = new Dictionary { - server, - monthly_cost_usd = hasCost ? monthly : (decimal?)null, - cost_reason = hasCost ? null : "monthly cost not set", - recommendation_count = rows.Count, - skipped_checks = skipped, - recommendations = rows.Select(RecommendationRow).ToList(), + ["server"] = server, + ["monthly_cost_usd"] = hasCost ? monthly : (decimal?)null, + ["cost_reason"] = hasCost ? null : "monthly cost not set", + ["recommendation_count"] = rows.Count, + ["skipped_checks"] = skipped, }; + if (secondaryNote != null) envelope["secondary_databases_note"] = secondaryNote; + envelope["recommendations"] = rows.Select(RecommendationRow).ToList(); + return envelope; } } diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js index c4af2da50d..6a2c7bc43e 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/finops/recommendations.js @@ -52,6 +52,8 @@ export const tab = { const skipped = data.skipped_checks || []; const parts = [noticeStrip(noticeText(rows.length))]; if (data.monthly_cost_usd == null) parts.push(noticeStrip("Monthly cost not set for this server, so savings estimates are blank.")); + /* Databases this server holds only as an availability group secondary copy are left to the primary's findings (#5558). */ + if (data.secondary_databases_note) parts.push(noticeStrip(data.secondary_databases_note)); if (skipped.length) parts.push(noticeStrip("Some checks could not run, so their findings are missing (not clean): " + skipped.join(", ") + ".")); parts.push(VIZ.table({ rows }, { rowsKey: "rows", columns: COLUMNS, emptyText: "No recommendations: nothing needs attention, or not enough has been collected yet." })); mount(body, parts); diff --git a/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs b/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs index 7083320abc..80fe5bf60c 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/FinOps/DarlingFinOpsRecommendationsReader.cs @@ -356,9 +356,17 @@ public static async Task> GetDatabaseEncryptionFact /// empty tab. is the per-server budget (0 → findings emit with no savings /// estimate, mirroring Lite's monthlyCost > 0 ? … : null). /// + /// Optional (#5558). The databases this node holds only as a secondary copy in an + /// Availability Group, as PgSecondaryReplicaScope.ReadAsync resolves them (that class lives in the Analysis project, + /// which depends on this one, so the caller resolves the set). The idle, dev/test, TDE-blocker and compression rules drop + /// them, because the primary reports the same finding for the same replicated database; the low IO latency rule keeps them, + /// since a replica's own storage is that replica's own cost. Null or empty skips nothing, which is how a stale snapshot, a + /// standalone server and Azure SQL Database arrive. The edition note still follows the instance-level collected role + /// (server_properties.ag_replica_role): an edition is one decision for the whole group, not a finding about a database. /// Optional. Invoked once per failed check with that check's label and the exception. /// It is not invoked for the VM check's inner CPU fallback, which degrades to the 24-hour window and still emits. - public static async Task> GetRecommendationsAsync(NpgsqlDataSource dataSource, int serverId, decimal monthlyCost, int commandTimeoutSeconds, Action? onCheckFailed = null, CancellationToken cancellationToken = default) + public static async Task> GetRecommendationsAsync(NpgsqlDataSource dataSource, int serverId, decimal monthlyCost, int commandTimeoutSeconds, Action? onCheckFailed = null, + IReadOnlyCollection? secondaryDatabases = null, CancellationToken cancellationToken = default) { var recommendations = new List(); var memoryCutoff = DateTime.SpecifyKind(DateTime.UtcNow.AddDays(-7), DateTimeKind.Unspecified); @@ -373,15 +381,23 @@ public static async Task> GetRecommendationsAsync(Npg // TDE is only the deciding factor on pre-2019 Enterprise — read the config snapshot just then. var tdeDbNames = new List(); + var tdeSecondaryOnly = false; if (isEnterprise && f.MajorVersion < 15) { - tdeDbNames = FinOpsRecommendationFigures.SelectTdeDatabaseNames( + var tdeAllNames = FinOpsRecommendationFigures.SelectTdeDatabaseNames( await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)); + /* #5558: a secondary copy's TDE is the primary's to report. When every TDE database here is a secondary copy, + "no Enterprise-only features" would be false and the blocker is the primary's, so the audit says nothing + (a secondary-role instance keeps its own edition note, which never reads this list). */ + tdeDbNames = AgReplicaScope.WithoutSecondaries(tdeAllNames, secondaryDatabases); + tdeSecondaryOnly = tdeAllNames.Count > 0 && tdeDbNames.Count == 0 + && !string.Equals(f.AgReplicaRole.Trim(), "Secondary", StringComparison.OrdinalIgnoreCase); } - recommendations.AddRange( - FinOpsRecommendationFigures.EditionAudit( - f.Edition, f.MajorVersion, f.CpuCount, tdeDbNames, monthlyCost, f.AgReplicaRole, f.IsHadrEnabled)); + if (!tdeSecondaryOnly) + recommendations.AddRange( + FinOpsRecommendationFigures.EditionAudit( + f.Edition, f.MajorVersion, f.CpuCount, tdeDbNames, monthlyCost, f.AgReplicaRole, f.IsHadrEnabled)); } } catch (Exception ex) @@ -458,7 +474,8 @@ and cannot be resized on its own. util.PhysicalMemoryMb is the database's own me try { var indexes = await DarlingFinOpsIndexAnalysisReader.GetIndexCleanupInputsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken); - var rec = FinOpsRecommendationFigures.Compression(indexes); + var rec = FinOpsRecommendationFigures.Compression( + indexes.Where(i => !AgReplicaScope.IsSkipped(secondaryDatabases, i.DatabaseName))); if (rec != null) { recommendations.Add(rec); @@ -477,7 +494,8 @@ and cannot be resized on its own. util.PhysicalMemoryMb is the database's own me enrolled hours ago, or one that was not collected for a stretch, has not been watched long enough to call any database idle. */ var idleDbs = (await DarlingFinOpsOptimizationReader.GetIdleDatabaseReadAsync( - dataSource, serverId, DateTime.UtcNow.AddDays(-7), commandTimeoutSeconds, cancellationToken)).Rows; + dataSource, serverId, DateTime.UtcNow.AddDays(-7), commandTimeoutSeconds, cancellationToken)).Rows + .Where(d => !AgReplicaScope.IsSkipped(secondaryDatabases, d.DatabaseName)).ToList(); if (idleDbs.Count > 0) { var allocatedTotalMb = 0m; @@ -504,7 +522,8 @@ database idle. */ try { var devDbs = FinOpsRecommendationFigures.MatchDevTestDatabases( - (await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)).Select(r => r.DatabaseName)); + (await GetDatabaseEncryptionFactsAsync(dataSource, serverId, commandTimeoutSeconds, cancellationToken)) + .Select(r => r.DatabaseName).Where(name => !AgReplicaScope.IsSkipped(secondaryDatabases, name))); var devTestRecommendation = FinOpsRecommendationFigures.DevTest(devDbs); if (devTestRecommendation != null) recommendations.Add(devTestRecommendation); diff --git a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs index 8edaf19f4f..84cba63551 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.Recommendations.cs @@ -25,8 +25,11 @@ public partial class FinOpsTab /// Reads the collected recommendations for this server and repaints the grid + count indicator. private async Task LoadFinOpsRecommendationsAsync() { - var data = await _dataService.GetRecommendationsAsync(_server.ServerId, _server.MonthlyCostUsd); + var (data, skippedNote) = await _dataService.GetRecommendationsWithNoteAsync(_server.ServerId, _server.MonthlyCostUsd); + /* #5558: databases this server holds only as an Availability Group secondary copy are left to the primary's findings. */ + FinOpsRecommendationsSecondaryNote.Text = skippedNote ?? ""; + FinOpsRecommendationsSecondaryNote.Visibility = skippedNote is null ? Visibility.Collapsed : Visibility.Visible; _finopsRecommendationsFilterMgr!.UpdateData(data); FinOpsRecommendationsNoDataMessage.Visibility = data.Count == 0 ? Visibility.Visible : Visibility.Collapsed; FinOpsRecommendationsCountIndicator.Text = data.Count > 0 diff --git a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml index 1627e6f189..04dce43efd 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml +++ b/Darling/PerformanceMonitor.Darling.Viewer/FinOpsTab.xaml @@ -1166,6 +1166,7 @@ + @@ -1177,7 +1178,9 @@ - + + +