Add db.system.name support to r2dbc#16251
Conversation
trask
force-pushed
the
r2dbc-db-system
branch
from
3b12f5d to
5263cef
Compare
trask
force-pushed
the
r2dbc-db-system
branch
from
5263cef to
7f91700
Compare
# Conflicts: # instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/db/DbClientAttributesExtractor.java
| this.system = | ||
| originalConnection != null | ||
| ? originalConnection | ||
| .getMetadata() | ||
| .getDatabaseProductName() | ||
| .toLowerCase(Locale.ROOT) | ||
| .split(" ")[0] | ||
| : OTHER_SQL; |
There was a problem hiding this comment.
db.system is currently captured in not the best way
This PR adds db.system.name, based on the jdbc url's "driver" component similar to jdbc instrumentation.
It adds DbClientAttributesGetter.getDbSystem() in order to support both old and new.
There was a problem hiding this comment.
Wondering whether this is necessary. The current convention seems to be that getDbSystemName returns the old value that is mapped to the new value in DbClientAttributesExtractor (but not in DbClientSpanNameExtractor.computeSpanNameStable). Perhaps we should start returning stable values from getDbSystemName and reverse the mapping?
Could also drop the old logic for getting the db system name.
There was a problem hiding this comment.
I agree we could make this behavioral breaking change without a major version bump as it's a bug fix.
My general thought with the database semconv changes has been to try to leave existing untouched and only fix things under the stable semconv flag.
Semi-related, I was planning to remove the centralized mapping out of the Instrumentation API (see also in #16277), under the idea that the Instrumentation API shouldn't know about the individual conventions for cassandra, elasticsearch, postresql, mysql, etc.
There was a problem hiding this comment.
In my opinion having the mapping in the instrumentation api is fine since it is only for transition, having it in one place could make removing it easier.
There was a problem hiding this comment.
also similar to #16304 (comment), this supports external instrumentations to control the mapping used (again though may not be super realistic scenario since they'd have to stick to our timeline as we'll be removing the support in 3.0)
github-actions
Bot
added
the
test native
2 participants
No description provided.