Enable SqlDiagnosticListener on .NET Standard

[mattjohnsonpint]

Enables SqlDiagnosticListener for .NET Standard using the existing .NET Core implementation.

Fixes #1928

[mattjohnsonpint]

Question: Do you want me to add an explicit package reference to System.Diagnostics.DiagnosticSource? I didn't yet, because it's currently imported transitively (Microsoft.Data.SqlClient => Azure.Identity => Azure.Core => System.Diagnostics.DiagnosticSource).

[mattjohnsonpint]

Actually, I think I will need to copy some more code over to the netfx project, as it looks like the listener is wired up only for the netcore versions of SqlCommand.cs, SqlConnection.cs, and SqlTransaction.cs.

[mattjohnsonpint]

I started copying over the invocation code over to the netfx version, but it turned out to be too much work. There's a lot of deviation between the two sets of sources, and I'm just not familiar enough with this code to make such a mass change.

It should be possible (and likely easy) for someone who's already acclimated to this project to make such improvements though. There aren't any hard blockers, as far as I can tell.

I force-pushed to revert the previous attempt. Now it just focuses on .NET Standard.

[mattjohnsonpint]

Anything I need to know about the four check failures? Looks like most of it passed just fine.

[lcheunglci]

I don't think the failures are related to your changes as we notice some intermittent failures that's happening in certain async tests which a draft PR was opened to address them.

[mattjohnsonpint]

Oh, I just noticed that #535 actually split several other source files. I'll make a few more changes. Thanks.

[mattjohnsonpint]

Ok, the only actual split in #535 was with SqlAuthenticationProviderManager, but it appears that the implementation details there were related more to EventSource than the SqlDiagnosticListener. Nonetheless, I see that the event source is already implemented for .NET Standard 2.1, just not .NET Standard 2.0. So the same concern applies. (.NET Core 3.1+5.0 are .NET Standard 2.1 compliant)

Looking further at the project file, I found that all of the files that were in the netstandard item group were actually compatible with .NET Standard 2.1. Other than SqlAuthenticationProviderManager, all of them appear to have been split due to APIs not in .NET Standard 2.0 such as ValueTask and new Guid(ReadOnlySpan<byte>), etc.

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

Lines 517 to 524 in 85edec9

	<ItemGroup Condition="'$(OSGroup)' != 'AnyOS' AND '$(TargetGroup)' == 'netstandard'">
	<Compile Include="Microsoft\Data\SqlClient\SNI\SNIStreams.Task.cs" />
	<Compile Include="Microsoft\Data\SqlClient\SNI\SslOverTdsStream.NetStandard.cs" />
	<Compile Include="Microsoft\Data\SqlClient\SqlAuthenticationProviderManager.NetStandard.cs" />
	<Compile Include="Microsoft\Data\SqlClient\SqlDiagnosticListener.NetStandard.cs" />
	<Compile Include="Microsoft\Data\SqlClient\SqlDelegatedTransaction.NetStandard.cs" />
	<Compile Include="Microsoft\Data\SqlClient\TdsParser.NetStandard.cs" />
	</ItemGroup>

I moved them to the appropriate item groups, and everything still compiles without error.

After the changes, the only files remaining in the netcoreapp-only group are:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj

Lines 561 to 567 in c53ab68

	<ItemGroup Condition="'$(OSGroup)' != 'AnyOS' AND '$(TargetGroup)' == 'netcoreapp'">
	<Compile Include="..\..\src\Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.cs">
	<Link>Microsoft\Data\SqlClient\SqlDependencyUtils.AssemblyLoadContext.cs</Link>
	</Compile>
	<Compile Include="Microsoft\Data\ProviderBase\DbConnectionPool.NetCoreApp.cs" />
	<Compile Include="Microsoft\Data\SqlClient\SqlConnectionFactory.AssemblyLoadContext.cs" />
	</ItemGroup>

I'm uncertain about these. They compile on both .NET Standard 2.0 and 2.1, but if that's the case then why were they split to netcoreapp only in the first place?

[mattjohnsonpint]

I just pushed another commit that updates the System.Diagnostics.DiagnosticSource package dependency. Although it is brought in transitively already, it's probably better to be explicit incase it gets dropped from the transitive reference in the future.

In doing so, I noticed that it was already being referenced for .NET Core. That's unnecessary, as it's already built-in there. It's only needed on the .NET Standard targets.

[DavoudEshtehari]

I built the project and tested using the package reference against TF netcoreapp3.1, which is not run on the pipeline, and it interestingly looks good 👍🏼. My theory for the netstandard diversion, especially v2.1 is that it wouldn't have been supported at the time those functionalities had been added or they have been classified based on a wrong assumption.

[lcheunglci]

/azurepipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -1.09 ⚠️

Comparison is base (a4f18ca) 70.82% compared to head (ca52e4b) 69.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
- Coverage   70.82%   69.73%   -1.09%     
==========================================
  Files         292      306      +14     
  Lines       61777    61661     -116     
==========================================
- Hits        43752    43002     -750     
- Misses      18025    18659     +634     

Flag	Coverage Δ
addons	92.88% <ø> (+0.49%)	⬆️
netcore	73.28% <ø> (-1.16%)	⬇️
netfx	67.91% <ø> (-1.29%)	⬇️

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

Impacted Files	Coverage Δ
.../Microsoft/Data/SqlClient/SqlDiagnosticListener.cs	100.00% <ø> (ø)

... and 78 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.