Remove private static cache from MongoDbHealthCheck (#2334)

* remove the static IMongoClient cache, force the users to follow the best practices (singleton), fixes #2148 * make the field lazy, so it's initialized the first time it's needed (and it's required only when db name was provided) * When running the tests locally during development, don't re-attempt as it prolongs the time it takes to run the tests. * add README * fix a link in Azure doc
Xabaril · Dec 6, 2024 · b21485e · b21485e
1 parent 24626cd
commit b21485e
Show file tree

Hide file tree

Showing 7 changed files with 166 additions and 290 deletions.
diff --git a/src/HealthChecks.Azure.Storage.Blobs/README.md b/src/HealthChecks.Azure.Storage.Blobs/README.md
@@ -39,5 +39,5 @@ void Configure(IHealthChecksBuilder builder)
 
 ### Breaking changes
 
-In the prior releases, `AzureBlobStorageHealthCheck` was a part of `HealthChecks.AzureStorage` package. It had a dependency on not just `Azure.Storage.Blobs`, but also `Azure.Storage.Queues` and `Azure.Storage.Files.Shares`. The packages have been split to avoid bringing unnecessary dependencies. Moreover, `AzureBlobStorageHealthCheck` was letting the users specify how `BlobServiceClient` should be created (from raw connection string or from endpoint uri and managed identity credentials), at a cost of maintaining an internal, static client instances cache. Now the type does not create client instances nor maintain an internal cache and **it's the caller responsibility to provide the instance of `BlobServiceClient`** (please see [#2040](https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2040) for more details). Since Azure SDK recommends treating clients as singletons <see href="https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/"/> and client instances can be expensive to create, it's recommended to register a singleton factory method for Azure SDK clients. So the clients are created only when needed and once per whole application lifetime.
+In the prior releases, `AzureBlobStorageHealthCheck` was a part of `HealthChecks.AzureStorage` package. It had a dependency on not just `Azure.Storage.Blobs`, but also `Azure.Storage.Queues` and `Azure.Storage.Files.Shares`. The packages have been split to avoid bringing unnecessary dependencies. Moreover, `AzureBlobStorageHealthCheck` was letting the users specify how `BlobServiceClient` should be created (from raw connection string or from endpoint uri and managed identity credentials), at a cost of maintaining an internal, static client instances cache. Now the type does not create client instances nor maintain an internal cache and **it's the caller responsibility to provide the instance of `BlobServiceClient`** (please see [#2040](https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2040) for more details). Since Azure SDK [recommends](https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients) treating clients as singletons and client instances can be expensive to create, it's recommended to register a singleton factory method for Azure SDK clients. So the clients are created only when needed and once per whole application lifetime.
 
diff --git a/src/HealthChecks.MongoDb/DependencyInjection/MongoDbHealthCheckBuilderExtensions.cs b/src/HealthChecks.MongoDb/DependencyInjection/MongoDbHealthCheckBuilderExtensions.cs
@@ -12,40 +12,14 @@ public static class MongoDbHealthCheckBuilderExtensions
     private const string NAME = "mongodb";
 
     /// <summary>
-    /// Add a health check for MongoDb database that list all databases on the system.
+    /// Add a health check for MongoDb that list all databases from specified <paramref name="clientFactory"/> or pings the database returned by <paramref name="databaseNameFactory"/>.
     /// </summary>
     /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongodbConnectionString">The MongoDb connection string to be used.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
+    /// <param name="clientFactory">
+    /// An optional factory to obtain <see cref="IMongoClient" /> instance.
+    /// When not provided, <see cref="MongoClient" /> or <see cref="IMongoClient" /> is simply resolved from <see cref="IServiceProvider"/>.
     /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        string mongodbConnectionString,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongodbConnectionString),
-            failureStatus,
-            tags,
-            timeout));
-    }
-
-    /// <summary>
-    /// Add a health check for MongoDb database that list all collections from specified database on <paramref name="mongoDatabaseName"/>.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongodbConnectionString">The MongoDb connection string to be used.</param>
-    /// <param name="mongoDatabaseName">The Database name to check.</param>
+    /// <param name="databaseNameFactory">An optional factory to obtain the name of the database to ping.</param>
     /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
     /// <param name="failureStatus">
     /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
@@ -56,56 +30,35 @@ public static IHealthChecksBuilder AddMongoDb(
     /// <returns>The specified <paramref name="builder"/>.</returns>
     public static IHealthChecksBuilder AddMongoDb(
         this IHealthChecksBuilder builder,
-        string mongodbConnectionString,
-        string mongoDatabaseName,
+        Func<IServiceProvider, IMongoClient>? clientFactory = default,
+        Func<IServiceProvider, string>? databaseNameFactory = default,
         string? name = default,
         HealthStatus? failureStatus = default,
         IEnumerable<string>? tags = default,
         TimeSpan? timeout = default)
     {
         return builder.Add(new HealthCheckRegistration(
             name ?? NAME,
-            sp => new MongoDbHealthCheck(mongodbConnectionString, mongoDatabaseName),
+            sp => Factory(sp, clientFactory, databaseNameFactory),
             failureStatus,
             tags,
             timeout));
-    }
 
-    /// <summary>
-    /// Add a health check for MongoDb database that list all databases on the system.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongodbConnectionStringFactory">A factory to build MongoDb connection string to use.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
-    /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        Func<IServiceProvider, string> mongodbConnectionStringFactory,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongodbConnectionStringFactory(sp)),
-            failureStatus,
-            tags,
-            timeout));
+        static MongoDbHealthCheck Factory(IServiceProvider sp, Func<IServiceProvider, IMongoClient>? clientFactory, Func<IServiceProvider, string>? databaseNameFactory)
+        {
+            // The user might have registered a factory for MongoClient type, but not for the abstraction (IMongoClient).
+            // That is why we try to resolve MongoClient first.
+            IMongoClient client = clientFactory?.Invoke(sp) ?? sp.GetService<MongoClient>() ?? sp.GetRequiredService<IMongoClient>();
+            string? databaseName = databaseNameFactory?.Invoke(sp);
+            return new(client, databaseName);
+        }
     }
 
     /// <summary>
-    /// Add a health check for MongoDb database that list all collections from specified database on <paramref name="mongoDatabaseName"/>.
+    /// Add a health check for MongoDb that pings the database.
     /// </summary>
     /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongodbConnectionStringFactory">A factory to build MongoDb connection string to use.</param>
-    /// <param name="mongoDatabaseName">The Database name to check.</param>
+    /// <param name="dbFactory">A factory to obtain <see cref="IMongoDatabase" /> instance.</param>
     /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
     /// <param name="failureStatus">
     /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
@@ -116,136 +69,21 @@ public static IHealthChecksBuilder AddMongoDb(
     /// <returns>The specified <paramref name="builder"/>.</returns>
     public static IHealthChecksBuilder AddMongoDb(
         this IHealthChecksBuilder builder,
-        Func<IServiceProvider, string> mongodbConnectionStringFactory,
-        string mongoDatabaseName,
+        Func<IServiceProvider, IMongoDatabase> dbFactory,
         string? name = default,
         HealthStatus? failureStatus = default,
         IEnumerable<string>? tags = default,
         TimeSpan? timeout = default)
     {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongodbConnectionStringFactory(sp), mongoDatabaseName),
-            failureStatus,
-            tags,
-            timeout));
-    }
+        Guard.ThrowIfNull(dbFactory);
 
-    /// <summary>
-    /// Add a health check for MongoDb that list all databases from specified <paramref name="mongoClientSettings"/>.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongoClientSettings">The MongoClientSettings to be used.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
-    /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        MongoClientSettings mongoClientSettings,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongoClientSettings),
-            failureStatus,
-            tags,
-            timeout));
-    }
-
-    /// <summary>
-    /// Add a health check for MongoDb database that list all collections from specified database on <paramref name="mongoDatabaseName"/>.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongoClientSettings">The MongoClientSettings to be used.</param>
-    /// <param name="mongoDatabaseName">The Database name to check.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
-    /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        MongoClientSettings mongoClientSettings,
-        string mongoDatabaseName,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongoClientSettings, mongoDatabaseName),
-            failureStatus,
-            tags,
-            timeout));
-    }
-
-    /// <summary>
-    /// Add a health check for MongoDb that list all databases from specified <paramref name="mongoClientFactory"/>.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongoClientFactory">A factory to build MongoClient to be used.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
-    /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        Func<IServiceProvider, IMongoClient> mongoClientFactory,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
-        return builder.Add(new HealthCheckRegistration(
-            name ?? NAME,
-            sp => new MongoDbHealthCheck(mongoClientFactory(sp)),
-            failureStatus,
-            tags,
-            timeout));
-    }
-
-    /// <summary>
-    /// Add a health check for MongoDb database that list all collections from specified database on <paramref name="mongoDatabaseName"/>.
-    /// </summary>
-    /// <param name="builder">The <see cref="IHealthChecksBuilder"/>.</param>
-    /// <param name="mongoClientFactory">A factory to build MongoClient to be used.</param>
-    /// <param name="mongoDatabaseName">The name of the database to check.</param>
-    /// <param name="name">The health check name. Optional. If <c>null</c> the type name 'mongodb' will be used for the name.</param>
-    /// <param name="failureStatus">
-    /// The <see cref="HealthStatus"/> that should be reported when the health check fails. Optional. If <c>null</c> then
-    /// the default status of <see cref="HealthStatus.Unhealthy"/> will be reported.
-    /// </param>
-    /// <param name="tags">A list of tags that can be used to filter sets of health checks. Optional.</param>
-    /// <param name="timeout">An optional <see cref="TimeSpan"/> representing the timeout of the check.</param>
-    /// <returns>The specified <paramref name="builder"/>.</returns>
-    public static IHealthChecksBuilder AddMongoDb(
-        this IHealthChecksBuilder builder,
-        Func<IServiceProvider, IMongoClient> mongoClientFactory,
-        string mongoDatabaseName,
-        string? name = default,
-        HealthStatus? failureStatus = default,
-        IEnumerable<string>? tags = default,
-        TimeSpan? timeout = default)
-    {
         return builder.Add(new HealthCheckRegistration(
             name ?? NAME,
-            sp => new MongoDbHealthCheck(mongoClientFactory(sp), mongoDatabaseName),
+            sp =>
+            {
+                IMongoDatabase db = dbFactory.Invoke(sp);
+                return new MongoDbHealthCheck(db.Client, db.DatabaseNamespace.DatabaseName);
+            },
             failureStatus,
             tags,
             timeout));
Original file line number	Diff line number	Diff line change
Expand Up		@@ -39,5 +39,5 @@ void Configure(IHealthChecksBuilder builder)

		### Breaking changes

		In the prior releases, `AzureBlobStorageHealthCheck` was a part of `HealthChecks.AzureStorage` package. It had a dependency on not just `Azure.Storage.Blobs`, but also `Azure.Storage.Queues` and `Azure.Storage.Files.Shares`. The packages have been split to avoid bringing unnecessary dependencies. Moreover, `AzureBlobStorageHealthCheck` was letting the users specify how `BlobServiceClient` should be created (from raw connection string or from endpoint uri and managed identity credentials), at a cost of maintaining an internal, static client instances cache. Now the type does not create client instances nor maintain an internal cache and it's the caller responsibility to provide the instance of `BlobServiceClient` (please see [#2040](https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2040) for more details). Since Azure SDK recommends treating clients as singletons <see href="https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients/"/> and client instances can be expensive to create, it's recommended to register a singleton factory method for Azure SDK clients. So the clients are created only when needed and once per whole application lifetime.
		In the prior releases, `AzureBlobStorageHealthCheck` was a part of `HealthChecks.AzureStorage` package. It had a dependency on not just `Azure.Storage.Blobs`, but also `Azure.Storage.Queues` and `Azure.Storage.Files.Shares`. The packages have been split to avoid bringing unnecessary dependencies. Moreover, `AzureBlobStorageHealthCheck` was letting the users specify how `BlobServiceClient` should be created (from raw connection string or from endpoint uri and managed identity credentials), at a cost of maintaining an internal, static client instances cache. Now the type does not create client instances nor maintain an internal cache and it's the caller responsibility to provide the instance of `BlobServiceClient` (please see [#2040](https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/2040) for more details). Since Azure SDK [recommends](https://devblogs.microsoft.com/azure-sdk/lifetime-management-and-thread-safety-guarantees-of-azure-sdk-net-clients) treating clients as singletons and client instances can be expensive to create, it's recommended to register a singleton factory method for Azure SDK clients. So the clients are created only when needed and once per whole application lifetime.