Azure health checks: How to steer the users towards best practices

[adamsitnik]

Azure SDK Clients are recommended to be used as singletons.

From https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/:

The main rule of Azure SDK client lifetime management is: treat clients as singletons.

Most of the existing Azure Health Checks allow the user to specify a client instance. Example:

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.AzureStorage/AzureBlobStorageHealthCheck.cs

Line 24 in 7060bdd

public AzureBlobStorageHealthCheck(BlobServiceClient blobServiceClient, AzureBlobStorageHealthCheckOptions options)

But they also provide constructors that take the arguments required to create such client instance. Example:

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.AzureStorage/AzureBlobStorageHealthCheck.cs

Line 12 in 7060bdd

public AzureBlobStorageHealthCheck(string connectionString, string? containerName = default, BlobClientOptions? clientOptions = default)

In such case, they are adding them to a static cache:

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.AzureStorage/AzureBlobStorageHealthCheck.cs

Line 14 in 7060bdd

ClientCache.GetOrAdd(connectionString, k => new BlobServiceClient(k, clientOptions)),

I believe that it leads to having at least two instances of the client created:

• one used by the app outside of the health chekc
• one used by the health check.

This is bad for perf, but also in some rare edge cases can potentially lead into a situation when the client used by the app is disconnected, but the client used by the HC is connected and HC returns "Healthy".

Moreover, caching is hard and having a custom cache can always lead into bugs that are hard to diagnose. And increased complexity that needs to be maintained.

I would like to propose following changes for health checks that use clients that should be used as singletons:

• each HC should expose only one constructor that takes an existing instance of the client (this forces the user to create them, takes complexity away from us).

Sample:

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.Azure.KeyVault.Secrets/AzureKeyVaultSecretsHealthCheck.cs

Line 31 in 7060bdd

public AzureKeyVaultSecretsHealthCheck(SecretClient secretClient, AzureKeyVaultSecretOptions options)

• removal of the static cache (less complexity, better size on disk, smaller chance for bugs)
• each HC should expose only one DI extension method. The method should by default resolve an instance of the client from IServiceProvider, but let the user override this behaviour.

Sample:

AspNetCore.Diagnostics.HealthChecks/src/HealthChecks.Azure.KeyVault.Secrets/DependencyInjection/AzureKeyVaultHealthChecksBuilderExtensions.cs

Lines 32 to 49 in 7060bdd

	public static IHealthChecksBuilder AddAzureKeyVaultSecrets(
	this IHealthChecksBuilder builder,
	Func<IServiceProvider, SecretClient>? secretClientFactory = default,
	Func<IServiceProvider, AzureKeyVaultSecretOptions>? optionsFactory = default,
	string? healthCheckName = default,
	HealthStatus? failureStatus = default,
	IEnumerable<string>? tags = default,
	TimeSpan? timeout = default)
	{
	return builder.Add(new HealthCheckRegistration(
	string.IsNullOrEmpty(healthCheckName) ? HEALTH_CHECK_NAME : healthCheckName!,
	sp => new AzureKeyVaultSecretsHealthCheck(
	secretClient: secretClientFactory?.Invoke(sp) ?? sp.GetRequiredService<SecretClient>(),
	options: optionsFactory?.Invoke(sp) ?? new AzureKeyVaultSecretOptions()),
	failureStatus,
	tags,
	timeout));
	}

This would let us steer the users towards best practices:

• the users are responsible for registering the client in the DI (using whatever ctor they choose)
• HC reuse the existing singleton instance

This should improve perf (only one instance per app) and complexity (very simple).
The downside is the introduction of breaking changes.

public void Configure(IHealthChecksBuilder builder)
{
    builder.Services.AddSingleton(sp => new SecretClient(new Uri("azure-key-vault-uri"), new DefaultAzureCredential()));
    builder
        .AddHealthChecks()
        .AddAzureKeyVaultSecrets(); // by default resolves SecretClient from the service provider
}

For those who need more than one instance per app, we would update to .NET 8 and let them user the keyed DI:

public void Configure(IHealthChecksBuilder builder)
{
    builder.Services
        .AddKeyedSingleton(serviceKey: "first", (serviceProvider, serviceKey) => new SecretClient(new Uri("azure-key-vault-uri-first"), new DefaultAzureCredential()))
        .AddKeyedSingleton(serviceKey: "second", (serviceProvider, serviceKey) => new SecretClient(new Uri("azure-key-vault-uri-second"), new DefaultAzureCredential()))
        .AddHealthChecks()
            .AddAzureKeyVaultSecrets(secretClientFactory: serviceProvider => serviceProvider.GetRequiredKeyedService<SecretClient>("first"), healthCheckName: "secrets_first")
            .AddAzureKeyVaultSecrets(secretClientFactory: serviceProvider => serviceProvider.GetRequiredKeyedService<SecretClient>("second"), healthCheckName: "secrets_second");
}

@unaizorrilla @sungam3r what is your opinion on that?

[adamsitnik]

This would fix #1371, #1567 and probably #1890 (I would need to take a deeper look)

[sungam3r]

I'm OK with proposed changes.