From ab6e259de4b5e30f53f52c806e5e4662638c3cd6 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 18 Sep 2026 01:51:04 -0400 Subject: [PATCH] Dashboard analysis perfmon reads divide by the measured interval (#3561) Mirror of #3560 (Lite/Darling) into the deprecated Dashboard's frozen analysis twins: cntr_value_delta spans one collection interval, not one second, so the raw reads overstated by the cadence (60x at 60s, 300x at 5min) against thresholds defined in requests/sec. All three reads move together, or the z-score compares across units: - SqlServerFactCollector PERFMON_*_SEC facts divide the delta by the row's measured sample_interval_seconds; interval <= 0 rows (unknowable delta) are filtered so rn = 1 lands on the newest usable row - a counter with only interval-0 rows emits no fact, never 0. The raw delta and the divisor ride the metadata. - SqlServerAnomalyDetector's batch-request window AVG/MAX divide by NULLIF(sample_interval_seconds, 0) with interval <= 0 rows filtered, keeping the window statistic in the same requests/sec unit as the BatchRequestFloor/Fallback bars. - SqlServerBaselineProvider's batch_requests arm computes the baseline population as the per-second rate; the restart signature stays on the RAW delta (its > 1000 bar predates the division). The window and perfmon SQL move to public consts (the Darling twin's tested shape) and GetBaselineQuery goes internal so Dashboard.Tests can pin the text - the Dashboard has no test database. Semantics verified live against SQL Server via a temp-table clone of collect.perfmon_stats. Fixes #3561 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01QMYFB4qv5tqGwMmdPsa4Zx --- .../PerfmonPerSecondSqlTests.cs | 102 ++++++++++++++++++ .../Analysis/SqlServerAnomalyDetector.cs | 30 ++++-- .../Analysis/SqlServerBaselineProvider.cs | 14 ++- .../SqlServerFactCollector.Resources.cs | 50 ++++++--- 4 files changed, 165 insertions(+), 31 deletions(-) create mode 100644 deprecated/Dashboard.Tests/PerfmonPerSecondSqlTests.cs diff --git a/deprecated/Dashboard.Tests/PerfmonPerSecondSqlTests.cs b/deprecated/Dashboard.Tests/PerfmonPerSecondSqlTests.cs new file mode 100644 index 0000000000..26de1de269 --- /dev/null +++ b/deprecated/Dashboard.Tests/PerfmonPerSecondSqlTests.cs @@ -0,0 +1,102 @@ +/* + * Performance Monitor Dashboard + * Copyright (c) 2026 Darling Data, LLC + * Licensed under the MIT License - see LICENSE file for details + */ + +using PerformanceMonitorDashboard.Analysis; +using Xunit; + +namespace PerformanceMonitorDashboard.Tests; + +/// +/// #3527/#3561: cntr_value_delta spans one COLLECTION INTERVAL, not one second — read raw, the +/// perfmon analysis reads overstate by the cadence (60x at 60s, 300x at 5min). All three reads +/// must divide by the row's measured sample_interval_seconds and skip interval <= 0 rows +/// (no delta was knowable: first sighting, reset, gap) — together or the baseline comparison is +/// cross-unit. The Dashboard has no test database, so these pin the SQL text, the mirror of the +/// Darling twin's #3560 pins (PgFactCollector.PerfmonSql / PgAnomalyDetector.BatchRequestWindowSql). +/// +public class PerfmonPerSecondSqlTests +{ + /* ---------------- read 1: the fact collector ---------------- */ + + [Fact] + public void PerfmonFactSql_SelectsTheMeasuredInterval_AndFiltersUnknowableRows() + { + var sql = SqlServerFactCollector.PerfmonSql; + + /* The interval rides both the CTE and the outer select so the C# division has its divisor. */ + Assert.Contains("sample_interval_seconds,", sql); + Assert.Contains("SELECT counter_name, cntr_value, cntr_value_delta, sample_interval_seconds", sql); + + /* Interval <= 0 rows are filtered INSIDE the CTE, so rn = 1 lands on the newest row a rate + can honestly be derived from — a counter with only interval-0 rows emits no fact, never 0. */ + Assert.Contains("AND sample_interval_seconds > 0", sql); + var filterAt = sql.IndexOf("AND sample_interval_seconds > 0", System.StringComparison.Ordinal); + var rnFilterAt = sql.IndexOf("FROM latest WHERE rn = 1", System.StringComparison.Ordinal); + Assert.True(filterAt >= 0 && filterAt < rnFilterAt, "the interval filter must sit inside the latest CTE, before rn = 1"); + } + + /* ---------------- read 2: the anomaly detector's window ---------------- */ + + [Fact] + public void BatchRequestWindow_DividesByMeasuredInterval_AndSkipsUnknowableRows() + { + var sql = SqlServerAnomalyDetector.BatchRequestWindowSql; + + Assert.Contains("AVG(cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0))", sql); + Assert.Contains("MAX(cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0))", sql); + Assert.Contains("sample_interval_seconds > 0", sql); + + /* A raw AVG/MAX of the delta is exactly the #3527 defect — pin its absence. */ + Assert.DoesNotContain("AVG(cntr_value_delta)", sql); + Assert.DoesNotContain("MAX(cntr_value_delta)", sql); + } + + /* ---------------- read 3: the baseline arm ---------------- */ + + [Fact] + public void BatchRequestBaseline_PopulationIsPerSecond_RestartSignatureStaysRaw() + { + var sql = SqlServerBaselineProvider.GetBaselineQuery(SqlServerMetricNames.BatchRequests); + + Assert.NotNull(sql); + + /* The baseline population is the per-second rate, in the detector's window unit. */ + Assert.Contains("cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0) AS v", sql); + Assert.Contains("AVG(v) AS mean_val", sql); + Assert.Contains("STDEV(v) AS stddev_val", sql); + Assert.Contains("AND sample_interval_seconds > 0", sql); + + /* The restart signature stays on the RAW delta: its > 1000 bar predates the division and + marks a counter reset regardless of cadence. */ + Assert.Contains("LAG(cntr_value_delta) OVER (ORDER BY collection_time) AS prev_value", sql); + Assert.Contains("WHERE NOT (cntr_value_delta = 0 AND ISNULL(prev_value, 0) > 1000)", sql); + + /* A raw AVG/STDEV of the delta is exactly the #3527 defect — pin its absence. */ + Assert.DoesNotContain("AVG(cntr_value_delta)", sql); + Assert.DoesNotContain("STDEV(cntr_value_delta)", sql); + } + + /* ---------------- the consistency constraint ---------------- */ + + /// + /// The whole fix is the three reads agreeing: the window statistic, the baseline population, + /// and the fact value all divide by the measured interval, or the z-score compares across + /// units. The fact collector's division happens in C# (delta / sample_interval_seconds after + /// the SQL delivers the divisor), so its SQL is pinned for the divisor column and the + /// interval filter; the two aggregate reads divide in the SQL itself. + /// + [Fact] + public void AllThreeReads_FilterUnknowableRows() + { + var factSql = SqlServerFactCollector.PerfmonSql; + var windowSql = SqlServerAnomalyDetector.BatchRequestWindowSql; + var baselineSql = SqlServerBaselineProvider.GetBaselineQuery(SqlServerMetricNames.BatchRequests); + + Assert.Contains("sample_interval_seconds > 0", factSql); + Assert.Contains("sample_interval_seconds > 0", windowSql); + Assert.Contains("sample_interval_seconds > 0", baselineSql); + } +} diff --git a/deprecated/Dashboard/Analysis/SqlServerAnomalyDetector.cs b/deprecated/Dashboard/Analysis/SqlServerAnomalyDetector.cs index b0bfc0076a..71ea6a89d5 100644 --- a/deprecated/Dashboard/Analysis/SqlServerAnomalyDetector.cs +++ b/deprecated/Dashboard/Analysis/SqlServerAnomalyDetector.cs @@ -832,6 +832,24 @@ FROM collect.file_io_stats } } + // Batch-request window: per-second rate per sample (#3527) — cntr_value_delta spans one collection + // interval, so divide by the row's MEASURED sample_interval_seconds. Interval <= 0 marks an + // unknowable delta (first sighting/reset/gap) and the row is skipped, never read as 0. Keeps the + // window statistic in the same requests/sec unit as the baseline and the + // BatchRequestFloor/Fallback thresholds. + public const string BatchRequestWindowSql = @" +SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; + +SELECT + AVG(cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0)) AS avg_batch, + MAX(cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0)) AS peak_batch, + COUNT(*) AS sample_count +FROM collect.perfmon_stats +WHERE collection_time >= @windowStart AND collection_time <= @windowEnd +AND counter_name = 'Batch Requests/sec' +AND cntr_value_delta >= 0 +AND sample_interval_seconds > 0;"; + /// /// Detects batch requests/sec anomalies using z-score against time-bucketed baseline. /// @@ -849,17 +867,7 @@ private async Task DetectBatchRequestAnomalies(AnalysisContext context, List= @windowStart AND collection_time <= @windowEnd -AND counter_name = 'Batch Requests/sec' -AND cntr_value_delta >= 0;"; + cmd.CommandText = BatchRequestWindowSql; cmd.Parameters.Add(new SqlParameter("@windowStart", context.TimeRangeStart)); cmd.Parameters.Add(new SqlParameter("@windowEnd", context.TimeRangeEnd)); diff --git a/deprecated/Dashboard/Analysis/SqlServerBaselineProvider.cs b/deprecated/Dashboard/Analysis/SqlServerBaselineProvider.cs index 5ba2c1c20f..c248222773 100644 --- a/deprecated/Dashboard/Analysis/SqlServerBaselineProvider.cs +++ b/deprecated/Dashboard/Analysis/SqlServerBaselineProvider.cs @@ -190,7 +190,8 @@ public async Task GetBaselineAsync(string metricName, DateTime a } } - private static string? GetBaselineQuery(string metricName) + // Internal for Dashboard.Tests (InternalsVisibleTo): the #3527 per-second pins read the arm SQL. + internal static string? GetBaselineQuery(string metricName) { // All queries return: hour_of_day, day_of_week, mean_val, stddev_val, sample_count // Day-of-week normalization: (DATEPART(weekday, x) + @@DATEFIRST - 1) % 7 gives Sunday=0 @@ -217,21 +218,28 @@ GROUP BY DATEPART(HOUR, collection_time), // Cumulative counter — restart exclusion via CTE with LAG. // server_start_time is inline in collect.perfmon_stats. // Exclude samples within 5 min of a detected restart. + // #3527: v is the per-second rate — the per-interval delta divided by the row's measured + // sample_interval_seconds — so the baseline population is in the same requests/sec unit as + // the detector's window statistic. Interval <= 0 rows (unknowable delta) are skipped, never + // read as 0. The restart signature stays on the RAW delta: its > 1000 bar predates the + // division and marks a counter reset regardless of cadence. SqlServerMetricNames.BatchRequests => @" SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; ;WITH filtered AS ( SELECT collection_time, cntr_value_delta, + cntr_value_delta * 1.0 / NULLIF(sample_interval_seconds, 0) AS v, LAG(cntr_value_delta) OVER (ORDER BY collection_time) AS prev_value FROM collect.perfmon_stats WHERE collection_time >= @windowStart AND collection_time < @windowEnd AND counter_name = 'Batch Requests/sec' AND cntr_value_delta >= 0 + AND sample_interval_seconds > 0 ) SELECT DATEPART(HOUR, collection_time) AS hour_of_day, (DATEPART(WEEKDAY, collection_time) + @@DATEFIRST - 1) % 7 AS day_of_week, - AVG(cntr_value_delta) AS mean_val, - STDEV(cntr_value_delta) AS stddev_val, + AVG(v) AS mean_val, + STDEV(v) AS stddev_val, COUNT(*) AS sample_count, COUNT(DISTINCT CAST(collection_time AS DATE)) AS distinct_days FROM filtered diff --git a/deprecated/Dashboard/Analysis/SqlServerFactCollector.Resources.cs b/deprecated/Dashboard/Analysis/SqlServerFactCollector.Resources.cs index 2dedac4eba..6a8e083501 100644 --- a/deprecated/Dashboard/Analysis/SqlServerFactCollector.Resources.cs +++ b/deprecated/Dashboard/Analysis/SqlServerFactCollector.Resources.cs @@ -197,19 +197,12 @@ FROM collect.memory_grant_stats } } - /// - /// Collects key perfmon throughput counters: Batch Requests/sec, compilations, recompilations. - /// Unscored context that distinguishes a busy server from a sick one (used by the AI surfaces). - /// - private async Task CollectPerfmonFactsAsync(AnalysisContext context, List facts) - { - try - { - using var connection = new SqlConnection(_connectionString); - await connection.OpenAsync(); - - using var cmd = connection.CreateCommand(); - cmd.CommandText = @" + // #3527: cntr_value_delta spans one COLLECTION INTERVAL, not one second — at a 60s cadence + // the raw delta is 60x the true rate. The honest rate divides by the row's MEASURED + // sample_interval_seconds; interval <= 0 marks an unknowable delta (first sighting, counter + // reset, gap), so those rows are filtered rather than emitted as 0 — rn = 1 lands on the + // newest row a rate can honestly be derived from. + public const string PerfmonSql = @" SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED; ;WITH latest AS ( @@ -217,15 +210,33 @@ private async Task CollectPerfmonFactsAsync(AnalysisContext context, List counter_name, cntr_value, cntr_value_delta, + sample_interval_seconds, ROW_NUMBER() OVER (PARTITION BY counter_name ORDER BY collection_time DESC) AS rn FROM collect.perfmon_stats WHERE collection_time >= @startTime AND collection_time <= @endTime AND counter_name IN ('Batch Requests/sec', 'SQL Compilations/sec', 'SQL Re-Compilations/sec') + AND sample_interval_seconds > 0 ) -SELECT counter_name, cntr_value, cntr_value_delta +SELECT counter_name, cntr_value, cntr_value_delta, sample_interval_seconds FROM latest WHERE rn = 1"; + /// + /// Collects key perfmon throughput counters: Batch Requests/sec, compilations, recompilations. + /// Unscored context that distinguishes a busy server from a sick one (used by the AI surfaces). + /// Fact values are per-second rates: the per-interval delta divided by the row's measured + /// sample_interval_seconds (#3527); the raw delta and the divisor ride the metadata. + /// + private async Task CollectPerfmonFactsAsync(AnalysisContext context, List facts) + { + try + { + using var connection = new SqlConnection(_connectionString); + await connection.OpenAsync(); + + using var cmd = connection.CreateCommand(); + cmd.CommandText = PerfmonSql; + cmd.Parameters.Add(new SqlParameter("@startTime", context.TimeRangeStart)); cmd.Parameters.Add(new SqlParameter("@endTime", context.TimeRangeEnd)); @@ -235,6 +246,7 @@ AND counter_name IN ('Batch Requests/sec', 'SQL Compilations/sec', 'SQL Re-Com var counterName = reader.GetString(0); var cntrValue = reader.IsDBNull(1) ? 0L : Convert.ToInt64(reader.GetValue(1)); var deltaValue = reader.IsDBNull(2) ? 0L : Convert.ToInt64(reader.GetValue(2)); + var intervalSeconds = reader.IsDBNull(3) ? 0L : Convert.ToInt64(reader.GetValue(3)); var (factKey, source) = counterName switch { @@ -246,8 +258,11 @@ AND counter_name IN ('Batch Requests/sec', 'SQL Compilations/sec', 'SQL Re-Com if (factKey == null) continue; - // All remaining counters are per-second rates — use the delta. - var value = (double)deltaValue; + // The delta spans one collection interval — divide by the measured interval for the + // per-second rate (#3527). The SQL already filters interval <= 0 (unknowable delta); + // this guard keeps a raw or zero value from ever escaping if that filter regresses. + if (intervalSeconds <= 0) continue; + var value = deltaValue / (double)intervalSeconds; facts.Add(new Fact { @@ -258,7 +273,8 @@ AND counter_name IN ('Batch Requests/sec', 'SQL Compilations/sec', 'SQL Re-Com Metadata = new Dictionary { ["cntr_value"] = cntrValue, - ["delta_cntr_value"] = deltaValue + ["delta_cntr_value"] = deltaValue, + ["sample_interval_seconds"] = intervalSeconds } }); }