Merge | Merge metrics/performance counter methodology #3251
Conversation
Move functionality into a new Microsoft.Data.SqlClient.Diagnostics.SqlClientMetrics type. This common interface encapsulates both event counters and performance counters.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3251 +/- ##
==========================================
- Coverage 72.98% 0 -72.99%
==========================================
Files 299 0 -299
Lines 57215 0 -57215
==========================================
- Hits 41760 0 -41760
+ Misses 15455 0 -15455
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:
|
...icrosoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientMetrics.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientMetrics.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientMetrics.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Diagnostics/SqlClientMetrics.cs
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@edwardneal ok, so looking at these failures, I thiiiink I understand why these failures are happening. Sorta. The MetricsTest file is only pulled into the test project if it's on test set 3 (or no test set), that's why only some of the test jobs are failing (specifically the ones that end with 3). That test file has references to the event source class, which is internal. Since we don't have internalsvisibleto the test projects (yet ... @mdaigle did you give up on that?), I don't think we can access that class from the test project. The part I don't get tho, if I build it in the IDE, it doesn't complain at all. But that build specifies no test set, which makes me wonder if something including a file or property that makes it all work. Nothing stood out at me, but I didn't go through it with a fine toothed comb. |
|
@benrr101 I'm going to revert the change I made to allow internalsVisibleTo because, like you saw, it works in the IDE, but not in MSBuild, which is confusing. MSBuild compiles using the reference assemblies which don't currently contain any of the internal types. Visual studio (and I'm assuming Rider, too) compiles using the implementation assemblies, which do contain the internal types. |
|
@edwardneal Update - apparently something is really messed up with how we have our projects set up.... As per @mdaigle, the ref projects are being used for the internalsvisibleto when building from command line, but the implementation projects are being used when building from IDE. Thus, we see the failures of your unit tests when we build the project during CI. I don't think we have a good plan for fixing this anytime soon (it's sort of predicated on a lot of other things being fixed). But in the meantime, Malcolm is going to back out the internalsvisibleto change so people don't get the wrong idea that they can use internal classes in test projects safely. To address this ... I don't know what's best. I'd like to keep the tests in the codebase so we can use them someday, but maybe just comment them out? (it grosses me out to even type that) |
|
Good catch, thanks - I'd not yet seen the test results. It's worth keeping in mind that this test originally used reflection; I'll swap it back to doing so, and we can avoid the problem completely. |
|
I've adjusted MetricsTest to use reflection again - could I have another CI run please? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@edwardneal please don't mind that I just went ahead and fixed the merge conflict for you :) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Relates to #1261, but also lays the groundwork for #2211.
SqlClientcurrently has two different ways to record metrics - .NET Core has counters on the EventSource, and .NET Framework has performance counters. These are usually recorded at the same time, but they're separated by various
#if NETconditional compilation blocks.This PR consolidates both metric surfaces into a single
SqlClientMetricsclass, with variousHardConnectRequestmethods which increment a performance counter or event counter. This means that we've no longer got this type of conditional compilation scattered throughout the connection logic.@benrr101 - this is the outcome of the conversation we had while merging DbConnectionInternal. I've split it out so it's easier to review.
I'd appreciate a CI run please.