From 4cc62fb7191a54bcd9a5ed231b2970819709ed37 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 15 Dec 2023 12:11:11 +0100 Subject: [PATCH] Improve Npgsql health check design (#2116) - offer two public NpgSqlHealthCheckOptions ctors: one that requires ConnectionString and another that requires DataSource. This allows us to ensure that every instance of this type is valid. - make DataSource property public, but readonly. This allows us to ensure that the customers are not providing both. - add README.md for Npgsql health check and explain what we recommend and why --- .../NpgSqlHealthCheckBuilderExtensions.cs | 86 ++++++++----------- src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs | 9 +- .../NpgSqlHealthCheckOptions.cs | 55 ++++++++++-- src/HealthChecks.NpgSql/README.md | 47 ++++++++++ .../DependencyInjection/RegistrationTests.cs | 40 ++------- .../HealthChecks.NpgSql.approved.txt | 9 +- .../HealthChecks.Npgsql.Tests.csproj | 4 + 7 files changed, 152 insertions(+), 98 deletions(-) create mode 100644 src/HealthChecks.NpgSql/README.md diff --git a/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs b/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs index fb0dd34653..2cc12cf563 100644 --- a/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs +++ b/src/HealthChecks.NpgSql/DependencyInjection/NpgSqlHealthCheckBuilderExtensions.cs @@ -37,7 +37,13 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - return builder.AddNpgSql(_ => connectionString, healthQuery, configure, name, failureStatus, tags, timeout); + Guard.ThrowIfNull(connectionString, throwOnEmptyString: true); + + return builder.AddNpgSql(new NpgSqlHealthCheckOptions(connectionString) + { + CommandText = healthQuery, + Configure = configure + }, name, failureStatus, tags, timeout); } /// @@ -65,14 +71,34 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - return builder.AddNpgSql(sp => new NpgsqlDataSourceBuilder(connectionStringFactory(sp)).Build(), healthQuery, configure, name, failureStatus, tags, timeout); + // This instance is captured in lambda closure, so it can be reused (perf) + NpgSqlHealthCheckOptions options = new() + { + CommandText = healthQuery, + Configure = configure, + }; + + return builder.Add(new HealthCheckRegistration( + name ?? NAME, + sp => + { + options.ConnectionString ??= Guard.ThrowIfNull(connectionStringFactory.Invoke(sp), throwOnEmptyString: true, paramName: nameof(connectionStringFactory)); + + return new NpgSqlHealthCheck(options); + }, + failureStatus, + tags, + timeout)); } /// /// Add a health check for Postgres databases. /// /// The . - /// A factory to build the NpgsqlDataSource to use. + /// + /// An optional factory to obtain instance. + /// When not provided, is simply resolved from . + /// /// The query to be used in check. /// An optional action to allow additional Npgsql specific configuration. /// The health check name. Optional. If null the type name 'npgsql' will be used for the name. @@ -83,9 +109,13 @@ public static IHealthChecksBuilder AddNpgSql( /// A list of tags that can be used to filter sets of health checks. Optional. /// An optional representing the timeout of the check. /// The specified . + /// + /// Depending on how the was configured, the connections it hands out may be pooled. + /// That is why it should be the exact same that is used by other parts of your app. + /// public static IHealthChecksBuilder AddNpgSql( this IHealthChecksBuilder builder, - Func dbDataSourceFactory, + Func? dbDataSourceFactory = null, string healthQuery = HEALTH_QUERY, Action? configure = null, string? name = default, @@ -93,9 +123,7 @@ public static IHealthChecksBuilder AddNpgSql( IEnumerable? tags = default, TimeSpan? timeout = default) { - Guard.ThrowIfNull(dbDataSourceFactory); - - NpgsqlDataSource? dataSource = null; + // This instance is captured in lambda closure, so it can be reused (perf) NpgSqlHealthCheckOptions options = new() { CommandText = healthQuery, @@ -106,49 +134,7 @@ public static IHealthChecksBuilder AddNpgSql( name ?? NAME, sp => { - // The Data Source needs to be created only once, - // as each instance has it's own connection pool. - // See https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1993 for more details. - - // Perform an atomic read of the current value. - NpgsqlDataSource? existingDataSource = Volatile.Read(ref dataSource); - if (existingDataSource is null) - { - // Create a new Data Source - NpgsqlDataSource fromFactory = dbDataSourceFactory(sp); - // Try to resolve the Data Source from DI. - NpgsqlDataSource? fromDI = sp.GetService(); - - if (fromDI is not null && fromDI.ConnectionString.Equals(fromFactory.ConnectionString)) - { - // If they are using the same ConnectionString, we can reuse the instance from DI. - // So there is only ONE NpgsqlDataSource per the whole app and ONE connection pool. - - if (!ReferenceEquals(fromDI, fromFactory)) - { - // Dispose it, as long as it's not the same instance. - fromFactory.Dispose(); - } - Interlocked.Exchange(ref dataSource, fromDI); - options.DataSource = fromDI; - } - else - { - // Perform an atomic exchange, but only if the value is still null. - existingDataSource = Interlocked.CompareExchange(ref dataSource, fromFactory, null); - if (existingDataSource is not null) - { - // Some other thread has created the data source in the meantime, - // we dispose our own copy, and use the existing instance. - fromFactory.Dispose(); - options.DataSource = existingDataSource; - } - else - { - options.DataSource = fromFactory; - } - } - } + options.DataSource ??= dbDataSourceFactory?.Invoke(sp) ?? sp.GetRequiredService(); return new NpgSqlHealthCheck(options); }, diff --git a/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs b/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs index 37a72c681e..379ab32357 100644 --- a/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs +++ b/src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs @@ -1,4 +1,6 @@ +using System.Diagnostics; using Microsoft.Extensions.Diagnostics.HealthChecks; +using Npgsql; namespace HealthChecks.NpgSql; @@ -11,7 +13,7 @@ public class NpgSqlHealthCheck : IHealthCheck public NpgSqlHealthCheck(NpgSqlHealthCheckOptions options) { - Guard.ThrowIfNull(options.DataSource); + Debug.Assert(options.ConnectionString is not null || options.DataSource is not null); Guard.ThrowIfNull(options.CommandText, true); _options = options; } @@ -21,7 +23,9 @@ public async Task CheckHealthAsync(HealthCheckContext context { try { - await using var connection = _options.DataSource!.CreateConnection(); + await using var connection = _options.DataSource is not null + ? _options.DataSource.CreateConnection() + : new NpgsqlConnection(_options.ConnectionString); _options.Configure?.Invoke(connection); await connection.OpenAsync(cancellationToken).ConfigureAwait(false); @@ -33,7 +37,6 @@ public async Task CheckHealthAsync(HealthCheckContext context return _options.HealthCheckResultBuilder == null ? HealthCheckResult.Healthy() : _options.HealthCheckResultBuilder(result); - } catch (Exception ex) { diff --git a/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs b/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs index 0138c30ea8..e4171c035e 100644 --- a/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs +++ b/src/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs @@ -9,20 +9,61 @@ namespace HealthChecks.NpgSql; /// public class NpgSqlHealthCheckOptions { + internal NpgSqlHealthCheckOptions() + { + // This ctor is internal on purpose: those who want to use NpgSqlHealthCheckOptions + // need to specify either ConnectionString or DataSource when creating it. + // Making the ConnectionString and DataSource setters internal + // allows us to ensure that the customers don't try to mix both concepts. + // By encapsulating all of that, we ensure that all instances of this type are valid. + } + /// - /// The Postgres connection string to be used. - /// Use property for advanced configuration. + /// Creates an instance of . /// - public string? ConnectionString + /// The PostgreSQL connection string to be used. + /// + /// supports additional configuration beyond the connection string, + /// such as logging, advanced authentication options, type mapping management, and more. + /// To further configure a data source, use and + /// the constructor. + /// + public NpgSqlHealthCheckOptions(string connectionString) { - get => DataSource?.ConnectionString; - set => DataSource = value is not null ? NpgsqlDataSource.Create(value) : null; + ConnectionString = Guard.ThrowIfNull(connectionString, throwOnEmptyString: true); } /// - /// The Postgres data source to be used. + /// Creates an instance of . + /// + /// The Postgres to be used. + /// + /// Depending on how the was configured, the connections it hands out may be pooled. + /// That is why it should be the exact same that is used by other parts of your app. + /// + public NpgSqlHealthCheckOptions(NpgsqlDataSource dataSource) + { + DataSource = Guard.ThrowIfNull(dataSource); + } + + /// + /// The Postgres connection string to be used. + /// + /// + /// supports additional configuration beyond the connection string, + /// such as logging, advanced authentication options, type mapping management, and more. + /// To further configure a data source, use and the constructor. + /// + public string? ConnectionString { get; internal set; } + + /// + /// The Postgres to be used. /// - public NpgsqlDataSource? DataSource { get; set; } + /// + /// Depending on how the was configured, the connections it hands out may be pooled. + /// That is why it should be the exact same that is used by other parts of your app. + /// + public NpgsqlDataSource? DataSource { get; internal set; } /// /// The query to be executed. diff --git a/src/HealthChecks.NpgSql/README.md b/src/HealthChecks.NpgSql/README.md new file mode 100644 index 0000000000..7dab42824e --- /dev/null +++ b/src/HealthChecks.NpgSql/README.md @@ -0,0 +1,47 @@ +## PostgreSQL Health Check + +This health check verifies the ability to communicate with [PostgreSQL](https://www.postgresql.org/). It uses the [Npgsql](https://www.npgsql.org/) library. + +## NpgsqlDataSource + +Starting with Npgsql 7.0 (and .NET 7), the starting point for any database operation is [NpgsqlDataSource](https://www.npgsql.org/doc/api/Npgsql.NpgsqlDataSource.html). The data source represents your PostgreSQL database, and can hand out connections to it, or support direct execution of SQL against it. The data source encapsulates the various Npgsql configuration needed to connect to PostgreSQL, as well as the **connection pooling which makes Npgsql efficient**. + +Npgsql's **data source supports additional configuration beyond the connection string**, such as logging, advanced authentication options, type mapping management, and more. + +## Recommended approach + +To take advantage of the performance `NpgsqlDataSource` has to offer, it should be used as a **singleton**. Otherwise, the app might end up with having multiple data source instances, all of which would have their own connection pools. This can lead to resources exhaustion and major performance issues (Example: [#1993](https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/1993)). + +We encourage you to use [Npgsql.DependencyInjection](https://www.nuget.org/packages/Npgsql.DependencyInjection) package for registering a singleton factory for `NpgsqlDataSource`. It allows easy configuration of your Npgsql connections and registers the appropriate services in your DI container. + +To make the shift to `NpgsqlDataSource` as easy as possible, the `Npgsql.DependencyInjection` package registers not just a factory for the data source, but also factory for `NpgsqlConnection` (and even `DbConnection`). So, your app does not need to suddenly start using `NpgsqlDataSource` everywhere. + +```csharp +void Configure(IServiceCollection services) +{ + services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=test"); + services.AddHealthChecks().AddNpgSql(); +} +``` + +By default, the `NpgsqlDataSource` instance is resolved from service provider. If you need to access more than one PostgreSQL database, you can use keyed DI services to achieve that: + +```csharp +void Configure(IServiceCollection services) +{ + services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=first", serviceKey: "first"); + services.AddHealthChecks().AddNpgSql(sp => sp.GetRequiredKeyedService("first")); + + services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=second", serviceKey: "second"); + services.AddHealthChecks().AddNpgSql(sp => sp.GetRequiredKeyedService("second")); +} +``` + + +## Connection String + +Raw connection string is of course still supported: + +```csharp +services.AddHealthChecks().AddNpgSql("Host=pg_server;Username=test;Password=test;Database=test"); +``` diff --git a/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs b/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs index 1d3252e4cd..de1731f0c9 100644 --- a/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs +++ b/test/HealthChecks.Npgsql.Tests/DependencyInjection/RegistrationTests.cs @@ -1,6 +1,4 @@ -using System.Reflection; using HealthChecks.NpgSql; -using Npgsql; namespace HealthChecks.Npgsql.Tests.DependencyInjection; @@ -21,7 +19,6 @@ public void add_health_check_when_properly_configured() registration.Name.ShouldBe("npgsql"); check.ShouldBeOfType(); - } [Fact] @@ -91,43 +88,18 @@ public void factory_is_called_only_once() factoryCalls.ShouldBe(1); } - [Theory] - [InlineData(true)] - [InlineData(false)] - public void factory_reuses_pre_registered_datasource_when_possible(bool sameConnectionString) + [Fact] + public void recommended_scenario_compiles_and_works_as_expected() { - const string connectionString = "Server=localhost"; ServiceCollection services = new(); - - services.AddSingleton(serviceProvider => - { - var dataSourceBuilder = new NpgsqlDataSourceBuilder(connectionString); - return dataSourceBuilder.Build(); - }); - - int factoryCalls = 0; - services.AddHealthChecks() - .AddNpgSql(_ => - { - Interlocked.Increment(ref factoryCalls); - return sameConnectionString ? connectionString : $"{connectionString}2"; - }, name: "my-npg-1"); + services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=test"); + services.AddHealthChecks().AddNpgSql(); using var serviceProvider = services.BuildServiceProvider(); - var options = serviceProvider.GetRequiredService>(); - var registration = options.Value.Registrations.Single(); + var healthCheck = registration.Factory(serviceProvider); - for (int i = 0; i < 10; i++) - { - var healthCheck = (NpgSqlHealthCheck)registration.Factory(serviceProvider); - var fieldInfo = typeof(NpgSqlHealthCheck).GetField("_options", BindingFlags.Instance | BindingFlags.NonPublic); - var npgSqlHealthCheckOptions = (NpgSqlHealthCheckOptions)fieldInfo!.GetValue(healthCheck)!; - - Assert.Equal(sameConnectionString, ReferenceEquals(serviceProvider.GetRequiredService(), npgSqlHealthCheckOptions.DataSource)); - } - - factoryCalls.ShouldBe(1); + healthCheck.ShouldBeOfType(); } } diff --git a/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt b/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt index d84990c4dd..01ebc13847 100644 --- a/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt +++ b/test/HealthChecks.Npgsql.Tests/HealthChecks.NpgSql.approved.txt @@ -7,11 +7,12 @@ namespace HealthChecks.NpgSql } public class NpgSqlHealthCheckOptions { - public NpgSqlHealthCheckOptions() { } + public NpgSqlHealthCheckOptions(Npgsql.NpgsqlDataSource dataSource) { } + public NpgSqlHealthCheckOptions(string connectionString) { } public string CommandText { get; set; } public System.Action? Configure { get; set; } - public string? ConnectionString { get; set; } - public Npgsql.NpgsqlDataSource? DataSource { get; set; } + public string? ConnectionString { get; } + public Npgsql.NpgsqlDataSource? DataSource { get; } public System.Func? HealthCheckResultBuilder { get; set; } } } @@ -20,8 +21,8 @@ namespace Microsoft.Extensions.DependencyInjection public static class NpgSqlHealthCheckBuilderExtensions { public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, HealthChecks.NpgSql.NpgSqlHealthCheckOptions options, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } - public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func dbDataSourceFactory, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func connectionStringFactory, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } + public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, System.Func? dbDataSourceFactory = null, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } public static Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder AddNpgSql(this Microsoft.Extensions.DependencyInjection.IHealthChecksBuilder builder, string connectionString, string healthQuery = "SELECT 1;", System.Action? configure = null, string? name = null, Microsoft.Extensions.Diagnostics.HealthChecks.HealthStatus? failureStatus = default, System.Collections.Generic.IEnumerable? tags = null, System.TimeSpan? timeout = default) { } } } \ No newline at end of file diff --git a/test/HealthChecks.Npgsql.Tests/HealthChecks.Npgsql.Tests.csproj b/test/HealthChecks.Npgsql.Tests/HealthChecks.Npgsql.Tests.csproj index 2542a96526..6ea8775da5 100644 --- a/test/HealthChecks.Npgsql.Tests/HealthChecks.Npgsql.Tests.csproj +++ b/test/HealthChecks.Npgsql.Tests/HealthChecks.Npgsql.Tests.csproj @@ -1,5 +1,9 @@ + + + +