SqlClient instrumentation span name is not suffient

[stephenjust]

Bug Report

opentelemetry.instrumentation.sqlclient\1.0.0-rc7
opentelemetry\1.1.0

Runtime: net472

Symptom

The SqlClient instrumentation always sets the operation DisplayName value to be equal to db.name. While the spec suggests this is OK, I would argue that it is not helpful given that db.name is already added as a tag.

In this spec the span name is described as:

The span name concisely identifies the work represented by the Span, for example, an RPC method name, a function name, or the name of a subtask or stage within a larger computation. The span name SHOULD be the most general string that identifies a (statistically) interesting class of Spans, rather than individual Span instances while still being human-readable. That is, "get_user" is a reasonable name, while "get_user/314159", where "314159" is a user ID, is not a good name due to its high cardinality. Generality SHOULD be prioritized over human-readability.

For systems that connect to many different databases, the current behavior generates a "name" value with a fairly high cardinality and no indication of what the operation being performed is.

What is the expected behavior?

Spans for SqlClient operations should denote the operation being performed as per the generic Trace spec. In my opinion the spec for database tracing common fields, it would make sense to do the same, and leave database name, table name, etc as Tagged fields. Even if the operation name was "SQL Execute" if no more specific operation name could be identified, that would better enable log filtering and be more understandable.

What is the actual behavior?

Span name == db.name

Reproduce

See https://github.com/open-telemetry/opentelemetry-dotnet/blob/6f3f7e5766612d5f6e71389d053ab3992051811b/src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs#L85

[johnduhart]

For systems that connect to many different databases, the current behavior generates a "name" value with a fairly high cardinality and no indication of what the operation being performed is.

I would argue that this is an edge case. Most applications are going to work with a single database, and using that as the span name will not create a cardinality problem, plus it adheres to the database tracing conventions.

I would not be against adding an opt-out for that behaviour.

[gusemery]

I agree, in the Java implementation the statement is captured and set as the Span Name, e.g. 'Select * from table1'.

I think that at a minimum, we should parse the first statement; or the Stored Procedure name and use that a span name.

[pellared]

It is not recommended to attempt any client-side parsing of db.statement just to get these properties, they should only be used if the library being instrumented already provides them. When it's otherwise impossible to get any meaningful span name, db.name or the tech-specific database name MAY be used.

From: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md

Imagine parsing a CTE or a T-SQL code. It could be buggy and decrease the performance of the application.

I suggest closing the issue.

[gusemery]

I disagree. I think that the objective of tracing is to figure out what is happening, and quickly diagnose it. Almost all of the other stacks work as I suggested, and it's not raised by the framework.

At least we should raise the span as whatever comes up from the Command.Text property. Thus no real loss; it's DB name or whatever is contained in the statement; however I'd suggest a little logic surrounding 'Where' statements as in removing them

-G

[johnduhart]

@gusemery The OpenTelemetry spec defines what should be included in the operation name, it's not open for creative interpretation by each client library. The underlying query can still be included as an attribute.

[gusemery]

@johnduhart I may be mistaken, however what I wrote above matches the spec. Unless I communicated it incorrectly, please see the spec here : https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/database/

I'm only stating that the .Net SQLClient is displaying a different span name than others. And while I understand it may have to be the DB name in some conditions; it's not equal to other frameworks.

[pellared]

For some libraries (e.g. ORM) it would be fine. But with SqlClient (which is very low level) I do not think it is possible without parsing the statements which is NOT recommended by the spec.

[martinjt]

I believe this has been fixed, if that is not the case then feel free to reopen.

[perlun]

Hmm... @martinjt, if this has been fixed, what was the outcome? Can the span name be overridden somehow?

I realize that parsing the SQL query is not doable or practical, but it would it be possible to at least be able to override the operation name where the user has opted-in to doing so? Or would this be contrary to the "spirit" (or letter) of the OpenTelemetry specs?

Our use case is somewhat like this: dotnet/SqlClient#2210 (comment). Set the query name to something explicit for all (or some) SqlCommands, and be able to get this into the collected telemetry data. Getting into a tag works, but it does mean that all DB queries towards a particular database will be "grouped together" which may be a bit to coarse-grained to be really useful e.g. when looking at the telemetry data in web UI.