diff --git a/CHANGELOG.md b/CHANGELOG.md index cf16beb68..f9511c4eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Lite + Darling: Linux hosts on SQL Server 2025 CU1+ get real other-process CPU again** ([#1630]) - the [#1048] guard NULLed `other_process_cpu_utilization` on EVERY Linux host, because the SCHEDULER_MONITOR ring buffer always reported `SystemIdle = 0` on Linux. SQL Server 2025 CU1 fixes the ring-buffer metrics (KB5078298), so the blanket guard was discarding a derivable value there forever. The guard now keys on the actual condition (`@is_linux = 1 AND system_idle = 0`) instead of the platform, so older Linux builds keep their honest NULL and 2025 CU1+ derives the real figure - version-proof by construction. Community fix by @argpna ([#1629]); applied to the shared collector (Lite + Darling) and the deprecated Dashboard's collection proc + NOC health query. +- **Lite + Darling: default trace collection works on SQL Server on Linux** ([#1633]) - the rollover-file path normalization assumed the trace file always carries a `_N` suffix (true on Windows); the initial file on Linux is plain `log.trc`, so the strip re-appended the extension onto the full path (`log.trc.trc`), `sys.fn_trace_gettable` raised Msg 19049 on the nonexistent file, and NO default-trace events were ever collected on Linux. The strip now falls back to the base path when there is no suffix to remove. Community fix by @argpna ([#1632]), live-verified against a 2025 Linux container; shared collector + the deprecated Dashboard's proc. +- **Full Dashboard (deprecated): the `report.trace_flag_changes` view no longer fails with Msg 245 on a trace-flag toggle** ([#1637]) - the view compared the bit-typed `previous_status`/`status` columns against `N'OFF'`/`N'ON'` literals; bit outranks nvarchar in type precedence, so SQL Server converted the literal to bit and the view failed the moment a toggle row flowed through it. It now compares `0`/`1`. Community fix by @argpna ([#1635]). +- **Default trace: the rollover strip no longer mangles paths whose DIRECTORY contains an underscore** ([#1636]) - follow-up to [#1633]: the strip keyed on the LAST underscore anywhere in the path, so a relocated trace in an underscore-named directory with an unsuffixed file (`/var/opt/my_sql/log/log.trc`) was cut at the directory underscore (`/var/opt/my.trc` - Msg 19049 again). An underscore now only counts as a rollover suffix when it sits after the last path separator (both separator families, since targets are Windows and Linux); truth-tabled live against SQL 2022 across nine path shapes. - **Lite + Darling: Azure SQL DB collection works again when the client is firewalled out at the logical server but allowed at the DATABASE level** ([#1634]) - reported against 3.2.0 by a user monitoring an Elastic Pool database ([#1631]): every database-scoped collector failed with `40615 - Cannot open server ... Client with IP address ... is not allowed to access the server`, and the long-query XE session could not enumerate databases, even though the database itself was fully reachable from SSMS. **This is a regression of the [#857] single-database fallback, introduced by [#1506].** That fix correctly stopped reading 40615 ("client IP not allowed at the logical server") as a statement about a login's RIGHTS to read `master` - it is a reachability error, and misreading it had permanently wedged a different user whose public IP rotated daily. But it also dropped 40615 from the single-database FALLBACK path, on the stated premise that "falling back to a user database is futile because the same rule blocks that connection too." That premise is wrong: Azure SQL Database evaluates **database-level** IP firewall rules BEFORE server-level ones, and a client whose IP matches a database-level rule (`sp_set_database_firewall_rule`) is granted a connection to that database with **no** server-level rule permitting it - while `master`, where server-level rules live, still requires one. "Blocked at the server, allowed at the database" is therefore a real, supported configuration that Microsoft explicitly recommends ("use database-level IP firewall rules whenever possible"), and it is precisely the [#857] case. The two questions - *"does this login lack rights to master?"* and *"should database-scoped collection degrade to single-database?"* - are now separated in the shared `SqlErrorClassification`: the rights list is unchanged (40615 still correctly excluded, so every [#1506] invariant holds), and a new fallback list is that set plus 40615, which is what both SKUs' Azure database-enumeration catch sites actually ask. Acting on it is safe because [#1506]'s other fix survives untouched: the resulting verdict is a revocable throttle, not a latch - it expires after 15 minutes, is discarded when a server returns from an outage, and is skipped entirely when there is no target database to fall back to. A client genuinely firewalled out with no database-level rule therefore just fails its fallback attempt with the same 40615, logs an ordinary collector error, and recovers on the next successful connect. Pinned from both suites, independently: 40615 is still not a rights denial, AND it does trigger the fallback; the rights set remains a strict subset of the fallback set; and the disjointness invariant is now asserted against the broader fallback set (40613 stays transient-and-retryable, never a reason to degrade). Lite + Darling. - **Darling: collectors no longer fail with `22021: invalid byte sequence for encoding "UTF8": 0x00`** ([#1614]) - SQL Server NVARCHAR allows embedded NUL characters and query text from `sys.dm_exec_sql_text` sometimes carries them, but Postgres `text` columns reject the byte, so one NUL-laden cached query failed the entire `query_stats` COPY batch every cycle (`DBCC FREEPROCCACHE` never helped because the app re-caches the same query). The Postgres row writer now strips NULs from every collected string - query text, plan XML, deadlock and blocked-process XML included - at the single COPY choke point. - **Darling Web: a custom view no longer resets the picked time range / server / filters every 60 seconds** ([#1619]) - the background refresh that keeps the dashboard live rebuilt each view from its declared default, snapping any range/server/filter change back within a minute. A per-view scope memory now survives the re-render, so a picked scope sticks until you change it or hard-reload. @@ -530,7 +534,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 [#1626]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1626 [#1627]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1627 [#1631]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1631 +[#1629]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1629 +[#1630]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1630 +[#1632]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1632 +[#1633]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1633 [#1634]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1634 +[#1635]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1635 +[#1636]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1636 +[#1637]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1637 [#1617]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1617 [#1621]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1621 [#1601]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1601 diff --git a/Lite.Tests/CpuUtilizationCollectorDefinitionTests.cs b/Lite.Tests/CpuUtilizationCollectorDefinitionTests.cs index 6da3591fc..d3ab65900 100644 --- a/Lite.Tests/CpuUtilizationCollectorDefinitionTests.cs +++ b/Lite.Tests/CpuUtilizationCollectorDefinitionTests.cs @@ -46,7 +46,10 @@ public void BuildQuery_RingBuffer_SuppressesOtherCpu_OnlyWhenSystemIdleIsZero() CollectorTestContext.Make(s_deltas, isAzureSqlDb: false)); Assert.Contains("WHEN @is_linux = 1 AND x.system_idle = 0", plan.Text, StringComparison.Ordinal); - Assert.DoesNotContain("WHEN @is_linux = 1\n", plan.Text, StringComparison.Ordinal); + + /* Normalize line endings first: on a CRLF checkout the template contains "= 1\r\n", which + the "\n"-suffixed needle would never match, making this guard vacuously pass. */ + Assert.DoesNotContain("WHEN @is_linux = 1\n", plan.Text.Replace("\r\n", "\n"), StringComparison.Ordinal); } [Fact] diff --git a/Lite.Tests/DefaultTraceEventsCollectorDefinitionTests.cs b/Lite.Tests/DefaultTraceEventsCollectorDefinitionTests.cs index e83c587af..502431836 100644 --- a/Lite.Tests/DefaultTraceEventsCollectorDefinitionTests.cs +++ b/Lite.Tests/DefaultTraceEventsCollectorDefinitionTests.cs @@ -91,6 +91,24 @@ nonexistent file. The CASE must fall back to the base t.path when CHARINDEX find Assert.Contains("ELSE t.path", text, StringComparison.Ordinal); } + [Fact] + public void BuildQuery_RolloverPathStrip_OnlyStripsUnderscoresInTheFilename() + { + /* A trace DIRECTORY containing an underscore with an unsuffixed file (#1636: + /var/opt/my_sql/log/log.trc) must NOT strip — the last underscore has to sit AFTER the + last path separator (either family, since targets are Windows and Linux) to count as a + rollover suffix. Otherwise LEFT cuts at the directory underscore and produces a + nonexistent path (/var/opt/my.trc → Msg 19049). */ + var text = DefaultTraceEventsCollector.Instance.BuildQuery(MakeContext()).Text; + + Assert.Contains( + @"AND (CHARINDEX(N'\', REVERSE(t.path)) = 0 OR CHARINDEX(N'_', REVERSE(t.path)) < CHARINDEX(N'\', REVERSE(t.path)))", + text, StringComparison.Ordinal); + Assert.Contains( + "AND (CHARINDEX(N'/', REVERSE(t.path)) = 0 OR CHARINDEX(N'_', REVERSE(t.path)) < CHARINDEX(N'/', REVERSE(t.path)))", + text, StringComparison.Ordinal); + } + [Fact] public void BuildQuery_CapturesTheCuratedEventSet() { diff --git a/PerformanceMonitor.Collectors/CpuUtilizationCollector.cs b/PerformanceMonitor.Collectors/CpuUtilizationCollector.cs index cdc8c762d..a04a2c66b 100644 --- a/PerformanceMonitor.Collectors/CpuUtilizationCollector.cs +++ b/PerformanceMonitor.Collectors/CpuUtilizationCollector.cs @@ -72,12 +72,13 @@ @start_time datetime2(7), /* Detect SQL Server on Linux. SystemIdle reports 0 in the SCHEDULER_MONITOR ring buffer on some Linux/SQL Server version combos, so 100 - SystemIdle - ProcessUtilization - fabricates a host figure that pins total CPU at 100% (Issue #1048). Prior to SQL Server - 2025 CU1 no DMV exposes true host CPU when that happens, so other_process is stored as NULL - there. sys.dm_os_linux_cpu_stats (2025 CU1+) exposes real host CPU jiffies but is a cumulative - counter requiring a two-sample delta, not a point-in-time snapshot like SCHEDULER_MONITOR, so - it isn't used here. sys.dm_os_host_info is 2017+; referenced via sp_executesql so SQL 2016 - never binds it (@is_linux = 0). */ + fabricates a host figure that pins total CPU at 100% (Issue #1048). The ring-buffer metrics + fix shipped in SQL Server 2025 CU1 (KB5078298 fix 4796293), so real SystemIdle values start + there, not at 2025 RTM. Prior to that no DMV exposes true host CPU when SystemIdle is 0, so + other_process is stored as NULL. sys.dm_os_linux_cpu_stats (2025 CU1+) exposes real host CPU + time but is a cumulative counter requiring a two-sample delta, not a point-in-time snapshot + like SCHEDULER_MONITOR, so it isn't used here. sys.dm_os_host_info is 2017+; referenced via + sp_executesql so SQL 2016 never binds it (@is_linux = 0). */ IF OBJECT_ID(N'sys.dm_os_host_info', N'V') IS NOT NULL EXEC sys.sp_executesql N'SELECT @linux = CASE WHEN hi.host_platform = N''Linux'' THEN 1 ELSE 0 END FROM sys.dm_os_host_info AS hi;', diff --git a/PerformanceMonitor.Collectors/DefaultTraceEventsCollector.cs b/PerformanceMonitor.Collectors/DefaultTraceEventsCollector.cs index 8b9fa946c..115af272c 100644 --- a/PerformanceMonitor.Collectors/DefaultTraceEventsCollector.cs +++ b/PerformanceMonitor.Collectors/DefaultTraceEventsCollector.cs @@ -114,12 +114,14 @@ public sealed class Row /* The curated, config-slice-free event set (see the class remarks). A {0} placeholder is spliced with the per-server excluded-database clause on ft.DatabaseName; the rollover-file base-path normalization - (strip _NN + re-append the extension) reads every retained file. The strip is guarded by a CHARINDEX - check: SQL Server on Linux names the initial trace file without a rollover suffix (log.trc, not - log_1.trc), so an unconditional strip finds no underscore, LEFT takes the full path unchanged, and - appending the extension back doubles it (log.trc.trc) - fn_trace_gettable then errors on the - nonexistent file (Msg 19049) and no events are collected. Falling back to t.path when CHARINDEX - returns 0 keeps the Windows behavior (which always has the _N suffix) unchanged. READ UNCOMMITTED + (strip _NN + re-append the extension) reads every retained file. The strip only fires when the + LAST underscore sits in the FILENAME, i.e. after the last path separator (either family — the + collector runs against Windows and Linux targets): SQL Server on Linux names the initial trace + file without a rollover suffix (log.trc, not log_1.trc), so an unguarded strip mangles the path + (log.trc.trc — #1633), and a trace DIRECTORY containing an underscore would mangle it a second + way (/var/opt/my_sql/log/log.trc -> /var/opt/my.trc — #1636); fn_trace_gettable then errors on + the nonexistent file (Msg 19049) and no events are collected. Falling back to t.path in both + cases keeps the Windows behavior (which always has the _N suffix) unchanged. READ UNCOMMITTED like every collector; OPTION(RECOMPILE) because the cutoff selectivity varies wildly between the all-history first run and the tiny steady-state windows. */ private const string QueryTemplate = @" @@ -152,6 +154,8 @@ CROSS APPLY sys.fn_trace_gettable ( CASE WHEN CHARINDEX(N'_', REVERSE(t.path)) > 0 + AND (CHARINDEX(N'\', REVERSE(t.path)) = 0 OR CHARINDEX(N'_', REVERSE(t.path)) < CHARINDEX(N'\', REVERSE(t.path))) + AND (CHARINDEX(N'/', REVERSE(t.path)) = 0 OR CHARINDEX(N'_', REVERSE(t.path)) < CHARINDEX(N'/', REVERSE(t.path))) THEN LEFT(t.path, LEN(t.path) - CHARINDEX(N'_', REVERSE(t.path))) + RIGHT(t.path, 4) ELSE t.path END, diff --git a/deprecated/Dashboard/Services/DatabaseService.NocHealth.cs b/deprecated/Dashboard/Services/DatabaseService.NocHealth.cs index 152418671..e1a06f29e 100644 --- a/deprecated/Dashboard/Services/DatabaseService.NocHealth.cs +++ b/deprecated/Dashboard/Services/DatabaseService.NocHealth.cs @@ -477,14 +477,15 @@ end_time DESC /* SystemIdle reports 0 in the SCHEDULER_MONITOR ring buffer on some Linux/SQL Server version combos, so 100 - SystemIdle - ProcessUtilization fabricates a - host figure that pins total CPU at 100% forever (Issue #1048). Prior to SQL - Server 2025 CU1 no DMV exposes true host CPU when that happens, so report - other/host CPU as NULL and let the alert engine fall back to the SQL-only - figure. sys.dm_os_linux_cpu_stats (2025 CU1+) exposes real host CPU jiffies - but is a cumulative counter requiring a two-sample delta, not a point-in-time - snapshot like SCHEDULER_MONITOR, so it isn't used here. sys.dm_os_host_info is - 2017+; referenced via sp_executesql so SQL 2016 (no Linux build) never binds it - (@is_linux stays 0). */ + host figure that pins total CPU at 100% forever (Issue #1048). The ring-buffer + metrics fix shipped in SQL Server 2025 CU1 (KB5078298 fix 4796293), so real + SystemIdle values start there, not at 2025 RTM. Prior to that no DMV exposes + true host CPU when SystemIdle is 0, so report other/host CPU as NULL and let + the alert engine fall back to the SQL-only figure. sys.dm_os_linux_cpu_stats + (2025 CU1+) exposes real host CPU time but is a cumulative counter requiring a + two-sample delta, not a point-in-time snapshot like SCHEDULER_MONITOR, so it + isn't used here. sys.dm_os_host_info is 2017+; referenced via sp_executesql so + SQL 2016 (no Linux build) never binds it (@is_linux stays 0). */ IF OBJECT_ID(N'sys.dm_os_host_info', N'V') IS NOT NULL BEGIN EXEC sys.sp_executesql diff --git a/install/18_collect_cpu_utilization_stats.sql b/install/18_collect_cpu_utilization_stats.sql index 4c0556d9e..653bf6d95 100644 --- a/install/18_collect_cpu_utilization_stats.sql +++ b/install/18_collect_cpu_utilization_stats.sql @@ -59,12 +59,14 @@ BEGIN Detect SQL Server on Linux. On some Linux/SQL Server version combos the SCHEDULER_MONITOR ring buffer reports SystemIdle = 0, so 100 - SystemIdle - ProcessUtilization fabricates a host figure that pins total CPU at 100% - forever (Issue #1048). Prior to SQL Server 2025 CU1 there is no DMV that - exposes true host CPU when that happens, so we store NULL for - other_process_cpu_utilization instead of a false value. sys.dm_os_linux_cpu_stats - (2025 CU1+) exposes real host CPU jiffies but is a cumulative counter - requiring a two-sample delta, not a point-in-time snapshot like - SCHEDULER_MONITOR, so it isn't used here. + forever (Issue #1048). The ring-buffer metrics fix shipped in SQL Server + 2025 CU1 (KB5078298 fix 4796293), so real SystemIdle values start there, + not at 2025 RTM. Prior to that there is no DMV that exposes true host CPU + when SystemIdle is 0, so we store NULL for other_process_cpu_utilization + instead of a false value. sys.dm_os_linux_cpu_stats (2025 CU1+) exposes + real host CPU time but is a cumulative counter requiring a two-sample + delta, not a point-in-time snapshot like SCHEDULER_MONITOR, so it isn't + used here. sys.dm_os_host_info exists only on SQL Server 2017+. It is referenced through sp_executesql so SQL Server 2016 (which has no Linux build) never binds it and diff --git a/install/29_collect_default_trace.sql b/install/29_collect_default_trace.sql index c754e8c66..a5ad506f7 100644 --- a/install/29_collect_default_trace.sql +++ b/install/29_collect_default_trace.sql @@ -359,15 +359,19 @@ BEGIN CROSS APPLY sys.fn_trace_gettable ( /* - SQL Server on Linux names the initial trace file without a rollover suffix - (log.trc, not log_1.trc). An unconditional strip finds no underscore, LEFT - takes the full path unchanged, and appending the extension back doubles it - (log.trc.trc) - fn_trace_gettable then errors on the nonexistent file - (Msg 19049) and no events are collected. Fall back to st.path when - CHARINDEX returns 0. + Strip the _N rollover suffix only when the LAST underscore sits in the + FILENAME, i.e. after the last path separator (either family). SQL Server + on Linux names the initial trace file without a rollover suffix (log.trc, + not log_1.trc), so an unguarded strip mangles the path (log.trc.trc, + issue 1633), and a trace DIRECTORY containing an underscore would mangle + it a second way (/var/opt/my_sql/log/log.trc -> /var/opt/my.trc, issue + 1636) - fn_trace_gettable then errors on the nonexistent file (Msg 19049) + and no events are collected. Fall back to st.path in both cases. */ CASE WHEN CHARINDEX(N'_', REVERSE(st.path)) > 0 + AND (CHARINDEX(N'\', REVERSE(st.path)) = 0 OR CHARINDEX(N'_', REVERSE(st.path)) < CHARINDEX(N'\', REVERSE(st.path))) + AND (CHARINDEX(N'/', REVERSE(st.path)) = 0 OR CHARINDEX(N'_', REVERSE(st.path)) < CHARINDEX(N'/', REVERSE(st.path))) THEN LEFT(st.path, LEN(st.path) - CHARINDEX(N'_', REVERSE(st.path))) + RIGHT(st.path, 4) ELSE st.path END,