Use MySqlDataSource in `AddMySql` by default #2096

bgrainger · 2023-11-18T16:27:50Z

What this PR does / why we need it:

This pushes users towards best practices (similar to what @adamsitnik described in #2040). The user should register a MySqlDataSource with DI, then call .AddHealthChecks().AddMySql() to use that same MySqlDataSource by default.

This PR also builds on top of #2093. They could be separated if needed, but there would be a merge conflict if they were opened separately, and both changes are useful.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Yes, the .AddMySql method now has an overload with all default parameters that "does the right thing".

Please make sure you've completed the relevant tasks for this PR, out of the following list:

I didn't see a place to update documentation or samples for MySql, but I'd be happy to do this to show how to add this health check properly.

Users can opt in to setting a command that will be executed on the server, but the default is now a more efficient ping.

This pushes users towards good defaults: firstly registering a MySqlDataSource as a singleton in DI, then just calling .AddHealthChecks().AddMySql() with no other arguments to retrieve and use that same data source for health checks.

adamsitnik

@bgrainger thank you for your contribution! I believe it's the step in the right direction, it needs only some polishing before getting merged.

adamsitnik · 2023-11-23T15:06:04Z

src/HealthChecks.MySql/MySqlHealthCheckOptions.cs

    /// </summary>
-    public string ConnectionString { get; set; } = null!;
+    public MySqlDataSource? DataSource { get; set; }


Exposing a DataSource can lead to issues similar to the ones I've fixed in #2045 for PostgreSQL. IMO we should make it internal.

Suggested change

public MySqlDataSource? DataSource { get; set; }

internal MySqlDataSource? DataSource { get; set; }

Are you worried that this could lead to API misuse because someone could create a new MySqlDataSource just to set this property?

I'm more worried that without this property, someone would be forced to set the ConnectionString property, which would be a worse outcome. Making this internal would also make it impossible for an external client to initialize this object properly (i.e., with a data source), making the AddMySql overload that takes it a "pit of failure".

I'm happy to make this internal if we also make AddMySql(this IHealthChecksBuilder builder, MySqlHealthCheckOptions options, ...) [Obsolete] because it doesn't allow one to set to set the data source. Thoughts?

After thinking about it for a while and experimenting with Npgsql in #2116 I got to the following conclusions:

Let's make the parameterless MySqlHealthCheckOptions ctor internal and add two pubic ctors: one that accepts ConnectionString and one that accepts DataSource.
Moreover, let's make the setters for these properties internal.

This allows us to ensure that every instance of this type is valid: it has either the connection string or the data source. Never both.

This is the only change I would like to make before merging this PR.

@bgrainger thoughts?

Sounds good to me; I'll update this PR with that pattern soon.

Let's make the parameterless MySqlHealthCheckOptions ctor internal ... Moreover, let's make the setters for these properties internal.

When developing this, I found that I didn't need a parameterless constructor nor to set the properties after construction. Thus, the MySqlHealthCheckOptions type has been updated to have just two public constructors and private setters for all properties.

When developing this, I found that I didn't need a parameterless constructor nor to set the properties after construction. Thus, the MySqlHealthCheckOptions type has been updated to have just two public constructors and private setters for all properties.

Great! 👍

test/HealthChecks.MySql.Tests/Functional/MySqlHealthCheckTests.cs

Conflicts: src/HealthChecks.MySql/HealthChecks.MySql.csproj

adamsitnik · 2023-12-04T10:43:30Z

@bgrainger I've created #2113, PTAL and let me know what do you think

adamsitnik

It's almost ready, PTAL at my comments. Thank you!

src/HealthChecks.MySql/README.md

adamsitnik · 2023-12-08T12:38:20Z

src/HealthChecks.MySql/MySqlHealthCheckOptions.cs

    /// </summary>
-    public string ConnectionString { get; set; } = null!;
+    public MySqlDataSource? DataSource { get; set; }


After thinking about it for a while and experimenting with Npgsql in #2116 I got to the following conclusions:

Let's make the parameterless MySqlHealthCheckOptions ctor internal and add two pubic ctors: one that accepts ConnectionString and one that accepts DataSource.
Moreover, let's make the setters for these properties internal.

This allows us to ensure that every instance of this type is valid: it has either the connection string or the data source. Never both.

This is the only change I would like to make before merging this PR.

@bgrainger thoughts?

There are two constructors: one to initialise with a MySqlDataSource, and one with a connection string. This helps enforce correct uasge of the API by ensuring that exactly one is set.

Conflicts: src/HealthChecks.MySql/HealthChecks.MySql.csproj

codecov-commenter · 2023-12-11T07:12:53Z

Codecov Report

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

Comparison is base (fb04fe8) 0.00% compared to head (c58614c) 64.23%.

Files	Patch %	Lines
src/HealthChecks.MySql/MySqlHealthCheck.cs	38.09%	9 Missing and 4 partials ⚠️
...encyInjection/MySqlHealthCheckBuilderExtensions.cs	77.27%	4 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    #2096       +/-   ##
===========================================
+ Coverage    0.00%   64.23%   +64.23%     
===========================================
  Files           7      248      +241     
  Lines         356     8396     +8040     
  Branches       28      592      +564     
===========================================
+ Hits            0     5393     +5393     
- Misses        356     2852     +2496     
- Partials        0      151      +151

Flag	Coverage Δ
MySql	`31.34% <66.66%> (?)`

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

LGTM, thank you very much @bgrainger !

adamsitnik · 2023-12-11T07:15:56Z

src/HealthChecks.MySql/MySqlHealthCheck.cs

+        Guard.ThrowIfNull(options);
+        if (options.DataSource is null && options.ConnectionString is null)
+            throw new InvalidOperationException("One of options.DataSource or options.ConnectionString must be specified.");
+        if (options.DataSource is not null && options.ConnectionString is not null)
+            throw new InvalidOperationException("Only one of options.DataSource or options.ConnectionString must be specified.");
        _options = options;


We don't need these checks anymore, as the public ctors ensure that the state is always valid.

Suggested change

Guard.ThrowIfNull(options);

if (options.DataSource is null && options.ConnectionString is null)

throw new InvalidOperationException("One of options.DataSource or options.ConnectionString must be specified.");

if (options.DataSource is not null && options.ConnectionString is not null)

throw new InvalidOperationException("Only one of options.DataSource or options.ConnectionString must be specified.");

_options = options;

_options = Guard.ThrowIfNull(options);

For some reason I can't apply this suggestion. I am going to merge it now since it's not blocking

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.

bgrainger added 2 commits November 17, 2023 07:12

Send a MySQL ping packet by default. Fixes Xabaril#2031

e7917f8

Users can opt in to setting a command that will be executed on the server, but the default is now a more efficient ping.

Add AddMySql overload for MySqlDataSource.

e58c262

This pushes users towards good defaults: firstly registering a MySqlDataSource as a singleton in DI, then just calling .AddHealthChecks().AddMySql() with no other arguments to retrieve and use that same data source for health checks.

github-actions bot added test mysql labels Nov 18, 2023

This was referenced Nov 18, 2023

Add Aspire.MySqlConnector dotnet/aspire#825

Merged

Support Health Checks in MySqlDataSourceBuilder mysql-net/MySqlConnector#1368

Closed

adamsitnik reviewed Nov 23, 2023

View reviewed changes

bgrainger added 2 commits November 23, 2023 08:47

Add package README for HealthChecks.MySql.

479d8f0

Merge master into mysql-data-source.

d62373e

Conflicts: src/HealthChecks.MySql/HealthChecks.MySql.csproj

This was referenced Dec 4, 2023

Npgsql: don't create more than one DataSource #2045

Merged

DB health checks: design #2113

Closed

adamsitnik mentioned this pull request Dec 8, 2023

Support for DotNet8 + EfCore8 #2120

Closed

adamsitnik reviewed Dec 8, 2023

View reviewed changes

This was referenced Dec 8, 2023

Implement DbDataSource support for Oracle #2122

Open

Implement DbDataSource support for SQL Server #2123

Open

bgrainger added 2 commits December 9, 2023 08:14

Use new MySqlHealthCheckOptions API.

f4af126

There are two constructors: one to initialise with a MySqlDataSource, and one with a connection string. This helps enforce correct uasge of the API by ensuring that exactly one is set.

Merge master into mysql-data-source.

c58614c

Conflicts: src/HealthChecks.MySql/HealthChecks.MySql.csproj

bgrainger mentioned this pull request Dec 9, 2023

MySql check no longer uses Ping #2031

Closed

adamsitnik added this to the V 8.0 milestone Dec 11, 2023

adamsitnik approved these changes Dec 11, 2023

View reviewed changes

adamsitnik merged commit 5be2f55 into Xabaril:master Dec 11, 2023
2 checks passed

adamsitnik mentioned this pull request Dec 11, 2023

Send a MySQL ping packet by default #2093

Closed

6 tasks

bgrainger mentioned this pull request Dec 16, 2023

Remove unnecessary precondition checks #2129

Merged

3 tasks

bgrainger mentioned this pull request Jan 15, 2024

AddCheck<MySqlHealthCheck> throws InvalidOperationException for missing MySqlHealthCheckOptions #2141

Closed

adamsitnik mentioned this pull request Jan 22, 2024

Remove private static cache from MongoDbHealthCheck #2148

Closed

eerhardt mentioned this pull request Dec 12, 2024

Remove private static cache from RabbitMQHealthCheck #2343

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MySqlDataSource in `AddMySql` by default #2096

Use MySqlDataSource in `AddMySql` by default #2096

bgrainger commented Nov 18, 2023 •

edited

Loading

adamsitnik left a comment

adamsitnik Nov 23, 2023

bgrainger Nov 23, 2023

adamsitnik Dec 8, 2023

bgrainger Dec 8, 2023

bgrainger Dec 9, 2023

adamsitnik Dec 11, 2023

adamsitnik commented Dec 4, 2023

adamsitnik left a comment

adamsitnik Dec 8, 2023

codecov-commenter commented Dec 11, 2023

adamsitnik left a comment

adamsitnik Dec 11, 2023

adamsitnik Dec 11, 2023

bgrainger Dec 16, 2023

	public MySqlDataSource? DataSource { get; set; }
	internal MySqlDataSource? DataSource { get; set; }

Use MySqlDataSource in AddMySql by default #2096

Use MySqlDataSource in AddMySql by default #2096

Conversation

bgrainger commented Nov 18, 2023 • edited Loading

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Dec 4, 2023

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 11, 2023

Codecov Report

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use MySqlDataSource in `AddMySql` by default #2096

Use MySqlDataSource in `AddMySql` by default #2096

bgrainger commented Nov 18, 2023 •

edited

Loading