Skip to content

Fix the alert notebook's collector-freshness check (#4378) #4416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 2 commits into from
Sep 26, 2026
Merged

Fix the alert notebook's collector-freshness check (#4378) #4416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1755dfc
Fix the alert notebook's collector-freshness check (#4378)
erikdarlingdata Sep 26, 2026
7b3e2cc
Count only successful collector runs as notebook freshness (#4378)
erikdarlingdata Sep 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions Darling/Darling.Tests/AlertNotebookAuthoredTemplateTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,16 @@ private static AlertIncident IncidentWithDatabase(string database) =>

/* ═══════════════════════════ metric routing ═══════════════════════════ */

/// <summary>Data-driven off <see cref="AlertNotebookEndpoint.s_authoredTemplates"/> itself: every
/// registered metric must route to its OWN row's <c>Entry.Id</c>, so a new family's row is covered here
/// without an added <c>InlineData</c> line.</summary>
public static IEnumerable<object[]> AllAuthoredMetricsWithExpectedId() =>
AlertNotebookEndpoint.s_authoredTemplates
.SelectMany(row => row.Metrics.Select(metric => new object[] { metric, row.Entry.Id }));

[Theory]
[InlineData("Blocking Detected", "authored/blocking")]
[InlineData("Blocking Wait Time", "authored/blocking")]
[InlineData("Deadlocks Detected", "authored/deadlocks")]
public void AuthoredTemplate_RoutesTheThreeAlertEngineMetrics_ToTheRightTemplate(string metric, string expectedId)
[MemberData(nameof(AllAuthoredMetricsWithExpectedId))]
public void AuthoredTemplate_RoutesEveryRegisteredMetric_ToItsOwnTemplate(string metric, string expectedId)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);

Expand All @@ -74,14 +79,16 @@ public void AuthoredTemplate_NullOrBlankMetric_StaysMechanical(string? metric)

/* ═══════════════════════════ both templates validate ═══════════════════════════ */

public static IEnumerable<object[]> BothTemplates()
{
yield return new object[] { "Blocking Detected" };
yield return new object[] { "Deadlocks Detected" };
}
/// <summary>Every metric <see cref="AlertNotebookEndpoint.s_authoredTemplates"/> registers, flattened
/// from its rows so the shared theories below cover a new authored family (#4223) the moment its row
/// lands here — nothing in this file has to change to pick it up.</summary>
public static IEnumerable<object[]> AllAuthoredMetrics() =>
AlertNotebookEndpoint.s_authoredTemplates
.SelectMany(row => row.Metrics)
.Select(metric => new object[] { metric });

[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void AuthoredTemplate_BuiltAgainstAMatchedIncident_PassesValidateNotebookDefinition(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand All @@ -103,7 +110,7 @@ public void AuthoredTemplate_BuiltAgainstAMatchedIncident_PassesValidateNotebook
}

[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void AuthoredTemplate_WithNoMatchedIncident_StillDegradesAndValidates(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand Down Expand Up @@ -131,7 +138,7 @@ public void AuthoredTemplate_WithNoMatchedIncident_StillDegradesAndValidates(str
/// so an authored read cell naming a read with no dispatch entry was invisible. This walks BOTH authored
/// templates' actual built cells instead of the section table.</summary>
[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void EveryAuthoredReadCell_NamesAKnownDispatchEntry(string metric)
{
var dispatch = DarlingWebEndpoints.BuildReadDispatch();
Expand Down Expand Up @@ -161,7 +168,7 @@ public void EveryAuthoredReadCell_NamesAKnownDispatchEntry(string metric)
/// this test the same way it would red a real save -- exactly the gap the brief calls out as missing from
/// the harness.</summary>
[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void EveryComposedCell_NamesAMeasureAndBreakdownInTheComposeCatalog(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand Down Expand Up @@ -209,7 +216,7 @@ public void ComposeCatalogCheck_RedsOnARenamedMeasure()
/// trend read, exempted by name); no read names <c>audit_config</c>/an <c>analyze_*</c> compute read; every
/// composed cell's window is <![CDATA[<=]]> 24h.</summary>
[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void Budget_ReadCellsHaveLimitsExceptTheTrendRead_ComposedWindowsAreAtMost24h(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand Down Expand Up @@ -252,7 +259,7 @@ public void Budget_ReadCellsHaveLimitsExceptTheTrendRead_ComposedWindowsAreAtMos
/* ═══════════════════════════ binding ═══════════════════════════ */

[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void Binding_ComposedCellRangeEqualsWindow_ReadCellAsOfEqualsWindowEnd(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand Down Expand Up @@ -283,7 +290,7 @@ public void Binding_ComposedCellRangeEqualsWindow_ReadCellAsOfEqualsWindowEnd(st
/* ═══════════════════════════ explicit viz ═══════════════════════════ */

[Theory]
[MemberData(nameof(BothTemplates))]
[MemberData(nameof(AllAuthoredMetrics))]
public void EveryReadOrPanelCell_HasAnExplicitViz(string metric)
{
var template = AlertNotebookEndpoint.AuthoredTemplate(metric);
Expand Down Expand Up @@ -320,7 +327,7 @@ public void BlockingByDatabaseBreakdown_NeverBindsADatabaseFilter_EvenWithAMatch
Assert.Null(byDatabase["filters"]);
}

/* ═══════════════════════════ the registration table (#4223 lane 0) ═══════════════════════════ */
/* ═══════════════════════════ the registration table (#4223) ═══════════════════════════ */

/// <summary>Every family in <see cref="AlertNotebookEndpoint.s_authoredTemplates"/> resolves to a
/// non-null entry for each of its own metric names, the ids are unique, and every entry's built cells
Expand Down
196 changes: 196 additions & 0 deletions Darling/Darling.Tests/AlertNotebookCollectorFreshnessLiveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
/*
* 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 Microsoft.Extensions.Logging.Abstractions;
using Npgsql;
using PerformanceMonitor.Collectors;
using PerformanceMonitor.Darling.Service;
using PerformanceMonitor.Darling.Storage;
using Xunit;

namespace Darling.Tests;

/// <summary>
/// #4378: the collector-freshness arm (<see cref="AlertNotebookEndpoint.ResolveStatusAsync"/>'s arms 3/4)
/// must count only <c>SUCCESS</c> collection_log rows as evidence the instrument is up. An <c>ERROR</c> row
/// after the anchor is proof the collector tried and failed, not proof it is fresh — the bug this fixes was
/// counting ANY status, so a run that failed every time still read "No resolution recorded" instead of the
/// honest "Unknown (not collected since T)". A <c>SKIPPED</c> row does not count either: it can be written
/// without the run ever contacting the target (see <c>Lite/Services/RemoteCollectorService.cs</c>'s
/// user-cancelled-MFA catch), and Darling's own per-run classifier never writes it.
/// </summary>
[Collection("live-postgres")]
public sealed class AlertNotebookCollectorFreshnessLiveTests
{
private const int ServerId = -437800;
private const string ServerName = "alert-notebook-freshness-4378";
private const string Collector = "wait_stats";

private static string? ConnectionString => Environment.GetEnvironmentVariable("DARLING_TEST_PG");

[Fact]
public async Task OnlyErrorRunsAfterAnchor_ReadsUnknown_NotCollectedSince()
{
var cs = ConnectionString;
Assert.SkipWhen(string.IsNullOrEmpty(cs),
"Set DARLING_TEST_PG to a Postgres connection string to run the live collector-freshness test.");

var ct = TestContext.Current.CancellationToken;
using var connection = new NpgsqlConnection(cs);
await connection.OpenAsync(ct);
await PgMigrations.MigrateAsync(connection, ct);
await DeleteRowsAsync(connection, ct);

var bodySucceeded = false;
try
{
var anchor = DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow.AddMinutes(-30));

/* An ERROR row after the anchor. Old code counted ANY status here, so it read this as fresh. */
await SeedAsync(connection, ct, anchor.AddMinutes(5), "ERROR");

await using var postgres = NpgsqlDataSource.Create(cs!);
var status = await AlertNotebookEndpoint.ResolveStatusAsync(
postgres, ServerId, fleetLevelStore: false, "High CPU", anchor, DateTime.UtcNow,
new System.Collections.Generic.List<PerformanceMonitor.Darling.Service.Mcp.DarlingAlertReader.AlertHistoryReadRow>(),
matchedRow: null, NullLogger.Instance, ct);

Assert.StartsWith("Unknown (not collected since ", status, StringComparison.Ordinal);

bodySucceeded = true;
}
finally
{
await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, DeleteRowsAsync);
}
}

[Fact]
public async Task OnlySkippedRunsAfterAnchor_ReadsUnknown_NotCollectedSince()
{
var cs = ConnectionString;
Assert.SkipWhen(string.IsNullOrEmpty(cs),
"Set DARLING_TEST_PG to a Postgres connection string to run the live collector-freshness test.");

var ct = TestContext.Current.CancellationToken;
using var connection = new NpgsqlConnection(cs);
await connection.OpenAsync(ct);
await PgMigrations.MigrateAsync(connection, ct);
await DeleteRowsAsync(connection, ct);

var bodySucceeded = false;
try
{
var anchor = DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow.AddMinutes(-30));

/* A SKIPPED row after the anchor. SKIPPED can be written without the run ever contacting the
target (a user-cancelled MFA sign-in), so it must not read as fresh. */
await SeedAsync(connection, ct, anchor.AddMinutes(5), "SKIPPED");

await using var postgres = NpgsqlDataSource.Create(cs!);
var status = await AlertNotebookEndpoint.ResolveStatusAsync(
postgres, ServerId, fleetLevelStore: false, "High CPU", anchor, DateTime.UtcNow,
new System.Collections.Generic.List<PerformanceMonitor.Darling.Service.Mcp.DarlingAlertReader.AlertHistoryReadRow>(),
matchedRow: null, NullLogger.Instance, ct);

Assert.StartsWith("Unknown (not collected since ", status, StringComparison.Ordinal);

bodySucceeded = true;
}
finally
{
await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, DeleteRowsAsync);
}
}

[Fact]
public async Task ASuccessRunAfterAnchor_ReadsNoResolutionRecorded()
{
var cs = ConnectionString;
Assert.SkipWhen(string.IsNullOrEmpty(cs),
"Set DARLING_TEST_PG to a Postgres connection string to run the live collector-freshness test.");

var ct = TestContext.Current.CancellationToken;
using var connection = new NpgsqlConnection(cs);
await connection.OpenAsync(ct);
await PgMigrations.MigrateAsync(connection, ct);
await DeleteRowsAsync(connection, ct);

var bodySucceeded = false;
try
{
var anchor = DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow.AddMinutes(-30));

await SeedAsync(connection, ct, anchor.AddMinutes(5), "SUCCESS");

await using var postgres = NpgsqlDataSource.Create(cs!);
var status = await AlertNotebookEndpoint.ResolveStatusAsync(
postgres, ServerId, fleetLevelStore: false, "High CPU", anchor, DateTime.UtcNow,
new System.Collections.Generic.List<PerformanceMonitor.Darling.Service.Mcp.DarlingAlertReader.AlertHistoryReadRow>(),
matchedRow: null, NullLogger.Instance, ct);

Assert.Equal("No resolution recorded", status);

bodySucceeded = true;
}
finally
{
await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, DeleteRowsAsync);
}
}

[Fact]
public async Task OnlyRowsBeforeAnchor_ReadsUnknown_NotCollectedSince()
{
var cs = ConnectionString;
Assert.SkipWhen(string.IsNullOrEmpty(cs),
"Set DARLING_TEST_PG to a Postgres connection string to run the live collector-freshness test.");

var ct = TestContext.Current.CancellationToken;
using var connection = new NpgsqlConnection(cs);
await connection.OpenAsync(ct);
await PgMigrations.MigrateAsync(connection, ct);
await DeleteRowsAsync(connection, ct);

var bodySucceeded = false;
try
{
var anchor = DarlingMcpTestData.TruncateToSeconds(DateTime.UtcNow.AddMinutes(-30));

await SeedAsync(connection, ct, anchor.AddMinutes(-5), "SUCCESS");

await using var postgres = NpgsqlDataSource.Create(cs!);
var status = await AlertNotebookEndpoint.ResolveStatusAsync(
postgres, ServerId, fleetLevelStore: false, "High CPU", anchor, DateTime.UtcNow,
new System.Collections.Generic.List<PerformanceMonitor.Darling.Service.Mcp.DarlingAlertReader.AlertHistoryReadRow>(),
matchedRow: null, NullLogger.Instance, ct);

Assert.StartsWith("Unknown (not collected since ", status, StringComparison.Ordinal);

bodySucceeded = true;
}
finally
{
await LiveStoreCleanup.RunAsync(cs!, bodySucceeded, DeleteRowsAsync);
}
}

/* ── helpers ── */

private static Task SeedAsync(NpgsqlConnection connection, CancellationToken ct, DateTime collectionTimeUtc, string status) =>
DarlingMcpTestData.ExecAsync(connection, ct, @"
INSERT INTO collection_log (log_id, server_id, server_name, collector_name, collection_time, status)
VALUES ($1, $2, $3, $4, $5, $6)",
CollectionIdGenerator.Next(), ServerId, ServerName, Collector, DarlingMcpTestData.Naive(collectionTimeUtc), status);

private static Task DeleteRowsAsync(NpgsqlConnection connection, CancellationToken ct) =>
DarlingMcpTestData.ExecAsync(connection, ct, "DELETE FROM collection_log WHERE server_id = $1", ServerId);
}
59 changes: 59 additions & 0 deletions Darling/Darling.Tests/AlertNotebookEndpointTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,65 @@ public void StatusFromHistory_FourArms_AreDistinctStrings_AndNoneSayOngoing()
}
}

[Fact]
public void StatusFromHistory_AgReconnected_AfterDisconnected_ReadsResolved()
{
var anchor = new DateTime(2026, 1, 1, 10, 0, 0, DateTimeKind.Utc);
var matched = Row(anchor, "AG Replica Disconnected", serverId: 7);

var status = AlertNotebookEndpoint.StatusFromHistory(
new List<DarlingAlertReader.AlertHistoryReadRow> { matched, Row(anchor + TimeSpan.FromMinutes(1), "AG Replica Reconnected", serverId: 7) },
"AG Replica Disconnected", anchor, matched, 7, false);

Assert.StartsWith("Resolved at ", status, StringComparison.Ordinal);
}

[Fact]
public void StatusFromHistory_ServerRestored_AfterUnreachable_ReadsResolved()
{
var anchor = new DateTime(2026, 1, 1, 10, 0, 0, DateTimeKind.Utc);
var matched = Row(anchor, "Server Unreachable", serverId: 7);

var status = AlertNotebookEndpoint.StatusFromHistory(
new List<DarlingAlertReader.AlertHistoryReadRow> { matched, Row(anchor + TimeSpan.FromMinutes(1), "Server Restored", serverId: 7) },
"Server Unreachable", anchor, matched, 7, false);

Assert.StartsWith("Resolved at ", status, StringComparison.Ordinal);
}

[Fact]
public void StatusFromHistory_AgFailover_HasNoRecoveryEdge()
{
// AG Failover is deliberately absent from NotebookRecoveryEdges (an event, not a clearing condition).
Assert.DoesNotContain(
AlertNotebookEndpoint.NotebookRecoveryEdges,
pair => string.Equals(pair.Firing, "AG Failover", StringComparison.OrdinalIgnoreCase));
}

/* ═══════════════════════════ pure: the collector-freshness read's own shape ═══════════════════════════ */

[Fact]
public void CollectorFreshnessSql_CountsOnlySuccess_NeverSkippedOrError()
{
var sql = AlertNotebookEndpoint.CollectorFreshnessSql;

Assert.Contains("status = 'SUCCESS'", sql, StringComparison.Ordinal);
Assert.DoesNotContain("'SKIPPED'", sql, StringComparison.Ordinal);
Assert.DoesNotContain("'ERROR'", sql, StringComparison.Ordinal);
}

[Fact]
public void ResolveStatusAsync_Source_SetsCommandTimeout_AndNeverReportsZeroElapsed()
{
var source = RepoFile.ReadRepoFile("Darling", "PerformanceMonitor.Darling.Service", "AlertNotebookEndpoint.cs");

Assert.Contains("CommandTimeout = McpCommandDeadlines.ReadSeconds", source, StringComparison.Ordinal);
Assert.DoesNotContain(
"DarlingWebFailureLog.Report(logger, \"/api/alert-notebook:collector-freshness\", 0, ex)", source, StringComparison.Ordinal);
Assert.DoesNotContain(
"DarlingWebFailureLog.Report(logger, \"/api/alert-notebook:status-history\", 0, ex)", source, StringComparison.Ordinal);
}

/* ═══════════════════════════ pure: window math and the future-`at` clamp ═══════════════════════════ */

/// <summary>A far-future <c>at</c> clamps to <c>now</c> rather than reading as a request to widen the
Expand Down
Loading
Loading