-
Notifications
You must be signed in to change notification settings - Fork 89
Report cpu_window on the top-CPU reads: the attribution denominator (#2320) #2323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f60095e
663638f
e89268a
83e8b21
e1f35d1
4733c03
f63893a
0ac0625
0b535b4
f652660
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * 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 PerformanceMonitor.Common; | ||
| using Xunit; | ||
|
|
||
| namespace Darling.Tests; | ||
|
|
||
| /// <summary> | ||
| /// Decision-table pins for the shared <see cref="CpuAttribution"/> (#2320) — the denominator the | ||
| /// top-CPU rankings never handed the caller. This SAME table is pinned identically in Lite.Tests so | ||
| /// the two SKUs cannot drift. The load-bearing cases are #2235's field numbers: the pre-fix reads | ||
| /// explained ~10% of one box and nothing said so, and the Datadog disagreement died the moment its | ||
| /// worker_time sum was divided by the box's consumed CPU-seconds and produced 137%. | ||
| /// </summary> | ||
| public sealed class CpuAttributionTests | ||
| { | ||
| /* The #2235 window: 8 vCPU, 2 hours, RDS CPU averaging 18% → 10,368 measured core-seconds. */ | ||
| private const double FieldAvgPct = 18.0; | ||
| private const int FieldCores = 8; | ||
| private const double FieldHours = 2.0; | ||
| private const int CoveredSamples = 120; | ||
| private const double FullSpan = 2.0; | ||
|
|
||
| [Fact] | ||
| public void TheFieldWindowComputesTheMeasuredDenominator() | ||
| { | ||
| var window = CpuAttribution.Compute( | ||
| attributedCpuMs: 3_000_000, FieldAvgPct, CoveredSamples, FullSpan, FieldCores, FieldHours); | ||
|
|
||
| Assert.NotNull(window); | ||
| Assert.Equal(10_368, window.Value.MeasuredSqlCpuSeconds, precision: 0); | ||
| Assert.Equal(3_000, window.Value.AttributedCpuSeconds, precision: 0); | ||
| Assert.Equal(0.289, window.Value.AttributedRatio, precision: 3); | ||
| } | ||
|
|
||
| /// <summary>Under half explained → the note says where the rest lives.</summary> | ||
| [Fact] | ||
| public void ALowRatioCarriesTheRemainderNote() | ||
| { | ||
| var window = CpuAttribution.Compute(3_000_000, FieldAvgPct, CoveredSamples, FullSpan, FieldCores, FieldHours); | ||
|
|
||
| Assert.NotNull(window!.Value.Note); | ||
| Assert.Contains("below the ranking cut", window.Value.Note, System.StringComparison.Ordinal); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The impossibility detector: attributed exceeding measured by more than sampling skew could | ||
| /// explain gets the skew note — the exact division that settled #2235's Datadog claim (137%). | ||
| /// </summary> | ||
| [Fact] | ||
| public void AttributedBeyondMeasuredCarriesTheSkewNote() | ||
| { | ||
| var window = CpuAttribution.Compute(14_229_000, FieldAvgPct, CoveredSamples, FullSpan, FieldCores, FieldHours); | ||
|
|
||
| Assert.NotNull(window); | ||
| Assert.True(window.Value.AttributedRatio > 1.3); | ||
| Assert.Contains("exceeds the measured total", window.Value.Note, System.StringComparison.Ordinal); | ||
| } | ||
|
|
||
| /// <summary>An ordinary healthy ratio says nothing — notes are for the two failure directions.</summary> | ||
| [Fact] | ||
| public void AMidRatioCarriesNoNote() | ||
| { | ||
| var window = CpuAttribution.Compute(8_000_000, FieldAvgPct, CoveredSamples, FullSpan, FieldCores, FieldHours); | ||
|
|
||
| Assert.NotNull(window); | ||
| Assert.Null(window.Value.Note); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Every way the denominator can be untrustworthy omits the window rather than fabricating one: | ||
| /// no average, no samples, unknown or nonsensical cores, a degenerate window, thin coverage, | ||
| /// and a hard-zero average (the ratio would divide by zero). | ||
| /// </summary> | ||
| [Fact] | ||
| public void AnUnsupportableDenominatorIsOmittedNeverFabricated() | ||
| { | ||
| Assert.Null(CpuAttribution.Compute(1_000, null, CoveredSamples, FullSpan, FieldCores, FieldHours)); | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, 0, FullSpan, FieldCores, FieldHours)); | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, CoveredSamples, FullSpan, null, FieldHours)); | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, CoveredSamples, FullSpan, 0, FieldHours)); | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, CoveredSamples, FullSpan, FieldCores, 0)); | ||
| /* A 24h window whose samples span only 5 hours: the average extrapolates a fragment — | ||
| span-based on purpose, so a SLOW but steady cadence still qualifies (the review catch). */ | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, 300, 5.0, FieldCores, 24)); | ||
| /* Two lonely points can bracket a wide span — the sample floor rejects them. */ | ||
| Assert.Null(CpuAttribution.Compute(1_000, FieldAvgPct, 2, FullSpan, FieldCores, FieldHours)); | ||
| Assert.Null(CpuAttribution.Compute(1_000, 0.0, CoveredSamples, FullSpan, FieldCores, FieldHours)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// The span floor's boundary: samples spanning exactly half the window qualify — and cadence | ||
| /// never enters it, so a server whose cpu_utilization schedule was slowed to 5 minutes (24 | ||
| /// samples in 2 hours) qualifies exactly like a 1-minute one (the review catch: a | ||
| /// count-per-minute expectation would have disqualified it permanently). | ||
| /// </summary> | ||
| [Fact] | ||
| public void SpanAtTheFloorQualifies_AtAnyCadence() | ||
| { | ||
| Assert.NotNull(CpuAttribution.Compute(1_000, FieldAvgPct, 60, FieldHours * 0.5, FieldCores, FieldHours)); | ||
| Assert.NotNull(CpuAttribution.Compute(1_000, FieldAvgPct, 24, FullSpan, FieldCores, FieldHours)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -155,6 +155,67 @@ FROM cpu_utilization_stats | |
| ORDER BY sample_time | ||
| """; | ||
|
|
||
| /// <summary> | ||
| /// #2320: the attribution denominator's raw ingredients for one window — the average SQL-process | ||
| /// CPU percent and how many samples that average rests on (the coverage gate's input; the shared | ||
| /// CpuAttribution.Compute owns the trust decision). Windows on collection_time like every other | ||
| /// windowed read here; the de-skewed sample_time is irrelevant to an average. | ||
| /// </summary> | ||
| public static async Task<(double? AvgSqlCpuPercent, int Samples, double SpanHours)> GetCpuWindowAverageAsync( | ||
| NpgsqlDataSource postgres, int serverId, DateTime startUtc, DateTime endUtc, CancellationToken cancellationToken = default) | ||
| { | ||
| /* The span (last minus first sample) rides along because the coverage gate judges SPAN, not | ||
| count-per-minute — the collector's cadence is user-configurable, so a count expectation | ||
| would permanently disqualify a legitimately slowed server (the review catch). NULL-valued | ||
| rows are excluded up front (the round-9 catch): AVG would skip them anyway, but COUNT(*) | ||
| and the span must rest on the same rows the average does, or Samples overstates the | ||
| evidence behind it. */ | ||
| const string sql = """ | ||
| SELECT | ||
| AVG(sqlserver_cpu_utilization)::float8, | ||
| COUNT(*)::int, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That matters because Today's single collector (
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. f652660 — fixed with one predicate instead of a COUNT-column swap: |
||
| COALESCE(EXTRACT(EPOCH FROM (MAX(collection_time) - MIN(collection_time))) / 3600.0, 0)::float8 | ||
| FROM cpu_utilization_stats | ||
| WHERE server_id = $1 | ||
| AND collection_time >= $2 | ||
| AND collection_time <= $3 | ||
| AND sqlserver_cpu_utilization IS NOT NULL | ||
| """; | ||
| await using var command = postgres.CreateCommand(sql); | ||
| AddInt(command, serverId); | ||
| AddTimestamp(command, startUtc); | ||
| AddTimestamp(command, endUtc); | ||
| await using var reader = await command.ExecuteReaderAsync(cancellationToken); | ||
| if (!await reader.ReadAsync(cancellationToken)) | ||
| { | ||
| return (null, 0, 0); | ||
| } | ||
|
|
||
| return (reader.IsDBNull(0) ? null : reader.GetDouble(0), reader.GetInt32(1), reader.IsDBNull(2) ? 0 : reader.GetDouble(2)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// #2320: the server's core count from its latest properties snapshot — the other half of the | ||
| /// denominator. Null when properties were never collected; the caller omits the ratio then. | ||
| /// </summary> | ||
| public static async Task<int?> GetLatestCpuCountAsync( | ||
| NpgsqlDataSource postgres, int serverId, CancellationToken cancellationToken = default) | ||
| { | ||
| /* COALESCE(vcore_count, cpu_count) — the same Azure-aware read the FinOps utilization CTE | ||
| uses in both SKUs: on Azure SQL DB the vcore count is the honest denominator. */ | ||
| const string sql = """ | ||
| SELECT COALESCE(vcore_count, cpu_count) | ||
| FROM server_properties | ||
| WHERE server_id = $1 | ||
| ORDER BY collection_time DESC | ||
| LIMIT 1 | ||
| """; | ||
| await using var command = postgres.CreateCommand(sql); | ||
| AddInt(command, serverId); | ||
| var result = await command.ExecuteScalarAsync(cancellationToken); | ||
| return result is int cores && cores > 0 ? cores : null; | ||
| } | ||
|
|
||
| public static async Task<List<CpuSample>> GetCpuUtilizationAsync( | ||
| NpgsqlDataSource postgres, int serverId, DateTime startUtc, CancellationToken cancellationToken = default) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pins the pure
CpuAttribution.Compute()math well, but nothing in either suite exercises the actual wiring that producescpu_windowthrough the real tool call —grep -rn "cpu_window"across the repo only matches the two production files (DarlingMcpDataTools.cs,McpQueryTools.cs), never a test.GetCpuWindowAverageAsync/GetLatestCpuCountAsync(both apps) are likewise never referenced from a test.Concretely,
DarlingMcpDataToolsTests' full-lifecycle test plants only onecpu_utilization_statsrow (PlantCpuAsync), which is belowCpuAttribution.MinSamples(3) — socpu_windowis alwaysnullon that path, and the test can't tell whether the SQL, unit conversion (µs→ms→s), or JSON field wiring is actually correct. A bug in any of those (wrong column, wrong table, a units slip) would ship silently sinceTryComputeCpuWindowAsyncswallows all exceptions into "omit the field."Worth adding at least one test per app that plants ≥3 CPU samples + a
server_properties/v_server_propertiesrow and asserts the populatedcpu_windowobject's actual values (measured/attributed/ratio) round-trip throughget_top_queries_by_cpuorget_top_procedures_by_cpu.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair — and the vcore COALESCE was exactly the kind of thing that would have hidden behind the degrade catch if the column hadn't existed. f652660 wires it: the gated live-PG round-trip now plants three CPU samples spanning 23h (meeting the gate's floors for the default window) and asserts through the REAL tool call that
cpu_windowis non-null,measured_sql_cpu_secondsequals the planted 40% × cpu_count 16 × 24h exactly, the ratio is below half, and the remainder note fires. A reader-SQL typo now fails the suite instead of nulling silently.