Skip to content

Enrich internal custom checks with flag showing them as such - #5840

Open
rbev wants to merge 5 commits into
masterfrom
rhys/flag-builtin-customchecks
Open

Enrich internal custom checks with flag showing them as such#5840
rbev wants to merge 5 commits into
masterfrom
rhys/flag-builtin-customchecks

Conversation

@rbev

@rbev rbev commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

This pull request introduces several improvements and new tests to the handling, classification, and verification of custom checks, particularly focusing on the distinction between internal and endpoint-originated checks. It also adds approval and diagnostic tests to ensure the correct registration and reporting of custom checks across both primary and audit instances, and makes minor code and test project adjustments to support these changes.

Custom Check Classification and API Verification:

  • Added new acceptance tests to verify that internal custom checks are correctly classified and reported via the API, and that endpoint-originated checks are not marked as internal. Tests also verify the presence and correct wire-format of the internal flag for both types of checks (When_custom_checks_are_classified.cs, When_a_persister_check_fails.cs, When_the_body_storage_check_is_reported.cs). [1] [2] [3]
  • Updated documentation to clarify the versioning and caching rules around computed fields like Internal in CustomCheckView, and how these are handled in the API and by clients (docs/data-versioning-design.md).

Test Infrastructure and Approval Testing:

  • Added approval tests in the audit test suite to snapshot and verify the set of audit custom check IDs, ensuring that any addition of new custom checks is accompanied by the necessary classification in the primary instance (AuditCustomCheckApprovals.cs, AuditCustomCheckApprovals.Audit_check_ids_are_snapshot.approved.txt). [1] [2]
  • Included the InternalCustomCheckClassification code in audit and persistence test projects to enable these approval tests without requiring expensive acceptance tests (ServiceControl.Audit.Persistence.Tests.RavenDB.csproj, ServiceControl.Audit.Persistence.Tests.csproj). [1] [2]

Code Consistency and Cleanup:

  • Updated references and using statements in custom check tests to consistently use the correct CustomCheckView and related types, improving clarity and maintainability (When_a_failing_custom_check_is_dismissed.cs, When_email_notifications_are_configured.cs, CustomCheckTests.cs). [1] [2] [3]
  • Enhanced the audit persistence custom check test to flag any custom checks missing from the internal classification, making omissions visible in test output (CustomCheckTests.cs).

Test Project Adjustments:

  • Removed tests from the RavenDB acceptance test project that are not relevant for RavenDB (e.g., file system body storage checks, which are EF Core-specific) (ServiceControl.AcceptanceTests.RavenDB.csproj).
  • Added a diagnostic test to print environment and .NET runtime information for troubleshooting in RavenDB acceptance tests (DiagPath.cs).

Persistence Layer Update:

  • Changed the EF Core custom check data store to return CustomCheckView objects instead of CustomCheck, aligning the API response with new requirements (CustomCheckDataStore.cs). [1] [2]

@rbev
rbev force-pushed the rhys/flag-builtin-customchecks branch from 9a89855 to 1e06c53 Compare September 1, 2026 05:16
@rbev
rbev requested review from andreasohlund and jpalac September 1, 2026 05:33
@rbev
rbev marked this pull request as ready for review September 1, 2026 05:33
[Route("customchecks")]
[HttpGet]
public async Task<IList<CustomCheck>> CustomChecks([FromQuery] PagingInfo pagingInfo, string status = null, CancellationToken cancellationToken = default)
public async Task<IList<CustomCheckView>> CustomChecks([FromQuery] PagingInfo pagingInfo, string status = null, CancellationToken cancellationToken = default)

@jpalac jpalac Sep 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to verify that this isn't going to be a problem if the SP and SC versions don't match. ie SC is updated but SP is not or SP is updated but SC is not.

Also, do we do scatter gather on this, in which case if some instances have this updated API call and others do not - will that cause an issue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new fields are boolean and nullable, so an old server will emit the custom checks as Internal = default(bool) to a new client which was the graceful degradation we spoke about.

This api is not present on Audit instances, they emit custom checks via events over the transport so no issues with scatter gather.

@rbev rbev changed the title Enrich internal custom checks with severity Enrich internal custom checks with flag showing them as such Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants