Repository navigation
Dash state-only alert values (#1846); split analysis Query Store dedups by replica (#1850) - #1890
Conversation
…lica (#1850) #1846 — alert history rendered "0.00" for alerts that have no number to report. The value columns are NOT NULL doubles, producers hand the store a display STRING, and the store's parser yields 0 for text carrying no digit, so a role ("PRIMARY"), a connection state ("DISCONNECTED"), a suspend reason or the literal "resolved" all landed as a sentinel that rendered as a measurement. Both grids now render an em dash for those metrics. Write side untouched: the column stays NOT NULL, 0 stays the sentinel, and rows already collected display correctly with no migration. Enumerated from the fire sites rather than the issue text, which found 19 metrics rather than 9: the seven AG metrics, Server Unreachable/Restored, and every resolution notice — Darling's BuildResolutionRecord hardcodes CurrentValueText "resolved" for both its own self-alert recoveries and the shared engine's resolution callback, and those reach history only in Darling. Resolutions match through the existing shared AlertMetricClassifier .IsResolution, so a future recovery metric is right without an edit, and the predicate lives in PerformanceMonitor.Common rather than being hand-mirrored into two formatters — the drift that class exists to stop. The dash is gated on a stored 0, never the name alone, and that gate is load-bearing: the parser scans to the first digit ANYWHERE, so AG Sync Fell Behind spells its lag into its prose and any of them can pick a digit out of an object name. Three Darling self-alerts are excluded for that reason (#1881), Store Disk Pressure most of all — its parsed value is percent-free, where a real 0 means a full volume. A measurement that happens to be zero is not a missing value; both suites pin that separately. #1850 — four analysis-layer Query Store dedups kept a key narrower than the read side's, with no replica_role. sys.query_store_runtime_stats is keyed by (plan_id, interval, execution_type, replica_group), and on a SQL Server 2022+ AG with Query Store for secondary replicas the primary holds ONE shared Query Store carrying every replica's rows — so the rn = 1 filter was not de-duplicating those rows, it was DROPPING one. An under-count, which is worse than the double-count the dedup exists to fix: a double-count is visible in the number, a dropped row is silent. #1845 used this reasoning for the read-side key; these four are the same defect one layer down. Split rather than blended: replica_role is carried through the grouping and the ranking, a regression is measured WITHIN a replica, and it is exposed in both drill-down row shapes. The joins use IS NOT DISTINCT FROM, never = — replica_role is NULL on every standalone/non-AG/pre-2022 server and NULL = NULL is UNKNOWN, so an equi-join would have silently disabled plan-regression detection for nearly every install. Verified live on real DuckDB and real PostgreSQL 18.4 + TimescaleDB, with a NULL-replica arm in both suites. Watched RED first on both stores: two replicas over the same two intervals, the old key reporting one offender at 3x against a true two offenders and 12x — dropping the primary reported the secondary's milder regression as the server's worst. One downstream consequence fixed mechanically: the force-plan recommendation reads fixed keys, so two rows for one query would have rendered two sp_query_store_force_plan calls naming the same query with different plan ids; it now keeps the worst per query, also watched red. Which replica should drive that recommendation is #1882. The Darling FROM/JOIN drift guard needed a real fix rather than an accommodation: it scanned comments and read the word "join" in English prose as a relation, and read the FROM inside IS NOT DISTINCT FROM as one too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alysis # Conflicts: # CHANGELOG.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| - **Alerts that have no number to report show a dash instead of "0.00"** ([#1846]) - the alert-history `current_value` and `threshold_value` columns are NOT NULL doubles, and a whole family of alerts has no measurement to put in them: their value is a role (`PRIMARY`), a connection state (`DISCONNECTED`), a suspend reason, a failure reason, or the literal `resolved`. Producers hand the history store a display STRING, the store's parser yields 0 for any text carrying no digit, and both grids rendered that sentinel as `0.00` - a number the alert never made. The read side now renders an em dash for those metrics, in Lite's Alert History and the Darling Viewer's alike. **The write side is untouched**: the column stays NOT NULL and 0 stays the stored sentinel, so this corrects display for rows already collected with no migration and no re-collection. | ||
|
|
||
| **The list is 19 metrics, not the 9 the issue named**, because it was enumerated from the fire sites rather than from the report. Nine were known: the seven AG metrics plus Server Unreachable and Server Restored. The other ten are every resolution notice - Darling's `BuildResolutionRecord` hardcodes `CurrentValueText: "resolved"` with a null numeric for BOTH its own self-alert recoveries (Collection Resumed, Capture Restored, Agent Restarted, Store Disk Pressure Resolved, Compression Job Recovered) and the shared engine's resolution callback (CPU Resolved, Blocking Cleared, Blocking Wait Cleared, Deadlocks Cleared, Poison Waits Cleared, Long-Running Queries Cleared, tempdb Space Resolved, Volume Free Space Resolved, Long-Running Jobs Cleared). Those reach alert history only in Darling, whose grid therefore carried most of the `0.00` rows. Resolutions are matched through the existing shared `AlertMetricClassifier.IsResolution` rather than listed by name, so a future recovery metric renders correctly without an edit, and the predicate lives in `PerformanceMonitor.Common` beside it rather than being hand-mirrored into two formatters - which is the drift that class was created to stop. |
There was a problem hiding this comment.
Arithmetic doesn't match the rest of this paragraph (and the actual pinned test lists). "Nine were known" is right (4 actionable AG + 3 AG/connection resolutions + Server Unreachable + Server Restored = 9), but "the other ten" enumerates 14 items, not 10: 5 self-alert recoveries (Collection Resumed, Capture Restored, Agent Restarted, Store Disk Pressure Resolved, Compression Job Recovered) + 9 shared-engine resolutions (CPU Resolved, Blocking Cleared, Blocking Wait Cleared, Deadlocks Cleared, Poison Waits Cleared, Long-Running Queries Cleared, tempdb Space Resolved, Volume Free Space Resolved, Long-Running Jobs Cleared).
9 + 14 = 23, not 19. This matches the actual count in both pinned StateOnlyMetrics() lists (Lite.Tests/AlertHistoryValueFormatTests.cs and Darling/Darling.Tests/ViewerWave3Tests.cs), which each have 23 entries, not 19. The PR description has the identical "19 metrics"/"the other ten" wording, so this looks like a copy-paste count error rather than a one-off typo. Given this paragraph's whole point is being more rigorous than the issue's number, worth fixing the number itself (19→23, ten→fourteen).
Review summaryReviewed the full diff (16 files, +1134/-36). Both fixes are well-scoped, well-tested, and Lite/Darling parity is maintained throughout. #1846 — dash for state-only alert values
#1850 — replica-scoped Query Store dedup
Security / correctness
No blocking issues found. The CHANGELOG count nit is minor/documentation-only. |
…alysis # Conflicts: # CHANGELOG.md
| ) AS regression_factor, | ||
| LEFT(l.query_text, 500) AS query_text | ||
| LEFT(l.query_text, 500) AS query_text, | ||
| l.replica_role |
There was a problem hiding this comment.
Correctness (minor): the top-5 cap a few lines down (ORDER BY regression_factor DESC / LIMIT 5) can now crowd out distinct queries.
Before this change, that LIMIT 5 always surfaced 5 distinct queries — the old dedup collapsed each query to exactly one row regardless of replica. Now that replica_role is part of the grain, a single query regressing on both PRIMARY and SECONDARY can legitimately occupy two of the five slots (as the new tests seed). On a busy AG primary with more than ~3 distinct regressed queries, this drill-down — and by extension FactRemediation.ExtractPlanRegressionTargets, which reads this same capped list and dedupes down to one row per query — can end up showing/remediating fewer distinct queries than before, because a replica-duplicate consumes a slot a different query would have taken pre-PR.
This is a real, if narrow, coverage regression that isn't mentioned in the PR description (which addresses the duplicate force-plan consequence but not this one), and isn't exercised by the new tests — they only ever seed one regressed query. Worth a conscious call, e.g. rank by DENSE_RANK() OVER (ORDER BY regression_factor DESC) per (database_name, query_id) before applying the row cap, or raise the limit, so five distinct queries are still guaranteed when they exist. Same shape applies to Lite/Analysis/DrillDownCollector.Queries.cs.
| regression_factor, | ||
| LEFT(query_text, 500) AS query_text | ||
| LEFT(query_text, 500) AS query_text, | ||
| replica_role |
There was a problem hiding this comment.
Same LIMIT 5 crowding concern as the Darling twin (PgDrillDownCollector.Queries.cs, CollectRegressedQueries): pre-PR this always returned 5 distinct queries; post-PR a single query regressing on two replicas can take two of the five slots, so a server with several distinct regressed queries can show/remediate fewer of them than before. Not covered by the new QueryStoreReplicaSplitAnalysisTests (single-query seed only). See the paired comment on the Darling file for detail.
Review summaryReviewed the diff for #1846 (dash for state-only alert values) and #1850 (replica-split Query Store dedups). Both changes are well-scoped, well-tested (live-DB pins in both suites, RED-first per the PR description), and Lite/Darling parity is maintained throughout — every SQL/formatter change in one app has a matching change in the other, and the two #1846 (dash rendering)
#1850 (replica-split dedups)
No SQL-injection, secrets, or file/process-boundary concerns — all new SQL is parameterized; the one string-concatenated |
1 participant
Closes #1846. Closes #1850.
#1846 — a dash where there was never a number
The alert-history
current_value/threshold_valuecolumns are NOT NULL doubles, and a family of alerts has no measurement to put in them: their value is a role (PRIMARY), a connection state (DISCONNECTED), a suspend reason, a failure reason, or the literalresolved. Producers hand the history store a display STRING, the store's parser yields 0 for text carrying no digit, and both grids rendered that sentinel as0.00— a number the alert never made. The read side now renders an em dash.Write side untouched. The column stays NOT NULL, 0 stays the stored sentinel. Rows already collected display correctly with no migration and no re-collection.
The list is 19 metrics, not the 9 the issue named
Enumerated from the fire sites rather than the report, as asked. Nine were known — the seven AG metrics plus Server Unreachable and Server Restored. The other ten are every resolution notice:
DarlingSelfAlertEvaluator.BuildResolutionRecordhardcodesCurrentValueText: "resolved"andThresholdValueText: ""with both numerics null, for BOTH Darling's own self-alert recoveries (Collection Resumed, Capture Restored, Agent Restarted, Store Disk Pressure Resolved, Compression Job Recovered) and the shared engine's resolution callback (CPU Resolved, Blocking Cleared, Blocking Wait Cleared, Deadlocks Cleared, Poison Waits Cleared, Long-Running Queries Cleared, tempdb Space Resolved, Volume Free Space Resolved, Long-Running Jobs Cleared). Those reach alert history only in Darling — Lite's resolution callback is toast-only — so Darling's grid carried most of the0.00rows.Resolutions match through the existing shared
AlertMetricClassifier.IsResolutionrather than being listed by name, so a future recovery metric renders correctly without an edit. The predicate lives inPerformanceMonitor.Commonbeside it rather than being hand-mirrored into the two formatters, which is the drift that class was created to stop.The 0-gate is load-bearing, not bookkeeping
AlertValueParser.ParseOrDefaultscans to the first digit anywhere, not to a leading number. So several of these do not reliably store 0:AG Sync Fell Behindspells its lag seconds into its prose, and any of them can pick a digit out of an object name (SQL01,Sales2024). Those rows keep rendering whatever was parsed, and a producer that starts supplying a real numeric is shown rather than hidden.Three Darling self-alerts are deliberately excluded for the same reason (
Collection Stopped,Store Runtime Upgrade,Compression Job Stuck— filed as #1881), andStore Disk Pressuremost of all: its parsed value is percent-free, where a genuine 0 means a full volume. A measurement that happens to be zero is not a missing value —Blocking Detectedat 0 still reads0, pinned separately in both suites.#1850 — the narrower key was dropping a replica, not de-duplicating
Four analysis-layer dedups predate #1841 tier 1 and kept
(database_name, query_id, plan_id, runtime_stats_interval_id, first_execution_time)with noreplica_role.sys.query_store_runtime_statsis keyed by (plan_id, interval, execution_type, replica_group), and on a SQL Server 2022+ AG with Query Store for secondary replicas enabled the primary holds ONE shared Query Store carrying every replica's rows — so two rows differing only inreplica_roleare distinct legitimate work, and thern = 1filter was DISCARDING one. An under-count, worse than the double-count the dedup exists to fix: a double-count is visible in the number, a dropped row is silent. #1845 used exactly this reasoning for the read-side key.Split, per the issue's option 3.
replica_roleis carried through the downstream grouping and the ranking, so a regression is measured WITHIN a replica (this replica's current plan against the best plan this replica has run, never a cross-replica comparison of two different workloads), and it is exposed in both drill-down row shapes. Adding it to the dedup alone would have made totals complete while blending primary and secondary workload into one indistinguishable number.The joins use
IS NOT DISTINCT FROM, never=This is the whole risk of the change.
replica_roleis NULL on every standalone server, every non-AG server and everything below SQL Server 2022, andNULL = NULLis UNKNOWN — an equi-join would have matched nothing and silently disabled plan-regression detection for the overwhelming majority of installs. Darling's drill-down also moved offJOIN ... USING, which is an equi-join. Both suites carry a NULL-replica arm for exactly that case.Fact-shape consequences
PgFactCollector/DuckDbFactCollector): no shape change.Fact.MetadataisDictionary<string, double>and cannot hold a replica dimension; the numbers simply become per-replica-correct.offender_countnow counts two distinct regressions where it counted one.replica_roleadded LAST in the row so existing reader ordinals are untouched. It flows to the MCP payload and to email/webhook/toast detail with no code change (AnalysisNotificationService.FlattenIntoenumerates every property). The drill-down is documented ephemeral and never persisted, so nothing has a pinned schema to break.FactRemediation.ExtractPlanRegressionTargetsreads fixed keys, so two rows for one query would have rendered twosp_query_store_force_plancalls naming the SAME query with DIFFERENT plan ids: mutually exclusive instructions where whichever the operator runs last silently wins. Fixed mechanically — keep the worst per(database, query_id), which theregression_factor DESCorder already provides. Which replica should drive the recommendation is a product question, filed as Force-plan recommendation is replica-blind now that plan regression splits by replica_role #1882.Testing
Watched RED first, on both stores, with the same seed shape: two replicas running the same two plans over the same two intervals, the secondary always collected one second later so the old partition drops the PRIMARY specifically.
offender_countworst_regression_factorThe factor is the point: dropping the primary did not merely lose a row, it reported the secondary's much milder regression as the server's worst. The remediation dedup was watched red separately (2 targets for one query).
-t:Rebuild: 0 warnings, 0 errors.search_pathtrapLiveStoreCleanupdocuments. It now SETs the path explicitly rather than inheriting it.One test-side fix that is a real fix rather than an accommodation: the Darling FROM/JOIN drift guard scanned SQL comments, so English prose about a join ("an equi-join here would…") parsed as a relation named
here, and it read theFROMinsideIS NOT DISTINCT FROMas one too. It now strips line comments and that operator before scanning, so the assertion itself stays exact.Deferred, filed before this PR
🤖 Generated with Claude Code