Add ADO.NET metrics specification #1328
Conversation
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
|
Need to make a decision on whether to use |
There was a problem hiding this comment.
This looks great @bgrainger, really looking forward to seeing it in action.
BTW I'm surprised the specs don't include anything around... command execution, which seems to be the single most important metric overall...
|
|
||
| ## Pool Names | ||
|
|
||
| OpenTelemetry Semantic Conventions assume that connection pools can be named. |
There was a problem hiding this comment.
Yeah - though note that no such concept currently exists in any ADO.NET provider as far as I'm aware. We did implement support for per-pool metrics in Npgsql via the older event counters API, using the connection string as the name - but that's not great.
I think introducing a name concept at the data source level is a great idea, opened npgsql/npgsql#5108 to track on the Npgsql side.
API nit: I'd call it HasName rather than UseName.
There was a problem hiding this comment.
Hmm, HasName feels to me like a Boolean property, not a method. All the ASP.NET Builder types seem to have standardised around .AddX or .UseX, hence this choice. (Most methods on NpgsqlDataSourceBuilder are UseX, too.)
| // Assume the existence of a connection pooling implementation. | ||
| private XyzsqlConnectionPool _pool; | ||
|
|
||
| public override async Task OpenAsync(CancellationToken cancellationToken) |
There was a problem hiding this comment.
A similar CloseAsync example could be useful as well (e.g. to show decrementing the "usage" counter), though obviously this is all for illustration purposes anyway.
Commands being executed is tracked by the semantic conventions for tracing; perhaps they didn't (yet?) want to duplicate count and timing of those operations in metrics compared to just aggregating information from the raw trace data? |
|
Sure, but the tracing side is for non-aggregate command-by-command information; I'd expect a command execution histogram metric just like there's a histogram for how long connections were used. Not to mention metrics for command errors, timeouts... |
| static readonly Histogram<float> WaitTimeHistogram = Meter.CreateHistogram<float>("db.client.connections.wait_time", | ||
| unit: "ms", description: "The time it took to obtain an open connection from the pool."); | ||
| static readonly Histogram<float> UseTimeHistogram = Meter.CreateHistogram<float>("db.client.connections.use_time", | ||
| unit: "ms", description: "The time it took to create a new connection."); |
There was a problem hiding this comment.
This appears to be a copy-paste miss, reference uses The time between borrowing a connection and returning it to the pool. as a description for db.client.connections.use_time
There was a problem hiding this comment.
Good catch; I found and fixed that copy/paste error in https://github.com/mysql-net/MySqlConnector/pull/1329/files#diff-22f5eec5ea345b8f0fd8c96f47bb2a7e59d35071c7ad2525d6f251a5ece19d5cR1963 but forgot to come back and update this example.
| ```csharp | ||
| using var dataSource = | ||
| new XyzsqlDataSourceBuilder(connectionString) | ||
| .UseName(“My Pool”) |
There was a problem hiding this comment.
| .UseName(“My Pool”) | |
| .UseName("My Pool") |
|
Archiving the proposed specification in this PR; the merged PR shall represent the definitive specification of the metrics behavior. |
Opening a PR for review and feedback on a proposed specification creating a standard for the way ADO.NET providers report metrics. The goal is to use System.Diagnostics.Metrics APIs to implement the OpenTelemetry Semantic Conventions for Database Metrics.
The
metricsbranch implements this specification.