Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Npgsql health check design #2116

Merged
merged 5 commits into from
Dec 15, 2023
Merged

Conversation

adamsitnik
Copy link
Collaborator

@adamsitnik adamsitnik commented Dec 4, 2023

This PR implements #2113 for Npgsql:

  • 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 ConnectionString and DataSource properties readonly. This allows us to ensure that the customers are not providing both.
  • add README.md and explain what we recommend and why

- 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 adamsitnik changed the title try to improve Npgsql health check design Improve Npgsql health check design Dec 8, 2023
@adamsitnik adamsitnik added this to the V 8.0 milestone Dec 8, 2023

This health check verifies the ability to communicate with [PostgreSQL](https://www.postgresql.org/). It uses the [Npgsql](https://www.npgsql.org/) library.

## NpgsqlDataSource
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji @NinoFloris PTAL at what I wrote here and let me know if this is correct

@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (81c6697) 64.94% compared to head (07ae4b5) 64.46%.
Report is 5 commits behind head on master.

Files Patch % Lines
...rc/HealthChecks.NpgSql/NpgSqlHealthCheckOptions.cs 81.81% 2 Missing ⚠️
...ncyInjection/NpgSqlHealthCheckBuilderExtensions.cs 97.61% 0 Missing and 1 partial ⚠️
src/HealthChecks.NpgSql/NpgSqlHealthCheck.cs 75.00% 0 Missing and 1 partial ⚠️

❗ 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%   64.46%   -0.49%     
==========================================
  Files         262      246      -16     
  Lines        8496     8318     -178     
  Branches      600      579      -21     
==========================================
- Hits         5518     5362     -156     
+ Misses       2828     2811      -17     
+ Partials      150      145       -5     
Flag Coverage Δ
AzureBlobStorage ?
AzureEventHubs ?
AzureFileStorage ?
AzureQueueStorage ?
AzureTableStorage ?
Dapr ?
Npgsql 47.32% <92.98%> (+6.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamsitnik
Copy link
Collaborator Author

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.

@adamsitnik adamsitnik merged commit 4cc62fb into Xabaril:master Dec 15, 2023
2 checks passed
@adamsitnik adamsitnik deleted the dbDesign branch December 15, 2023 11:11
Copy link

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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**.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

void Configure(IServiceCollection services)
{
services.AddNpgsqlDataSource("Host=pg_server;Username=test;Password=test;Database=test");
services.AddHealthChecks().AddNpgSql();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants