Introduce per-HealthCheckRegistration parameters

[francedot]

Introduce per-HealthCheckRegistration parameters

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Change the delay and periodicity on a per-Healthcheck basis. Currently all health checks share the same delay and period.

Description

In the current implementation, delay and periodicity of the healthchecks are set at the HealthCheckPublisherOptions configuration level and shared across all HealthCheckRegistration.

This PR introduces per-HealthCheck delay, period (namely Individual Healthchecks) by extending the HealthCheckRegistration and HealthCheckPublisherHostedService`.

If no Delay or Period is specified during the IHealthChecksBuilder.Add(HealthCheckRegistration registration), default values from the HealthCheckPublisherOptions will be used instead.

The HealthCheck API layer will change for:

HealthCheckRegistration.cs

namespace Microsoft.Extensions.Diagnostics.HealthChecks;

public sealed class HealthCheckRegistration
{
     public HealthCheckRegistration(string name, IHealthCheck instance, HealthStatus? failureStatus, IEnumerable<string>? tags, TimeSpan? timeout);
     public HealthCheckRegistration(string name, Func<IServiceProvider, IHealthCheck> factory, HealthStatus? failureStatus, IEnumerable<string>? tags, TimeSpan? timeout);

+    public TimeSpan? Delay { get; set; }
+    public TimeSpan? Period { get; set; }
}

Here is an example on how to use the modified HealthChecksBuilder:

builder.Services.AddHealthChecks()
   .Add(new HealthCheckRegistration(
               name: "SampleHealthCheck1",
               instance: new DelegateHealthCheck(_ => Task.FromResult(HealthCheckResult.Healthy(HealthyMessage))),
               failureStatus: null,
               tags: null,
               timeout: default)
          {
               Delay = TimeSpan.FromSeconds(40),
               Period = TimeSpan.FromSeconds(30)
          });

Fixes #42645

[ghost]

Thanks for your PR, @francedot. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[halter73]

The API added here doesn't exactly match the proposed API in #42645. Let's line these up and get it approved before merging.

[francedot]

Proposed API change lined up for AddCheck, AddAsyncCheck, AddCheck<T>, AddTypeActivatedCheck<T> in HealthChecksBuilderDelegateExtensions.cs and HealthChecksBuilderAddCheckExtensions.cs

[halter73]

This looks really good. My only remaining comments are pretty minor test-related stuff.

[halter73]

Does this mean the test takes at least 9 seconds? 😨

Ideally, we'd use a mockable timer for this, but since that'd be a lot of work that wasn't done before either, I think we can settle for making it reasonably fast.

Can we divide all of the delays by a factor of 10? Same for both tests. That should shave 16 seconds of these tests.

[francedot]

@halter73 - I have updated the tests to block on an HC with delay 2s. With delays / 10 I am ending up with non-deterministic tests (at least locally).

[halter73]

How was it failing with all of the delays divided by a factor of 10? I tried it on my machine before suggesting it, and it seemed to consistently pass. I didn't change any of the periods.

Maybe my machine is just faster or something, but I feel like we could reduce all the delays by a factor of 10 and then just delay an extra second before checking the report entries, it should pass almost always. The periods are sufficiently large that we shouldn't get any repeats either.

I would love for these to be really deterministic though. So it'd even pass given a starved scheduler and whatnot. If you're feeling ambitious, you could kick the tires on the new TimeProvider API. It was basically made for exactly this use case, so you wouldn't have to go and make your own custom abstraction. It was just merged 4 days ago, so it's not available quite yet in the aspnetcore repo, but it should arrive in the next SDK update which usually happens Monday. It's already available in the latest installer.

We should also verify exactly what report entries were recorded and which weren't especially in the predicate tests. It's not enough to do a couple contains checks when part of what we need to verify is we're filtering health checks out.

[halter73]

Given that we're tight on time, and using TimeProvider will also involve our first reference to https://www.nuget.org/packages/Microsoft.Bcl.TimeProvider as a package, I filed #49715 as a follow up. These kind of target-specific dependencies have caused issues in the recent past and we're trying to reduce risk.

HealthChecks previously used Timer and was not updated to use TimeProvider like auth or Kestrel, so this is not a regression. @Tratcher

[halter73]

We should also shorten the delays by a factor of 10 in this test and check the entries after the finally when we know the timers are stopped.

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

[francedot]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 42646 in repo dotnet/aspnetcore

[Tratcher]

/azp run

[azure-pipelines]

No commit pushedDate could be found for PR 42646 in repo dotnet/aspnetcore

[halter73]

@francedot Thank you for continuing to push on this. This looks great! I filed a follow up issue to use TimeProvider. Hopefully it's resilient to potentially slow CI machines 🤞 😄