diff --git a/CHANGELOG.md b/CHANGELOG.md index 9664dfa6ad..319c5e7b3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Top Procedures collects on Azure SQL Database** ([#1833]) - `procedure_stats` was one of two database-scoped collectors that never opted into the Azure per-database connection path: it ran once on the server entry's own connection - whose catalog defaults to `master` when the entry's Database field is blank - and its Azure variant's `WHERE s.database_id = DB_ID()` then matched only master's procedures. Zero user rows, logged `SUCCESS`, empty Top Procedures grid that looked healthy, while Top Queries (which does run per database) kept working right next to it - the asymmetry that fingerprinted the bug. The collector now declares `RunsPerDatabase` on Azure like `query_stats` and its five database-scoped siblings, and the per-database connection makes `DB_ID()` mean each user database in turn - exactly what that predicate was written for. Both Lite and Darling get it (shared definition). The other collector with the same gap is Query Store, which needs a per-database rework on Azure rather than a one-line override - tracked as [#1836], with the invisible-zero-rows Collection Health gap that hid both defects tracked as [#1837]. + - **Chart x-axis timestamps now honor the Local/Server/UTC time toggle** ([#1831]) - Lite plots chart X values in server time and converts for display at render, but the ONE render surface doing the axis labels - the single shared `LabelFormatter` all three apps reach - never consulted the display mode, so every chart's bottom axis showed server time no matter what the toggle said, while grids, slicers, tooltips, and the crosshair all converted around it. Refresh and reconnect could not help: they re-plotted the same unconverted labels. The formatter now converts each label through the `UiTimeContext` hook Lite wired at startup for exactly this purpose and never used here; the Darling Viewer pre-converts its plotted X and deliberately leaves the hook at identity, so the change is a no-op there by construction - the double-conversion trap a Viewer-style port into Lite would have hit is documented at the site and pinned by test. Three companions ride along, all the same defect in different clothes: the display-mode toggle now re-plots the charts immediately (it refreshed nine grids and six slicers and zero charts, so even the fixed formatter only showed on the next data cycle); the Queries heatmap's hand-built axis labels convert like that chart's own tooltip always did; and the three history windows switch to the shared formatter, gaining both the conversion and the date-change labels. The deprecated Dashboard's startup hack that force-pinned the dropdown to ServerTime ("charts always render in server time") is retired - and its saved preference, which was written on every change and read never, is finally restored at startup. New tests pin the formatter's conversion, the Viewer's identity no-op, and the display-date boundary behavior - nothing anywhere tested the formatter before. - **High CPU alerts no longer record 0 as their value in Alert History** ([#1830]) - the alert fired with no numeric value, and the history stores' fallback tried to parse the display text `"87% (Total CPU)"` - which ends with a parenthesis, so the trailing-`%` trim did nothing, the parse failed, and the `: 0` arm silently stored zero for every High CPU row ever written, in Lite and Darling alike (the deprecated Dashboard stores the text and was immune). Detection, thresholds, and every notification surface were always correct - the toast, email, and webhook all carry the text - so only the audit trail and the MCP alert tool were corrupted, which is exactly what made it invisible. The engine now passes real numerics for High CPU, Blocking Detected, and Deadlocks Detected; per-event delivery carries each incident's occurrence count (and the overflow trailer's COUNT - its `"+N more incident(s)"` text is unparseable by design and was the second live instance of the same coercion); and both stores' fallback goes through a shared leading-numeric parser so a future decorated text cannot re-coin a silent 0. Parses with CurrentCulture on purpose - the producers format with it, and the reporter's own locale writes `92,5%`. Two tests that PINNED the null-numerics behavior are flipped to pin the fix, and a store round-trip now asserts the exact field case - `"87% (Total CPU)"`, no numerics - lands as 87, not 0. @@ -1964,4 +1966,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1823]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1823 [#1828]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1828 [#1831]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1831 +[#1833]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1833 +[#1836]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1836 +[#1837]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1837 [#1830]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1830 diff --git a/Lite.Tests/ProcedureStatsCollectorDefinitionTests.cs b/Lite.Tests/ProcedureStatsCollectorDefinitionTests.cs index de815e2eeb..5556c4709b 100644 --- a/Lite.Tests/ProcedureStatsCollectorDefinitionTests.cs +++ b/Lite.Tests/ProcedureStatsCollectorDefinitionTests.cs @@ -76,6 +76,18 @@ public void BuildQuery_Azure_SingleDatabase_TokenLeftInPlace() Assert.Contains("/*EXCLUSION_FILTER*/", plan.Text, StringComparison.Ordinal); } + [Fact] + public void RunsPerDatabase_OnAzureOnly() + { + /* #1833: without the per-database override, the Azure variant ran once on the server + entry's own connection (master when the Database field is blank), where its + database_id = DB_ID() filter matched nothing — zero user rows, logged SUCCESS. The + per-database connection is what makes that predicate mean each user database. */ + Assert.True(ProcedureStatsCollector.Instance.RunsPerDatabase(new CollectorTargetInfo { IsAzureSqlDb = true })); + Assert.False(ProcedureStatsCollector.Instance.RunsPerDatabase(new CollectorTargetInfo())); + Assert.False(ProcedureStatsCollector.Instance.RunsPerDatabase(new CollectorTargetInfo { IsAzureManagedInstance = true })); + } + [Fact] public void BuildQuery_HandlesConvertToVarchar130_NeverTruncated() { diff --git a/PerformanceMonitor.Collectors/ProcedureStatsCollector.cs b/PerformanceMonitor.Collectors/ProcedureStatsCollector.cs index 142de78b83..06ad2523ab 100644 --- a/PerformanceMonitor.Collectors/ProcedureStatsCollector.cs +++ b/PerformanceMonitor.Collectors/ProcedureStatsCollector.cs @@ -274,6 +274,17 @@ the xml type still return. dm_exec_text_query_plan(plan_handle, 0, -1) returns a public override string TargetTable => "procedure_stats"; + /// + /// #1833: on Azure SQL Database this collector must run per database, like query_stats and the + /// other database-scoped collectors already do. Without the override it ran once on the server + /// entry's own connection — whose catalog defaults to master when the Database field is blank — + /// and the Azure variant's WHERE s.database_id = DB_ID() then filtered to master's + /// procedures: zero user rows, logged SUCCESS, and an empty Top Procedures grid that looked + /// healthy. The per-database connection makes DB_ID() each user database in turn, which + /// is exactly what that predicate was written for. + /// + public override bool RunsPerDatabase(CollectorTargetInfo target) => target.IsAzureSqlDb; + public override CollectorQuery BuildQuery(CollectorContext context) { var planSelect = context.CapturePlanXml ? PlanSelectFragment : "";