Introduce per-HealthCheckRegistration parameters

[francedot]

Introduce per-HealthCheckRegistration parameters

• [X] You've read the Contributor Guide and Code of Conduct.
• [X] You've included unit or integration tests for your change, where applicable.
• [X] You've included inline docs for your change, where applicable.
• [X] 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, periodicity and timeout options of the healthchecks are set at the HealthCheckPublisherOptions configuration level and shared across all HealthCheckRegistration.

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

If no Delay, Period or Timout is specified during the AddCheck(...), 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 HealthCheckRegistrationParameters
+ {
+    public HealthCheckRegistrationParameters(TimeSpan? timeout, TimeSpan? delay, TimeSpan? period, bool isEnabled);

+    public TimeSpan Timeout{ get; }
+    public TimeSpan Delay { get; }
+    public TimeSpan Period { get; }
+    public bool IsEnabled { get; set; }
+ }

namespace Microsoft.Extensions.Diagnostics.HealthChecks;

public sealed class HealthCheckRegistration
{
+    public HealthCheckRegistration(string name, IHealthCheck instance, HealthStatus? failureStatus, IEnumerable<string>? tags, HealthCheckRegistrationParameters? parameters);

+    public HealthCheckRegistrationParameters? Parameters { get; set; }
}

HealthChecksBuilderAddCheckExtensions.cs

namespace Microsoft.Extensions.DependencyInjection;

public static HealthChecksBuilderAddCheckExtensions
{
+    public static IHealthChecksBuilder AddIndividualCheck(this IHealthChecksBuilder builder, string name, Func<HealthCheckResult> check, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
+    public static IHealthChecksBuilder AddIndividualCheck<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>(this IHealthChecksBuilder builder, string name, HealthStatus? failureStatus = null, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
+    public static IHealthChecksBuilder AddTypeActivatedIndividualCheck<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>(this IHealthChecksBuilder builder, string name, HealthStatus? failureStatus, IEnumerable<string> tags, HealthCheckRegistrationParameters? parameters, params object[] args);
}

HealthChecksBuilderDelegateExtensions.cs

namespace Microsoft.Extensions.DependencyInjection;

public static HealthChecksBuilderDelegateExtensions
{
+    public static IHealthChecksBuilder AddIndividualCheck(this IHealthChecksBuilder builder, string name, Func<HealthCheckResult> check, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
+    public static IHealthChecksBuilder AddIndividualCheck(this IHealthChecksBuilder builder, string name, Func<CancellationToken, HealthCheckResult> check, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
+    public static IHealthChecksBuilder AddAsyncIndividualCheck(this IHealthChecksBuilder builder, string name, Func<Task<HealthCheckResult>> check, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
+    public static IHealthChecksBuilder AddAsyncIndividualCheck(this IHealthChecksBuilder builder, string name, Func<CancellationToken, Task<HealthCheckResult>> check, IEnumerable<string>? tags = null, HealthCheckRegistrationParameters? parameters);
}

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

builder.Services.AddHealthChecks()
    .AddIndividualCheck("SampleHealthCheck1", parameters: new(delay: TimeSpan.FromSeconds(40), period: TimeSpan.FromSeconds(30)))
    .AddIndividualCheck<SampleHealthCheck2>(parameters: new(delay: TimeSpan.FromSeconds(40), period: TimeSpan.FromSeconds(45)))
    .AddTypeActivatedIndividualCheck<SampleHealthCheckWithArgs>(
        "SampleHealthCheck3",
        failureStatus: HealthStatus.Degraded,
        tags: new[] { "SampleHealthCheck3" },
        args: new object[] { 1, "Arg" }
        parameters: new (delay: TimeSpan.FromSeconds(40)
                            period: TimeSpan.FromSeconds(75))
);

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

[francedot]

Renaming from AddIndividualCheck to AddCheck will cause an ambiguous reference as we will end up with two public APIs conflicting on the optional params. This is unless we don't want to update the PublicAPI.Shipped.txt with a unique overload e.g.:

-    public static IHealthChecksBuilder AddCheck(this IHealthChecksBuilder builder, string name, Func<HealthCheckResult> check, IEnumerable<string>? tags = null, TimeSpan? timeout = default);
+    public static IHealthChecksBuilder AddCheck(this IHealthChecksBuilder builder, string name, Func<HealthCheckResult> check, IEnumerable<string>? tags = null, TimeSpan? timeout = default, TimeSpan? delay = default, TimeSpan? period = default);

/cc @BrennanConroy @halter73

[francedot]

Summarizing the open points from this PR:

[francedot]

AddCheck Naming:

• Renaming of AddTypeActivatedCheck due to source breaking with params object[] tags: users might have passed a TimeSpan as params.
• Renaming of AddCheck, AddAsyncCheck due to ambiguous reference caused by same optional params (see above comment).
• Proposed naming: AddIndividualCheck, AddIsolatedCheck, AddServiceCheck (as they will happen in DefaultHealthCheckService)

[francedot]

Predicate Injection:

With the adopted design, individual HCs cannot use the predicate param passed in the CheckHealthAsync to filter out HCs that should not be run – individual HCs are initiated in the cctor of DefaultHealthCheckService instead and run isolated from the publisher. Therefore, for individual (non-default) HCs we are defaulting to the publisher Predicate.

For default HCs (the ones with no specified period and delay), these are run as part of the same CheckHealthAsync (following previous design) and we can still use the predicate passed as a parameter.

Is the current design acceptable?

[francedot]

Cancellation:

For the same reason above, cancellation parameter passed in the CheckHealthAsync has no effect on the individual HCs, since they run isolated from the publisher.

[francedot]

Default values for Period and Delay:

HealthCheckRegistration sets the default values for Delay and Period to Timeout.InfiniteTimeSpan and TimeSpan.Zero (default(TimeSpan)). DefaultHealthCheckService follows the same convention when deciding whether an HC is default or individual. Do we have better default values for these?

[francedot]

Migration to ValueTask:

Updating the AddAsyncIndividualCheck APIs to ValueTask will force us to update also its dependencies on DelegateHealthCheck with a cctor accepting Func<CancellationToken, ValueTask<HealthCheckResult>>. This would require some work to change the Public.API.Shipped file for all the async AddCheck, with subsequent breaking change for the users using Task today..

Thoughts?

[francedot]

AddCheck Naming:

• Renaming of AddTypeActivatedCheck due to source breaking with params object[] tags: users might have passed a TimeSpan as params.
• Renaming of AddCheck, AddAsyncCheck due to ambiguous reference caused by same optional params (see above comment).
• Proposed naming: AddIndividualCheck, AddIsolatedCheck, AddServiceCheck (as they will happen in DefaultHealthCheckService)

Not requiring anymore a new name for the API - this has been addressed as part of Introduce HealthCheckOptions.

Notes: requires changing the PublicAPI contracts for AddCheck adding an optional HealthCheckOptions.

[francedot]

Default values for Period and Delay:

HealthCheckRegistration sets the default values for Delay and Period to Timeout.InfiniteTimeSpan and TimeSpan.Zero (default(TimeSpan)). DefaultHealthCheckService follows the same convention when deciding whether an HC is default or individual. Do we have better default values for these?

Addressed as part of Introduce HealthCheckOptions.

Default values are now inferred from the nullable properties.

[BrennanConroy]

requires changing the PublicAPI contracts for AddCheck adding an optional HealthCheckOptions.

Why not add a new AddCheck overload that takes the new HealthCheckOptions type? Adding a new optional param to an existing method is likely binary breaking.

[francedot]

requires changing the PublicAPI contracts for AddCheck adding an optional HealthCheckOptions.

Why not add a new AddCheck overload that takes the new HealthCheckOptions type? Adding a new optional param to an existing method is likely binary breaking.

This is because we would end up with an ambiguous reference between the AddCheck(..) and AddCheck(.., tags?, options?)

[BrennanConroy]

This goes back to what we mentioned about dropping the defaults from the old overload.

consider an existing method MyMethod(int a = 1). If you introduce an overload of MyMethod with two optional parameters a and b, you can preserve compatibility by moving the default value of a to the new overload. Now the two overloads are MyMethod(int a) and MyMethod(int a = 1, int b = 2).

https://docs.microsoft.com/dotnet/core/compatibility/#properties-fields-parameters-and-return-values

[halter73]

What if instead of adding all these parameters to an AddCheck overload, we took HealthCheckPublisherOptions? That way if we add any more properties to these options, we don't need to add even more AddCheck overloads.

[francedot]

Migration to ValueTask:

Updating the AddAsyncIndividualCheck APIs to ValueTask will force us to update also its dependencies on DelegateHealthCheck with a cctor accepting Func<CancellationToken, ValueTask<HealthCheckResult>>. This would require some work to change the Public.API.Shipped file for all the async AddCheck, with subsequent breaking change for the users using Task today..

Thoughts?

Addressed by https://github.com/dotnet/aspnetcore/pull/42646/commits/0c464ca900a4e87e96dda112e912fd04c78436a8. AsTask conversion in DelegateHealthCheck required as we're relying on Task<HealthCheckResult> in IHealthCheck. Thoughts?

[francedot]

What if instead of adding all these parameters to an AddCheck overload, we took HealthCheckPublisherOptions? That way if we add any more properties to these options, we don't need to add even more AddCheck overloads.

My main point against it is that HealthCheckPublisherOptions also contains a Predicate, which is not meant to be a per-HC property (or at least I cannot think of any applications for it).

[francedot]

Cancellation:

For the same reason above, cancellation parameter passed in the CheckHealthAsync has no effect on the individual HCs, since they run isolated from the publisher.

Should have been addressed by https://github.com/dotnet/aspnetcore/pull/42646/commits/578398cda623a7649d00ee4a16c264042775041c. We are now propagating cancellation starting from the PublisherHostedService.

[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 remove the pending ci rerun label to kick off a new CI run.

[ghost]

/azp run

[azure-pipelines[bot]]

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

[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 remove the pending ci rerun label to kick off a new CI run.

[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[bot]]

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

[Tratcher]

/azp run

[azure-pipelines[bot]]

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