dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
817 stars 271 forks source link

Emit OpenTelemetry tracing #2210

Open roji opened 7 months ago

roji commented 7 months ago

OpenTelemetry has become the de-facto standard for distributed, cross-platform tracing; it is being rapidly adopted both inside the .NET ecosystem and outside. OTel defines semantic conventions for how a database client traces information; these specifications are currently in experimental state, but stabilization is likely start happening soon.

There is a separate instrumentation library, OpenTelemetry.Instrumentation.SqlClient, which AFAIK uses the DiagnosticSource instrumentation already emitted by SqlClient to then emit OpenTelemetry tracing.

However, ideally SqlClient would emit this tracing data directly, without requiring a 3rd-party library.

Wraith2 commented 7 months ago

Why is it better to emit third party specific information rather than have their adapter deal with the diagnostic information we already emit?

roji commented 7 months ago

@Wraith2 this isn't 3rd-party specific information, but rather the emerging standard for how database clients emit tracing information; fundational components in the .NET ecosystem (such as ASP.NET itself, but also Npgsql) are being instrumented to emit OpenTelemetry tracing directly, which is the best user experience and guarantees both accurate tracing and best performance. This really belongs in the driver rather than some 3rd-party adapter which may or may not have access to the needed information in an efficient way.

Wraith2 commented 7 months ago

If there is additional information needed in the diagnostic data that we emit then we should identify and add it so that it can be available to the open telemetry adapter. I've already got an issue open at open telemetry about changing the diagnostic information to be strongly typed so it's more aot friendly so adding any new information would be best done along with that work.

I don't see why it's beneficial to have open telemetry as s direct dependency/implementation inside sqlclient though. It adds complexity and another set of diagnostic logging (we already have 2) and isn't needed by many people. If we want to make things pay-for-play then an external adapter that people can opt into sounds like the best approach to me.

roji commented 7 months ago

I don't see why it's beneficial to have open telemetry as s direct dependency/implementation inside sqlclient though.

There's no OpenTelemetry dependency - everything needed to emit tracing (or metrics) data is already present in the framework (e.g. System.Diagnostics.ActivitySource). In addition, the APIs have been optimized so that their overheads when tracing isn't enabled is negligible.

So there's no real pay-per-play aspect here, and no real downside to implementing this in-the-box for people.

As I wrote above, this is becoming the de-facto standard for distributed tracing - not just in .NET. Note that this is also a different type of tracing: this isn't about adding tracing events to each and every function, generating very large dumps for method-by-method debugging; it's much more about emitting high-level information about what database commands are being executed, how long they're taking, and reporting various aspects about them. It's indeed probably possible to derive that information from SqlClient's existing tracing, but that kind of approach is always less efficient and requires that users discover and add yet another dependency.

I suggest taking a look at how this look for Npgsql; here's the doc page (I know some demos are planned for this in .NET Conf).

vishweshbankwar commented 7 months ago

I don't see why it's beneficial to have open telemetry as s direct dependency/implementation inside sqlclient though.

There's no OpenTelemetry dependency - everything needed to emit tracing (or metrics) data is already present in the framework (e.g. System.Diagnostics.ActivitySource). In addition, the APIs have been optimized so that their overheads when tracing isn't enabled is negligible.

So there's no real pay-per-play aspect here, and no real downside to implementing this in-the-box for people.

As I wrote above, this is becoming the de-facto standard for distributed tracing - not just in .NET. Note that this is also a different type of tracing: this isn't about adding tracing events to each and every function, generating very large dumps for method-by-method debugging; it's much more about emitting high-level information about what database commands are being executed, how long they're taking, and reporting various aspects about them. It's indeed probably possible to derive that information from SqlClient's existing tracing, but that kind of approach is always less efficient and requires that users discover and add yet another dependency.

I suggest taking a look at how this look for Npgsql; here's the doc page (I know some demos are planned for this in .NET Conf).

+1. Having libraries natively instrumented is the ideal case. As an example, HttpClient and ASP.NET Core are also moving towards that change. .NET8.0 has added metrics support in HttpClient and ASP.NET Core. Reference: https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics-aspnetcore.

Tracing is considered for .NET9.0: see https://github.com/dotnet/runtime/issues/93019.

@roji How does Npgsql handle experimental semantic conventions part?

roji commented 7 months ago

@vishweshbankwar Npgsql implements the current database tracing semantic conventions in their experimental state, and that support is itself documented as experimental in our docs. When the specs mature and reach stable, we'll align to those (users may need to react).

cijothomas commented 7 months ago

Upvoting :) ❤️ to see this!