Alert history: High CPU stores its real value, not 0 (#1830)

[erikdarlingdata]

Summary

Fixes #1830: every High CPU row in Alert History showed 0% (rendered 0,0 % in the reporter's locale). Write-time defect, present in Lite and Darling since the alert engine port: the High CPU outcome fired with NumericCurrentValue: null, and both history stores' fallback parsed the display text "87% (Total CPU)" — which ends with ), so TrimEnd('%') was a no-op, double.TryParse failed, and the : 0 arm silently stored zero. Detection, cooldowns, toasts, email, and webhooks were always correct (they use the text); only the stored audit trail and the MCP alert tool were wrong — which is exactly why nobody saw it until now. The deprecated Dashboard stores the text and was immune.

• Engine (PerformanceMonitor.Alerting, shared → both apps fixed at once): High CPU passes alertCpuValue + threshold; Blocking Detected and Deadlocks Detected pass their counts too, so their stored values stop depending on parse luck.
• Per-event delivery, both apps: PerEventNotification.Message gains NumericValue — 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, storing 0 in the field today). Lite's SendDetectedAlertAsync also stops dropping the outcome numerics on its summary route — the engine fix alone couldn't reach Lite's blocking/deadlock rows.
• Defense in depth: both stores' fallback now goes through a shared AlertValueParser.ParseOrDefault (leading-numeric extraction). Parses with CurrentCulture deliberately — the producers format with it, and a de-DE "92,5%" must parse as 92.5; hardening to invariant would break exactly the reporter's locale. State-string alerts (AG "PRIMARY", Server Unreachable) still store 0 into the NOT NULL column — that's a read-side design question, deliberately not smuggled into this fix.

Test plan

• The two assertions that pinned the bug (Assert.Null(fired.NumericCurrentValue) in Darling's AlertEngineTests and Lite's LiteAlertForwardingTests) watched red against the fix, then flipped to pin the real values
• Per-event pins in both apps: per-incident numerics {1,2,3} / {3,1,1}, overflow row carries its count, thresholds forwarded on every send
• New store round-trip at the exact seam that shipped broken: "87% (Total CPU)", no numerics → stored 87, not 0
• New AlertValueParserTests over the real producer strings, including the comma-decimal locale under de-DE and the no-number state strings
• Lite.Tests alert surface 236/236; Darling.Tests alert surface 49/49 (2 skipped = gated live-PG); both builds 0 warnings

🤖 Generated with Claude Code

[claude]

Correctness (plausible edge case): grouped/thousands-separator numbers are silently truncated to their first digit-group.

The scan advances on ASCII digits and (once) the culture's NumberDecimalSeparator, but never accounts for NumberGroupSeparator. For en-US input "1,234" the loop reads '1', then hits , — which doesn't match decimalSeparator (".") — and stops, returning 1 instead of 1234. Under de-DE the grouping character is ., so a grouped de-DE value like "1.234,5" would similarly truncate to 1.

None of today's producers format with grouping (effectiveBlockingCount.ToString() etc. are ungrouped), so this doesn't fire yet. But AlertValueParser is now the general-purpose fallback both history stores lean on for any future decorated text, and the whole point of #1830 was to stop numeric values depending on the producer's exact string shape — a parser that quietly returns a truncated (but plausible-looking) number for a grouped value is the same failure mode in a new guise, and harder to notice than the old : 0 case since the output isn't obviously wrong.

Worth either handling NumberGroupSeparator in the scan (skip it between digit runs, same guarded-lookahead shape as the decimal separator) or documenting that grouped input is unsupported so a future caller doesn't assume otherwise.

[claude]

Nit: this file already has using PerformanceMonitor.Notifications; (line 12), so the fully-qualified PerformanceMonitor.Notifications.AlertValueParser.ParseOrDefault(...) here and on the next line can just be AlertValueParser.ParseOrDefault(...). Same in Darling/PerformanceMonitor.Darling.Service/PgAlertHistoryStore.cs, which also already has the using.

[claude]

Review

Overview: Fixes #1830 — High CPU (and Blocking/Deadlocks) alert history rows stored 0 instead of the real numeric value, because the fallback parse (double.TryParse(text.TrimEnd('%'))) choked on decorated display text like "87% (Total CPU)" (trailing ), so the trim was a no-op). The fix is layered correctly:

• The shared PerformanceMonitor.Alerting.AlertEngine now passes real NumericCurrentValue/NumericThresholdValue for High CPU, Blocking Detected, and Deadlocks Detected — fixing both apps at the source.
• Per-event delivery (PerEventNotification.Message.NumericValue) carries each incident's occurrence count and the overflow trailer's count, in both Lite's SendDetectedAlertAsync and Darling's DarlingAlertDeliverer.
• Both history stores' fallback now goes through a new shared AlertValueParser.ParseOrDefault (leading-numeric extraction, CurrentCulture-aware) instead of the broken TrimEnd('%') parse, as defense in depth.

Lite/Darling parity: Good — the root cause lived in the shared AlertEngine/PerformanceMonitor.Notifications, so both apps get the engine-level fix simultaneously. The two per-app deliverers (LiteAlertDeliverer, DarlingAlertDeliverer) each got the matching fix for the numerics that only they were responsible for forwarding (Lite's summary route, Darling's per-event route), and both are exercised by tests. AlertValueParser itself is single-instance shared code (not duplicated per app), so one shared test suite in Lite.Tests for it is reasonable rather than a parity gap.

Correctness: left two inline notes — a plausible latent edge case in the new AlertValueParser (grouped/thousands-separator numbers silently truncate to the first digit group; doesn't affect today's producers but is a sharp edge for future ones), and a trivial redundant fully-qualified type reference in both history stores now that using PerformanceMonitor.Notifications; is already present. Everything else checked out:

• The CPU branch's NumericCurrentValue: alertCpuValue passes a double? whose .HasValue is genuinely guaranteed by the surrounding cpuExceeded check — no null-forwarding risk.
• Both RecordAlertAsync implementations remain fully parameterized (no SQL/string-building changes here) — no injection concerns.
• Test coverage is thorough and targeted: the two assertions that pinned the original bug were flipped to pin the fix, new AlertValueParserTests exercise the real producer strings (including the de-DE comma-decimal case and no-number state strings), and a new store round-trip test pins the exact regressed case ("87% (Total CPU)", no numerics → 87, not 0).

Minor: Lite/Services/LiteAlertDeliverer.cs's class-level doc comment (around line 36) still says delivery for Blocking/Deadlocks Detected carries "like the old loop — no numeric values on these two metrics," which is no longer true now that SendDetectedAlertAsync forwards outcome.NumericCurrentValue/NumericThresholdValue. Worth a follow-up doc tweak so future readers don't trust the stale claim (this line isn't part of the diff, so no inline comment on it, but flagging it here).

No missing-index or other DMV-style suggestions here — out of scope per repo convention. Overall this is a well-scoped, well-tested fix; nothing here blocks merging.