[semconv]: Add support for new formal DB semantic convention keys #1162
Conversation
ADOT Java currently uses some deprecated incubating semconv keys. This PR adds support for the newly introduced formal semconv keys that replace them, while maintaining backward compatibility by falling back to the legacy keys when necessary. **Deprecated keys:** - DB_NAME - DB_OPERATION - DB_STATEMENT - DB_SYSTEM (Reference: https://github.com/open-telemetry/semantic-conventions-java/blob/release/v1.34.0/semconv-incubating/src/main/java/io/opentelemetry/semconv/incubating/DbIncubatingAttributes.java#L322-L327) **New keys:** - DB_NAMESPACE - DB_OPERATION_NAME - DB_QUERY_TEXT - DB_SYSTEM_NAME **Tests performed:** - Unit tests: `./gradlew build test` - Smoke/contract tests: `./gradlew appsignals-tests:contract-tests:contractTests` - Manual E2E test with SpringBoot sample app.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1162 +/- ##
=============================================
- Coverage 85.71% 67.21% -18.51%
- Complexity 19 525 +506
=============================================
Files 3 54 +51
Lines 49 2693 +2644
Branches 5 373 +368
=============================================
+ Hits 42 1810 +1768
- Misses 3 747 +744
- Partials 4 136 +132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -349,10 +354,7 @@ private static void setRemoteEnvironment(SpanData span, AttributesBuilder builde | |||
| private static String generateRemoteOperation(SpanData span) { | |||
| String remoteOperation = UNKNOWN_REMOTE_OPERATION; | |||
| if (isKeyPresent(span, URL_FULL) || isKeyPresent(span, HTTP_URL)) { | |||
There was a problem hiding this comment.
Not blocking: If you're changing the below code, may as well update this code to use isKeyPresentWithFallback.
| @@ -949,6 +948,15 @@ private static String getRemoteService(SpanData span, AttributeKey<String> remot | |||
| return remoteService; | |||
| } | |||
|
|
|||
| static String getRemoteServiceWithFallback( | |||
There was a problem hiding this comment.
Not blocking: you could just update the signature of getRemoteService to be getRemoteService(SpanData span, AttributeKey<String>... remoteServiceKeys) and have it iterate through all keys until there is a hit. I imagine a future where we have to support 3+ different keys, given how long it takes upstream to update.
| @@ -949,6 +948,15 @@ private static String getRemoteService(SpanData span, AttributeKey<String> remot | |||
| return remoteService; | |||
| } | |||
|
|
|||
| static String getRemoteServiceWithFallback( | |||
| SpanData span, AttributeKey<String> remoteSvcKey, AttributeKey<String> remoteSvcFallbackKey) { | |||
There was a problem hiding this comment.
Lets try to follow class convention, don't abbreviate:
| SpanData span, AttributeKey<String> remoteSvcKey, AttributeKey<String> remoteSvcFallbackKey) { | |
| SpanData span, AttributeKey<String> remoteServiceKey, AttributeKey<String> remoteServiceFallbackKey) { |
There was a problem hiding this comment.
The tests in this class are intentionally robust as the logic is complex and we really want to be sure what it is doing. Lets take the effort and update relevant tests with these new variables. Namely:
- testGetDBStatementRemoteOperation - add case that shows DB_QUERY_TEXT takes priority over DB_STATEMENT but not DB_OPERATION
- testDBClientSpanWithRemoteResourceAttributes - add case for DB_NAMESPACE
- testRemoteAttributesCombinations - add cases for DB_SYSTEM_NAME/DB_OPERATION_NAME
ADOT Java currently uses some deprecated incubating semconv keys. This PR adds support for the newly introduced formal semconv keys that replace them, while maintaining backward compatibility by falling back to the legacy keys when necessary.
Deprecated keys:
(Reference: https://github.com/open-telemetry/semantic-conventions-java/blob/release/v1.34.0/semconv-incubating/src/main/java/io/opentelemetry/semconv/incubating/DbIncubatingAttributes.java#L322-L327)
New keys:
Tests performed:
./gradlew build test./gradlew appsignals-tests:contract-tests:contractTestsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.