From a56b10dae09d0940c8fb6de0ab6807291cd9189f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 18 Jun 2026 11:03:32 -0400 Subject: [PATCH] #1141: fix per-event cards losing forensic detail + Current Value (gotqn feedback) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses gotqn's two findings from testing the dev build (the #1140 fingerprint itself tested great — stable key + climbing Occurrences): 1. Per-event cards no longer carry LESS detail than Summary. AlertIncident now carries transient DetailFields (forensic facts), populated by the groupers from the representative event: blocking -> Database / Contentious Object / Blocked Query / Blocking Query / Lock Mode; deadlock -> Victim SQL / Processes (Lite), Query / Wait Resource / Lock Mode (Dashboard). PerEventNotification.Split renders them onto each per-incident card and now also carries the source AttachmentXml/FileName so per-event email keeps the deadlock_graph.xml / blocked_process_report.xml. Summary rendering is untouched (AlertIncidentRenderer.Apply leaves DetailFields off to avoid duplicating the builder's own items), and DetailFields are not persisted. 2. Per-event "Current Value" is now the occurrence count (a number, matching Summary), not the involved-objects string (which already shows as its own fact). Lite 500 + Dashboard 487 tests green (3 new: detail preserved, attachment carried, Current Value = count); both apps build 0 warnings. Refs #1141 #1140 Co-Authored-By: Claude Opus 4.8 (1M context) --- Dashboard/MainWindow.xaml.cs | 19 ++++++++- Lite.Tests/PerEventNotificationTests.cs | 39 ++++++++++++++++- Lite/MainWindow.xaml.cs | 15 ++++++- .../AlertContext.cs | 13 +++++- .../AlertIncidentRenderer.cs | 42 ++++++++++++------- .../IncidentGrouping.cs | 37 +++++++++++++--- .../PerEventNotification.cs | 32 +++++++++----- 7 files changed, 159 insertions(+), 38 deletions(-) diff --git a/Dashboard/MainWindow.xaml.cs b/Dashboard/MainWindow.xaml.cs index 0184479b..84341f8c 100644 --- a/Dashboard/MainWindow.xaml.cs +++ b/Dashboard/MainWindow.xaml.cs @@ -2273,7 +2273,7 @@ await _emailAlertService.TrySendAlertEmailAsync( AlertIncidentRenderer.Apply(context, BlockingIncidentGrouper.Group( serverName, events.Select(e => new BlockingIncidentGrouper.BlockedEvent( - e.DatabaseName, e.ContentiousObject, e.QueryText, null, e.WaitTimeMs ?? 0))) + e.DatabaseName, e.ContentiousObject, e.QueryText, null, e.WaitTimeMs ?? 0, e.LockMode))) .Select(g => g.Incident).ToList()); return context; @@ -2351,7 +2351,8 @@ deadlock event across ALL events in the window. */ serverName, deadlocks.GroupBy(d => d.EventDate).Select(g => new DeadlockIncidentGrouper.DeadlockEvent( DeadlockObjectExtractor.FromGraphXml( - g.Select(x => x.DeadlockGraph).FirstOrDefault(x => !string.IsNullOrEmpty(x)))))) + g.Select(x => x.DeadlockGraph).FirstOrDefault(x => !string.IsNullOrEmpty(x))), + DeadlockDetailFields(g)))) .Select(g => g.Incident).ToList()); return context; @@ -2368,6 +2369,20 @@ deadlock event across ALL events in the window. */ /// A deadlock is only excluded when ALL process nodes have a currentdbname in the excluded list. /// Cross-database deadlocks involving any non-excluded database will still be reported. /// + /* #1141: forensic detail carried on a deadlock incident so per-event cards keep the query + + wait resource + lock mode (Summary mode shows them via the builder's own items). */ + private static List? DeadlockDetailFields(IEnumerable participants) + { + var rep = participants.FirstOrDefault(x => !string.IsNullOrWhiteSpace(x.Query)) ?? participants.FirstOrDefault(); + if (rep is null) return null; + var f = new List(); + if (!string.IsNullOrWhiteSpace(rep.DatabaseName)) f.Add(new AlertIncidentField("Database", rep.DatabaseName)); + if (!string.IsNullOrWhiteSpace(rep.Query)) f.Add(new AlertIncidentField("Query", Truncate(rep.Query))); + if (!string.IsNullOrWhiteSpace(rep.WaitResource)) f.Add(new AlertIncidentField("Wait Resource", rep.WaitResource)); + if (!string.IsNullOrWhiteSpace(rep.LockMode)) f.Add(new AlertIncidentField("Lock Mode", rep.LockMode)); + return f.Count > 0 ? f : null; + } + private static bool IsDeadlockExcluded(DeadlockItem deadlock, List excludedDatabases) { if (string.IsNullOrEmpty(deadlock.DeadlockGraph)) return false; diff --git a/Lite.Tests/PerEventNotificationTests.cs b/Lite.Tests/PerEventNotificationTests.cs index 018a1d7d..e2f5eadb 100644 --- a/Lite.Tests/PerEventNotificationTests.cs +++ b/Lite.Tests/PerEventNotificationTests.cs @@ -34,7 +34,44 @@ public void Split_WithinCap_OneMessagePerIncident() Assert.Equal(3, messages.Count); Assert.All(messages, m => Assert.False(m.IsOverflow)); Assert.All(messages, m => Assert.Single(m.Context.Incidents!)); - Assert.Equal("db.dbo.T0", messages[0].CurrentValue); + Assert.Equal("1", messages[0].CurrentValue); // Current Value = occurrence count (incident 0 => 1), not the object list + } + + [Fact] + public void Split_PerEventCard_CarriesIncidentDetailFields() + { + var ctx = new AlertContext(); + var incident = new AlertIncident("k", new[] { "db.dbo.Orders" }, OccurrenceCount: 3, + DetailFields: new[] { new AlertIncidentField("Victim SQL", "UPDATE x"), new AlertIncidentField("Processes", "spids 51,52") }); + AlertIncidentRenderer.Apply(ctx, new[] { incident }); + + var msg = Assert.Single(PerEventNotification.Split(ctx, 10)); + var item = Assert.Single(msg.Context.Details); + Assert.Contains(item.Fields, f => f.Label == "Victim SQL" && f.Value == "UPDATE x"); + Assert.Contains(item.Fields, f => f.Label == "Processes" && f.Value == "spids 51,52"); + Assert.Contains(item.Fields, f => f.Label == "Dedup Key" && f.Value == "k"); + } + + [Fact] + public void Split_CarriesSourceAttachment() + { + var ctx = WithIncidents(2); + ctx.AttachmentXml = ""; + ctx.AttachmentFileName = "deadlock_graph.xml"; + foreach (var m in PerEventNotification.Split(ctx, 10)) + { + Assert.Equal("", m.Context.AttachmentXml); + Assert.Equal("deadlock_graph.xml", m.Context.AttachmentFileName); + } + } + + [Fact] + public void Split_CurrentValueIsOccurrenceCount() + { + var ctx = new AlertContext(); + AlertIncidentRenderer.Apply(ctx, new[] { new AlertIncident("k", new[] { "db.dbo.A" }, OccurrenceCount: 7) }); + var msg = Assert.Single(PerEventNotification.Split(ctx, 10)); + Assert.Equal("7", msg.CurrentValue); } [Fact] diff --git a/Lite/MainWindow.xaml.cs b/Lite/MainWindow.xaml.cs index d188c435..6bc9298d 100644 --- a/Lite/MainWindow.xaml.cs +++ b/Lite/MainWindow.xaml.cs @@ -2300,7 +2300,7 @@ to database + literal-stripped query pair only when the object did not resolve. var groups = BlockingIncidentGrouper.Group( serverName, events.Select(e => new BlockingIncidentGrouper.BlockedEvent( - e.DatabaseName, e.ContentiousObject, e.BlockedSqlText, e.BlockingSqlText, e.WaitTimeMs))); + e.DatabaseName, e.ContentiousObject, e.BlockedSqlText, e.BlockingSqlText, e.WaitTimeMs, e.LockMode))); const int maxGroups = 10; var shown = groups.Take(maxGroups).ToList(); @@ -2401,7 +2401,8 @@ recurrences over the same objects collapse to one incident with a count. */ var groups = DeadlockIncidentGrouper.Group( serverName, deadlocks.Select(d => new DeadlockIncidentGrouper.DeadlockEvent( - DeadlockObjectExtractor.FromGraphXml(d.DeadlockGraphXml)))); + DeadlockObjectExtractor.FromGraphXml(d.DeadlockGraphXml), + DeadlockDetailFields(d.VictimSqlText, d.ProcessSummary)))); AlertIncidentRenderer.Apply(context, groups.Select(g => g.Incident).ToList()); return context; @@ -2413,6 +2414,16 @@ recurrences over the same objects collapse to one incident with a count. */ } } + /* #1141: forensic detail carried on a deadlock incident so per-event cards keep the victim SQL + + process summary (Summary mode shows them via the builder's own items). */ + private static List? DeadlockDetailFields(string? victimSql, string? processes) + { + var f = new List(); + if (!string.IsNullOrWhiteSpace(victimSql)) f.Add(new AlertIncidentField("Victim SQL", TruncateText(victimSql))); + if (!string.IsNullOrWhiteSpace(processes)) f.Add(new AlertIncidentField("Processes", processes!)); + return f.Count > 0 ? f : null; + } + private static bool IsDeadlockExcluded(DeadlockRow row, List excludedDatabases) { if (string.IsNullOrEmpty(row.DeadlockGraphXml)) return false; diff --git a/PerformanceMonitor.Notifications/AlertContext.cs b/PerformanceMonitor.Notifications/AlertContext.cs index b6822ad3..4ee9422e 100644 --- a/PerformanceMonitor.Notifications/AlertContext.cs +++ b/PerformanceMonitor.Notifications/AlertContext.cs @@ -52,7 +52,18 @@ public sealed record AlertIncident( string DedupKey, IReadOnlyList InvolvedObjects, int OccurrenceCount = 1, - string? WaitRange = null); + string? WaitRange = null, + IReadOnlyList? DetailFields = null); + +/// +/// A forensic label/value pair carried on an for #1141 Per-event delivery +/// (e.g. Victim SQL / Processes for a deadlock; Database / Blocked Query / Blocking Query / Lock Mode +/// for a blocking chain). Transient: populated by the incident groupers, rendered into the per-event +/// card by , and deliberately NOT persisted (the Summary card already +/// lists the per-incident detail via the builders, and the per-event card's rendered Details are what +/// get saved). Summary rendering ignores it, so that path is unchanged. +/// +public sealed record AlertIncidentField(string Label, string Value); /// /// A single detail item (e.g., one blocking chain or one deadlock participant). diff --git a/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs b/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs index 60fbffcb..f18ace9d 100644 --- a/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs +++ b/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs @@ -37,22 +37,34 @@ public static void Apply(AlertContext context, IReadOnlyList? inc for (int n = 0; n < incidents.Count; n++) { - var incident = incidents[n]; - var item = new AlertDetailItem - { - Heading = incidents.Count == 1 ? "Incident" : $"Incident {n + 1} of {incidents.Count}" - }; - item.Fields.Add(("Dedup Key", incident.DedupKey)); - item.Fields.Add(("Involved Objects", - incident.InvolvedObjects.Count > 0 - ? string.Join(", ", incident.InvolvedObjects) - : "(unresolved)")); - if (incident.OccurrenceCount > 1) - item.Fields.Add(("Occurrences", incident.OccurrenceCount.ToString())); - if (!string.IsNullOrEmpty(incident.WaitRange)) - item.Fields.Add(("Wait Range", incident.WaitRange)); + var heading = incidents.Count == 1 ? "Incident" : $"Incident {n + 1} of {incidents.Count}"; + // Summary mode leaves the forensic DetailFields off — the builder already lists the + // per-incident detail in its own items, so including them here would duplicate. + context.Details.Add(BuildItem(incidents[n], heading, includeDetailFields: false)); + } + } - context.Details.Add(item); + /// + /// Builds one detail item for an incident. When is true the + /// incident's forensic are emitted first — used by #1141 + /// Per-event delivery, where each card carries a single incident and has room for its full detail + /// (Victim SQL / Processes / queries) rather than only the dedup metadata. + /// + public static AlertDetailItem BuildItem(AlertIncident incident, string heading, bool includeDetailFields) + { + var item = new AlertDetailItem { Heading = heading }; + if (includeDetailFields && incident.DetailFields is { Count: > 0 }) + { + foreach (var f in incident.DetailFields) + item.Fields.Add((f.Label, f.Value)); } + item.Fields.Add(("Dedup Key", incident.DedupKey)); + item.Fields.Add(("Involved Objects", + incident.InvolvedObjects.Count > 0 ? string.Join(", ", incident.InvolvedObjects) : "(unresolved)")); + if (incident.OccurrenceCount > 1) + item.Fields.Add(("Occurrences", incident.OccurrenceCount.ToString())); + if (!string.IsNullOrEmpty(incident.WaitRange)) + item.Fields.Add(("Wait Range", incident.WaitRange)); + return item; } } diff --git a/PerformanceMonitor.Notifications/IncidentGrouping.cs b/PerformanceMonitor.Notifications/IncidentGrouping.cs index eabdce56..45ae8c03 100644 --- a/PerformanceMonitor.Notifications/IncidentGrouping.cs +++ b/PerformanceMonitor.Notifications/IncidentGrouping.cs @@ -35,7 +35,8 @@ public readonly record struct BlockedEvent( string? ContentiousObject, string? BlockedQuery, string? BlockingQuery, - long WaitTimeMs); + long WaitTimeMs, + string? LockMode = null); /// One distinct blocking incident: a representative chain, its true occurrence count and /// wait range, and the dedup . @@ -98,10 +99,14 @@ public static List Group(string serverName, IEnumerable b.OccurrenceCount.CompareTo(a.OccurrenceCount)); @@ -138,6 +143,21 @@ private static string FormatWaitRange(long minMs, long maxMs) string Sec(long ms) => (ms / 1000.0).ToString("F1", CultureInfo.InvariantCulture) + "s"; return minMs == maxMs ? Sec(maxMs) : Sec(minMs) + "-" + Sec(maxMs); } + + // Forensic detail for a blocking incident's per-event card (#1141): the representative chain's + // database, contentious object, the blocked/blocking query pair (truncated), and lock mode. + private static List BlockingDetail(BlockedEvent e) + { + var f = new List(); + if (!string.IsNullOrWhiteSpace(e.Database)) f.Add(new AlertIncidentField("Database", e.Database!)); + if (!string.IsNullOrWhiteSpace(e.ContentiousObject)) f.Add(new AlertIncidentField("Contentious Object", e.ContentiousObject!)); + if (!string.IsNullOrWhiteSpace(e.BlockedQuery)) f.Add(new AlertIncidentField("Blocked Query", Truncate(e.BlockedQuery!))); + if (!string.IsNullOrWhiteSpace(e.BlockingQuery)) f.Add(new AlertIncidentField("Blocking Query", Truncate(e.BlockingQuery!))); + if (!string.IsNullOrWhiteSpace(e.LockMode)) f.Add(new AlertIncidentField("Lock Mode", e.LockMode!)); + return f; + } + + private static string Truncate(string s) => s.Length <= 300 ? s : s.Substring(0, 300) + "…"; } /// @@ -149,8 +169,11 @@ private static string FormatWaitRange(long minMs, long maxMs) /// public static class DeadlockIncidentGrouper { - /// One deadlock event projected to the distinct fully-qualified objects it involved. - public readonly record struct DeadlockEvent(IReadOnlyList Objects); + /// One deadlock event projected to the distinct fully-qualified objects it involved, plus + /// optional forensic detail (Victim SQL / Processes) carried onto the incident for per-event cards. + public readonly record struct DeadlockEvent( + IReadOnlyList Objects, + IReadOnlyList? DetailFields = null); /// One distinct deadlock incident: the involved object set, occurrence count, and fingerprint. public sealed record DeadlockGroup(IReadOnlyList Objects, int OccurrenceCount, AlertIncident Incident); @@ -165,6 +188,7 @@ public static List Group(string serverName, IEnumerable(); var counts = new Dictionary(StringComparer.Ordinal); var incidents = new Dictionary(StringComparer.Ordinal); + var details = new Dictionary?>(StringComparer.Ordinal); foreach (var e in events ?? Enumerable.Empty()) { @@ -178,6 +202,7 @@ public static List Group(string serverName, IEnumerable Group(string serverName, IEnumerable Split(AlertContext source, int maxPerCycle) foreach (var incident in incidents.Take(cap)) { - var ctx = new AlertContext { SeverityOverride = source.SeverityOverride }; - AlertIncidentRenderer.Apply(ctx, new[] { incident }); + var ctx = NewContext(source); + ctx.Incidents = new List { incident }; + // includeDetailFields: true — the per-event card has room for this one incident's full + // forensic detail (Victim SQL / Processes / queries), which Summary's batched card splits + // across the builder's own items. + ctx.Details.Add(AlertIncidentRenderer.BuildItem(incident, "Incident", includeDetailFields: true)); messages.Add(new Message(ctx, DescribeIncident(incident), IsOverflow: false)); } var overflow = incidents.Skip(cap).ToList(); if (overflow.Count > 0) { - var ctx = new AlertContext { SeverityOverride = source.SeverityOverride }; - AlertIncidentRenderer.Apply(ctx, overflow); + var ctx = NewContext(source); + ctx.Incidents = new List(overflow); + for (int n = 0; n < overflow.Count; n++) + ctx.Details.Add(AlertIncidentRenderer.BuildItem(overflow[n], $"Incident {n + 1} of {overflow.Count}", includeDetailFields: true)); messages.Add(new Message(ctx, $"+{overflow.Count} more incident(s) this cycle", IsOverflow: true)); } return messages; } - // The alert's "current value" string for a single-incident message: the involved objects, or the - // dedup key when no objects resolved, so the notification headline names what the incident is about. - private static string DescribeIncident(AlertIncident incident) + // A fresh per-incident context that carries over the source's severity override AND the attachment + // (deadlock_graph.xml / blocked_process_report.xml) so per-event email keeps the forensic file. + private static AlertContext NewContext(AlertContext source) => new() { - if (incident.InvolvedObjects.Count > 0) - return string.Join(", ", incident.InvolvedObjects); - return incident.DedupKey; - } + SeverityOverride = source.SeverityOverride, + AttachmentXml = source.AttachmentXml, + AttachmentFileName = source.AttachmentFileName + }; + + // The alert's "current value" for a single-incident card: the occurrence count (a number, matching + // the Summary card's count), not the involved-objects string (which already shows as its own fact). + private static string DescribeIncident(AlertIncident incident) => incident.OccurrenceCount.ToString(); }