DiagnosticSource listener/event names for SqlClient and other ADO providers

[roji]

As part of PRs submitted by @liuhaoyang to add DiagnosticSource tracing support to Npgsql and to MySqlConnector, a conversation has started in npgsql/npgsql#1910 about how this support should look like.

The DiagnosticSource docs set forth some clear guidelines and what to do and what not to do - have different listeners for different event categories (for efficient filtering), have short event name, etc. However, as @bgrainger noted, looking at the SqlClient implementation doesn't seem to implement things in this way:

• All SqlClient events are emitted under a single listener (SqlClientDiagnosticListener), rather than splitting them across different listeners for connection, command, transaction.
• Event names are prefixed with System.Data.SqlClient. even though they're already scoped to the SqlClient listener, making long event names (the guidelines recommend < 16 characters)
• Event names are named Before/After rather than the recommended Start/Stop.

The first question is whether it makes sense for MySqlConnector and Npgsql to implement things differently than SqlClient, following the guidelines. This would mean less portable/coherent behavior across providers, but would be a better and more guidelines-conforming implementation.

The second question is what the backwards compatibility guarantees of DiagnosticSource listeners and event names are, and whether a change in SqlClient's DiagnosticSource implementation would make sense.

Note also that @bgrainger proposed adding DiagnosticSource support by wrapping DbConnection/DbCommand/DbTransaction objects, rather than building in support into the drivers themselves. Apart from being truly pay-per-play, it would also allow getting events from providers which haven't implemented anything yet and may not do so. However, a benchmark showed that the overhead of DiagnosticsSource is extremely small when it isn't enabled (3.6ns per check), so we seem to be in agreement that it's worth building support into the provider rather than wrapping. It would be interesting to hear what you guys think.

Resources:

• https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/DiagnosticSourceUsersGuide.md

[roji]

/cc @bgrainger @liuhaoyang @divega @ajcvickers

[liuhaoyang]

FYI @yuleyule66

[divega]

Thanks for the heads up @roji.

We have talked about the possible benefits of having a standardized pattern for event names and categories across ADO.NET providers. Ideally a diagnostics tool listening would be able to identify at least the common events without having knowledge of specific providers. We also talked about the possibility of adding helpers in System.Data.Common to make this easier.

Since you are doing/discussing this now, I think it would be great if you could come up with a good design that helps those goals, regardless of whether we add helpers in System.Data.Common now or whether you have a copy somewhere else.

Personally I don't think you need to align what you do with the diagnostics code in SqlClient for .NET Core if there are problems with it and if starting from scratch helps you come up with something better. Hopefully we can update SqlClient latter to align with whatever you come up with.

Besides @ajcvickers, I abelieve @anpete, @avanderhoorn, @vancem and others here can help with feedback.

[bgrainger]

I started drafting some thoughts/proposal about ADO.NET DiagnosticSource events yesterday; sharing the document here in case it's useful: https://docs.google.com/document/d/1SKrW3uKQk8l9UOkGmaW_TjKk0qCm_lpZm5p5u69Zp4E/edit?usp=sharing

(Google Docs for now, happy to move to Markdown if people think it's useful. DM me if you want editing rights.)

[roji]

@bgrainger have made some comments, I think it may be a good idea to move the conversation here, to make it more accessible etc.

[vancem]

I would recommend that you create an markdown file for the spec in the ngpsql repo and submit it as a pull request (and put a link to it here). It allows us to use line level pull request to give feedback, and it becomes a permanent document afterward (write it as if you were explaining how to use it to new users).

As @divega indicated, it is better to follow the guidelines than to try to be consistent with DiagnosticSources that predate the guidelines. Realistically we will always have code that is not following best practices, but the less of it we have the better. It still is reasonably early days.

Note that you have a choice on whether to pass the DbTransation and DbConnection as the object, or to pass some numeric ID. The former works great IN the process, but the later is more useful OUTSIDE the process. Generally it is probably better to send an ID, (and then you can add events to tell more about that ID as needed). Also sending real objects can lead to abuse where code modifies the object during the event callback (bad, but we can't prevent it). Using IDs makes this impossible.

I do believe you want to work with Activities. They are the way of tracking intervals of time (and more importantly causality). Ideally you would not need a OperationID in your event because Activities already do this. (some redundancy is OK, especially if you have native code where activity tracking may not be perfect).

@brianrob

We are also updating the DiagnosticSource guidance see dotnet/corefx#29552.

[roji]

Relevant:

• https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry.Instrumentation.SqlClient/README.md
• https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md