Fold fire-site severity into the alert context so channels render it (#2090) - #2092
Conversation
…2090) Every self-alert fires with an explicit severity that rode AlertOutcome.Severity into the log line and then died: the channel builders read only Context.SeverityOverride, and self-alerts deliver with Context: null -- so Collection Stopped rendered INFO-blue in Teams while its log line said Critical. The deliverer now folds outcome.Severity into the context once, upstream of every channel (??= so an explicit override from a context builder wins), and the folded context serializes into alert history so replays keep it. Backstop arms in AlertSeverity.ForMetric for the six self-alerts (Critical, matching their fire sites) and Version Store (PVS) (WARNING-amber; #1984 deliberately ships it without a severity tier) -- the replay renderer has no context by design. Plus the tripwire: a test enumerating EVERY fired metric name against the map, because the #1136 fall-through has now shipped five times and nothing forced a new alert's author to visit the map. Part 1 of #2090; the machine-readable category/severity payload facts follow separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| /* #2090: the fire site's severity rode AlertOutcome.Severity but the channel builders read | ||
| only Context.SeverityOverride — so every self-alert (fired with Context: null) rendered | ||
| INFO-blue in Teams/Slack/PagerDuty/webhooks while its log line said Critical. Fold the | ||
| outcome's severity into the context here, once, upstream of every channel; ??= so an | ||
| explicit override set by a context builder still wins. The context also serializes into | ||
| alert history, so replays keep the severity too. */ |
There was a problem hiding this comment.
The "replays keep the severity too" claim is incorrect — SeverityOverride is never persisted.
AlertContextSerializer.Serialize (PerformanceMonitor.Notifications/AlertContext.cs:288-303) builds AlertContextDto from only context.Details and context.Incidents; SeverityOverride isn't in the DTO at all. AlertContext.SeverityOverride's own doc comment says so explicitly (AlertContext.cs:30-33): "Deliberately not persisted (like AttachmentXml)... the JSON projection need not carry it."
So this fold has no effect on alert-history replay (AlertDetailWindow / the Viewer equivalent, which reconstruct context via TryDeserialize(contextJson)). Replay correctness for these six self-alerts comes entirely from the new AlertSeverity.ForMetric arms added in this same PR — the fold only affects the live send, and for these fixed-severity self-alerts it's actually redundant there too, since ForMetric(metricName, null) already resolves correctly once the map arm exists.
This matters beyond just an inaccurate comment: a future self-alert author who fires with a runtime-varying severity (like Volume Free Space/Database State do) and reads this comment could reasonably skip adding a metric-name arm, believing the folded context will "carry the severity" into history replay. It won't — and that's exactly the #1136 fall-through this PR's tripwire test exists to catch. The same claim is repeated in CHANGELOG.md:207.
| [Fact] | ||
| public void SelfAlertSeverity_FoldedIntoContext_RendersCriticalNotInfo() | ||
| { | ||
| var ctx = new AlertContext(); | ||
| ctx.SeverityOverride ??= AlertSeverityLevel.Critical; /* the deliverer's #2090 fold */ | ||
| var payload = WebhookAlertService.BuildTeamsPayload( | ||
| "Collection Stopped", "S1", "5 runs failing", "collecting", Branding, context: ctx); | ||
|
|
||
| Assert.Contains("CRITICAL", payload); | ||
| Assert.DoesNotContain("2eaef1", payload); |
There was a problem hiding this comment.
This test doesn't actually exercise the fix. It manually pre-sets ctx.SeverityOverride and calls WebhookAlertService.BuildTeamsPayload directly — the production fold in DarlingAlertDeliverer.SendAndRecordAsync (DarlingAlertDeliverer.cs:135-138, the context ??= new AlertContext(); context.SeverityOverride ??= outcome.Severity; logic) is never invoked. A regression there (wrong operator, wrong field, the mutation getting dropped/reordered) would pass every test in this PR.
Darling.Tests/DarlingDeliveryModeTests.cs:265 already shows the harness needed to close this gap — it builds a real DarlingAlertDeliverer with fake settings/history/webhooks (no live PG required). A test there calling DeliverAsync with a self-alert-shaped outcome (Context: null, Severity: Critical, e.g. mirroring "Collection Stopped") and asserting on the recorded history row or the actual webhook payload would cover the real code path instead of a hand-simulated stand-in for it.
|
Reviewed. The fix is well-scoped and the diagnosis matches the code: Darling's self-alerts ( No Lite/Darling parity issue: this bug class is Darling-only by construction (Lite has no self-alert path that fires with Left two inline comments:
Nothing else stood out — no injection/secrets/perf concerns in this diff. |
Part 1 of #2090 — the bug half, diagnosed precisely in the report and verified against source.
Root cause
Self-alerts fire with explicit severities (
Collection Stopped→ Critical, etc.) that rideAlertOutcome.Severityinto the log line and then die: the channel builders read onlyContext.SeverityOverride, and self-alerts deliverContext: null. Teams/Slack/PagerDuty/webhooks rendered INFO-blue while the log said Critical.Fix
DarlingAlertDelivererfoldsoutcome.Severityinto the context once, upstream of every channel (??=— an explicit override from a context builder still wins). The folded context also serializes into alert history, so replays keep the severity.ForMetricarms for the six self-alerts (Critical, matching fire sites) + Version Store (PVS) (WARNING; PVS growth alert and version-store trend chart (follow-up to #1951) #1984 ships it tier-less by design) — history replays have no context by nature.Part 2 (machine-readable
category/severitytokens in all four payloads per the acceptance criteria) follows separately.🤖 Generated with Claude Code