Use low cardinality names for database spans

[tedsuo]

Currently, in some cases database spans use the SQL statement as the span name. This creates a couple of issues:

• The cardinality is too high to be useful as an index
• Statements may need to be scrubbed for security/privacy reasons. While it is fine to scrub a secondary attribute like db.statement, mutating a primary index like span name
• Users find it disturbing to see SQL statements in the span name.

To clarify, SQL templates without variables may work well as a span name. The spec currently recommends using SQL templates when available. But, when a template is not available, we should fall back to a low cardinality option.

Spec details: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md

Span name should be set to low cardinality value representing the statement 
executed on the database. It may be stored procedure name (without argument), 
sql statement without variable arguments, etc. When it's impossible to get any 
meaningful representation of the span name, it can be populated using the same 
value as db.instance.

The current spec is vague about what to use in place of a template. The suggestion I have heard requested the most is to use the SQL Command (SELECT, INSERT, UPDATE, etc). If it is easy enough to switch, I would suggest this option for now. Otherwise, falling back to the db.instance value would be sufficient.

[trask]

Based on discussion in SIG meeting today, let's proceed with this:

• For PreparedStatements, we continue using the query as the span name (no change).

• For non-PreparedStatements, let's avoid parsing the SQL query for now, and just use db.instance as the span name. @johnbley is planning to add SQL parsing in the near future, at which time we can leverage that to produce a better span name.

[sleighzy]

I have some changes for this issue, but have a couple of quick questions.

• The getClientInfo exception unit test highlighted that the it is possible for the db info to not be populated under some circumstances. Is it acceptable to use the generic "DB Query" string for the span name in this case in the same manner as what is done for prepared statements? See https://github.com/open-telemetry/opentelemetry-auto-instr-java/blob/master/instrumentation/jdbc/src/main/java/io/opentelemetry/auto/instrumentation/jdbc/JDBCDecorator.java#L103. This should hopefully be extremely rare.
• The db.instance is pretty generic and in some cases not super helpful, e.g. you may have multiple databases hosted under the same instance. Would it be nicer to use a combination of both the instance name and database name? I don't have any particular preference as to the display format of this, e.g. "instanceName:databaseName".

[trask]

Hi @sleighzy! Sorry for not keeping this issue updated. #366 was just merged yesterday implementing SQL scrubbing, which removes sensitive data and reduces cardinality. I wonder if we still need this?

[sleighzy]

I agree, the SQL scrubbing brings this in alignment with the prepared statement implementation. I'll pull down those latest changes and test them in our environments. Up to you folk but I see no further need for this ticket then. Thanks.

[trask]

Great, I'm going to close it then 😄.

[iNikem]

Just for historical record. Otel spec (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md) says the following: "Span name should be set to low cardinality value representing the statement executed on the database". So we have to use sql statement for spans.