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

Enable SqlDiagnosticListener on .NET Standard #1931

Merged
merged 4 commits into from Apr 3, 2023

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Feb 23, 2023

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

Fixes #1928

@mattjohnsonpint
Copy link
Contributor Author

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
Copy link
Contributor Author

mattjohnsonpint commented Feb 23, 2023

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 mattjohnsonpint marked this pull request as draft February 23, 2023 04:38
@mattjohnsonpint mattjohnsonpint changed the title Enable SqlDiagnosticListener on .NET Standard and .NET Framework Enable SqlDiagnosticListener on .NET Standard Feb 23, 2023
@mattjohnsonpint
Copy link
Contributor Author

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 mattjohnsonpint marked this pull request as ready for review February 23, 2023 06:41
@mattjohnsonpint
Copy link
Contributor Author

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

@lcheunglci
Copy link
Contributor

lcheunglci commented Feb 24, 2023

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 mattjohnsonpint marked this pull request as draft February 24, 2023 17:46
@mattjohnsonpint
Copy link
Contributor Author

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

@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented Feb 24, 2023

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.

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

<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
Copy link
Contributor Author

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.

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review February 25, 2023 16:01
@DavoudEshtehari
Copy link
Member

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

/azurepipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov
Copy link

codecov bot commented Mar 28, 2023

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.

@JRahnama JRahnama merged commit 4e323a0 into dotnet:main Apr 3, 2023
130 checks passed
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
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.

DiagnosticListener no longer works on .NET Core 3.1 or .NET 5
4 participants