diff --git a/Darling/Darling.Tests/CustomAlertLastFiredLiveTests.cs b/Darling/Darling.Tests/CustomAlertLastFiredLiveTests.cs new file mode 100644 index 000000000..996a4c575 --- /dev/null +++ b/Darling/Darling.Tests/CustomAlertLastFiredLiveTests.cs @@ -0,0 +1,167 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Threading; +using System.Threading.Tasks; +using Npgsql; +using NpgsqlTypes; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Darling.Storage; +using Xunit; + +namespace Darling.Tests; + +/// +/// #3360 gated live round-trips (DARLING_TEST_PG) for the "last fired" correlation the rule LIST read +/// () adds. The correlation is pure SQL — a LEFT JOIN of the rule set to +/// a grouped config_alert_log subquery on the immutable Custom:<id> fire key — so it can only be +/// exercised against a real Postgres: a fire row counts, a resolve row for the SAME rule does NOT (it carries a +/// different metric_name), one rule's fires never bleed into another's, and a never-fired rule reports null, +/// all resolved in ONE list read. All test rows are tagged with a per-run prefix / server name and deleted in +/// cleanup, so the shared store is left as it was found. +/// +[Collection("live-postgres")] +public sealed class CustomAlertLastFiredLiveTests +{ + private const string SampleDefinition = + "{\"metric\":{\"source\":\"wait_stats\",\"measure\":\"wait_time_ms\",\"aggregate\":\"sum\",\"hours\":0.25},\"predicate\":{\"op\":\"ge\",\"warnThreshold\":1}}"; + + private static string RequireLivePostgres() + { + var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(connectionString), + "Set DARLING_TEST_PG to a Postgres connection string (owner/superuser) to run the custom-alert last-fired live tests."); + return connectionString!; + } + + private static async Task MigrateAndOpenAsync(string connectionString, CancellationToken ct) + { + await using (var migrate = new NpgsqlConnection(connectionString)) + { + await migrate.OpenAsync(ct); + await PgMigrations.MigrateAsync(migrate, ct); + } + + var dataSourceConnectionString = new NpgsqlConnectionStringBuilder(connectionString) + { + SearchPath = "collect,config,public", + }.ConnectionString; + + return NpgsqlDataSource.Create(dataSourceConnectionString); + } + + /// Inserts ONE config_alert_log row with the given metric_name + naive-UTC alert_time, tagged + /// with so cleanup can delete exactly this run's rows. The other columns are the + /// minimal valid no-channel shape (the correlation reads only metric_name + alert_time). + private static async Task InsertAlertLogAsync( + NpgsqlDataSource dataSource, string serverName, string metricName, DateTime alertTimeUtc, CancellationToken ct) + { + await using var command = dataSource.CreateCommand(@" +INSERT INTO config_alert_log + (alert_time, server_id, server_name, metric_name, current_value, threshold_value, + alert_sent, notification_type, send_error, muted, detail_text, context_json) +VALUES ($1, 1, $2, $3, 0, 0, false, 'none', NULL, false, NULL, NULL)"); + command.Parameters.Add(new NpgsqlParameter + { + NpgsqlDbType = NpgsqlDbType.Timestamp, + Value = DateTime.SpecifyKind(alertTimeUtc, DateTimeKind.Unspecified), + }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = serverName }); + command.Parameters.Add(new NpgsqlParameter { TypedValue = metricName }); + await command.ExecuteNonQueryAsync(ct); + } + + private static async Task CreateRuleAsync( + CustomAlertRuleStore store, string name, CancellationToken ct) + { + var result = Assert.IsType( + await store.CreateAsync(name, description: null, SampleDefinition, enabled: true, "lastfiredtest", ct)); + return result.Rule!; + } + + [Fact] + public async Task ListAsync_ReportsLastFired_PerRule_ExcludingResolveRows_InOneRead() + { + var connectionString = RequireLivePostgres(); + var ct = TestContext.Current.CancellationToken; + await using var dataSource = await MigrateAndOpenAsync(connectionString, ct); + + var store = new CustomAlertRuleStore(dataSource); + var prefix = "carlf_" + Guid.NewGuid().ToString("N") + "_"; + var bodySucceeded = false; + try + { + // Three rules: alpha (fires twice, then a resolve), bravo (fires once), charlie (never fires). + var alpha = await CreateRuleAsync(store, prefix + "alpha", ct); + var bravo = await CreateRuleAsync(store, prefix + "bravo", ct); + var charlie = await CreateRuleAsync(store, prefix + "charlie", ct); + + var t0 = new DateTime(2026, 1, 2, 3, 0, 0); + var alphaFire1 = t0; // alpha first fire + var bravoFire = t0.AddMinutes(5); // bravo's only fire + var alphaFire2 = t0.AddMinutes(10); // alpha's LATER fire -> MAX for alpha + var alphaResolveByName = t0.AddMinutes(20); // resolve row for alpha (different metric_name) -> excluded + var alphaResolveLikeKey = t0.AddMinutes(30); // 'Custom: Resolved' -> matches LIKE but NOT the join key + + // alpha's fires (on its immutable fire key) — the later one is the answer. + await InsertAlertLogAsync(dataSource, prefix, CustomAlertEvaluator.MetricNameFor(alpha.Id), alphaFire1, ct); + await InsertAlertLogAsync(dataSource, prefix, CustomAlertEvaluator.MetricNameFor(alpha.Id), alphaFire2, ct); + + // A NATURAL resolve for alpha: metric_name ' Resolved' (DeliverResolveAsync). It is LATER than the + // last fire, so if it were mistaken for a fire alpha's last_fired would be wrong. It must be excluded — + // it does not even match LIKE 'Custom:%'. + await InsertAlertLogAsync(dataSource, prefix, alpha.Name + " Resolved", alphaResolveByName, ct); + + // An ADVERSARIAL row 'Custom: Resolved': it DOES match the LIKE 'Custom:%' pre-filter, and is the + // latest row of all, but is not EXACTLY 'Custom:', so the join key must reject it. This proves the + // exact join (not the LIKE) is what correlates a fire. + await InsertAlertLogAsync( + dataSource, prefix, CustomAlertEvaluator.MetricNameFor(alpha.Id) + " Resolved", alphaResolveLikeKey, ct); + + // bravo fires once — its own key only. + await InsertAlertLogAsync(dataSource, prefix, CustomAlertEvaluator.MetricNameFor(bravo.Id), bravoFire, ct); + + // charlie: no rows at all. + + // ONE read resolves last_fired for every rule (never a per-rule query). + var summaries = await store.ListAsync(ct); + var alphaSummary = Assert.Single(summaries, s => s.Id == alpha.Id); + var bravoSummary = Assert.Single(summaries, s => s.Id == bravo.Id); + var charlieSummary = Assert.Single(summaries, s => s.Id == charlie.Id); + + // alpha: MAX over its FIRE rows only — the second fire, NOT the (later) resolve rows. + Assert.NotNull(alphaSummary.LastFired); + Assert.Equal(alphaFire2, alphaSummary.LastFired!.Value); + + // bravo: its own single fire — alpha's fires do not bleed in. + Assert.NotNull(bravoSummary.LastFired); + Assert.Equal(bravoFire, bravoSummary.LastFired!.Value); + + // charlie: never fired -> null. + Assert.Null(charlieSummary.LastFired); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(connectionString, bodySucceeded, async (cleanup, cleanupCt) => + { + using (var deleteLog = new NpgsqlCommand("DELETE FROM config.config_alert_log WHERE server_name = $1", cleanup)) + { + deleteLog.Parameters.Add(new NpgsqlParameter { TypedValue = prefix }); + await deleteLog.ExecuteNonQueryAsync(cleanupCt); + } + + using var deleteRules = new NpgsqlCommand("DELETE FROM config.custom_alert_rules WHERE name LIKE $1", cleanup); + deleteRules.Parameters.Add(new NpgsqlParameter { TypedValue = prefix + "%" }); + await deleteRules.ExecuteNonQueryAsync(cleanupCt); + }); + } + } +} diff --git a/Darling/Darling.Tests/DarlingWebEndpointsTests.cs b/Darling/Darling.Tests/DarlingWebEndpointsTests.cs index 9218ca862..9529c3519 100644 --- a/Darling/Darling.Tests/DarlingWebEndpointsTests.cs +++ b/Darling/Darling.Tests/DarlingWebEndpointsTests.cs @@ -259,7 +259,7 @@ public void BuildRuleSummariesNode_IsABareArray_WithEnabled_AndNoDefinitionBody( var summaries = new List { new(Id: 11, Name: "blocking", Description: null, Enabled: true, Version: 2, - UpdatedAt: new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc), UpdatedBy: "mcp"), + UpdatedAt: new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc), UpdatedBy: "mcp", LastFired: null), }; var array = DarlingWebEndpoints.BuildRuleSummariesNode(summaries); @@ -270,4 +270,28 @@ public void BuildRuleSummariesNode_IsABareArray_WithEnabled_AndNoDefinitionBody( Assert.True((bool)only["enabled"]!); Assert.False(only.ContainsKey("definition")); // the list projection never carries the definition body } + + [Fact] + public void BuildRuleSummariesNode_CarriesLastFired_OrNullWhenNeverFired() + { + var firedAt = new DateTime(2026, 3, 4, 5, 6, 7, DateTimeKind.Utc); + var summaries = new List + { + new(Id: 1, Name: "fired", Description: null, Enabled: true, Version: 1, + UpdatedAt: firedAt, UpdatedBy: null, LastFired: firedAt), + new(Id: 2, Name: "never", Description: null, Enabled: true, Version: 1, + UpdatedAt: firedAt, UpdatedBy: null, LastFired: null), + }; + + var array = DarlingWebEndpoints.BuildRuleSummariesNode(summaries); + + var fired = Assert.IsType(array[0]); + Assert.True(fired.ContainsKey("last_fired")); + Assert.Equal(firedAt, fired["last_fired"]!.GetValue()); + + // The key is ALWAYS present (a stable wire shape) — a never-fired rule carries JSON null, not an absent key. + var never = Assert.IsType(array[1]); + Assert.True(never.ContainsKey("last_fired")); + Assert.Null(never["last_fired"]); + } } diff --git a/Darling/PerformanceMonitor.Darling.Service/CustomAlertRuleStore.cs b/Darling/PerformanceMonitor.Darling.Service/CustomAlertRuleStore.cs index e60119b94..5f447abba 100644 --- a/Darling/PerformanceMonitor.Darling.Service/CustomAlertRuleStore.cs +++ b/Darling/PerformanceMonitor.Darling.Service/CustomAlertRuleStore.cs @@ -42,11 +42,50 @@ public CustomAlertRuleStore(NpgsqlDataSource dataSource) _dataSource = dataSource ?? throw new ArgumentNullException(nameof(dataSource)); } - /// The lightweight list read: the (potentially large) definition body is never selected. + /// + /// The lightweight list read: the (potentially large) definition body is never selected. LEFT JOINs + /// each rule to when it LAST FIRED (#3360) — MAX(alert_time) over the config_alert_log rows keyed + /// on the rule's IMMUTABLE fire key 'Custom:' || id (). + /// + /// The correlation is that EXACT fire key only. A resolve / teardown row is written under a DIFFERENT + /// metric_name — '<display name> Resolved' (CustomAlertEvaluator.DeliverResolveAsync / + /// WriteTeardownResolutionAsync) — which can never equal 'Custom:<numeric id>', so a resolve + /// is never counted as a fire and one rule's fires never bleed into another's. The LIKE 'Custom:%' is only + /// a pre-filter that shrinks the grouped set; the = 'Custom:' || id join key is what actually correlates, + /// so even a (contrived) row like 'Custom:5 Resolved' — which passes the LIKE — does NOT join to rule 5. + /// + /// Every fire DELIVERY writes a Custom:<id> row regardless of mute / channel (the deliver path + /// records the incident even when muted or channel-less), so MAX(alert_time) over those rows means "the + /// last time the rule's condition fired (the incident was recorded)" — the intended meaning of "last fired" — + /// and the read is deliberately NOT filtered on alert_sent / notification_type. It is ONE + /// round-trip: the grouped subquery scans the fire rows once and joins to the whole rule set, never a per-rule + /// query. The idx_config_alert_log_time (server_id, metric_name, alert_time) index leads with + /// server_id, so this deliberately cross-server aggregate scans rather than seeks — acceptable on this + /// cold, operator-triggered list render over the retention-bounded history table (the per-(server, metric) + /// cooldown seeds still seek that index). A rule that has never fired LEFT-JOINs to a NULL last_fired. + /// public const string ListSql = @" -SELECT id, name, description, enabled, version, updated_at, updated_by -FROM custom_alert_rules -ORDER BY name"; +SELECT + r.id, + r.name, + r.description, + r.enabled, + r.version, + r.updated_at, + r.updated_by, + f.last_fired +FROM custom_alert_rules AS r +LEFT JOIN +( + SELECT + metric_name, + MAX(alert_time) AS last_fired + FROM config_alert_log + WHERE metric_name LIKE 'Custom:%' + GROUP BY metric_name +) AS f + ON f.metric_name = 'Custom:' || r.id +ORDER BY r.name"; /// The full single-rule read (includes definition). $1 id. public const string GetSql = @" @@ -195,7 +234,9 @@ public async Task> ListAsync(CancellationT reader.GetBoolean(3), reader.GetInt32(4), reader.GetDateTime(5), - reader.IsDBNull(6) ? null : reader.GetString(6))); + reader.IsDBNull(6) ? null : reader.GetString(6), + // last_fired: naive-UTC like updated_at (timestamp column -> Kind=Unspecified), NULL when never fired. + reader.IsDBNull(7) ? (DateTime?)null : reader.GetDateTime(7))); } return results; @@ -613,7 +654,10 @@ public sealed record CustomAlertRule( DateTime UpdatedAt, string? UpdatedBy); -/// The lightweight list projection — every field except the definition body. +/// The lightweight list projection — every field except the definition body. +/// (#3360) is the UTC instant this rule most recently FIRED, or null when it never has — see +/// for how it is correlated on the immutable Custom:<id> +/// fire key (resolve rows excluded). public sealed record CustomAlertRuleSummary( long Id, string Name, @@ -621,7 +665,8 @@ public sealed record CustomAlertRuleSummary( bool Enabled, int Version, DateTime UpdatedAt, - string? UpdatedBy); + string? UpdatedBy, + DateTime? LastFired); /// /// The discriminated outcome of a store operation. Closed (the private base constructor blocks external diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs index 385ef527d..3f608e771 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWebEndpoints.cs @@ -2260,7 +2260,10 @@ enabled state is an enabled rule (the real full-document editor always sends the }; /// The bare-array list wire shape (no definition body), carrying enabled so the list can - /// badge a paused rule without fetching each full definition — mirrors the MCP list surface. + /// badge a paused rule without fetching each full definition — plus last_fired (#3360), the naive-UTC + /// ISO-8601 instant the rule most recently fired (JSON null when it never has) so a card can show + /// "last fired …" / "never fired" without a per-rule read. Mirrors the MCP list surface (they share this + /// builder AND , so the field reaches both surfaces identically). internal static JsonArray BuildRuleSummariesNode(IReadOnlyList rules) { var array = new JsonArray(); @@ -2275,6 +2278,7 @@ internal static JsonArray BuildRuleSummariesNode(IReadOnlyList ListCustomAlertRules(NpgsqlDataSource postgres) diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/css/editor.css b/Darling/PerformanceMonitor.Darling.Service/wwwroot/css/editor.css index 0ed454ad5..b936f0a91 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/css/editor.css +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/css/editor.css @@ -86,6 +86,9 @@ -webkit-box-orient: vertical; } .view-card .vc-meta { font-size: 0.72rem; color: var(--muted); } +/* #3360: "last fired …" — a fired rule reads in --fg to draw the eye to active rules; "never fired" is dimmed. */ +.view-card .vc-fired { font-size: 0.72rem; color: var(--fg); margin-bottom: 0.15rem; } +.view-card .vc-fired.never { color: var(--muted); font-style: italic; } /* enriched card: panel count + distinct viz-type chips (fetched per view — the list summary omits the definition) */ .view-card .vc-chips { display: flex; flex-wrap: wrap; gap: 0.3rem; margin-bottom: 0.45rem; } diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/alert-rules.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/alert-rules.js index c0f455a2e..38b2c4238 100644 --- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/alert-rules.js +++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/alert-rules.js @@ -9,9 +9,9 @@ /* * Custom Alert Rules (#3285, Component 7): the list page — the twin of pages/views.js's renderViewList, for * user-authored alert rules instead of custom views. A rule is stored JSON { metric, predicate, hysteresis, scope } - * behind the /api/alerts CRUD; this page lists the summaries as cards (name, enabled/paused, and — enriched per - * card from the rule's definition — its metric, condition, and scope), and offers New rule + a "start from a - * template" menu of the server-authored starter templates. + * behind the /api/alerts CRUD; this page lists the summaries as cards (name, enabled/paused, when it last fired, + * and — enriched per card from the rule's definition — its metric, condition, and scope), and offers New rule + a + * "start from a template" menu of the server-authored starter templates. * * Unlike a custom view, an alert rule has no read-only "rendered" surface — the card links straight to the editor * (alert-editor.js). Edit affordances (New / New from template) show only when the session reports can_edit (the @@ -123,11 +123,14 @@ function templateMenu(templates) { ]); } -/* One rule -> a card linking to its editor. The summary carries name + enabled synchronously; the metric / - condition / scope chips are filled in progressively from the rule's own definition (enrichCard), so the card - renders immediately and never blocks on that fetch. */ +/* One rule -> a card linking to its editor. The summary carries name + enabled + last_fired synchronously; the + metric / condition / scope chips are filled in progressively from the rule's own definition (enrichCard), so the + card renders immediately and never blocks on that fetch. */ function ruleCard(r) { const meta = "v" + r.version + " · updated " + relTime(r.updated_at) + (r.updated_by ? " by " + r.updated_by : ""); + /* last_fired (#3360): the summary correlates the rule's fires (config_alert_log 'Custom:' rows, resolves + excluded) into one field. Non-null -> a relative time via the shared relTime helper; null -> "never fired". */ + const fired = r.last_fired ? "Last fired " + relTime(r.last_fired) : "Never fired"; const chips = el("div", { class: "vc-chips" }); const card = el("a", { class: "view-card card", href: "#/alert-rule/" + encodeURIComponent(r.id) }, [ el("div", { class: "vc-name" }, [ @@ -136,6 +139,7 @@ function ruleCard(r) { ]), r.description ? el("div", { class: "vc-desc", text: r.description }) : null, chips, + el("div", { class: "vc-fired" + (r.last_fired ? "" : " never"), text: fired }), el("div", { class: "vc-meta", text: meta }), ]); enrichCard(r.id, chips);