CosmosDbHealthCheck - consider registration overloads that accept a CosmosClient instance

[danielmpetrov]

Hi. I appreciate the library and your effort. 👍

What would you like to be added:

I would like to see an overload for CosmosDb health checks that accepts a CosmosClient instance.

Why is this needed:

According to the official Cosmos SDK v3 documentation (see https://docs.microsoft.com/en-us/azure/cosmos-db/sql/performance-tips-dotnet-sdk-v3-sql#sdk-usage), the CosmosClient was designed to be singleton for optimal performance. However, the library does not expose a dependency injection overload that accepts a client, but instead internally creates and maintains a new client for each health check - https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/blob/master/src/HealthChecks.CosmosDb/CosmosDbHealthCheck.cs#L58.

This can cause a frustrating developer experience when reading cosmos diagnostic logs and seeing more client instances than expected.

[sungam3r]

instead internally creates and maintains a new client for each health check

Not true. Clients are cached in static dictionary by connection.

[danielmpetrov]

You're right, it's a new client per connection. The main point still stands, that is an additional client per connection, which is not ideal. Would you be open to a pull request?

[sungam3r]

I don't understand yet how it can be client not per connection if CosmosClient constructor accepts connectionString argument 😕 .

[danielmpetrov]

It cannot be, but the app connecting to cosmos db must create a client for business operations, saving, updating, deleting records, etc. The health check then also creates a client, to ping the database. The end result in two clients in the same application for the same connection string.

[sungam3r]

Understood.

I would like to see an overload for CosmosDb health checks that accepts a CosmosClient instance.

Sure. PR is welcome.

[danielmpetrov]

Good news, #1405 has added the exact API that we needed. 🎉

Thank you @wsugarman for the contribution. 👍

[wsugarman]

@danielmpetrov Haha! Happy to (inadvertently) help!

[danielmpetrov]

It's a really good change, but registration methods that directly accept a client will still be beneficial in cases where we have apps that reach out to multiple cosmos accounts.

Code sample:

var client1 = new CosmosClient(connection1);
var client2 = new CosmosClient(connection2);

// ...

services
    .AddHealthChecks()
    .AddCosmosDb(client1, name: "sample 1")
    .AddCosmosDb(client2, name: "sample 2");

In this case we don't directly register the clients in the container because they share a type, so the overloads from #1405 won't work. However, that PR sets this change to be a very simple one, since the health check now has the appropriate constructor overloads.

I could add the registration methods if this still makes sense to everyone. I think this will give the user the ultimate flexibility.

[sungam3r]

PR is welcome.