Supporting Event counter in .Net core

[DavoudEshtehari]

The following counters are added to .Net core 3.1, .Net standard 2.1 and above:

1. HardConnects: The number of actual connections that are being made to servers
2. HardDisconnects: The number of actual disconnects that are being made to servers
3. ActiveHardConnections: The number of actual active connections that are being made to servers
4. SoftConnects: The number of connections we get from the pool
5. SoftDisconnects: The number of connections we return to the pool
6. ActiveSoftConnections: The number of active pool connections
7. NumberOfNonPooledConnections: The number of connections that are not using connection pooling
8. NumberOfPooledConnections: The number of connections that are managed by the connection pooler
9. NumberOfActiveConnectionPoolGroups: The number of unique connection strings
10. NumberOfInactiveConnectionPoolGroups: The number of unique connection strings waiting for pruning
11. NumberOfActiveConnectionPools: The number of connection pools
12. NumberOfInactiveConnectionPools: The number of connection pools
13. NumberOfActiveConnections: The number of connections currently in-use
14. NumberOfFreeConnections : The number of connections currently available for use
15. NumberOfStasisConnections: The number of connections currently waiting to be made ready for use
16. NumberOfReclaimedConnections: The number of connections we reclaim from GC'd external connections

For Microsoft.Data.SqlClient.EventSource, you can use .Net core global CLI tools: dotnet-counters and dotnet-trace in Windows or Linux and PerfView in Windows.

• dotnet-counters monitor Microsoft.Data.SqlClient.EventSource -p
• PerfView /onlyProviders=*Microsoft.Data.SqlClient.EventSource:EventCounterIntervalSec=1 collect

related issue #523

[cheenamalhotra]

Please add test cases to validate counters that provide maximum possible code coverage.

[Wraith2]

When trying to use pre-processor definitions to include functionality it's hard to make sure that you include code only where it is supported and make sure it is included in later builds using definitions that don't exist yet. For example using #if NETCOREAPP3 || NETSTANDARD21 is ok but when a NET50 build target or later is added the code won't be included without a time consuming audit of the codebase to add it to these lists.

Annoying as it is, the best way to avoid this is to is to use exclusions. so something like #if !NETCOREAPP20 && !NETCOREAPP21 && !NETSTANDARD20 because the code is then included in any new TFM's that are added and it should be the case that any new TFM contains the required api.

[roji]

In addition to the below implementation comment, the de-facto standard seems to be to just use the package name/namespace - so I'd use Microsoft.Data.SqlClient (and omit the trailing .EventSource)

[DavoudEshtehari]

When trying to use pre-processor definitions to include functionality it's hard to make sure that you include code only where it is supported and make sure it is included in later builds using definitions that don't exist yet. For example using #if NETCOREAPP3 || NETSTANDARD21 is ok but when a NET50 build target or later is added the code won't be included without a time consuming audit of the codebase to add it to these lists.

Annoying as it is, the best way to avoid this is to is to use exclusions. so something like #if !NETCOREAPP20 && !NETCOREAPP21 && !NETSTANDARD20 because the code is then included in any new TFM's that are added and it should be the case that any new TFM contains the required api.

Great point @Wraith2,

Our approach is to keep update the directive definitions in the csproj file to avoid redundant directive conditions in addition to support next .Net target frameworks.
So, according to the current supported frameworks netcoreapp2.1;netcoreapp3.1;netstandard2.0;netstandard2.1 , the following definition supports .Netcore 3.1 and above:

  <PropertyGroup Condition="'$(TargetGroup)' == 'netcoreapp' AND '$(TargetFramework)' != 'netcoreapp2.1'">
    <DefineConstants>$(DefineConstants);NETCORE3</DefineConstants>
  </PropertyGroup>

[DavoudEshtehari]

In addition to the below implementation comment, the de-facto standard seems to be to just use the package name/namespace - so I'd use Microsoft.Data.SqlClient (and omit the trailing .EventSource)

This would be a good idea. The only concern is this is a breaking change.

[roji]

In addition to the below implementation comment, the de-facto standard seems to be to just use the package name/namespace - so I'd use Microsoft.Data.SqlClient (and omit the trailing .EventSource)

This would be a good idea. The only concern is this is a breaking change.

Did you already have an EventSource in .NET Core before this PR? I'm not sure what the backwards compat bar is on EventSource names.

In any case, the perf counters (which are new) could go into a new EventSource as well with the new naming, even if that could be slightly confusing.

[pi3k14]

We are now looking at December and this was ready in September ...
What is not going on?

[DavoudEshtehari]

Hi @pi3k14,
According to the higher priority on the other tasks and the recent GA release, working on it was postponed to later. The project and milestone will be updated after planning.

[winseros]

@DavoudEshtehari, maybe any help wanted on this one?

[DavoudEshtehari]

We always welcome other contributors' help @winseros.
To contribute to this work item you can create a new fork from this source code first. The remaining task is the assess and validate the counters through the comprehensive unit tests.

[winseros]

Hi @DavoudEshtehari, I inspected the PR and see there are some tests in the EventCounterTest.cs.

So there have to be more sophisticated tests or assertions?

[DavoudEshtehari]

Hi @winseros,

Happy holidays. 🙂
Those tests assess the implemented event counters. Investigating their accuracy according to where these counters are called needs a profound study of the driver to discover the new possible spots and adding related tests if it's required.

[roji]

@DavoudEshtehari you may want to check out the event counter testing approach being done by @ajcvickers for EF in dotnet/efcore#23826. Actually setting up an EventListener may be quite flaky (and not to mention slow) - the idea is to simply use reflection to access the counters directly.

[DavoudEshtehari]

the idea is to simply use reflection to access the counters directly

Hi @roji,

After checking the source code, we found EF core increment/decrement the inner variables without checking if EventSource is enabled by the IsEnabled() method and the only use of this method is before creating the PollingCounter objects inside the EntityFrameworkEventSource .OnEventCommand event.
The inner variables always manipulate; even, when there is no listener. May I ask you to clarify its reason if it was intentional?
Generally, we skip manipulating the inner variables when there is no listener by checking the IsEnabled(). So, we couldn't use reflection in these tests.

[roji]

@DavoudEshtehari the logic is that the increment/decrement is too cheap to justify the IsEnabled check (ASP.NET do the same with the event counters). In fact, logic branches (conditions) frequently turn out to be more expensive at the CPU level than the code they encompass.

Note the difference with normal, non-polling event counters, where calling the method actually triggers the event being sent, as opposed to a simple increment/decrement. In that case the condition is indeed justified since sending the event is an expensive operation.

[omatrot]

Is there something we could do to allow this merge request to make progress? I have a weird problem with connection pooling and can't diagnose it without Event counters.

[cheenamalhotra]

Just 1 pending suggestion, rest approved.