Repository navigation
Deliver the alert's detail text on every channel - #3302
Conversation
An alert's DetailText reached the history store, the Viewer, the MCP reader and the triage endpoint, and no delivery channel. The email template's detail section was gated on AlertContext.Details -- a different, structured field -- and a self-alert fires with context: null, so an email-only operator received a metric name, a value, a threshold and two timestamps with the remedy discarded. Reported on #3296. Email (both bodies), Teams, Slack, the generic channel and PagerDuty now carry it. The prose is resolved once per firing and suppressed when it is only a flattening of the structured context, so engine alerts render unchanged. Also: the #2813 Retention Held detail now names the service restart, which is the step that actually arms a held policy.
Mutation testing found the one uncovered hop: dropping the detail on the way to the email template alone passed every pin, because email has no interception point short of the protocol. That is the exact defect #3296 reported, so the loopback sink now speaks enough SMTP to capture the DATA and decode both transfer encodings, and the assertion is that the phrase appears TWICE -- once per MIME part.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review summaryThis PR does what it says: Lite/Darling parity looks good — the change lives entirely in the shared One correctness/security issue worth a close look before merge — left as an inline comment on Everything else — the |
Review raised that the prose detail bypasses the redaction that replaces copy-paste remediation T-SQL with a hint on every webhook channel. The rule's subject is the GENERATED payload -- IsCodeBlock bodies and RemediationAction, built by FactRemediation, multi-statement and in two shapes destructive. Prose is authored per alert, and where it names a command at all that command is the recovery action and the sentence is the alert; redacting it delivers "see email or the in-app dialog" to an operator whose only channel is the one being redacted. What made that a habit rather than a fact is that nothing stopped a generated command from being folded into the prose. FindingMessageFormatter.DetailText is finding metadata and the command rides only in the code-block item, so that is now asserted instead of assumed, and both docs say which surface the rule covers.
Review summaryOverall this is a well-scoped, well-tested fix — the fan-out resolves Two things worth a look before merge (left as inline comments):
No SQL in this PR, no missing-index-DMV concerns, nothing else stood out on security/performance. |
…w token Review found two fire sites that hand-author a detail text restating their own structured fields under different labels. The suppression gate compares the prose against the context's flattening, so it never fired for them and both alerts would now render the same facts twice on every channel. Per #2109 the structured fields are the canonical copy, so the restatement goes and the detail text is their flattening, as at the other eleven sites. The one thing each prose said that its fields did not -- no baseline yet, and that the query is running on the optimizer's plan -- is a field. The generic channel's new token was also in neither app's Settings help text, so nothing told an operator it exists. Both lists carry it now, and Lite's automation list picks up the triage_url it never got. The token set is one list the matcher is built from, and the help text is checked against that list rather than against a second copy of it: this is the second drift of the same kind.
Review: #3297 — deliver alert DetailText on every channelReviewed all 6 commits (the diff is already self-corrected across several review passes: SMTP-wire pin, What I checked
Minor, non-blocking observation In No inline comments — didn't find anything I'd block on. |
…splay name #3308 threaded a custom rule's display name through the same five-channel seam this branch threads the alert's prose detail through, so every signature that gained a parameter from one side gained one from the other. Both are kept: detailText first (the positional form AlertIncidentRenderTests uses), displayName second. The generic channel takes the prose but not the display name — its payload has no title, and {{metric}} is the machine key an automation correlates on.
The two fields the conflict was in are both string?, so losing one or transposing the
pair at any of the three hops between the deliverer and the payload builders is not a
compile error — and measured against HEAD, it is not a test failure either. Dropping
displayName in DarlingAlertDeliverer, and again in EmailSendCore's webhook fan-out,
both left every suite green: each side's tests cover its payload BUILDERS, and neither
could cover the hops with the other side's field also present.
This drives the whole Darling path once with both set and reads the bytes that left
the process. Bodies are identified by a channel-native marker rather than by arrival
order, and the generic channel's deliberate exemption — prose yes, display name no,
because {{metric}} is a machine key and that channel renders no title — is asserted
here instead of assumed.
|
Reviewed. This is a well-scoped, thoroughly tested fix — traced the full delivery path (email HTML/plain text, Teams, Slack, generic webhook, PagerDuty) against One minor nit left as an inline comment on |
Expected State already renders "(no baseline yet)" whenever the baseline is missing — it has since #2109, and expectedText is computed from the same `pending` flag the extra Baseline field was conditioned on. So the field said the same thing under a second label, and shipped it to every channel now that the detail block is delivered. The three canonical fields stand alone again, and the pin counts occurrences rather than enumerating field names: a name list would pass while the fact arrived under a label nobody thought to enumerate, which is the failure being guarded.
Review summaryTraced the full delivery path this PR adds ( One correctness issue found and left as an inline comment on Analysis-finding alerts will render their Diagnosis facts twice on every channel. Everything else — the SMTP/webhook wire-level test harness, the T-SQL-redaction scoping decision, the |
1 participant
An alert's
DetailText— the block that says what happened, what it costs, and what to do about it — reached the history store, the WPF viewer, the MCP reader and the triage endpoint, and no delivery channel.WebhookAlertServiceheld zero references to it across all 1,534 lines. The email template does have a detail section, but it is gated onAlertContext.Details, a separate and structured collection; self-alerts fire withcontext: null, so it never rendered and the detail had no path to the message at all. Reported from the field on #3296, where the operator received a metric name, a current value, a threshold and two timestamps, and had to open an issue to find out what the alert meant.Which surfaces were broken
All five, and PagerDuty only looked like an exception.
DETAILSsection--- Details ---DetailsMessageCard section,textnot a factsectionblock under the fields{{context}}, plus a new discrete{{detail}}tokencustom_details["Details"]BuildPagerDutyCustomDetailsalready flattenedAlertContext.Details, which is why #2710's note reads as though the channel carried the detail. It carried the structured collection; a prose-only self-alert took the empty-Detailsbranch that reducescustom_detailsto{"Sent by": …}.The generic channel was a token question, and a new token alone would have fixed nothing for anyone: the shipped default template carries
{{context}}, so does every template an operator has already saved, and none of them can reference a token that did not exist when they were written. So the prose leads{{context}}— which is what makes the fix arrive with no operator action — and{{detail}}exists on top of that for a template that wants the prose on its own, mapped into a ticket body field rather than mixed with the flattened structure.What keeps engine alerts unchanged
Every engine alert's
DetailTextisContextToDetailTextof its own context —AlertEnginebuilds it that way at eleven fire sites — so carrying it on top of the structured render would print the same content twice in exactly the alerts that already read correctly.AlertDetailText.ProseForDeliverysuppresses on that equality, resolved once per firing in the fan-out the waytriageUrlalready is.It is deliberately not "suppress whenever a context is present": the #2109 AG-database alerts carry both a structured context and independent prose naming the
SET HADR RESUMEremedy, and that prose must still be delivered. The comparison is against the sharedAlertDetailText.Flatten, so the two texts are equal by construction —AlertContextBuilders.ContextToDetailTextnow forwards to it rather than holding a second copy. Should they ever disagree anyway the alert is delivered twice, which is visible and harmless, rather than having its remedy dropped again.The wording fix
The #2813 Retention Held detail said the policy "arms ITSELF once its consumer covers everything raw holds" and stopped there. True and one step short:
TimescaleSupport.EnsureRetentionPoliciesAsyncis the only thing that arms a held policy and it has exactly one call site, the service startup path. An operator following the old wording runs--backfill-rollups, watches the hourly Critical keep firing, and concludes the backfill failed. The detail now names the restart and says why it is not optional — the wordingdocs/retention-hold-runbook.mdalready uses. That runbook's "the alert's email does not carry any of the above" rough edge is now stale and is removed; the untunable-thresholds one stays.IAlertDeliverer's doc comment forDetailTextsaid it was a rendering ofContext, which is the statement that makes delivering it look redundant. It now says what a self-alert actually puts there.Verification
There is no dogfooding path to this defect. No email is configured on any of the three stores and there is no webhook channel yet, so it can only ever be found by a user filing an issue, which is how it was found. The pins are the whole guard, and every one of them was mutation-tested: the detail deleted from the payload, red confirmed, restored, and the restore verified by blob hash rather than by diffstat.
TheDeliverer_PutsDetailTextOnTheWire_OnEmailAndEveryRedirectableWebhookChanneldrives the whole Darling path —DeliverAsync, the send core, the fan-out, each builder — and asserts against the bytes that left the process, via a loopback sink per channel. Deliberately not a payload-builder assertion: the defect was never in a builder, it was that nothing passed the text to one, and a builder-level pin is green on a fan-out that drops the argument. Mutation testing proved that: droppingprosein the fan-out kills only this test, and every builder pin stays green.It also found the one hop that no pin covered — the send core handing the email template the detail. Email has no interception point short of the protocol, and that hop is the channel #3296 actually reported, so the sink now speaks enough SMTP to capture the DATA and decode both transfer encodings (.NET picks base64 for the utf-8 plain part and quoted-printable for the us-ascii HTML one). The assertion is that the phrase appears twice, once per MIME part, so losing either body fails.
PagerDuty's endpoint is the hardcoded Events v2 URL and cannot be redirected at a sink; its builder is pinned separately and takes its prose from the same single resolution in the fan-out as the three that are checked on the wire.
Full solution builds clean with zero warnings. The Windows-only suites were run on macOS by compiling the actual committed test
.csfiles intonet10.0console harnesses over an xunit shim, in three scopes: the eight Lite render/payload classes (111/111), the two Darling delivery classes (44 pass, 1 skip), andAlertEngineTests(69/69, 66 async tests confirmed awaited rather than reported as hollow passes). The shim has a--self-checkmode with two control classes — six that must pass, five that must fail, including a failure raised after anawaitand structural equality on two distinct-but-equalList<T>— so a hollow pass and a red that belongs to the harness are both ruled out by measurement rather than by care. A shimmed subject reports SKIP and is never counted as a pass:TheViewerRow_RendersThroughTheSharedDescriber's subject is the WPFViewerAlertRow, so it skips here and CI runs it for real.The new tests are confirmed to have RUN, by count rather than by name — MTP prints no per-test
Passedlines, so the grep that used to confirm this returns nothing. Againstdevata220d210,Lite.Testsgoes 3646 to 3663, exactly the 17 cases added here (15 facts plus a two-case theory), andDarling.Tests8600 to 8604, exactly the 4 (2 here, plus the merged-seam wire pin and the no-duplicate-fact pin below).Skippedis unchanged — 338 in the Windows job, 6 in the Postgres one — and none of the touched classes appears in either skip list. TheDarling whole-tree guardsjob reports in seconds because its gate saw thebuildjob run the full Darling suite on this commit and says so; that is its documented skip path, not an empty run.One mutation survived, and it is not a gap in this change: passing
nullinstead of the per-event split's detail text inDarlingAlertDeliverer. That path's detail text is by construction the flattening of its own split context, so the gate suppresses it on either branch and no channel's output moves. It does reach the history row, which no test asserts — that is #1236/#1141's coverage, untouched here.Two things review found, and what changed for each
The prose bypasses the never-on-a-webhook T-SQL redaction. The premise is right — the #2109 AG alert's
DetailTextreally does containALTER DATABASE [db] SET HADR RESUME, and it now rides out unredacted. Declined, with the reason recorded in the code. That rule's subject is the GENERATED payload:IsCodeBlockbodies andRemediationActionbuilt byFactRemediation, multi-statement, two of the seven shapes destructive, carrying a two-sided risk-disclosure header that has to be read before the script is run. Prose is authored per alert, and where it names a command at all that command IS the recovery action and the sentence IS the alert; replacing it with "see email or the in-app dialog" delivers nothing to an operator whose only channel is the one being redacted, which is #3296 over again. It is also not a disclosure control — the payload already carriesresource_name,databaseand{{incidents_json}}with the same object names.What the finding got right underneath that is the drift risk: nothing stopped a generated command from being folded into the prose, at which point it really would bypass the rule with no test noticing.
FindingMessageFormatter.DetailTextis finding metadata and the command rides only in the code-block item, so that is now asserted rather than assumed, and both docs say which surface the rule covers.Two engine alerts would have delivered their facts twice. Correct, and fixed. Database-State-Changed and Force-Plan-Failing hand-author a detail text restating their own structured fields under different labels, so the suppression gate never fired for them. Per #2109 the fields are the canonical copy, so the restatement goes and the detail text is their flattening, as at the other eleven fire sites; the one thing each prose said that its fields did not — no baseline yet, and that the query is running on the optimizer's plan — is a field now. All thirteen
detailTextlocals inAlertEnginenow come fromContextToDetailText.And the new token was in neither app's Settings help text, so nothing told an operator it exists. Both lists carry it, and Lite's automation list picks up the
{{triage_url}}it never got when #2710 added it to Darling's. That is the second drift of the same kind, so the token set is now one list the matcher is built from and the help text is checked against that list — not against a second copy free to go stale the same way. Adding a token to the engine and not to the windows fails.Merged with #3303's display name, which landed on the same seam first
#3308 threaded a custom rule's human display name through the same five-channel delivery seam this branch threads the alert's prose detail through, and landed on
devfirst. Every signature betweenDarlingAlertDelivererand the payload builders had gained one parameter from each side, so four files conflicted. Both are kept,detailTextahead ofdisplayNameso this branch's positional call sites inAlertIncidentRenderTestsstay valid while #3308's nameddisplayName:sites are unaffected.The two intents compose per channel rather than collapsing into one rule, because they are different kinds of thing. The prose is alert CONTENT, so every channel carries it. The display name is a TITLE concern, so it reaches the Teams card title and summary, the Slack header, the PagerDuty summary and the email subject and heading — and deliberately not the generic channel, which renders no title and whose
{{metric}}field is the machine key an automation correlates on. That exemption was #3308's decision and is preserved; it is now asserted rather than left as a comment.Neither side's severity, cooldown, dedup or mute key moves:
AlertSeverity.ForMetric,DerivePagerDutyDedupKeyand the history row all still key on the immutable metric name.The hops the conflict was in had no pin, measured rather than assumed
detailTextanddisplayNameare bothstring?. Dropping one, or transposing the pair, at any of the three hops between the deliverer and the builders is not a compile error — and against the pre-merge code it was not a test failure either. DroppingdisplayNameinDarlingAlertDeliverer, and again inEmailSendCore's webhook fan-out, each left every suite green: #3308's tests cover its payload BUILDERS, and neither side could cover a hop with the other side's field also present, because neither side had both fields.That is the same asymmetry this branch already found and closed on the prose side, so it is closed the same way.
TheDeliverer_CarriesBothTheProseAndTheDisplayName_ThroughTheOneFanOutdrives the whole Darling path once with both fields set and reads the bytes that left the process. Bodies are identified by a channel-native marker rather than by arrival order. Four mutations confirm it discriminates: droppingdisplayNameat either hop, transposing the pair at the merged call, and handing the generic channel the display name in place of the machine key all turn it red.One more duplicated fact, found by the review pass on the merged head
The pending database-state alert carried a
Baseline: none — first observed in a critical statefield alongside anExpected Statethat already renders(no baseline yet)— computed from the samependingflag, and present since #2109. So it stated one fact under two labels, and now that the detail block is delivered that is something an operator reads twice rather than an internal duplication. It is the same defect class this PR exists to prevent, so it is fixed here rather than noted: the three canonical fields stand alone again, and the commit comment claiming the no-baseline fact was not already a field is corrected.DatabaseState_PendingCriticalFirstObservation_StatesTheMissingBaselineOncecounts occurrences rather than enumerating field names, because a name list would pass while the fact arrived under a label nobody thought to enumerate — which is the failure being guarded. Re-adding the field turns it red.Open, and not fixed here: analysis-finding alerts deliver their Diagnosis facts twice
FindingMessageFormatteris a third producer of(Context, DetailText)pairs, after the twoAlertEnginesites above, and this PR does not touch it.BuildContextputs Story / Severity / Notify threshold / Confidence / Facts / Database / Window into aDiagnosisitem;DetailTextindependently formats the same seven values with different labels (Facts in chainvsFacts, one combined severity line vs two fields,-vs→in the window). The two are therefore never string-equal, the suppression gate treats the prose as independent, and every channel renders both. Reproduced against the real formatter and the real email template:ProseForDeliveryreturns the prose, and the plain-text body carries all six facts twice, adjacent.It is not fixed here because
FindingMessageFormatter.DetailTextis not only a delivery string — it is the persistedconfig_alert_log.detail_textfor every analysis row, what the MCP reader and the triage page read, the Dashboard's no-channel tray-row text, and the inputAlertMuteContext.PopulateFromDetailTextparses. Making itAlertDetailText.Flatten(BuildContext(...)), which is the #2109 rule the twoAlertEnginesites follow and the right direction, changes what that column holds for the largest alert category: it gains theDiagnosisheading, a separateNotify thresholdfield, the advice headline, the remediation-T-SQL heading and every drill-down field. That belongs in its own change with its own review.Widening the gate is the wrong direction: any "suppress when the context's facts all appear in the prose" rule also drops the #2109 AG alerts'
SET HADR RESUMEremedy, which is the case the gate exists not to drop.The review thread on
AlertDetailText.csis left unresolved for this reason.CHANGELOG
The
[Unreleased]entry text, for whoever is batching them: