Merge | Enable SqlClientDiagnosticListener on .NET Framework #3493
Conversation
|
I was kinda wondering how hard it would be to add diagnostics to netfx while messing around with the differences in sqlcommand. It'd definitely ease up the differences. I thiiiink the best way to sequence these changes would be to, as you suggested, leave SqlCommand alone until we get it merged (will take a while, i'm trying a new strategy this time). The merged SqlCommand will have all the diagnostics code wrapped in #if NET, so we can just remove them when it's all ready. But I'd also like to get @David-Engel's feedback on this before you get too deep. |
benrr101
added
Public API 🆕
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3493 +/- ##
==========================================
- Coverage 63.19% 60.54% -2.66%
==========================================
Files 274 269 -5
Lines 62478 61247 -1231
==========================================
- Hits 39486 37083 -2403
- Misses 22992 24164 +1172
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think this is great to have, given that .NET Framework support is now available through the provided package. We have learned that products generally benefit from these diagnostics, so I am in full support of porting this over to NetFx! |
This reverts commit d14a561. Enables ongoing merge work on SqlCommand to continue.
|
Thanks all. I'll pull this out of draft for review, and we'll just make sure that the telemetry in SqlCommand is fully merged by the time v7.0 is released. There are only about 100 lines of changes there, so it should be reasonably simple. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Looking back, it seems we didn't add it to netfx initially because the underlying support wasn't there. Now it's there via the extension package. Seems like a no brainer to extend SqlClientDiagnosticListener to NetFx now. |
|
@edwardneal - Can you resolve conflicts here? I'll do a review once that's done. |
|
Thanks @paulmedynski, I've just completed the merge. |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Description
This continues work on the SqlConnection merge, tackling one of a few functional differences between netcore and netfx. The SqlClientDiagnosticListener and its associated strongly-typed objects make up a very large API surface, and this PR ports them all over. It also makes the necessary changes to SqlConnection and SqlTransaction (but not SqlCommand) to send the associated events.
I've limited the scope here to SqlConnection and SqlTransaction to stay out of the way of #3473 and its successors. I can wait for #3473 to be merged and turn this into a single clean PR which enables SqlClientDiagnosticListener everywhere. If that's likely to slow down your work as you work through the SqlCommand merge @benrr101 then we can review this PR as-is.
By implication, I've added a dependency from the netfx SqlClient on System.Diagnostics.DiagnosticSource 8.0.1.
Issues
Contributes to #1261. Avoids touching SqlCommand to avoid merge conflicts with #3473. Loosely relates to #2210.
Testing
Existing tests suffice; I've enabled test coverage for the newly-added methods, and this coverage passes.