diff --git a/Darling/Darling.Tests/Viewer4577Tests.cs b/Darling/Darling.Tests/Viewer4577Tests.cs new file mode 100644 index 0000000000..bca835032a --- /dev/null +++ b/Darling/Darling.Tests/Viewer4577Tests.cs @@ -0,0 +1,202 @@ +using System; +using System.Globalization; +using System.IO; +using PerformanceMonitor.PlanAnalysis; +using Xunit; + +namespace Darling.Tests; + +// Locks in MetricFormatter — the one place optimizer costs and durations are shaped for a +// human. Contract for costs: at most four decimals, trailing zeros stripped, thousands +// separators, exactly zero is "0", a non-zero cost too small for four decimals is "<0.0001" +// and never "0", and never scientific notation. Contract for durations: the statements-grid +// ladder, ms under a second, seconds with one decimal under a minute, m+s beyond that. +// +// Every case passes InvariantCulture explicitly. Production formats in the caller's culture +// (the panels next to these values already use "N0"/"N1"), so pinning the provider here is +// what keeps the expected strings honest on a machine that separates numbers differently. +// +// Ported from erikdarlingdata/PerformanceStudio@caa57ce's MetricFormatterTests. +public class Viewer4577Tests +{ + private static readonly CultureInfo Invariant = CultureInfo.InvariantCulture; + + [Theory] + // Exactly zero is the whole point of the change: "0.000000" was the worst offender. + [InlineData(0.0, "0")] + // Trailing zeros come off, and four decimals is the cap. + [InlineData(3526.21, "3,526.21")] + [InlineData(16.7659, "16.7659")] + [InlineData(0.0289, "0.0289")] + [InlineData(0.5, "0.5")] + // Whole numbers lose the decimal point entirely rather than showing ".0000". + [InlineData(1.0, "1")] + [InlineData(42.0, "42")] + // Past four decimals it rounds, it does not truncate. + [InlineData(0.00123456, "0.0012")] + [InlineData(1.99999, "2")] + // Thousands separators, including a cost big enough that "G" would go exponential. + [InlineData(44940.5, "44,940.5")] + [InlineData(1234567.891, "1,234,567.891")] + [InlineData(1e12, "1,000,000,000,000")] + // Negative is not a cost showplan produces, but it must not turn into garbage if one arrives. + [InlineData(-16.7659, "-16.7659")] + public void FormatCost_ShapesTheNumber(double cost, string expected) + { + Assert.Equal(expected, MetricFormatter.FormatCost(cost, Invariant)); + } + + [Theory] + // The case that must NOT read "0": a cost this small is tiny, not absent. Everything + // below half of the last shown digit rounds away, so that is where the label starts. + [InlineData(0.000001)] + [InlineData(0.00001)] + [InlineData(0.000049)] + public void FormatCost_TinyButRealCostNeverReadsAsZero(double cost) + { + Assert.Equal("<0.0001", MetricFormatter.FormatCost(cost, Invariant)); + + // And the distinction is the point — a real zero still reads "0". + Assert.NotEqual(MetricFormatter.FormatCost(0, Invariant), MetricFormatter.FormatCost(cost, Invariant)); + } + + [Fact] + public void FormatCost_TinyNegativeKeepsItsSign() + { + Assert.Equal(">-0.0001", MetricFormatter.FormatCost(-0.000001, Invariant)); + } + + [Fact] + public void FormatCost_JustAboveTheThresholdShowsTheDigitInstead() + { + // 0.00006 survives rounding to four decimals, so it gets the digit rather than the label. + Assert.Equal("0.0001", MetricFormatter.FormatCost(0.00006, Invariant)); + } + + [Theory] + // No showplan cost is ever this small or this large, but a format string that falls back to + // scientific notation would corrupt the panel silently, so assert it cannot happen. + [InlineData(1e-30)] + [InlineData(1e20)] + [InlineData(double.MaxValue)] + public void FormatCost_NeverUsesScientificNotation(double cost) + { + var text = MetricFormatter.FormatCost(cost, Invariant); + + Assert.DoesNotContain("E", text, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + // Under a second: milliseconds, no scaling. + [InlineData(0L, "0ms")] + [InlineData(1L, "1ms")] + [InlineData(847L, "847ms")] + [InlineData(999L, "999ms")] + // A second and up: seconds with one decimal. + [InlineData(1000L, "1.0s")] + [InlineData(3475L, "3.5s")] + [InlineData(59_999L, "60.0s")] + // A minute and up: minutes plus whole seconds. + [InlineData(60_000L, "1m 0s")] + [InlineData(1_235_000L, "20m 35s")] + [InlineData(3_600_000L, "60m 0s")] + public void FormatDuration_ClimbsTheLadder(long ms, string expected) + { + Assert.Equal(expected, MetricFormatter.FormatDuration(ms, Invariant)); + } + + [Fact] + public void FormatDuration_MatchesTheLadderTheStatementsGridAlreadyUsed() + { + /* The helper exists to replace a private copy of exactly this ladder in StatementRow. + If someone "improves" the boundaries here, the statements grid and every panel that + now shares this helper move together — which is the point — so the boundaries are + worth pinning rather than leaving to whoever edits next. */ + Assert.Equal("999ms", MetricFormatter.FormatDuration(999, Invariant)); + Assert.Equal("1.0s", MetricFormatter.FormatDuration(1000, Invariant)); + Assert.Equal("59.9s", MetricFormatter.FormatDuration(59_900, Invariant)); + Assert.Equal("1m 0s", MetricFormatter.FormatDuration(60_000, Invariant)); + } + + [Theory] + // Below a millisecond, where the whole-ms ladder would report a query that ran as one + // that did not. Two rungs: the number itself, then a floor for the vanishingly small. + [InlineData(0.4, "0.4ms")] + [InlineData(0.42, "0.42ms")] + [InlineData(0.425, "0.43ms")] + [InlineData(0.01, "0.01ms")] + [InlineData(0.009, "<0.01ms")] + [InlineData(0.0000001, "<0.01ms")] + // Zero is zero, and a duration is never negative - but if one ever arrives it reads as 0ms + // rather than as a fraction with a minus sign. + [InlineData(0.0, "0ms")] + [InlineData(-0.4, "0ms")] + // At a millisecond and above it is the same ladder, reached by rounding rather than truncating. + [InlineData(1.0, "1ms")] + [InlineData(1.6, "2ms")] + [InlineData(999.4, "999ms")] + [InlineData(1000.0, "1.0s")] + public void FormatDuration_HasTwoMoreRungsBelowAMillisecond(double ms, string expected) + { + Assert.Equal(expected, MetricFormatter.FormatDuration(ms, Invariant)); + } + + [Fact] + public void FormatDuration_AgreesWithItselfWhicheverOverloadIsCalled() + { + /* The Query Store Overview kept its own copy of the sub-millisecond rungs because this + helper had none. It delegates now, so the two must not drift: a whole number of + milliseconds reads the same whether it arrives as a long or as a double. */ + foreach (var ms in new long[] { 0, 1, 7, 999, 1000, 59_900, 60_000, 1_235_000 }) + Assert.Equal( + MetricFormatter.FormatDuration(ms, Invariant), + MetricFormatter.FormatDuration((double)ms, Invariant)); + } + + /// + /// Census pin: no cost display site in PerformanceMonitor.Ui still hand-formats with a raw + /// ":F6" or ":F4" numeric format string for a cost. MemoryFractionInput/Output keep their own + /// ":F4" (a different metric family PerformanceStudio leaves alone), so this only forbids + /// ":F6", which was exclusively the unrouted cost sites this issue replaces. + /// + [Fact] + public void NoRawCostFormatStringSurvivesInTheUi() + { + var uiDir = Path.Combine(RepoRoot(), "PerformanceMonitor.Ui"); + Assert.True(Directory.Exists(uiDir), $"PerformanceMonitor.Ui not found at {uiDir}"); + + var offenders = new System.Collections.Generic.List(); + var filesScanned = 0; + + foreach (var file in Directory.EnumerateFiles(uiDir, "*.cs", SearchOption.AllDirectories)) + { + filesScanned++; + var text = File.ReadAllText(file); + if (text.Contains(":F6")) + offenders.Add(file); + } + + /* Floor over the walk itself, same idiom as ViewerButtonAccessKeyTests.RepoRoot: a walk that + silently found nothing would satisfy an empty-offenders assertion for the wrong reason. */ + Assert.True(filesScanned >= 5, $"only {filesScanned} .cs files found under {uiDir}; the walk is not finding the source"); + + Assert.True(offenders.Count == 0, "Raw \":F6\" cost formatting left in: " + string.Join(", ", offenders)); + } + + /// Same walk-up idiom as ViewerButtonAccessKeyTests.RepoRoot. + private static string RepoRoot() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + for (var i = 0; i < 10 && directory is not null; i++) + { + if (File.Exists(Path.Combine(directory.FullName, "PerformanceMonitor.sln"))) + return directory.FullName; + + directory = directory.Parent; + } + + throw new DirectoryNotFoundException( + "Could not locate the repository root (walked up from the test binary looking for " + + "PerformanceMonitor.sln). This test scans the source tree, so it cannot run without it."); + } +} diff --git a/PerformanceMonitor.PlanAnalysis/MetricFormatter.cs b/PerformanceMonitor.PlanAnalysis/MetricFormatter.cs new file mode 100644 index 0000000000..52646c4013 --- /dev/null +++ b/PerformanceMonitor.PlanAnalysis/MetricFormatter.cs @@ -0,0 +1,86 @@ +using System; +using System.Globalization; + +namespace PerformanceMonitor.PlanAnalysis; + +/// +/// The shared shape for the two metric families that reach a human: optimizer costs +/// and durations. Costs come out of showplan at full float precision, which reads as +/// noise in a panel ("16.765900", "0.000000"), and durations come out as raw +/// milliseconds, which stop being readable somewhere past a few seconds. +/// +/// Human-display strings only. Machine-readable output — MCP tool results, plan XML +/// round-tripping — must NOT come through here: those keep the raw values so whatever +/// reads them can do its own arithmetic. +/// +public static class MetricFormatter +{ + /// + /// Magnitude below which a cost rounds away to nothing at four decimals. Anything + /// under this is reported as "smaller than the smallest thing we show" rather than + /// rounded to "0". + /// + private const double SmallestShownCost = 0.00005; + + /// + /// An optimizer cost (operator, subtree, I/O, CPU) with at most four decimal places, + /// trailing zeros stripped, and thousands separators like the rest of the UI: + /// 3526.210000 becomes "3,526.21", 16.765900 becomes "16.7659", 0.028900 becomes + /// "0.0289". Exactly zero is "0", never "0.0000". + /// + /// A non-zero cost too small to survive four decimals reads "<0.0001" rather than + /// "0", so a cost that exists never displays as free. Never scientific notation. + /// + public static string FormatCost(double cost, IFormatProvider? provider = null) + { + provider ??= CultureInfo.CurrentCulture; + + if (cost == 0) + return "0"; + + if (Math.Abs(cost) < SmallestShownCost) + return cost > 0 ? "<0.0001" : ">-0.0001"; + + // "#,##0.####" both caps the decimals at four and drops the trailing zeros, and + // unlike "G"/"R" it can never fall back to scientific notation on a huge cost. + return cost.ToString("#,##0.####", provider); + } + + /// + /// The same ladder the statements grid already uses (under a second stays in + /// milliseconds like "847ms", under a minute scales to seconds with one decimal like + /// "3.5s", and anything longer splits into minutes and seconds like "20m 35s"), for a + /// duration that can be finer than a millisecond. + /// + /// + /// The ladder below starts at whole milliseconds, so an average CPU of 0.4ms — an ordinary + /// number on a healthy OLTP database — rounds to "0ms" and reports a query that ran as one that + /// did not. Query Store keeps microseconds, so its numbers arrive here as fractions and get two + /// extra rungs below 1ms; anything already counted in whole milliseconds climbs exactly the + /// rungs it always did, because that is what the overload below hands it. + /// + public static string FormatDuration(double ms, IFormatProvider? provider = null) + { + provider ??= CultureInfo.CurrentCulture; + + if (ms > 0 && ms < 1) + return ms < 0.01 ? "<0.01ms" : ms.ToString("0.##", provider) + "ms"; + + return FormatDuration((long)Math.Round(ms), provider); + } + + public static string FormatDuration(long ms, IFormatProvider? provider = null) + { + provider ??= CultureInfo.CurrentCulture; + + if (ms < 1000) + return ms.ToString("N0", provider) + "ms"; + + if (ms < 60_000) + return (ms / 1000.0).ToString("F1", provider) + "s"; + + var minutes = (ms / 60_000).ToString("N0", provider); + var seconds = (ms % 60_000 / 1000).ToString("N0", provider); + return $"{minutes}m {seconds}s"; + } +} diff --git a/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs b/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs index 3e4fdcbec9..5ff1f961ac 100644 --- a/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs +++ b/PerformanceMonitor.Ui/PlanViewerControl.Properties.cs @@ -307,10 +307,10 @@ private void ShowPropertiesPanel(PlanNode node) // === Estimated Costs Section === AddPropertySection("Estimated Costs"); - AddPropertyRow("Operator Cost", $"{node.EstimatedOperatorCost:F6} ({node.CostPercent}%)"); - AddPropertyRow("Subtree Cost", $"{node.EstimatedTotalSubtreeCost:F6}"); - AddPropertyRow("I/O Cost", $"{node.EstimateIO:F6}"); - AddPropertyRow("CPU Cost", $"{node.EstimateCPU:F6}"); + AddPropertyRow("Operator Cost", $"{MetricFormatter.FormatCost(node.EstimatedOperatorCost)} ({node.CostPercent}%)"); + AddPropertyRow("Subtree Cost", MetricFormatter.FormatCost(node.EstimatedTotalSubtreeCost)); + AddPropertyRow("I/O Cost", MetricFormatter.FormatCost(node.EstimateIO)); + AddPropertyRow("CPU Cost", MetricFormatter.FormatCost(node.EstimateCPU)); // === Estimated Rows Section === AddPropertySection("Estimated Rows"); diff --git a/PerformanceMonitor.Ui/PlanViewerControl.Tooltips.cs b/PerformanceMonitor.Ui/PlanViewerControl.Tooltips.cs index bd34003233..bb9aae76e8 100644 --- a/PerformanceMonitor.Ui/PlanViewerControl.Tooltips.cs +++ b/PerformanceMonitor.Ui/PlanViewerControl.Tooltips.cs @@ -48,8 +48,8 @@ private ToolTip BuildNodeTooltip(PlanNode node, List? allWarnings = // Cost AddTooltipSection(stack, "Costs"); - AddTooltipRow(stack, "Cost", $"{node.CostPercent}% of statement ({node.EstimatedOperatorCost:F6})"); - AddTooltipRow(stack, "Subtree Cost", $"{node.EstimatedTotalSubtreeCost:F6}"); + AddTooltipRow(stack, "Cost", $"{node.CostPercent}% of statement ({MetricFormatter.FormatCost(node.EstimatedOperatorCost)})"); + AddTooltipRow(stack, "Subtree Cost", MetricFormatter.FormatCost(node.EstimatedTotalSubtreeCost)); // Rows AddTooltipSection(stack, "Rows"); @@ -66,8 +66,8 @@ private ToolTip BuildNodeTooltip(PlanNode node, List? allWarnings = if (node.EstimateIO > 0 || node.EstimateCPU > 0 || node.EstimatedRowSize > 0) { AddTooltipSection(stack, "Estimates"); - if (node.EstimateIO > 0) AddTooltipRow(stack, "I/O Cost", $"{node.EstimateIO:F6}"); - if (node.EstimateCPU > 0) AddTooltipRow(stack, "CPU Cost", $"{node.EstimateCPU:F6}"); + if (node.EstimateIO > 0) AddTooltipRow(stack, "I/O Cost", MetricFormatter.FormatCost(node.EstimateIO)); + if (node.EstimateCPU > 0) AddTooltipRow(stack, "CPU Cost", MetricFormatter.FormatCost(node.EstimateCPU)); if (node.EstimatedRowSize > 0) AddTooltipRow(stack, "Avg Row Size", $"{node.EstimatedRowSize} B"); } diff --git a/PerformanceMonitor.Ui/PlanViewerControl.xaml.cs b/PerformanceMonitor.Ui/PlanViewerControl.xaml.cs index d7ada0dabc..8e81e05268 100644 --- a/PerformanceMonitor.Ui/PlanViewerControl.xaml.cs +++ b/PerformanceMonitor.Ui/PlanViewerControl.xaml.cs @@ -396,15 +396,8 @@ public class StatementRow public PlanStatement Statement { get; set; } = null!; // Display helpers — grid binds to these, sorting uses the raw properties via SortMemberPath - public string CpuDisplay => FormatDuration(CpuMs); - public string ElapsedDisplay => FormatDuration(ElapsedMs); - public string UdfDisplay => UdfMs > 0 ? FormatDuration(UdfMs) : ""; + public string CpuDisplay => MetricFormatter.FormatDuration(CpuMs); + public string ElapsedDisplay => MetricFormatter.FormatDuration(ElapsedMs); + public string UdfDisplay => UdfMs > 0 ? MetricFormatter.FormatDuration(UdfMs) : ""; public string CostDisplay => EstCost > 0 ? $"{EstCost:F2}" : ""; - - private static string FormatDuration(long ms) - { - if (ms < 1000) return $"{ms}ms"; - if (ms < 60_000) return $"{ms / 1000.0:F1}s"; - return $"{ms / 60_000}m {(ms % 60_000) / 1000}s"; - } }