diff --git a/docs/data-versioning-design.md b/docs/data-versioning-design.md index 360f3be85c..dadb89dbcb 100644 --- a/docs/data-versioning-design.md +++ b/docs/data-versioning-design.md @@ -12,6 +12,8 @@ This is the primary (error) instance only. The audit instance still carries a `s **If a field the response renders can change without the version changing, a client caches that page for ever and nothing reveals it.** No log line, no exception, no failing test. +The rule covers fields that can change on their own. A field that is a **pure function of a covered field** cannot: it moves only when its source does, and the source already moves the version, so the page is never stale on the field's own account. `CustomCheckView.Internal` is that case — it is a computed, get-only property classified out of `CustomCheckId` at read time (see `InternalCustomCheckClassification`), which is itself a version term, and the view rather than the stored `CustomCheck` is what `/api/customchecks` renders. Being get-only, it cannot be assigned at all, so the reflection test above never sees it as a field that could drift. The one residual window is a ServiceControl upgrade that reclassifies while a client holds a pre-upgrade tag, and it closes itself: internal checks re-report every 5s to 1h, which moves `ReportedAt` and therefore the version. + The promise is scoped to **one URL**, because a client only ever sends a validator back to the URL that issued it. So what must never happen is one URL answering `304` when its own body would have differed. Two different URLs sharing a value is harmless: an HTTP cache is keyed on the whole URL. That scoping is what makes a backend's own token usable. RavenDB's result etag stands for the state of the index behind the query, so it moves on any write the query could see, but it says nothing about which page was asked for: every `/api/errors` URL shares one value, whatever the page, sort or filter. The EF Core persisters compose over the rows they returned, so theirs differ per page. Both satisfy the rule. diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/Monitoring/CustomChecks/When_a_persister_check_fails.cs b/src/ServiceControl.AcceptanceTests.RavenDB/Monitoring/CustomChecks/When_a_persister_check_fails.cs new file mode 100644 index 0000000000..a635fda807 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests.RavenDB/Monitoring/CustomChecks/When_a_persister_check_fails.cs @@ -0,0 +1,67 @@ +namespace ServiceControl.AcceptanceTests.RavenDB.Monitoring.CustomChecks +{ + using System; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using Operations; + using ServiceBus.Management.Infrastructure.Settings; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + using CheckStatus = global::ServiceControl.Persistence.Status; + + // Sibling of When_critical_storage_threshold_reached: proves a + // persister-implemented internal check that is forced to fail comes back classified through the API. + // "ServiceControl database" cannot be forced to fail in this environment (the shared embedded server means + // UseEmbeddedServer is false, so CheckFreeDiskSpace always passes) — see plan §8.6. + [TestFixture] + class When_a_persister_check_fails : AcceptanceTest + { + [SetUp] + public void SetupIngestion() => + SetSettings = static s => + { + s.DisableHealthChecks = false; + }; + + RavenPersisterSettings PersisterSettings => (RavenPersisterSettings)Settings.PersisterSpecificSettings; + + [Test] + public async Task Forced_failure_is_classified_internal() + { + CustomCheckView ingestionCheck = null; + + await Define() + .WithEndpoint(b => b + .When(context => context.Logs.ToArray().Any(i => i.Message.StartsWith(ErrorIngestion.LogMessages.StartedInfrastructure)), + (_, _) => + { + PersisterSettings.MinimumStorageLeftRequiredForIngestion = 100; + PersisterSettings.DatabasePath = TestContext.CurrentContext.TestDirectory; + return Task.CompletedTask; + })) + .Done(async c => + { + var result = await this.TryGetSingle("/api/customchecks", x => x.CustomCheckId == "Message Ingestion Process" && x.Status == CheckStatus.Fail); + ingestionCheck = result; + return result; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(ingestionCheck, Is.Not.Null, "the forced storage-threshold failure never showed up"); + Assert.That(ingestionCheck.Internal, Is.True); + } + } + + public class Sender : EndpointConfigurationBuilder + { + public Sender() => + EndpointSetup(c => c.ReportCustomChecksTo(Settings.DEFAULT_INSTANCE_NAME, TimeSpan.FromSeconds(1))); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj b/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj index 7cc11bc96c..33be55ce4f 100644 --- a/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj +++ b/src/ServiceControl.AcceptanceTests.RavenDB/ServiceControl.AcceptanceTests.RavenDB.csproj @@ -36,6 +36,9 @@ + + + diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs index 11ebf3f150..29c462b580 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_a_failing_custom_check_is_dismissed.cs @@ -6,13 +6,14 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.EndpointTemplates; + using Contracts.CustomChecks; using NServiceBus; using NServiceBus.AcceptanceTesting; using NServiceBus.CustomChecks; using NUnit.Framework; using ServiceBus.Management.Infrastructure.Settings; - using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheck; using CheckStatus = global::ServiceControl.Persistence.Status; + using CustomCheck = NServiceBus.CustomChecks.CustomCheck; class When_a_failing_custom_check_is_dismissed : AcceptanceTest { diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs new file mode 100644 index 0000000000..8b1995386e --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs @@ -0,0 +1,124 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System; + using System.Linq; + using System.Threading; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.EndpointTemplates; + using NServiceBus; + using NServiceBus.AcceptanceTesting; + using NServiceBus.CustomChecks; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + using CheckStatus = global::ServiceControl.Persistence.Status; + + [TestFixture] + class When_custom_checks_are_classified : AcceptanceTest + { + // Runs at startup with TimeSpan.Zero, so acceptance tests can assert on it without waiting an interval. + const string InternalId = "ServiceControl Primary Instance"; + + [Test] + public async Task Internal_checks_are_flagged_internal_and_endpoint_checks_are_not() + { + // The acceptance test runner disables internal custom checks by default; this test needs them. + SetSettings = settings => { settings.DisableHealthChecks = false; }; + + CustomCheckView internalCheck = null; + CustomCheckView endpointCheck = null; + string wireBody = null; + + await Define() + .WithEndpoint() + .Done(async c => + { + var checks = await this.TryGetMany("/api/customchecks"); + + internalCheck ??= checks.Items.SingleOrDefault(x => x.CustomCheckId == InternalId); + endpointCheck ??= checks.Items.SingleOrDefault(x => x.CustomCheckId == "MyCustomCheckId" && x.Status == CheckStatus.Fail); + + // The view computes Internal from the check id, so deserializing alone would not + // prove the endpoint emits it. Grab the raw payload once and assert on the wire itself. + if (internalCheck != null && endpointCheck != null && wireBody == null) + { + var raw = await this.GetRaw("/api/customchecks"); + wireBody = await raw.Content.ReadAsStringAsync(); + } + + return internalCheck != null && endpointCheck != null && wireBody != null; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(internalCheck, Is.Not.Null, "primary internal checks report at startup; nothing was found"); + Assert.That(internalCheck.Internal, Is.True); + + Assert.That(endpointCheck, Is.Not.Null); + Assert.That(endpointCheck.Internal, Is.False); + + // What the wire actually carries: + Assert.That(wireBody, Does.Contain("\"internal\":true"), "internal checks must render internal:true on the wire"); + Assert.That(wireBody, Does.Contain("\"internal\":false"), "endpoint checks must render internal:false on the wire"); + } + } + + [Test] + public async Task Every_expected_internal_check_is_flagged_internal() + { + // The acceptance test runner disables internal custom checks by default; this test needs them. + SetSettings = settings => { settings.DisableHealthChecks = false; }; + + var expectedIds = new[] + { + "ServiceControl Primary Instance", + "ServiceControl Remotes", + "Saga Audit Configuration", + // RavenDB persister checks also assert here on the RavenDB acceptance variant: + "Error Message Ingestion Process", + "Error Message Ingestion", + }; + + var seen = new System.Collections.Generic.List(); + + await Define() + .Done(async c => + { + var checks = await this.TryGetMany("/api/customchecks"); + foreach (var item in checks.Items) + { + // The Done predicate polls, so keep one row per check id + if (seen.All(s => s.Id != item.Id)) + { + seen.Add(item); + } + } + + return expectedIds.All(e => seen.Any(s => s.CustomCheckId == e)); + }) + .Run(); + + foreach (var id in expectedIds) + { + var check = seen.Single(s => s.CustomCheckId == id); + Assert.That(check.Internal, Is.True, id); + } + } + + class Context : ScenarioContext; + + public class EndpointWithFailingCustomCheck : EndpointConfigurationBuilder + { + public EndpointWithFailingCustomCheck() => + EndpointSetup(c => c.ReportCustomChecksTo(Settings.DEFAULT_INSTANCE_NAME, TimeSpan.FromSeconds(1))); + + class FailingCustomCheck() : CustomCheck("MyCustomCheckId", "MyCategory", TimeSpan.FromSeconds(1)) + { + public override Task PerformCheck(CancellationToken cancellationToken = default) => + Task.FromResult(CheckResult.Failed("Some reason")); + } + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs index 15c915b4e1..542de55ade 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs @@ -10,14 +10,15 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.EndpointTemplates; + using Contracts.CustomChecks; using NServiceBus; using NServiceBus.AcceptanceTesting; using NServiceBus.CustomChecks; using NUnit.Framework; using ServiceBus.Management.Infrastructure.Settings; using ServiceControl.Notifications; - using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheck; using CheckStatus = global::ServiceControl.Persistence.Status; + using CustomCheck = NServiceBus.CustomChecks.CustomCheck; class When_email_notifications_are_configured : AcceptanceTest { diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_the_body_storage_check_is_reported.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_the_body_storage_check_is_reported.cs new file mode 100644 index 0000000000..a1fbfa6c4b --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_the_body_storage_check_is_reported.cs @@ -0,0 +1,37 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + + [TestFixture] + class When_the_body_storage_check_is_reported : AcceptanceTest + { + [SetUp] + public void EnableInternalChecks() => + SetSettings = static s => s.DisableHealthChecks = false; + + [Test] + public async Task Should_be_classified_internal() + { + CustomCheckView bodyStorageCheck = null; + + await Define() + .Done(async c => + { + var result = await this.TryGetSingle("/api/customchecks", x => x.CustomCheckId == "ServiceControl body storage"); + bodyStorageCheck = result; + return result; + }) + .Run(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(bodyStorageCheck, Is.Not.Null, "the EF Core body storage check never reported"); + Assert.That(bodyStorageCheck.Internal, Is.True); + } + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj index 2c798be958..31ca62448b 100644 --- a/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj +++ b/src/ServiceControl.Audit.Persistence.Tests.RavenDB/ServiceControl.Audit.Persistence.Tests.RavenDB.csproj @@ -23,6 +23,9 @@ + + diff --git a/src/ServiceControl.Audit.Persistence.Tests/CustomCheckTests.cs b/src/ServiceControl.Audit.Persistence.Tests/CustomCheckTests.cs index c50e624880..1666155669 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/CustomCheckTests.cs +++ b/src/ServiceControl.Audit.Persistence.Tests/CustomCheckTests.cs @@ -2,6 +2,7 @@ { using System; using System.Linq; + using Contracts.CustomChecks; using Microsoft.Extensions.DependencyInjection; using NServiceBus.CustomChecks; using NUnit.Framework; @@ -17,8 +18,8 @@ public void VerifyCustomChecks() => string.Join(Environment.NewLine, from check in ServiceProvider.GetServices() orderby check.Category, check.Id - select $"{check.Category}: {check.Id}" + select $"{check.Category}: {check.Id}{(InternalCustomCheckClassification.IsInternal(check.Id) ? "" : " - MISSING FROM InternalCustomCheckClassification")}" ) ); } -} +} \ No newline at end of file diff --git a/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj b/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj index ac37ef4bce..219f89cb81 100644 --- a/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj +++ b/src/ServiceControl.Audit.Persistence.Tests/ServiceControl.Audit.Persistence.Tests.csproj @@ -24,6 +24,9 @@ + + diff --git a/src/ServiceControl.Audit.UnitTests/ApprovalFiles/AuditCustomCheckApprovals.Audit_check_ids_are_snapshot.approved.txt b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/AuditCustomCheckApprovals.Audit_check_ids_are_snapshot.approved.txt new file mode 100644 index 0000000000..11d5e1e4e0 --- /dev/null +++ b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/AuditCustomCheckApprovals.Audit_check_ids_are_snapshot.approved.txt @@ -0,0 +1,2 @@ +ServiceControl Health: Audit Message Ingestion Process +ServiceControl.Audit Health: Audit Message Ingestion \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt new file mode 100644 index 0000000000..dfa8cd1979 --- /dev/null +++ b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt @@ -0,0 +1,2 @@ +ServiceControl Health: Audit Message Ingestion Process => internal +ServiceControl.Audit Health: Audit Message Ingestion => internal \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj b/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj index c4ee2eb333..c6334d7c62 100644 --- a/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj +++ b/src/ServiceControl.Audit.UnitTests/ServiceControl.Audit.UnitTests.csproj @@ -22,6 +22,9 @@ + + \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs b/src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs new file mode 100644 index 0000000000..18e824d544 --- /dev/null +++ b/src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs @@ -0,0 +1,40 @@ +namespace ServiceControl.Audit.UnitTests.API +{ + using System; + using System.Linq; + using Audit.Infrastructure.Settings; + using Contracts.CustomChecks; + using NUnit.Framework; + using NServiceBus.CustomChecks; + using Particular.Approvals; + + [TestFixture] + class CustomChecksTest + { + // Mirrors the primary's InternalCustomCheckClassification audit section (string literals — the audit + // assembly is not referenced by the primary). Adding a custom check to the audit instance MUST be + // accompanied by an entry in the primary registry; this snapshot makes that visible. The audit + // RavenDB persister checks (CheckDirtyMemory, CheckFreeDiskSpace, CheckRavenDBIndexLag) are runtime + // plugins not referenced here, so they are covered by the persistence approval tests instead + [Test] + public void VerifyCustomChecks() + { + var settings = (object)new Settings("LearningTransport", "InMemory"); + + var discovered = + from type in typeof(Settings).Assembly.GetTypes() + where type is { IsAbstract: false, IsInterface: false } + && typeof(ICustomCheck).IsAssignableFrom(type) + let constructor = type.GetConstructors().Single() + let constructorParameters = constructor.GetParameters() + .Select(p => p.ParameterType == typeof(Settings) ? settings : null) + .ToArray() + let instance = (ICustomCheck)constructor.Invoke(constructorParameters) + let classified = InternalCustomCheckClassification.IsInternal(instance.Id) + orderby instance.Category, instance.Id + select $"{instance.Category}: {instance.Id} => {(classified ? "internal" : "MISSING FROM REGISTRY")}"; + + Approver.Verify(string.Join(Environment.NewLine, discovered)); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs b/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs index 8469fe9324..1916fbf5f3 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/CustomCheckDataStore.cs @@ -50,7 +50,7 @@ await context.UpsertAsync([detail.GetDeterministicId()], return status; }, cancellationToken); - public Task>> GetStats(PagingInfo paging, string? status = null, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (context, token) => + public Task>> GetStats(PagingInfo paging, string? status = null, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (context, token) => { var query = context.CustomChecks.AsQueryable().AsNoTracking(); @@ -66,7 +66,7 @@ public Task>> GetStats(PagingInfo paging, string? .ThenBy(c => c.Id) .Skip(paging.Offset) .Take(paging.PageSize) - .Select(c => new CustomCheck + .Select(c => new CustomCheckView { Id = c.Id.ToString(), CustomCheckId = c.CustomCheckId, @@ -85,7 +85,7 @@ public Task>> GetStats(PagingInfo paging, string? var totalCount = await query.CountAsync(token); - return new QueryResult>(checks, checks.ToQueryStatsInfo("checks", totalCount)); + return new QueryResult>(checks, checks.ToQueryStatsInfo("checks", totalCount)); }, cancellationToken); public Task DeleteCustomCheck(Guid id, CancellationToken cancellationToken = default) => ExecuteWithDbContext(async (context, token) => await context.CustomChecks.AsNoTracking().Where(cc => cc.Id == id).ExecuteDeleteAsync(token), cancellationToken); diff --git a/src/ServiceControl.Persistence/CustomCheck.cs b/src/ServiceControl.Persistence.RavenDB/CustomChecks/CustomCheck.cs similarity index 53% rename from src/ServiceControl.Persistence/CustomCheck.cs rename to src/ServiceControl.Persistence.RavenDB/CustomChecks/CustomCheck.cs index c76554ea81..4441869def 100644 --- a/src/ServiceControl.Persistence/CustomCheck.cs +++ b/src/ServiceControl.Persistence.RavenDB/CustomChecks/CustomCheck.cs @@ -1,11 +1,11 @@ -namespace ServiceControl.Contracts.CustomChecks +#nullable enable +namespace ServiceControl.Contracts.CustomChecks { using System; using ServiceControl.Operations; using ServiceControl.Persistence; - using ServiceControl.Persistence.Infrastructure; - public class CustomCheck : IVersionedRow + public class CustomCheck { public string? Id { get; set; } public string? CustomCheckId { get; set; } @@ -14,10 +14,5 @@ public class CustomCheck : IVersionedRow public DateTime ReportedAt { get; set; } public string? FailureReason { get; set; } public EndpointDetails? OriginatingEndpoint { get; set; } - object?[] IVersionedRow.GetVersionFields() => - [ - Id, CustomCheckId, Category, Status, ReportedAt, FailureReason, - OriginatingEndpoint?.Name, OriginatingEndpoint?.HostId, OriginatingEndpoint?.Host - ]; } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs b/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs index 5ae267faf8..e9cfd8f89d 100644 --- a/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/RavenCustomCheckDataStore.cs @@ -2,6 +2,7 @@ { using System; using System.Collections.Generic; + using System.Linq; using System.Threading; using System.Threading.Tasks; using Raven.Client.Documents; @@ -45,7 +46,7 @@ public async Task UpdateCustomCheckStatus(CustomCheckDetail de static string MakeId(Guid id) => $"CustomChecks/{id}"; - public async Task>> GetStats(PagingInfo paging, string status = null, CancellationToken cancellationToken = default) + public async Task>> GetStats(PagingInfo paging, string status = null, CancellationToken cancellationToken = default) { using var session = await sessionProvider.OpenSession(cancellationToken: cancellationToken); var query = @@ -57,9 +58,25 @@ public async Task>> GetStats(PagingInfo paging, s .Paging(paging) .ToListAsync(cancellationToken); - return new QueryResult>(results, stats.ToQueryStatsInfo()); + // Project to the read model right away: the API gets a copy, never the tracked document. + var views = results + .Select(ToCustomCheckView) + .ToList(); + + return new QueryResult>(views, stats.ToQueryStatsInfo()); } + static CustomCheckView ToCustomCheckView(CustomCheck customCheck) => new() + { + Id = customCheck.Id, + CustomCheckId = customCheck.CustomCheckId, + Category = customCheck.Category, + Status = customCheck.Status, + ReportedAt = customCheck.ReportedAt, + FailureReason = customCheck.FailureReason, + OriginatingEndpoint = customCheck.OriginatingEndpoint + }; + public async Task DeleteCustomCheck(Guid id, CancellationToken cancellationToken = default) { var documentId = MakeId(id); diff --git a/src/ServiceControl.Persistence.Tests.RavenDB/ApprovalFiles/RavenPersistedTypes.Verify.approved.txt b/src/ServiceControl.Persistence.Tests.RavenDB/ApprovalFiles/RavenPersistedTypes.Verify.approved.txt index 4c418dae95..88bca6dca0 100644 --- a/src/ServiceControl.Persistence.Tests.RavenDB/ApprovalFiles/RavenPersistedTypes.Verify.approved.txt +++ b/src/ServiceControl.Persistence.Tests.RavenDB/ApprovalFiles/RavenPersistedTypes.Verify.approved.txt @@ -1,4 +1,4 @@ -ServiceControl.Contracts.CustomChecks.CustomCheck, ServiceControl.Persistence, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null +ServiceControl.Contracts.CustomChecks.CustomCheck, ServiceControl.Persistence.RavenDB, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null ServiceControl.MessageFailures.FailedMessage, ServiceControl.Persistence, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null ServiceControl.MessageFailures.GroupComment, ServiceControl.Persistence, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null ServiceControl.MessageFailures.QueueAddress, ServiceControl.Persistence, Version=6.0.0.0, Culture=neutral, PublicKeyToken=null diff --git a/src/ServiceControl.Persistence.Tests/CustomCheckTests.cs b/src/ServiceControl.Persistence.Tests/CustomCheckTests.cs index 34fd2ece69..997afc94af 100644 --- a/src/ServiceControl.Persistence.Tests/CustomCheckTests.cs +++ b/src/ServiceControl.Persistence.Tests/CustomCheckTests.cs @@ -2,6 +2,7 @@ namespace ServiceControl.Persistence.Tests { using System; using System.Linq; + using Contracts.CustomChecks; using Microsoft.Extensions.DependencyInjection; using NServiceBus.CustomChecks; using NUnit.Framework; @@ -17,7 +18,7 @@ public void VerifyCustomChecks() => string.Join(Environment.NewLine, from check in ServiceProvider.GetServices() orderby check.Category, check.Id - select $"{check.Category}: {check.Id}" + select $"{check.Category}: {check.Id}{(InternalCustomCheckClassification.IsInternal(check.Id) ? "" : " - MISSING FROM InternalCustomCheckClassification")}" ) ); } diff --git a/src/ServiceControl.Persistence/CustomCheckView.cs b/src/ServiceControl.Persistence/CustomCheckView.cs new file mode 100644 index 0000000000..9a5310563c --- /dev/null +++ b/src/ServiceControl.Persistence/CustomCheckView.cs @@ -0,0 +1,36 @@ +namespace ServiceControl.Contracts.CustomChecks +{ + using System; + using ServiceControl.Operations; + using ServiceControl.Persistence; + using ServiceControl.Persistence.Infrastructure; + + /// + /// One custom check as read back and returned by the API. Unlike the stored , + /// it also tells ServicePulse whether the check is one ServiceControl ships itself (primary, audit or + /// transport check) or one reported by a monitored endpoint. The flag is classified from the check id + /// at read time and is never persisted. + /// + public class CustomCheckView : IVersionedRow + { + public string? Id { get; set; } + public string? CustomCheckId { get; set; } + public string? Category { get; set; } + public Status Status { get; set; } + public DateTime ReportedAt { get; set; } + public string? FailureReason { get; set; } + public EndpointDetails? OriginatingEndpoint { get; set; } + + /// + /// True when this check is one ServiceControl ships itself (primary, audit or transport check), + /// false when it was reported by a monitored endpoint. Computed from the check id. + /// + public bool Internal => InternalCustomCheckClassification.IsInternal(CustomCheckId); + + object?[] IVersionedRow.GetVersionFields() => + [ + Id, CustomCheckId, Category, Status, ReportedAt, FailureReason, + OriginatingEndpoint?.Name, OriginatingEndpoint?.HostId, OriginatingEndpoint?.Host + ]; + } +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence/ICustomChecksDataStore.cs b/src/ServiceControl.Persistence/ICustomChecksDataStore.cs index 5b48c625ee..212a8ce61a 100644 --- a/src/ServiceControl.Persistence/ICustomChecksDataStore.cs +++ b/src/ServiceControl.Persistence/ICustomChecksDataStore.cs @@ -11,7 +11,7 @@ public interface ICustomChecksDataStore { Task UpdateCustomCheckStatus(CustomCheckDetail detail, CancellationToken cancellationToken = default); - Task>> GetStats(PagingInfo paging, string? status = null, CancellationToken cancellationToken = default); + Task>> GetStats(PagingInfo paging, string? status = null, CancellationToken cancellationToken = default); Task DeleteCustomCheck(Guid id, CancellationToken cancellationToken = default); Task GetNumberOfFailedChecks(CancellationToken cancellationToken = default); } diff --git a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs new file mode 100644 index 0000000000..4a7fc96e7f --- /dev/null +++ b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs @@ -0,0 +1,60 @@ +//Nullable enable is explicit here because this file is +//included in audit test projects that do not have it enabled. +#nullable enable +namespace ServiceControl.Contracts.CustomChecks +{ + using System; + using System.Collections.Generic; + + /// + /// Classifies the custom checks ServiceControl ships itself so ServicePulse can tell them apart from + /// checks reported by monitored endpoints. Feeds the computed + /// property, which is the only place the classification is ever rendered. + /// + /// Only the primary instance serves /api/customchecks, so only it needs the classification: its own + /// checks arrive via InternalCustomCheckManager, and the audit instance's checks arrive as + /// ReportCustomCheckResult messages (a wire contract owned by the NServiceBus.CustomChecks package, so + /// nothing extra can travel in the message — the id has to be recognized here). Consequence: the audit + /// section below is a list of string literals. New audit-instance checks MUST be added here; there are + /// approval tests that enforce this. + /// + public static class InternalCustomCheckClassification + { + // Keyed by CustomCheckId only, deliberately not by (id, category): + // - "RavenDB dirty memory" is reported by both the primary ("ServiceControl Health") + // and the audit instance ("ServiceControl.Audit Health") with the same id; + // - "Audit Message Ingestion Process" is reported by the audit instance under the + // category "ServiceControl Health" (unlike its siblings). + // Comparison is ordinal-ignore-case, mirroring CustomChecksMailNotification.IsHealthCheck. + static readonly HashSet internalIds = + new(StringComparer.OrdinalIgnoreCase) + { + // ----- Primary instance ----- + "ServiceControl Primary Instance", + "ServiceControl Remotes", + "Saga Audit Configuration", + "Error Message Ingestion", + "Error Message Ingestion Process", + "Error Database Index Errors", // RavenDB persister + "Error Database Index Lag", // RavenDB persister + "RavenDB dirty memory", // primary AND audit + "ServiceControl database", // RavenDB persister + "Message Ingestion Process", // RavenDB persister + "ServiceControl body storage", // EF Core persisters + "Dead Letter Queue", // ASBS / IBMMQ / MSMQ + + // ----- Audit instance (forwarded to the primary via ReportCustomCheckResult) ----- + "Audit Message Ingestion", + "Audit Message Ingestion Process", + "Audit Database Index Lag", + "ServiceControl.Audit database", + }; + + /// + /// True when the id is one ServiceControl ships itself (primary, audit or transport check), + /// false when it was reported by a monitored endpoint and has no platform-health semantics. + /// + public static bool IsInternal(string? customCheckId) => + customCheckId is not null && internalIds.Contains(customCheckId); + } +} \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt b/src/ServiceControl.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt new file mode 100644 index 0000000000..6c1b03a3d2 --- /dev/null +++ b/src/ServiceControl.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt @@ -0,0 +1,5 @@ +Configuration: Saga Audit Configuration => internal +Health: ServiceControl Primary Instance => internal +Health: ServiceControl Remotes => internal +ServiceControl Health: Error Message Ingestion => internal +ServiceControl Health: Error Message Ingestion Process => internal \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs b/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs new file mode 100644 index 0000000000..c5536ad574 --- /dev/null +++ b/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs @@ -0,0 +1,94 @@ +namespace ServiceControl.UnitTests.CustomChecks +{ + using System; + using System.Text.Json; + using NUnit.Framework; + using ServiceControl.Contracts.CustomChecks; + using ServiceControl.Infrastructure.WebApi; + using ServiceControl.Operations; + using ServiceControl.Persistence; + using ServiceControl.Persistence.Infrastructure; + + [TestFixture] + class InternalCustomCheckClassificationTests + { + static CustomCheckView Check(string id, string category = "Health") => new() + { + Id = "CustomChecks/1", + CustomCheckId = id, + Category = category, + Status = Status.Fail, + ReportedAt = new DateTime(2026, 8, 1, 9, 0, 0, DateTimeKind.Utc), + OriginatingEndpoint = new EndpointDetails { Name = "test-host", Host = "localhost", HostId = DeterministicGuid.MakeId("test-host", "host") } + }; + + [Test] + public void Internal_checks_are_flagged_internal() + { + var check = Check("ServiceControl Primary Instance"); + + Assert.That(check.Internal, Is.True); + } + + [TestCase("Error Message Ingestion")] + [TestCase("Dead Letter Queue")] + [TestCase("ServiceControl body storage")] + [TestCase("Audit Message Ingestion Process")] + public void Every_shipped_check_is_internal(string id) + { + var check = Check(id); + + Assert.That(check.Internal, Is.True, $"{id} is not in the registry"); + } + + [Test] + public void Matching_ignores_case_and_category_so_the_same_id_works_for_primary_and_audit() + { + // "RavenDB dirty memory" is reported by the primary under "ServiceControl Health" + // and by the audit instance under "ServiceControl.Audit Health" + var primary = Check("RavenDB dirty memory", "ServiceControl Health"); + var audit = Check("ravendb dirty memory", "ServiceControl.Audit Health"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(primary.Internal, Is.True); + Assert.That(audit.Internal, Is.True); + } + } + + [Test] + public void Endpoint_checks_are_not_internal() + { + var check = Check("MyCustomCheckId", "MyCategory"); + + Assert.That(check.Internal, Is.False); + } + + [Test] + public void The_wire_shape_is_additive_only() + { + var check = Check("ServiceControl Primary Instance"); + + var json = JsonSerializer.Serialize(new[] { check }, SerializerOptions.Default); + + // New field present: + Assert.That(json, Does.Contain("\"internal\":true")); + // Every pre-existing field still present, unchanged: + Assert.That(json, Does.Contain("\"custom_check_id\":\"ServiceControl Primary Instance\"")); + Assert.That(json, Does.Contain("\"category\":\"Health\"")); + Assert.That(json, Does.Contain("\"status\":\"fail\"")); + Assert.That(json, Does.Contain("\"reported_at\"")); + Assert.That(json, Does.Contain("\"originating_endpoint\"")); + } + + [Test] + public void External_checks_render_internal_false_on_the_wire() + { + var check = Check("MyCustomCheckId", "MyCategory"); + + var json = JsonSerializer.Serialize(new[] { check }, SerializerOptions.Default); + + Assert.That(json, Does.Contain("\"internal\":false")); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/CustomChecksTest.cs b/src/ServiceControl.UnitTests/CustomChecksTest.cs new file mode 100644 index 0000000000..87bfbf7db2 --- /dev/null +++ b/src/ServiceControl.UnitTests/CustomChecksTest.cs @@ -0,0 +1,35 @@ +namespace ServiceControl.UnitTests.API +{ + using System; + using System.Linq; + using NUnit.Framework; + using NServiceBus.CustomChecks; + using Particular.Approvals; + using ServiceBus.Management.Infrastructure.Settings; + using ServiceControl.Contracts.CustomChecks; + + [TestFixture] + class CustomChecksTest + { + [Test] + public void VerifyCustomChecks() + { + var settings = (object)new Settings(); + + var discovered = + from type in typeof(Settings).Assembly.GetTypes() + where type is { IsAbstract: false, IsInterface: false } + && typeof(ICustomCheck).IsAssignableFrom(type) + let constructor = type.GetConstructors().Single() + let constructorParameters = constructor.GetParameters() + .Select(p => p.ParameterType == typeof(Settings) ? settings : null) + .ToArray() + let instance = (ICustomCheck)constructor.Invoke(constructorParameters) + let classified = InternalCustomCheckClassification.IsInternal(instance.Id) + orderby instance.Category, instance.Id + select $"{instance.Category}: {instance.Id} => {(classified ? "internal" : "MISSING FROM REGISTRY")}"; + + Approver.Verify(string.Join(Environment.NewLine, discovered)); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs b/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs index 75f221890b..71cb9ad5b0 100644 --- a/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs +++ b/src/ServiceControl/CustomChecks/Web/CustomCheckController.cs @@ -21,7 +21,7 @@ public class CustomCheckController(ICustomChecksDataStore checksDataStore, IMess [Authorize(Policy = Permissions.ErrorCustomChecksView)] [Route("customchecks")] [HttpGet] - public async Task> CustomChecks([FromQuery] PagingInfo pagingInfo, string status = null, CancellationToken cancellationToken = default) + public async Task> CustomChecks([FromQuery] PagingInfo pagingInfo, string status = null, CancellationToken cancellationToken = default) { var stats = await checksDataStore.GetStats(pagingInfo, status, cancellationToken);