From 1e06c53c16ba4fbc0f0103f3ba494744c3eb294d Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Tue, 1 Sep 2026 13:06:08 +0800 Subject: [PATCH 1/7] Enrich internal custom checks with severity --- docs/data-versioning-design.md | 2 + .../When_a_persister_check_fails.cs | 69 +++++++++ ...viceControl.AcceptanceTests.RavenDB.csproj | 3 + ...hen_a_failing_custom_check_is_dismissed.cs | 2 +- .../When_custom_checks_are_classified.cs | 132 ++++++++++++++++++ ...When_email_notifications_are_configured.cs | 2 +- ...heck_registry_covers_all_shipped_checks.cs | 93 ++++++++++++ ...When_the_body_storage_check_is_reported.cs | 44 ++++++ .../API/AuditCustomCheckApprovals.cs | 39 ++++++ ....Audit_check_ids_are_snapshot.approved.txt | 2 + ...en_audit_internal_checks_are_classified.cs | 60 ++++++++ .../Implementation/CustomCheckDataStore.cs | 6 +- .../CustomChecks}/CustomCheck.cs | 11 +- .../RavenCustomCheckDataStore.cs | 21 ++- .../CustomCheckSeverity.cs | 13 ++ .../CustomCheckView.cs | 43 ++++++ .../ICustomChecksDataStore.cs | 2 +- .../InternalCustomCheckClassification.cs | 60 ++++++++ .../InternalCustomCheckRegistryApprovals.cs | 39 ++++++ ...p_assembly_is_in_the_registry.approved.txt | 5 + .../InternalCustomCheckClassificationTests.cs | 105 ++++++++++++++ .../CustomChecks/Web/CustomCheckController.cs | 2 +- 22 files changed, 738 insertions(+), 17 deletions(-) create mode 100644 src/ServiceControl.AcceptanceTests.RavenDB/Monitoring/CustomChecks/When_a_persister_check_fails.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs create mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_the_body_storage_check_is_reported.cs create mode 100644 src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs create mode 100644 src/ServiceControl.Audit.UnitTests/ApprovalFiles/AuditCustomCheckApprovals.Audit_check_ids_are_snapshot.approved.txt create mode 100644 src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs rename src/{ServiceControl.Persistence => ServiceControl.Persistence.RavenDB/CustomChecks}/CustomCheck.cs (53%) create mode 100644 src/ServiceControl.Persistence/CustomCheckSeverity.cs create mode 100644 src/ServiceControl.Persistence/CustomCheckView.cs create mode 100644 src/ServiceControl.Persistence/InternalCustomCheckClassification.cs create mode 100644 src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs create mode 100644 src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt create mode 100644 src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs diff --git a/docs/data-versioning-design.md b/docs/data-versioning-design.md index 360f3be85c..4d16eff161 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` and `CustomCheckView.Severity` are that case — both are computed, get-only properties 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, they cannot be assigned at all, so the reflection test above never sees them as fields 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..97cc8b2319 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests.RavenDB/Monitoring/CustomChecks/When_a_persister_check_fails.cs @@ -0,0 +1,69 @@ +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 CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + using CheckStatus = global::ServiceControl.Persistence.Status; + + // Sibling of When_critical_storage_threshold_reached (see .plans/internal-customchecks.md §8.2): 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_and_degraded() + { + 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); + Assert.That(ingestionCheck.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); + } + } + + 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..b7eac25348 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,12 +6,12 @@ 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; 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..98af7a8abe --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs @@ -0,0 +1,132 @@ +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 CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; + 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_with_severity_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/Severity from the check id, so deserializing alone would not + // prove the endpoint emits them. 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(internalCheck.Severity, Is.EqualTo(CustomCheckSeverity.Unavailable)); + + Assert.That(endpointCheck, Is.Not.Null); + Assert.That(endpointCheck.Internal, Is.False); + Assert.That(endpointCheck.Severity, Is.Null); + + // 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("\"severity\":\"unavailable\""), "the primary instance check must render severity:unavailable on the wire"); + Assert.That(wireBody, Does.Contain("\"internal\":false"), "endpoint checks must render internal:false on the wire"); + } + } + + [Test] + public async Task Severity_matches_the_spiked_platform_health_config_for_every_internal_check_present() + { + // The acceptance test runner disables internal custom checks by default; this test needs them. + SetSettings = settings => { settings.DisableHealthChecks = false; }; + + var expected = new (string Id, CustomCheckSeverity Severity)[] + { + ("ServiceControl Primary Instance", CustomCheckSeverity.Unavailable), + ("ServiceControl Remotes", CustomCheckSeverity.Unavailable), + ("Saga Audit Configuration", CustomCheckSeverity.Ignore), + // RavenDB persister checks also assert here on the RavenDB acceptance variant: + ("Error Message Ingestion Process", CustomCheckSeverity.Degraded), + ("Error Message Ingestion", CustomCheckSeverity.Degraded), + }; + + 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 expected.All(e => seen.Any(s => s.CustomCheckId == e.Id)); + }) + .Run(); + + foreach (var (id, severity) in expected) + { + var check = seen.Single(s => s.CustomCheckId == id); + using (Assert.EnterMultipleScope()) + { + Assert.That(check.Internal, Is.True, id); + Assert.That(check.Severity, Is.EqualTo(severity), 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..b874340548 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,13 +10,13 @@ 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; class When_email_notifications_are_configured : AcceptanceTest diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs new file mode 100644 index 0000000000..325eb5cdab --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs @@ -0,0 +1,93 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NServiceBus.CustomChecks; + using NUnit.Framework; + using ServiceBus.Management.Infrastructure.Settings; + using ServiceControl.Contracts.CustomChecks; + + // Prong 2 of drift protection (see .plans/internal-customchecks.md §7.6): persister and transport checks + // are runtime-loaded plugin assemblies, not compile-visible to ServiceControl.UnitTests, so the unit-test + // approval (Prong 1) cannot see them. This test runs after the instance boots — when the plugin assemblies + // are loaded — and asserts every shipped product check discoverable at runtime is classified by the registry. + // + // Some checks (e.g. DeadLetterQueueCheck) perform work in their constructor, so they cannot be instantiated + // with null constructor arguments. Those are skipped here; they are already covered directly by the registry + // unit tests (Every_shipped_check_has_a_severity) and by the API acceptance assertions. + [TestFixture] + class When_internal_check_registry_covers_all_shipped_checks : AcceptanceTest + { + [Test] + public async Task Every_discovered_product_check_is_classified() + { + var missing = new List(); + var scanned = false; + + await Define() + .Done(_ => + { + if (scanned) + { + return true; + } + + scanned = true; + + var settings = (object)new Settings(); + + var productChecks = + from assembly in AppDomain.CurrentDomain.GetAssemblies() + let name = assembly.GetName().Name + where name != null + && name.StartsWith("ServiceControl") + && !name.Contains("Test") + && !name.Contains("Acceptance") + from type in assembly.GetTypes() + where type is { IsAbstract: false, IsInterface: false } + && typeof(ICustomCheck).IsAssignableFrom(type) + select type; + + foreach (var type in productChecks) + { + string id; + try + { + var constructor = type.GetConstructors().Single(); + var args = constructor.GetParameters() + .Select(p => p.ParameterType == typeof(Settings) ? settings : null) + .ToArray(); + var instance = (ICustomCheck)constructor.Invoke(args); + id = instance.Id; + } + catch (Exception) + { + // Constructor does work (e.g. DeadLetterQueueCheck dereferences its settings); + // cannot be classified by reflection. It is covered by the registry unit tests instead. + continue; + } + + if (InternalCustomCheckClassification.SeverityFor(id) is null) + { + missing.Add(id); + } + } + + return true; + }) + .Run(); + + Assert.That(missing, Is.Empty, + "Every check ServiceControl ships must have a severity in the registry. " + + "If you added a check, add it to InternalCustomCheckClassification " + + "(see .plans/internal-customchecks.md §7.3). Missing: " + + string.Join(", ", missing)); + } + + class Context : ScenarioContext; + } +} \ No newline at end of file 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..1da2346162 --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_the_body_storage_check_is_reported.cs @@ -0,0 +1,44 @@ +namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks +{ + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + + // "ServiceControl body storage" is an EF Core persister check (SQL Server / PostgreSQL variants) that was + // missing from the ServicePulse spike table. This proves goal 5 of .plans/internal-customchecks.md server-side: + // a shipped check the spike table does not know about is still classified internal + degraded by the API. + // Excluded from the RavenDB variant (bodies are stored in the database there, so the check does not exist) — + // see the Compile Remove in ServiceControl.AcceptanceTests.RavenDB.csproj. + [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_and_degraded() + { + 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); + Assert.That(bodyStorageCheck.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); + } + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs new file mode 100644 index 0000000000..0628ecfb68 --- /dev/null +++ b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs @@ -0,0 +1,39 @@ +namespace ServiceControl.Audit.UnitTests.API +{ + using System; + using System.Linq; + using Audit.Infrastructure.Settings; + using NUnit.Framework; + using NServiceBus.CustomChecks; + using Particular.Approvals; + + [TestFixture] + class AuditCustomCheckApprovals + { + // 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 multi-instance acceptance test instead + // (see .plans/internal-customchecks.md §7.8). + [Test] + public void Audit_check_ids_are_snapshot() + { + 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) + orderby instance.Category, instance.Id + select $"{instance.Category}: {instance.Id}"; + + Approver.Verify(string.Join(Environment.NewLine, discovered)); + } + } +} \ No newline at end of file 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.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs b/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs new file mode 100644 index 0000000000..dfd52933ca --- /dev/null +++ b/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs @@ -0,0 +1,60 @@ +namespace ServiceControl.MultiInstance.AcceptanceTests.Monitoring +{ + using System.Collections.Generic; + using System.Linq; + using System.Threading.Tasks; + using AcceptanceTesting; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + using TestSupport; + using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; + using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; + + // Primary + audit instances. The audit forwards its checks to the primary as + // ReportCustomCheckResult messages, so this is the test that proves the primary's + // hard-coded audit IDs (string literals — the audit assembly is not referenced) stay correct. + [TestFixture] + class When_audit_internal_checks_are_classified : AcceptanceTest + { + [Test] + public async Task Audit_checks_arriving_as_messages_are_flagged_internal_and_degraded() + { + var expectedIds = new[] + { + "Audit Message Ingestion Process", + "Audit Message Ingestion", + }; + + var seen = new List(); + + await Define() + .Done(async c => + { + var checks = await this.TryGetMany("/api/customchecks", instanceName: ServiceControlInstanceName); + 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(id => seen.Any(s => s.CustomCheckId == id)); + }) + .Run(); + + foreach (var id in expectedIds) + { + var check = seen.Single(s => s.CustomCheckId == id); + using (Assert.EnterMultipleScope()) + { + Assert.That(check.Internal, Is.True, id); + Assert.That(check.Severity, Is.EqualTo(CustomCheckSeverity.Degraded), id); + } + } + } + + class Context : ScenarioContext; + } +} \ 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/CustomCheckSeverity.cs b/src/ServiceControl.Persistence/CustomCheckSeverity.cs new file mode 100644 index 0000000000..c2ea498300 --- /dev/null +++ b/src/ServiceControl.Persistence/CustomCheckSeverity.cs @@ -0,0 +1,13 @@ +namespace ServiceControl.Contracts.CustomChecks +{ + /// + /// The impact a failing internal custom check has on platform health, as rendered by ServicePulse. + /// Serialized lowercase on the wire: "ignore", "degraded", "unavailable". + /// + public enum CustomCheckSeverity + { + Ignore = 0, + Degraded = 1, + Unavailable = 2 + } +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence/CustomCheckView.cs b/src/ServiceControl.Persistence/CustomCheckView.cs new file mode 100644 index 0000000000..0d8a5f8fce --- /dev/null +++ b/src/ServiceControl.Persistence/CustomCheckView.cs @@ -0,0 +1,43 @@ +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, and how severe a failing internal check is + /// for platform health. Both are classified from the check id at read time and are 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 => Severity is not null; + + /// + /// Platform-health severity for internal checks. Computed from the check id, so it cannot drift + /// independently of it. Null — and therefore omitted from the response — for endpoint checks, + /// which have no platform-health semantics. + /// + public CustomCheckSeverity? Severity => InternalCustomCheckClassification.SeverityFor(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..fed64b2ddd --- /dev/null +++ b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs @@ -0,0 +1,60 @@ +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 and grade platform health accordingly. Feeds the computed + /// and properties, 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 + /// severity cannot travel in the message — it has to be re-derived here). Consequence: the audit section + /// below is a list of string literals. New audit-instance checks MUST be added here; the tests listed in + /// .plans/internal-customchecks.md §8 exist to catch omissions. + /// + 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 severity; + // - "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 Dictionary severityById = + new(StringComparer.OrdinalIgnoreCase) + { + // ----- Primary instance ----- + ["ServiceControl Primary Instance"] = CustomCheckSeverity.Unavailable, + ["ServiceControl Remotes"] = CustomCheckSeverity.Unavailable, + ["Saga Audit Configuration"] = CustomCheckSeverity.Ignore, + ["Error Message Ingestion"] = CustomCheckSeverity.Degraded, + ["Error Message Ingestion Process"] = CustomCheckSeverity.Degraded, + ["Error Database Index Errors"] = CustomCheckSeverity.Degraded, // RavenDB persister + ["Error Database Index Lag"] = CustomCheckSeverity.Degraded, // RavenDB persister + ["RavenDB dirty memory"] = CustomCheckSeverity.Degraded, // primary AND audit + ["ServiceControl database"] = CustomCheckSeverity.Degraded, // RavenDB persister + ["Message Ingestion Process"] = CustomCheckSeverity.Degraded, // RavenDB persister + ["ServiceControl body storage"] = CustomCheckSeverity.Degraded, // EF Core persisters + ["Dead Letter Queue"] = CustomCheckSeverity.Degraded, // ASBS / IBMMQ / MSMQ + + // ----- Audit instance (forwarded to the primary via ReportCustomCheckResult) ----- + ["Audit Message Ingestion"] = CustomCheckSeverity.Degraded, + ["Audit Message Ingestion Process"] = CustomCheckSeverity.Degraded, + ["Audit Database Index Lag"] = CustomCheckSeverity.Degraded, + ["ServiceControl.Audit database"] = CustomCheckSeverity.Degraded, + }; + + /// + /// The severity of a shipped check, or null when the id is not one ServiceControl knows — which + /// means the check was reported by a monitored endpoint and has no platform-health semantics. + /// + public static CustomCheckSeverity? SeverityFor(string? customCheckId) => + customCheckId is not null && severityById.TryGetValue(customCheckId, out var severity) + ? severity + : null; + } +} \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs b/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs new file mode 100644 index 0000000000..00ae97a473 --- /dev/null +++ b/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs @@ -0,0 +1,39 @@ +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 InternalCustomCheckRegistryApprovals + { + [Test] + public void Every_shipped_check_in_the_app_assembly_is_in_the_registry() + { + // HINT: The primary references persister and transport assemblies as runtime-loaded plugins + // (ReferenceOutputAssembly="false" Private="false"), so only checks compiled into the + // ServiceControl app assembly are visible here. The persister/transport checks are covered + // by the runtime-assembly-scan acceptance test (see .plans/internal-customchecks.md §7.6). + 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 severity = InternalCustomCheckClassification.SeverityFor(instance.Id) + orderby instance.Category, instance.Id + select $"{instance.Category}: {instance.Id} => {severity?.ToString() ?? "MISSING FROM REGISTRY"}"; + + Approver.Verify(string.Join(Environment.NewLine, discovered)); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt b/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt new file mode 100644 index 0000000000..5a6e4c0db0 --- /dev/null +++ b/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt @@ -0,0 +1,5 @@ +Configuration: Saga Audit Configuration => Ignore +Health: ServiceControl Primary Instance => Unavailable +Health: ServiceControl Remotes => Unavailable +ServiceControl Health: Error Message Ingestion => Degraded +ServiceControl Health: Error Message Ingestion Process => Degraded \ 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..662e7b9449 --- /dev/null +++ b/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs @@ -0,0 +1,105 @@ +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_and_carry_their_severity() + { + var check = Check("ServiceControl Primary Instance"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(check.Internal, Is.True); + Assert.That(check.Severity, Is.EqualTo(CustomCheckSeverity.Unavailable)); + } + } + + [TestCase("Error Message Ingestion", ExpectedResult = CustomCheckSeverity.Degraded)] + [TestCase("Dead Letter Queue", ExpectedResult = CustomCheckSeverity.Degraded)] + [TestCase("ServiceControl body storage", ExpectedResult = CustomCheckSeverity.Degraded)] + [TestCase("Audit Message Ingestion Process", ExpectedResult = CustomCheckSeverity.Degraded)] + public CustomCheckSeverity Every_shipped_check_has_a_severity(string id) + { + var check = Check(id); + + Assert.That(check.Internal, Is.True, $"{id} is not in the registry"); + return check.Severity!.Value; + } + + [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.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); + Assert.That(audit.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); + } + } + + [Test] + public void Endpoint_checks_are_not_internal_and_carry_no_severity() + { + var check = Check("MyCustomCheckId", "MyCategory"); + + using (Assert.EnterMultipleScope()) + { + Assert.That(check.Internal, Is.False); + Assert.That(check.Severity, Is.Null); + } + } + + [Test] + public void The_wire_shape_is_additive_only() + { + var check = Check("ServiceControl Primary Instance"); + + var json = JsonSerializer.Serialize(new[] { check }, SerializerOptions.Default); + + // New fields present: + Assert.That(json, Does.Contain("\"internal\":true")); + Assert.That(json, Does.Contain("\"severity\":\"unavailable\"")); + // 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_omit_severity_on_the_wire() + { + var check = Check("MyCustomCheckId", "MyCategory"); + + var json = JsonSerializer.Serialize(new[] { check }, SerializerOptions.Default); + + Assert.That(json, Does.Contain("\"internal\":false")); + Assert.That(json, Does.Not.Contain("severity")); + } + } +} \ 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); From 2ec3dfeb023b4bb0608d4ea478ddc66d12f857c5 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Tue, 1 Sep 2026 13:33:04 +0800 Subject: [PATCH 2/7] Fix ambiguous types --- .../CustomChecks/When_a_failing_custom_check_is_dismissed.cs | 1 + .../CustomChecks/When_email_notifications_are_configured.cs | 1 + 2 files changed, 2 insertions(+) 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 b7eac25348..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 @@ -13,6 +13,7 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using NUnit.Framework; using ServiceBus.Management.Infrastructure.Settings; 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_email_notifications_are_configured.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_configured.cs index b874340548..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 @@ -18,6 +18,7 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using ServiceBus.Management.Infrastructure.Settings; using ServiceControl.Notifications; using CheckStatus = global::ServiceControl.Persistence.Status; + using CustomCheck = NServiceBus.CustomChecks.CustomCheck; class When_email_notifications_are_configured : AcceptanceTest { From 97bb57ade828c2181158bfd735cc1a995bf7d0cb Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Tue, 1 Sep 2026 15:56:23 +0800 Subject: [PATCH 3/7] Remove severity in favor of a simple 'Internal' flag --- docs/data-versioning-design.md | 2 +- .../DiagPath.cs | 45 +++++++++++++++ .../When_a_persister_check_fails.cs | 6 +- .../When_custom_checks_are_classified.cs | 34 +++++------ ...heck_registry_covers_all_shipped_checks.cs | 15 ++--- ...When_the_body_storage_check_is_reported.cs | 9 +-- .../API/AuditCustomCheckApprovals.cs | 1 - ...en_audit_internal_checks_are_classified.cs | 9 +-- .../CustomCheckSeverity.cs | 13 ----- .../CustomCheckView.cs | 13 +---- .../InternalCustomCheckClassification.cs | 57 +++++++++---------- .../InternalCustomCheckRegistryApprovals.cs | 8 +-- ...p_assembly_is_in_the_registry.approved.txt | 10 ++-- .../InternalCustomCheckClassificationTests.cs | 37 +++++------- 14 files changed, 118 insertions(+), 141 deletions(-) create mode 100644 src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs delete mode 100644 src/ServiceControl.Persistence/CustomCheckSeverity.cs diff --git a/docs/data-versioning-design.md b/docs/data-versioning-design.md index 4d16eff161..dadb89dbcb 100644 --- a/docs/data-versioning-design.md +++ b/docs/data-versioning-design.md @@ -12,7 +12,7 @@ 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` and `CustomCheckView.Severity` are that case — both are computed, get-only properties 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, they cannot be assigned at all, so the reflection test above never sees them as fields 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 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. diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs b/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs new file mode 100644 index 0000000000..5eade0036f --- /dev/null +++ b/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs @@ -0,0 +1,45 @@ +namespace ServiceControl.AcceptanceTests.RavenDB +{ + using System; + using System.Diagnostics; + using NUnit.Framework; + + [TestFixture] + class DiagPath + { + static void Run(string label, string file, string args) + { + Console.WriteLine($"=== {label}: {file} {args} ==="); + var psi = new ProcessStartInfo(file, args) { RedirectStandardOutput = true, UseShellExecute = false, CreateNoWindow = true }; + try + { + using var p = Process.Start(psi)!; + var output = p.StandardOutput.ReadToEnd(); + p.WaitForExit(); + foreach (var line in output.Split('\n')) + { + if (line.Contains("NETCore.App") || line.Contains("runtimes installed") || line.Contains("Base Path")) + { + Console.WriteLine(" " + line.Trim()); + } + } + } + catch (Exception e) + { + Console.WriteLine(" FAILED: " + e.Message); + } + } + + [Test] + public void PrintPathAndDotnetInfo() + { + Console.WriteLine("DIAG PATH=" + Environment.GetEnvironmentVariable("PATH")); + Console.WriteLine("DIAG CWD=" + Environment.CurrentDirectory); + Run("PATH-dotnet", "dotnet", "--info"); + Run("tmp-wrap", "/tmp/wrap/dotnet", "--info"); + Run("tmp-dotnet8", "/tmp/dotnet8/dotnet", "--info"); + Run("home-dotnet", "/home/piuser/.dotnet/dotnet", "--info"); + Assert.Pass(); + } + } +} \ No newline at end of file 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 index 97cc8b2319..a635fda807 100644 --- 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 @@ -10,11 +10,10 @@ namespace ServiceControl.AcceptanceTests.RavenDB.Monitoring.CustomChecks using NUnit.Framework; using Operations; using ServiceBus.Management.Infrastructure.Settings; - using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; using CheckStatus = global::ServiceControl.Persistence.Status; - // Sibling of When_critical_storage_threshold_reached (see .plans/internal-customchecks.md §8.2): proves a + // 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. @@ -31,7 +30,7 @@ public void SetupIngestion() => RavenPersisterSettings PersisterSettings => (RavenPersisterSettings)Settings.PersisterSpecificSettings; [Test] - public async Task Forced_failure_is_classified_internal_and_degraded() + public async Task Forced_failure_is_classified_internal() { CustomCheckView ingestionCheck = null; @@ -56,7 +55,6 @@ await Define() { Assert.That(ingestionCheck, Is.Not.Null, "the forced storage-threshold failure never showed up"); Assert.That(ingestionCheck.Internal, Is.True); - Assert.That(ingestionCheck.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); } } 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 index 98af7a8abe..8b1995386e 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_custom_checks_are_classified.cs @@ -11,7 +11,6 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using NServiceBus.CustomChecks; using NUnit.Framework; using ServiceBus.Management.Infrastructure.Settings; - using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; using CheckStatus = global::ServiceControl.Persistence.Status; @@ -22,7 +21,7 @@ class When_custom_checks_are_classified : AcceptanceTest const string InternalId = "ServiceControl Primary Instance"; [Test] - public async Task Internal_checks_are_flagged_with_severity_and_endpoint_checks_are_not() + 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; }; @@ -40,8 +39,8 @@ await Define() internalCheck ??= checks.Items.SingleOrDefault(x => x.CustomCheckId == InternalId); endpointCheck ??= checks.Items.SingleOrDefault(x => x.CustomCheckId == "MyCustomCheckId" && x.Status == CheckStatus.Fail); - // The view computes Internal/Severity from the check id, so deserializing alone would not - // prove the endpoint emits them. Grab the raw payload once and assert on the wire itself. + // 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"); @@ -56,33 +55,30 @@ await Define() { Assert.That(internalCheck, Is.Not.Null, "primary internal checks report at startup; nothing was found"); Assert.That(internalCheck.Internal, Is.True); - Assert.That(internalCheck.Severity, Is.EqualTo(CustomCheckSeverity.Unavailable)); Assert.That(endpointCheck, Is.Not.Null); Assert.That(endpointCheck.Internal, Is.False); - Assert.That(endpointCheck.Severity, Is.Null); // 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("\"severity\":\"unavailable\""), "the primary instance check must render severity:unavailable on the wire"); Assert.That(wireBody, Does.Contain("\"internal\":false"), "endpoint checks must render internal:false on the wire"); } } [Test] - public async Task Severity_matches_the_spiked_platform_health_config_for_every_internal_check_present() + 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 expected = new (string Id, CustomCheckSeverity Severity)[] + var expectedIds = new[] { - ("ServiceControl Primary Instance", CustomCheckSeverity.Unavailable), - ("ServiceControl Remotes", CustomCheckSeverity.Unavailable), - ("Saga Audit Configuration", CustomCheckSeverity.Ignore), + "ServiceControl Primary Instance", + "ServiceControl Remotes", + "Saga Audit Configuration", // RavenDB persister checks also assert here on the RavenDB acceptance variant: - ("Error Message Ingestion Process", CustomCheckSeverity.Degraded), - ("Error Message Ingestion", CustomCheckSeverity.Degraded), + "Error Message Ingestion Process", + "Error Message Ingestion", }; var seen = new System.Collections.Generic.List(); @@ -100,18 +96,14 @@ await Define() } } - return expected.All(e => seen.Any(s => s.CustomCheckId == e.Id)); + return expectedIds.All(e => seen.Any(s => s.CustomCheckId == e)); }) .Run(); - foreach (var (id, severity) in expected) + foreach (var id in expectedIds) { var check = seen.Single(s => s.CustomCheckId == id); - using (Assert.EnterMultipleScope()) - { - Assert.That(check.Internal, Is.True, id); - Assert.That(check.Severity, Is.EqualTo(severity), id); - } + Assert.That(check.Internal, Is.True, id); } } diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs index 325eb5cdab..94eb1c9511 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs @@ -4,21 +4,15 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; - using AcceptanceTesting; using NServiceBus.AcceptanceTesting; using NServiceBus.CustomChecks; using NUnit.Framework; using ServiceBus.Management.Infrastructure.Settings; using ServiceControl.Contracts.CustomChecks; - // Prong 2 of drift protection (see .plans/internal-customchecks.md §7.6): persister and transport checks - // are runtime-loaded plugin assemblies, not compile-visible to ServiceControl.UnitTests, so the unit-test - // approval (Prong 1) cannot see them. This test runs after the instance boots — when the plugin assemblies - // are loaded — and asserts every shipped product check discoverable at runtime is classified by the registry. - // // Some checks (e.g. DeadLetterQueueCheck) perform work in their constructor, so they cannot be instantiated // with null constructor arguments. Those are skipped here; they are already covered directly by the registry - // unit tests (Every_shipped_check_has_a_severity) and by the API acceptance assertions. + // unit tests (Every_shipped_check_is_internal) and by the API acceptance assertions. [TestFixture] class When_internal_check_registry_covers_all_shipped_checks : AcceptanceTest { @@ -71,7 +65,7 @@ from type in assembly.GetTypes() continue; } - if (InternalCustomCheckClassification.SeverityFor(id) is null) + if (!InternalCustomCheckClassification.IsInternal(id)) { missing.Add(id); } @@ -82,9 +76,8 @@ from type in assembly.GetTypes() .Run(); Assert.That(missing, Is.Empty, - "Every check ServiceControl ships must have a severity in the registry. " + - "If you added a check, add it to InternalCustomCheckClassification " + - "(see .plans/internal-customchecks.md §7.3). Missing: " + + "Every check ServiceControl ships must be in the internal registry. " + + "If you added a check, add it to InternalCustomCheckClassification. Missing: " + string.Join(", ", missing)); } 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 index 1da2346162..a1fbfa6c4b 100644 --- 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 @@ -4,14 +4,8 @@ namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks using AcceptanceTesting; using NServiceBus.AcceptanceTesting; using NUnit.Framework; - using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; - // "ServiceControl body storage" is an EF Core persister check (SQL Server / PostgreSQL variants) that was - // missing from the ServicePulse spike table. This proves goal 5 of .plans/internal-customchecks.md server-side: - // a shipped check the spike table does not know about is still classified internal + degraded by the API. - // Excluded from the RavenDB variant (bodies are stored in the database there, so the check does not exist) — - // see the Compile Remove in ServiceControl.AcceptanceTests.RavenDB.csproj. [TestFixture] class When_the_body_storage_check_is_reported : AcceptanceTest { @@ -20,7 +14,7 @@ public void EnableInternalChecks() => SetSettings = static s => s.DisableHealthChecks = false; [Test] - public async Task Should_be_classified_internal_and_degraded() + public async Task Should_be_classified_internal() { CustomCheckView bodyStorageCheck = null; @@ -37,7 +31,6 @@ await Define() { Assert.That(bodyStorageCheck, Is.Not.Null, "the EF Core body storage check never reported"); Assert.That(bodyStorageCheck.Internal, Is.True); - Assert.That(bodyStorageCheck.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); } } } diff --git a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs index 0628ecfb68..11bdfdaf32 100644 --- a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs +++ b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs @@ -15,7 +15,6 @@ class AuditCustomCheckApprovals // 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 multi-instance acceptance test instead - // (see .plans/internal-customchecks.md §7.8). [Test] public void Audit_check_ids_are_snapshot() { diff --git a/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs b/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs index dfd52933ca..13f4253aad 100644 --- a/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs +++ b/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs @@ -7,7 +7,6 @@ namespace ServiceControl.MultiInstance.AcceptanceTests.Monitoring using NServiceBus.AcceptanceTesting; using NUnit.Framework; using TestSupport; - using CustomCheckSeverity = global::ServiceControl.Contracts.CustomChecks.CustomCheckSeverity; using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; // Primary + audit instances. The audit forwards its checks to the primary as @@ -17,7 +16,7 @@ namespace ServiceControl.MultiInstance.AcceptanceTests.Monitoring class When_audit_internal_checks_are_classified : AcceptanceTest { [Test] - public async Task Audit_checks_arriving_as_messages_are_flagged_internal_and_degraded() + public async Task Audit_checks_arriving_as_messages_are_flagged_internal() { var expectedIds = new[] { @@ -47,11 +46,7 @@ await Define() foreach (var id in expectedIds) { var check = seen.Single(s => s.CustomCheckId == id); - using (Assert.EnterMultipleScope()) - { - Assert.That(check.Internal, Is.True, id); - Assert.That(check.Severity, Is.EqualTo(CustomCheckSeverity.Degraded), id); - } + Assert.That(check.Internal, Is.True, id); } } diff --git a/src/ServiceControl.Persistence/CustomCheckSeverity.cs b/src/ServiceControl.Persistence/CustomCheckSeverity.cs deleted file mode 100644 index c2ea498300..0000000000 --- a/src/ServiceControl.Persistence/CustomCheckSeverity.cs +++ /dev/null @@ -1,13 +0,0 @@ -namespace ServiceControl.Contracts.CustomChecks -{ - /// - /// The impact a failing internal custom check has on platform health, as rendered by ServicePulse. - /// Serialized lowercase on the wire: "ignore", "degraded", "unavailable". - /// - public enum CustomCheckSeverity - { - Ignore = 0, - Degraded = 1, - Unavailable = 2 - } -} \ No newline at end of file diff --git a/src/ServiceControl.Persistence/CustomCheckView.cs b/src/ServiceControl.Persistence/CustomCheckView.cs index 0d8a5f8fce..9a5310563c 100644 --- a/src/ServiceControl.Persistence/CustomCheckView.cs +++ b/src/ServiceControl.Persistence/CustomCheckView.cs @@ -8,8 +8,8 @@ namespace ServiceControl.Contracts.CustomChecks /// /// 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, and how severe a failing internal check is - /// for platform health. Both are classified from the check id at read time and are never persisted. + /// 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 { @@ -25,14 +25,7 @@ public class CustomCheckView : IVersionedRow /// 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 => Severity is not null; - - /// - /// Platform-health severity for internal checks. Computed from the check id, so it cannot drift - /// independently of it. Null — and therefore omitted from the response — for endpoint checks, - /// which have no platform-health semantics. - /// - public CustomCheckSeverity? Severity => InternalCustomCheckClassification.SeverityFor(CustomCheckId); + public bool Internal => InternalCustomCheckClassification.IsInternal(CustomCheckId); object?[] IVersionedRow.GetVersionFields() => [ diff --git a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs index fed64b2ddd..4564e1161a 100644 --- a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs +++ b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs @@ -5,56 +5,53 @@ namespace ServiceControl.Contracts.CustomChecks /// /// Classifies the custom checks ServiceControl ships itself so ServicePulse can tell them apart from - /// checks reported by monitored endpoints and grade platform health accordingly. Feeds the computed - /// and properties, which - /// is the only place the classification is ever rendered. + /// 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 - /// severity cannot travel in the message — it has to be re-derived here). Consequence: the audit section - /// below is a list of string literals. New audit-instance checks MUST be added here; the tests listed in - /// .plans/internal-customchecks.md §8 exist to catch omissions. + /// 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 + /// acceptance 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 severity; + // 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 Dictionary severityById = + static readonly HashSet internalIds = new(StringComparer.OrdinalIgnoreCase) { // ----- Primary instance ----- - ["ServiceControl Primary Instance"] = CustomCheckSeverity.Unavailable, - ["ServiceControl Remotes"] = CustomCheckSeverity.Unavailable, - ["Saga Audit Configuration"] = CustomCheckSeverity.Ignore, - ["Error Message Ingestion"] = CustomCheckSeverity.Degraded, - ["Error Message Ingestion Process"] = CustomCheckSeverity.Degraded, - ["Error Database Index Errors"] = CustomCheckSeverity.Degraded, // RavenDB persister - ["Error Database Index Lag"] = CustomCheckSeverity.Degraded, // RavenDB persister - ["RavenDB dirty memory"] = CustomCheckSeverity.Degraded, // primary AND audit - ["ServiceControl database"] = CustomCheckSeverity.Degraded, // RavenDB persister - ["Message Ingestion Process"] = CustomCheckSeverity.Degraded, // RavenDB persister - ["ServiceControl body storage"] = CustomCheckSeverity.Degraded, // EF Core persisters - ["Dead Letter Queue"] = CustomCheckSeverity.Degraded, // ASBS / IBMMQ / MSMQ + "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"] = CustomCheckSeverity.Degraded, - ["Audit Message Ingestion Process"] = CustomCheckSeverity.Degraded, - ["Audit Database Index Lag"] = CustomCheckSeverity.Degraded, - ["ServiceControl.Audit database"] = CustomCheckSeverity.Degraded, + "Audit Message Ingestion", + "Audit Message Ingestion Process", + "Audit Database Index Lag", + "ServiceControl.Audit database", }; /// - /// The severity of a shipped check, or null when the id is not one ServiceControl knows — which - /// means the check was reported by a monitored endpoint and has no platform-health semantics. + /// 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 CustomCheckSeverity? SeverityFor(string? customCheckId) => - customCheckId is not null && severityById.TryGetValue(customCheckId, out var severity) - ? severity - : null; + 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/API/InternalCustomCheckRegistryApprovals.cs b/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs index 00ae97a473..36e7efc4aa 100644 --- a/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs +++ b/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs @@ -14,10 +14,6 @@ class InternalCustomCheckRegistryApprovals [Test] public void Every_shipped_check_in_the_app_assembly_is_in_the_registry() { - // HINT: The primary references persister and transport assemblies as runtime-loaded plugins - // (ReferenceOutputAssembly="false" Private="false"), so only checks compiled into the - // ServiceControl app assembly are visible here. The persister/transport checks are covered - // by the runtime-assembly-scan acceptance test (see .plans/internal-customchecks.md §7.6). var settings = (object)new Settings(); var discovered = @@ -29,9 +25,9 @@ from type in typeof(Settings).Assembly.GetTypes() .Select(p => p.ParameterType == typeof(Settings) ? settings : null) .ToArray() let instance = (ICustomCheck)constructor.Invoke(constructorParameters) - let severity = InternalCustomCheckClassification.SeverityFor(instance.Id) + let classified = InternalCustomCheckClassification.IsInternal(instance.Id) orderby instance.Category, instance.Id - select $"{instance.Category}: {instance.Id} => {severity?.ToString() ?? "MISSING FROM REGISTRY"}"; + select $"{instance.Category}: {instance.Id} => {(classified ? "internal" : "MISSING FROM REGISTRY")}"; Approver.Verify(string.Join(Environment.NewLine, discovered)); } diff --git a/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt b/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt index 5a6e4c0db0..6c1b03a3d2 100644 --- a/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt +++ b/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt @@ -1,5 +1,5 @@ -Configuration: Saga Audit Configuration => Ignore -Health: ServiceControl Primary Instance => Unavailable -Health: ServiceControl Remotes => Unavailable -ServiceControl Health: Error Message Ingestion => Degraded -ServiceControl Health: Error Message Ingestion Process => Degraded \ No newline at end of file +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 index 662e7b9449..c5536ad574 100644 --- a/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs +++ b/src/ServiceControl.UnitTests/CustomChecks/InternalCustomCheckClassificationTests.cs @@ -23,27 +23,22 @@ class InternalCustomCheckClassificationTests }; [Test] - public void Internal_checks_are_flagged_and_carry_their_severity() + public void Internal_checks_are_flagged_internal() { var check = Check("ServiceControl Primary Instance"); - using (Assert.EnterMultipleScope()) - { - Assert.That(check.Internal, Is.True); - Assert.That(check.Severity, Is.EqualTo(CustomCheckSeverity.Unavailable)); - } + Assert.That(check.Internal, Is.True); } - [TestCase("Error Message Ingestion", ExpectedResult = CustomCheckSeverity.Degraded)] - [TestCase("Dead Letter Queue", ExpectedResult = CustomCheckSeverity.Degraded)] - [TestCase("ServiceControl body storage", ExpectedResult = CustomCheckSeverity.Degraded)] - [TestCase("Audit Message Ingestion Process", ExpectedResult = CustomCheckSeverity.Degraded)] - public CustomCheckSeverity Every_shipped_check_has_a_severity(string id) + [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"); - return check.Severity!.Value; } [Test] @@ -56,21 +51,17 @@ public void Matching_ignores_case_and_category_so_the_same_id_works_for_primary_ using (Assert.EnterMultipleScope()) { - Assert.That(primary.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); - Assert.That(audit.Severity, Is.EqualTo(CustomCheckSeverity.Degraded)); + Assert.That(primary.Internal, Is.True); + Assert.That(audit.Internal, Is.True); } } [Test] - public void Endpoint_checks_are_not_internal_and_carry_no_severity() + public void Endpoint_checks_are_not_internal() { var check = Check("MyCustomCheckId", "MyCategory"); - using (Assert.EnterMultipleScope()) - { - Assert.That(check.Internal, Is.False); - Assert.That(check.Severity, Is.Null); - } + Assert.That(check.Internal, Is.False); } [Test] @@ -80,9 +71,8 @@ public void The_wire_shape_is_additive_only() var json = JsonSerializer.Serialize(new[] { check }, SerializerOptions.Default); - // New fields present: + // New field present: Assert.That(json, Does.Contain("\"internal\":true")); - Assert.That(json, Does.Contain("\"severity\":\"unavailable\"")); // 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\"")); @@ -92,14 +82,13 @@ public void The_wire_shape_is_additive_only() } [Test] - public void External_checks_omit_severity_on_the_wire() + 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")); - Assert.That(json, Does.Not.Contain("severity")); } } } \ No newline at end of file From d22263030fcd6f7f7a09974a458fe4fb90c81e79 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Tue, 1 Sep 2026 16:24:41 +0800 Subject: [PATCH 4/7] Add approval based tests to ensure registration of custom checks --- .../ServiceControl.Audit.Persistence.Tests.RavenDB.csproj | 3 +++ .../CustomCheckTests.cs | 5 +++-- .../ServiceControl.Audit.Persistence.Tests.csproj | 3 +++ .../ApprovalFiles/RavenPersistedTypes.Verify.approved.txt | 2 +- src/ServiceControl.Persistence.Tests/CustomCheckTests.cs | 3 ++- 5 files changed, 12 insertions(+), 4 deletions(-) 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.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")}" ) ); } From 38b090adbcd45bc2e5d2e706758f74ee1f515a13 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Tue, 1 Sep 2026 16:30:57 +0800 Subject: [PATCH 5/7] Replace acceptance tests with approval tests --- ...heck_registry_covers_all_shipped_checks.cs | 86 ------------------- .../API/AuditCustomCheckApprovals.cs | 2 +- ...en_audit_internal_checks_are_classified.cs | 55 ------------ .../InternalCustomCheckClassification.cs | 2 +- 4 files changed, 2 insertions(+), 143 deletions(-) delete mode 100644 src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs delete mode 100644 src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs deleted file mode 100644 index 94eb1c9511..0000000000 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_internal_check_registry_covers_all_shipped_checks.cs +++ /dev/null @@ -1,86 +0,0 @@ -namespace ServiceControl.AcceptanceTests.Monitoring.CustomChecks -{ - using System; - using System.Collections.Generic; - using System.Linq; - using System.Threading.Tasks; - using NServiceBus.AcceptanceTesting; - using NServiceBus.CustomChecks; - using NUnit.Framework; - using ServiceBus.Management.Infrastructure.Settings; - using ServiceControl.Contracts.CustomChecks; - - // Some checks (e.g. DeadLetterQueueCheck) perform work in their constructor, so they cannot be instantiated - // with null constructor arguments. Those are skipped here; they are already covered directly by the registry - // unit tests (Every_shipped_check_is_internal) and by the API acceptance assertions. - [TestFixture] - class When_internal_check_registry_covers_all_shipped_checks : AcceptanceTest - { - [Test] - public async Task Every_discovered_product_check_is_classified() - { - var missing = new List(); - var scanned = false; - - await Define() - .Done(_ => - { - if (scanned) - { - return true; - } - - scanned = true; - - var settings = (object)new Settings(); - - var productChecks = - from assembly in AppDomain.CurrentDomain.GetAssemblies() - let name = assembly.GetName().Name - where name != null - && name.StartsWith("ServiceControl") - && !name.Contains("Test") - && !name.Contains("Acceptance") - from type in assembly.GetTypes() - where type is { IsAbstract: false, IsInterface: false } - && typeof(ICustomCheck).IsAssignableFrom(type) - select type; - - foreach (var type in productChecks) - { - string id; - try - { - var constructor = type.GetConstructors().Single(); - var args = constructor.GetParameters() - .Select(p => p.ParameterType == typeof(Settings) ? settings : null) - .ToArray(); - var instance = (ICustomCheck)constructor.Invoke(args); - id = instance.Id; - } - catch (Exception) - { - // Constructor does work (e.g. DeadLetterQueueCheck dereferences its settings); - // cannot be classified by reflection. It is covered by the registry unit tests instead. - continue; - } - - if (!InternalCustomCheckClassification.IsInternal(id)) - { - missing.Add(id); - } - } - - return true; - }) - .Run(); - - Assert.That(missing, Is.Empty, - "Every check ServiceControl ships must be in the internal registry. " + - "If you added a check, add it to InternalCustomCheckClassification. Missing: " + - string.Join(", ", missing)); - } - - class Context : ScenarioContext; - } -} \ No newline at end of file diff --git a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs index 11bdfdaf32..39e8c0350f 100644 --- a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs +++ b/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs @@ -14,7 +14,7 @@ class AuditCustomCheckApprovals // 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 multi-instance acceptance test instead + // plugins not referenced here, so they are covered by the persistence approval tests instead [Test] public void Audit_check_ids_are_snapshot() { diff --git a/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs b/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs deleted file mode 100644 index 13f4253aad..0000000000 --- a/src/ServiceControl.MultiInstance.AcceptanceTests/Monitoring/When_audit_internal_checks_are_classified.cs +++ /dev/null @@ -1,55 +0,0 @@ -namespace ServiceControl.MultiInstance.AcceptanceTests.Monitoring -{ - using System.Collections.Generic; - using System.Linq; - using System.Threading.Tasks; - using AcceptanceTesting; - using NServiceBus.AcceptanceTesting; - using NUnit.Framework; - using TestSupport; - using CustomCheckView = global::ServiceControl.Contracts.CustomChecks.CustomCheckView; - - // Primary + audit instances. The audit forwards its checks to the primary as - // ReportCustomCheckResult messages, so this is the test that proves the primary's - // hard-coded audit IDs (string literals — the audit assembly is not referenced) stay correct. - [TestFixture] - class When_audit_internal_checks_are_classified : AcceptanceTest - { - [Test] - public async Task Audit_checks_arriving_as_messages_are_flagged_internal() - { - var expectedIds = new[] - { - "Audit Message Ingestion Process", - "Audit Message Ingestion", - }; - - var seen = new List(); - - await Define() - .Done(async c => - { - var checks = await this.TryGetMany("/api/customchecks", instanceName: ServiceControlInstanceName); - 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(id => seen.Any(s => s.CustomCheckId == id)); - }) - .Run(); - - foreach (var id in expectedIds) - { - var check = seen.Single(s => s.CustomCheckId == id); - Assert.That(check.Internal, Is.True, id); - } - } - - class Context : ScenarioContext; - } -} \ No newline at end of file diff --git a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs index 4564e1161a..c4ebcca638 100644 --- a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs +++ b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs @@ -13,7 +13,7 @@ namespace ServiceControl.Contracts.CustomChecks /// 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 - /// acceptance tests that enforce this. + /// approval tests that enforce this. /// public static class InternalCustomCheckClassification { From f3185380b4c80dfb92609041fd440119739b6c49 Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Wed, 2 Sep 2026 12:34:51 +0800 Subject: [PATCH 6/7] explicitly enable nullable on shared file --- .../InternalCustomCheckClassification.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs index c4ebcca638..4a7fc96e7f 100644 --- a/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs +++ b/src/ServiceControl.Persistence/InternalCustomCheckClassification.cs @@ -1,3 +1,6 @@ +//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; From 72d6f3c12b8cab084ed0f9eb059f2eb9d542decf Mon Sep 17 00:00:00 2001 From: Rhys Bevilaqua Date: Wed, 2 Sep 2026 14:03:20 +0800 Subject: [PATCH 7/7] tidy up approval tests --- .../DiagPath.cs | 45 ------------------- ...ChecksTest.VerifyCustomChecks.approved.txt | 2 + .../ServiceControl.Audit.UnitTests.csproj | 3 ++ .../CustomChecksTest.cs} | 8 ++-- ...hecksTest.VerifyCustomChecks.approved.txt} | 0 ...gistryApprovals.cs => CustomChecksTest.cs} | 4 +- 6 files changed, 12 insertions(+), 50 deletions(-) delete mode 100644 src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs create mode 100644 src/ServiceControl.Audit.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt rename src/ServiceControl.Audit.UnitTests/{API/AuditCustomCheckApprovals.cs => Verification/CustomChecksTest.cs} (83%) rename src/ServiceControl.UnitTests/ApprovalFiles/{InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt => CustomChecksTest.VerifyCustomChecks.approved.txt} (100%) rename src/ServiceControl.UnitTests/{API/InternalCustomCheckRegistryApprovals.cs => CustomChecksTest.cs} (91%) diff --git a/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs b/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs deleted file mode 100644 index 5eade0036f..0000000000 --- a/src/ServiceControl.AcceptanceTests.RavenDB/DiagPath.cs +++ /dev/null @@ -1,45 +0,0 @@ -namespace ServiceControl.AcceptanceTests.RavenDB -{ - using System; - using System.Diagnostics; - using NUnit.Framework; - - [TestFixture] - class DiagPath - { - static void Run(string label, string file, string args) - { - Console.WriteLine($"=== {label}: {file} {args} ==="); - var psi = new ProcessStartInfo(file, args) { RedirectStandardOutput = true, UseShellExecute = false, CreateNoWindow = true }; - try - { - using var p = Process.Start(psi)!; - var output = p.StandardOutput.ReadToEnd(); - p.WaitForExit(); - foreach (var line in output.Split('\n')) - { - if (line.Contains("NETCore.App") || line.Contains("runtimes installed") || line.Contains("Base Path")) - { - Console.WriteLine(" " + line.Trim()); - } - } - } - catch (Exception e) - { - Console.WriteLine(" FAILED: " + e.Message); - } - } - - [Test] - public void PrintPathAndDotnetInfo() - { - Console.WriteLine("DIAG PATH=" + Environment.GetEnvironmentVariable("PATH")); - Console.WriteLine("DIAG CWD=" + Environment.CurrentDirectory); - Run("PATH-dotnet", "dotnet", "--info"); - Run("tmp-wrap", "/tmp/wrap/dotnet", "--info"); - Run("tmp-dotnet8", "/tmp/dotnet8/dotnet", "--info"); - Run("home-dotnet", "/home/piuser/.dotnet/dotnet", "--info"); - Assert.Pass(); - } - } -} \ 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/API/AuditCustomCheckApprovals.cs b/src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs similarity index 83% rename from src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs rename to src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs index 39e8c0350f..18e824d544 100644 --- a/src/ServiceControl.Audit.UnitTests/API/AuditCustomCheckApprovals.cs +++ b/src/ServiceControl.Audit.UnitTests/Verification/CustomChecksTest.cs @@ -3,12 +3,13 @@ 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 AuditCustomCheckApprovals + 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 @@ -16,7 +17,7 @@ class AuditCustomCheckApprovals // 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 Audit_check_ids_are_snapshot() + public void VerifyCustomChecks() { var settings = (object)new Settings("LearningTransport", "InMemory"); @@ -29,8 +30,9 @@ from type in typeof(Settings).Assembly.GetTypes() .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}"; + select $"{instance.Category}: {instance.Id} => {(classified ? "internal" : "MISSING FROM REGISTRY")}"; Approver.Verify(string.Join(Environment.NewLine, discovered)); } diff --git a/src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt b/src/ServiceControl.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt similarity index 100% rename from src/ServiceControl.UnitTests/ApprovalFiles/InternalCustomCheckRegistryApprovals.Every_shipped_check_in_the_app_assembly_is_in_the_registry.approved.txt rename to src/ServiceControl.UnitTests/ApprovalFiles/CustomChecksTest.VerifyCustomChecks.approved.txt diff --git a/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs b/src/ServiceControl.UnitTests/CustomChecksTest.cs similarity index 91% rename from src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs rename to src/ServiceControl.UnitTests/CustomChecksTest.cs index 36e7efc4aa..87bfbf7db2 100644 --- a/src/ServiceControl.UnitTests/API/InternalCustomCheckRegistryApprovals.cs +++ b/src/ServiceControl.UnitTests/CustomChecksTest.cs @@ -9,10 +9,10 @@ namespace ServiceControl.UnitTests.API using ServiceControl.Contracts.CustomChecks; [TestFixture] - class InternalCustomCheckRegistryApprovals + class CustomChecksTest { [Test] - public void Every_shipped_check_in_the_app_assembly_is_in_the_registry() + public void VerifyCustomChecks() { var settings = (object)new Settings();