-
Notifications
You must be signed in to change notification settings - Fork 285
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
Proof of concept: OpenTelemetry metrics #2409
base: main
Are you sure you want to change the base?
Conversation
This adds an initial framework for updated telemetry using .NET metrics. Many of these telemetry points aren't connected, and the old telemetry hasn't been removed. This has also only been applied to the .NET Core project.
private readonly DbConnectionPoolGroupOptions _options; | ||
private long _poolCount; | ||
|
||
public long TotalMaxPoolSize => _poolCount * (long)_options.MaxPoolSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the options can come from user input is it worth guarding against overflow here and below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think one could be triggered. MaxPoolSize ultimately comes via SqlConnectionString.MaxPoolSize, which is defined as an int and has protection against negative values.
An overflow would be triggered if there were uint.MaxValue
connection pools in the connection pool group with a MaxPoolSize of int.MaxValue
, but the list of connection pools is stored as a ConcurrentDictionary in DbConnectionPoolGroup (which is limited to storing int.MaxValue
entries.)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Telemetry/MetricConstants.cs
Outdated
Show resolved
Hide resolved
…and MetricTagValues
Although it came with NET7, updating |
…T 6.0. This allows the use of UpDownCounter for usage counters.
This hasn't been connected to anything unique to .NET Framework yet, and hasn't been tested.
Thanks - I've just updated this to allow the use of UpDownCounter. I've added enough code to show how I'd expect the .NET Framework version of the library to work and to ensure it compiles, but I'm going to leave this in draft until the overall design can be reviewed. |
Contributes to #2211.
This adds an initial framework for updated telemetry using .NET's new metrics API. Leaving it in draft because I'm conscious that the issue hasn't yet had design approval from the SqlClient team, but this should be a reasonably generic and performant base to feedback on build on for the metrics design. It also provides a spot to put the eventual tracing APIs (when those are eventually standardised) and allows us to remove the project-specific connection pool counters.
I've added instrumentation for the metrics laid out in the OTel specifications, and for the existing .NET Core event counters. In most cases, this isn't wired up to anything.
From some very basic benchmarking, this has no measurable GC or CPU overhead. I don't think it has any explicit lock contention either - just a bundle of Interlocked.Increment/.Decrement calls. The OpenTelemetry SDK is another matter!
Although there's going to be some memory overhead here, it should be minimal. Since most of the tags only vary at the connection pool group level, I'm planning to cache them there and allow SqlConnection and SqlCommand to draw on that reference (unless pooling is disabled, or a specific SqlConnection wants to specify a custom tag.)
There are two trivial points, one piece of additional work, and one specification design decision which I'm not sure I agree with.
Counter<T>
states that it's only supposed to increase. I'm decrementing this type of counter because we currently have to target .NET Standard and .NET 6.0, andUpDownCounter<T>
is only available in .NET 7+.SqlStatistics
. Since the collection overhead of SqlStatistics is practically nil, I'd like to always enable such and push the data into the corresponding metrics. The precision ofDateTime.UtcNow
(used by SqlStatistics) is 0.5ms-15ms, so this might need to start using stopwatches instead, particularly for local or LAN SQL Server instances.The specification design decision surrounds the use of the
pool.name
tag. In cases where the connection string uses Windows authentication (or, in future, perhaps some other kind of process-/thread-level identity impersonation mechanism) it's possible for two connections to have the same connection string, but to be in different connection pools because they've been opened under an impersonation context. The specification rolls both of these up by their connection string. I'm thinking about whether or not to add adb.user
tag to handle this - it'd make it clearer that multiple pools are involved, but might break compatibility with downstream clients which expect one metric perpool.name
.Tagging @roji and @JRahnama for any comments on the topic, direction or methodology. There aren't that many ways to implement these metrics, but I'd prefer to have some feedback from the SqlClient team so that I'm not surprising anyone with an x000-line PR for merge.