Add Aspire.MySqlConnector #825

bgrainger · 2023-11-15T00:06:19Z

Using the component name "Aspire.MySqlConnector" to leave the door open for "Aspire.MySql.Data" (based on the MySql.Data library) in the future.

Fixes #786

Using the component name "Aspire.MySqlConnector" to leave the door open for "Aspire.MySql.Data" (based on the MySql.Data library) in the future.

bgrainger · 2023-11-15T00:12:43Z

As per my comment I'd be very happy for Aspire to be "opinionated" about MySQL libraries, and just call this component "Aspire.MySql" instead of "Aspire.MySqlConnector". (MySql.Data doesn't implement DbDataSource and it's not clear if/when that library will implement any new ADO.NET features.)

mitchdenny · 2023-11-15T01:36:42Z

This is awesome. We should also think about what the AppHost side of things might look like in this PR. For example:

var builder = DistributedApplication.CreateBuilder(args);
var mysql = builder.AddMySql("mysql");

var backend = builder.AddProject<Projects.Backend>("backend")
   .WithReference(mysql);

You can checkout code in src/Aspire.Hosting/Postgres as an example. The AppHost doesn't really need to choose between one MySQL provider or another - at this layer its just about getting the inner loop up and running by spinning up a container locally for the dev.

DamianEdwards · 2023-11-15T01:38:13Z

New packages will need to be mirrored before PR checks can pass I think?

eerhardt · 2023-11-15T02:11:36Z

New packages will need to be mirrored before PR checks can pass I think?

I'll take care of the mirroring in the morning.

@bgrainger - can you also add tests? See

for examples.

eerhardt · 2023-11-15T02:13:28Z

src/Components/Aspire.MySqlConnector/Aspire.MySqlConnector.csproj

+    <IsPackable>true</IsPackable>
+    <PackageTags>$(ComponentDatabasePackageTags) mysqlconnector mysql sql</PackageTags>
+    <Description>A MySQL client that integrates with Aspire, including health checks, metrics, logging, and telemetry.</Description>
+    <PackageIconFullPath>$(SharedDir)SQL_256x.png</PackageIconFullPath>


@DamianEdwards - should this have the same icon as the https://www.nuget.org/packages/MySqlConnector/ package?

That would be ideal, assuming we have permission to do so 😀

I created the MySqlConnector package icon (using free sources) and will give any necessary permission to include it here.

src/Components/Aspire.MySqlConnector/AspireMySqlConnectorExtensions.cs

eerhardt · 2023-11-15T02:18:04Z

src/Components/Aspire.MySqlConnector/MySqlConnectorCommon.cs

+
+internal static class MySqlConnectorCommon
+{
+    public static void AddMySqlMetrics(MeterProviderBuilder meterProviderBuilder)


I'm not sure we need this given we only have 1 component. Is there an EntityFramework "MySqlConnector" library?

The EF Core library built on top of MySqlConnector is https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql.

I'm happy to add support for that too, but will probably do it as a separate PR.

src/Components/Aspire.MySqlConnector/MySqlConnectorCommon.cs

danmoseley · 2023-11-15T05:41:56Z

FYI @ajcvickers @roji for awareness of more data related Aspire packages...

mitchdenny · 2023-11-15T05:50:33Z

@bgrainger I've just had a look through the AppModel side of the PR and everything looks in order (great work!).

roji · 2023-11-15T10:02:25Z

Great to see this @bgrainger!

After this we can also add the Pomelo EF provider for MySQL (/cc @lauxjpn).

Using the component name "Aspire.MySqlConnector" to leave the door open for "Aspire.MySql.Data" (based on the MySql.Data library) in the future.

On this... Unlike the situation with SQL Server and PostgreSQL, where there really is basically only one (main) driver (SqlClient and Npgsql respectively), with MySql there really are two - the community MySqlConnector being wrapped here (with the Pomelo EF connector built on top of it) and the "official" providers from Oracle. I'm definitely a big supporter of the former pair - and also of the idea of being opinionated in Aspire - but I think we should continue to include the driver name here (i.e. Aspire.MySqlConnector rather than Aspire.MySql).

Note that even for Npgsql we have Aspire.Npgsql (rather than Aspire.PostgreSQL).

bgrainger · 2023-11-15T14:46:18Z

I'm planning to come back and add Pomelo EF support, but will probably do that as a separate PR.

danmoseley · 2023-11-15T14:51:47Z

src/Components/Aspire.MySqlConnector/MySqlConnector.png

@DamianEdwards do we need perms to include this

I'll revert this, to avoid any possible problems over asset permissions.

From my reading, we'd need written permission to use the logo. I can work offline to enquire about that.

joperezr · 2023-11-15T18:01:27Z

Added MySqlConnector.DependencyInjection and AspNetCore.HealthChecks.MySql to dotnet-public feed. I'll re-run CI now.

joperezr · 2023-11-15T18:01:37Z

/azp run dotnet.aspire

azure-pipelines · 2023-11-15T18:01:43Z

Pull request contains merge conflicts.

Conflicts: Directory.Packages.props

danmoseley · 2023-11-15T21:58:56Z

It'd be great to have a MySql sample (in https://github.com/dotnet/aspire-samples) once this is in, if you're interested in that followup.

danmoseley · 2023-11-15T21:59:36Z

Maybe we should leave the logo out for now, so we're not waiting on Damian to talk to Oracle.

eerhardt

This is looking really great! Thank you for your contribution, @bgrainger. (and on the first day we made the repo public 😄.

I just have a few comments.

Regarding the name of the component. What you have in your current code is correct. See the policy at:

aspire/src/Components/README.md

Lines 5 to 8 in ab931f0

    
           ## Naming 
        
           - Each component's name should contain just an `Aspire.` prefix. 
        
           - When component is built around `ABC` client library, it should contain the client library name in its name. Example: `Aspire.ABC`.

Since this component is built around MySqlConnector, the correct name for it is Aspire.MySqlConnector.

src/Aspire.Hosting/MySql/MySqlContainerResource.cs

eerhardt · 2023-11-15T22:20:16Z

src/Components/Aspire.MySqlConnector/Aspire.MySqlConnector.csproj

+    <TargetFramework>$(NetCurrent)</TargetFramework>
+    <IsPackable>true</IsPackable>
+    <PackageTags>$(ComponentDatabasePackageTags) mysqlconnector mysql sql</PackageTags>
+    <Description>A MySQL client that integrates with Aspire, including health checks, metrics, logging, and telemetry.</Description>


Does MySQL need a ® sign after it, like we do for PostgreSQL?

src/Components/Telemetry.md

eerhardt · 2023-11-15T22:42:45Z

src/Components/Aspire.MySqlConnector/AspireMySqlConnectorExtensions.cs

+                    ConnectionString = serviceKey is null
+                        ? sp.GetRequiredService<MySqlDataSource>().ConnectionString
+                        : sp.GetRequiredKeyedService<MySqlDataSource>(serviceKey).ConnectionString


How do you feel about opening a PR to https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks to make this consistent with Npgsql? We shouldn't be creating another DbDataSource and checking the health of that. Instead, we should be using the same DbDataSource instance as the app is using. That way if that DbDataSource ever gets corrupt, the health check is accurate. From the Components README:

aspire/src/Components/README.md

Line 68 in ab931f0

- Consider whether the Health Check should reuse the same client object registered in DI by the component or not. Reusing the same client object has the advantages of getting the same configuration, logging, etc during the health check.

See also Xabaril/AspNetCore.Diagnostics.HealthChecks#1832

Sounds good; I have another reason to open a PR there, too: Xabaril/AspNetCore.Diagnostics.HealthChecks#2031.

PR: Xabaril/AspNetCore.Diagnostics.HealthChecks#2096

src/Components/Telemetry.md

tests/Aspire.MySqlConnector.Tests/ConformanceTests.cs

All requirements are met except for public API review.

src/Components/Aspire.MySqlConnector/AspireMySqlConnectorExtensions.cs

This allows us to use the built-in support for keyed services.

danmoseley · 2023-11-17T02:02:54Z

src/Aspire.Hosting/MySql/MySqlDatabaseResource.cs

+        }
+        else
+        {
+            throw new DistributedApplicationException("Parent resource connection string was null.");


@eerhardt should we start moving these things to a resx? I assume we won't localize unless the base libraries start to localize. So perhaps there isn't much value.

ASP.NET Core doesn't have a lot of its exception messages in resx's either. For example

https://github.com/dotnet/aspnetcore/blob/f8f03ea3764826d009f7ceb9bb91e482ce4a3fa9/src/Shared/Json/JsonSerializerExtensions.cs#L26

If we ever needed to localize our exceptions, it wouldn't be that hard to find these. But odds are, after 8 years of .NET Core System.* libraries not being localized, I'd be surprised if it ever happens.

bgrainger · 2023-11-17T02:06:26Z

Needs dotnet-public to be updated with v2.3.1 of MySqlConnector and MySqlConnector.DependencyInjection packages.

danmoseley · 2023-11-17T02:09:29Z

Needs dotnet-public to be updated with v2.3.1 of MySqlConnector and MySqlConnector.DependencyInjection packages.

I kicked that, should be about 10 minutes.

danmoseley · 2023-11-17T04:41:57Z

the package was mirrored and the build succeeds.

Conflicts: Directory.Packages.props

eerhardt · 2023-11-17T16:25:49Z

@bgrainger - any reason this is in "Draft"? I didn't see a reason it was marked draft.

bgrainger · 2023-11-17T18:07:45Z

Put it in draft while I waited to update to MySqlConnector 2.3.1; ready for review now.

eerhardt

This looks great. Thanks again, @bgrainger! The unresolved comments can be addressed in follow up PRs. I think this is ready to be merged.

I'll wait until EOD before mering this, to allow for any additional feedback from other reviewers.

davidfowl · 2023-11-18T05:53:48Z

Amazing 🚀

Add Aspire.MySqlConnector. Fixes dotnet#786

abb3d0b

Using the component name "Aspire.MySqlConnector" to leave the door open for "Aspire.MySql.Data" (based on the MySql.Data library) in the future.

eerhardt reviewed Nov 15, 2023

View reviewed changes

bgrainger added 5 commits November 14, 2023 18:36

Rename config section to Aspire:MySqlConnector.

0c3f020

Inline AddMeter call.

1deac05

Add AppHost for MySQL.

b6a5e0e

Rename config section to Aspire:MySqlConnector.

3255804

Add Aspire.MySqlConnector.Tests.

5b16736

danmoseley reviewed Nov 15, 2023

View reviewed changes

bgrainger force-pushed the mysql branch from 2e48452 to 5b16736 Compare November 15, 2023 15:00

Merge main into mysql.

ca9ef22

Conflicts: Directory.Packages.props

eerhardt reviewed Nov 15, 2023

View reviewed changes

bgrainger added 3 commits November 15, 2023 15:40

Use MySqlConnector conventions for connection string options.

9f85ccf

Remove extraneous configuration level in JSON.

68077a2

Add MySqlConnector to component progress documentation.

b1c30a9

All requirements are met except for public API review.

bgrainger mentioned this pull request Nov 15, 2023

Use seconds as units for duration metrics mysql-net/MySqlConnector#1396

Closed

bgrainger commented Nov 16, 2023

View reviewed changes

src/Components/Aspire.MySqlConnector/AspireMySqlConnectorExtensions.cs Outdated Show resolved Hide resolved

danmoseley added the area-integrations Issues pertaining to Aspire Integrations packages label Nov 16, 2023

bgrainger marked this pull request as draft November 16, 2023 18:50

Update MySqlConnector to 2.3.1.

a64653f

This allows us to use the built-in support for keyed services.

danmoseley reviewed Nov 17, 2023

View reviewed changes

danmoseley closed this Nov 17, 2023

danmoseley reopened this Nov 17, 2023

Merge main into mysql.

4ad05e2

Conflicts: Directory.Packages.props

eerhardt mentioned this pull request Nov 17, 2023

Integrate LocalStack for Developing AWS Applications Locally #875

Closed

bgrainger added 2 commits November 17, 2023 10:05

Merge main into mysql.

28c84fa

Add new counter in MySqlConnector 2.3.1.

ec14198

bgrainger marked this pull request as ready for review November 17, 2023 18:07

eerhardt approved these changes Nov 17, 2023

View reviewed changes

eerhardt merged commit e097223 into dotnet:main Nov 18, 2023
2 checks passed

This was referenced Nov 27, 2023

Add mongodb container and connection support #986

Merged

Add OracleManagedDataAccess Aspire Component #1004

Closed

bgrainger mentioned this pull request Dec 1, 2023

Add Pomelo.EntityFrameworkCore.MySql component #1161

Merged

eerhardt mentioned this pull request Jan 23, 2024

Aspire: Support mysql.server.v0, mysql.database.v0 Azure/azure-dev#3221

Closed

github-actions bot locked and limited conversation to collaborators Apr 29, 2024

	## Naming

	- Each component's name should contain just an `Aspire.` prefix.
	- When component is built around `ABC` client library, it should contain the client library name in its name. Example: `Aspire.ABC`.

Add Aspire.MySqlConnector #825

Add Aspire.MySqlConnector #825

Conversation

bgrainger commented Nov 15, 2023

bgrainger commented Nov 15, 2023

mitchdenny commented Nov 15, 2023

DamianEdwards commented Nov 15, 2023

eerhardt commented Nov 15, 2023

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

danmoseley commented Nov 15, 2023 • edited Loading

mitchdenny commented Nov 15, 2023

roji commented Nov 15, 2023

bgrainger commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Nov 15, 2023

joperezr commented Nov 15, 2023

azure-pipelines bot commented Nov 15, 2023

danmoseley commented Nov 15, 2023

danmoseley commented Nov 15, 2023

eerhardt 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

Choose a reason for hiding this comment

bgrainger commented Nov 17, 2023

danmoseley commented Nov 17, 2023

danmoseley commented Nov 17, 2023

eerhardt commented Nov 17, 2023

bgrainger commented Nov 17, 2023

eerhardt left a comment

Choose a reason for hiding this comment

davidfowl commented Nov 18, 2023

danmoseley commented Nov 15, 2023 •

edited

Loading