Improve Npgsql health check design #2116
Conversation
- make NpgSqlHealthCheckOptions.DataSource internal, so the users can't make mistakes by assigning it to a new data source for every health check invocation - make sure not more than a single DataSource is created - when the user provides ConnectionString, try to resolve DataSource just once. If it's present and the connection string is the same, reuse it
- 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.
adamsitnik
changed the title
|
|
||
| This health check verifies the ability to communicate with [PostgreSQL](https://www.postgresql.org/). It uses the [Npgsql](https://www.npgsql.org/) library. | ||
|
|
||
| ## NpgsqlDataSource |
There was a problem hiding this comment.
@roji @NinoFloris PTAL at what I wrote here and let me know if this is correct
…ly a connection string was provided
|
I was unable to get a review, so I've eyeballed it multiple times. It LGTM, so I am merging it. In case I am wrong, we are going to release a fix later. |
There was a problem hiding this comment.
Sorry for the super late review, but overall everything here seems good! Yeah, the important thing is to have health checks automatically resolve a singleton NpgsqlDataSource from DI (previously registered via Npgsql.DependencyInjection), and for other cases, to provide ways to register with either a provided data source or a connection string.
|
|
||
| ## 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**. |
There was a problem hiding this comment.
I'd maybe go softer and say "the recommended starting point", as connection strings are still fully supported etc.
| } | ||
| ``` | ||
|
|
||
| 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: |
| void Configure(IServiceCollection services) | ||
| { | ||
| services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=test"); | ||
| services.AddHealthChecks().AddNpgSql(); |
There was a problem hiding this comment.
Yep, that's the ideal way things should work - the data source is registered independently as a singleton in DI (via Npgsql.DependencyInjection), and then is implicitly picked up by anyone needing it (health checks, EF Core...). A bit similar to an ILoggerFactory.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files@@ Coverage Diff @@
## master #2116 +/- ##
===========================================
- Coverage 64.94% 42.71% -22.23%
===========================================
Files 262 7 -255
Lines 8496 206 -8290
Branches 600 18 -582
===========================================
- Hits 5518 88 -5430
+ Misses 2828 112 -2716
+ Partials 150 6 -144
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
RabbitMQHealthCheck is maintaining it's private static cache of instances. It's wrong, as it can cause a memory leak (it won't ever be freed). Moreover, it's can create instances of IConnection. This is wrong, as it can lead into a situation when we have multiple instances of IConnection that are connected to the same server: one used by the app and another created and used by the health check. And we should have only one. We should do the same as in Xabaril#2040, Xabaril#2116 and Xabaril#2096 I left RabbitMQ.v6 as-is to limit the breaking change. Only when an existing user moves from RabbitMQ.Client v6 to v7 will they need to update their health check (along with other breaking changes in RabbitMQ.Client).
* Remove private static cache from RabbitMQHealthCheck RabbitMQHealthCheck is maintaining it's private static cache of instances. It's wrong, as it can cause a memory leak (it won't ever be freed). Moreover, it's can create instances of IConnection. This is wrong, as it can lead into a situation when we have multiple instances of IConnection that are connected to the same server: one used by the app and another created and used by the health check. And we should have only one. We should do the same as in #2040, #2116 and #2096 I left RabbitMQ.v6 as-is to limit the breaking change. Only when an existing user moves from RabbitMQ.Client v6 to v7 will they need to update their health check (along with other breaking changes in RabbitMQ.Client). * Add test when no IConnection is registered in DI.
This PR implements #2113 for Npgsql:
NpgSqlHealthCheckOptionsctors: one that requiresConnectionStringand another that requiresDataSource. This allows us to ensure that every instance of this type is valid.ConnectionStringandDataSourceproperties readonly. This allows us to ensure that the customers are not providing both.