Deprecate DbClientCommonAttributesGetter #15139
Conversation
trask
changed the title
trask
changed the title
...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesGetter.java
Show resolved
Hide resolved
...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesGetter.java
Show resolved
Hide resolved
| @Override | ||
| public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) { | ||
| super.onStart(attributes, parentContext, request); | ||
| // Common attributes |
There was a problem hiding this comment.
could consider placing the common part to util class or perhaps even a static package private method in DbClientAttributesExtractor that could also be called from here.
There was a problem hiding this comment.
I think this worked out, ptal
...ava/io/opentelemetry/instrumentation/api/incubator/semconv/db/SqlClientAttributesGetter.java
Show resolved
Hide resolved
| import io.opentelemetry.instrumentation.api.internal.SpanKeyProvider; | ||
| import javax.annotation.Nullable; | ||
|
|
||
| abstract class DbClientCommonAttributesExtractor< |
There was a problem hiding this comment.
can delete this right away since not public
There was a problem hiding this comment.
could also consider keeping the common extractor, since it is just an implementation detail, deleting it is fine too
| public final class SqlClientAttributesExtractor<REQUEST, RESPONSE> | ||
| extends DbClientCommonAttributesExtractor< | ||
| REQUEST, RESPONSE, SqlClientAttributesGetter<REQUEST, RESPONSE>> { | ||
| implements AttributesExtractor<REQUEST, RESPONSE>, SpanKeyProvider { |
There was a problem hiding this comment.
this change is ok since DbClientCommonAttributesExtractor wasn't public
| /** | ||
| * SqlClientAttributesExtractor will try to populate db.operation.name based on {@link | ||
| * #getRawQueryTexts(REQUEST)}, but this can be used to explicitly provide the operation name. | ||
| */ | ||
| @Override | ||
| @Nullable | ||
| default String getDbOperationName(REQUEST request) { | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * SqlClientAttributesExtractor will try to populate db.operation.name based on {@link | ||
| * #getRawQueryTexts(REQUEST)}, but this can be used to explicitly provide the operation name. | ||
| */ | ||
| @Override | ||
| @Nullable | ||
| default String getDbQueryText(REQUEST request) { | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * SqlClientAttributesExtractor will try to populate db.operation.name based on {@link | ||
| * #getRawQueryTexts(REQUEST)}, but this can be used to explicitly provide the operation name. | ||
| */ | ||
| @Override | ||
| @Nullable | ||
| default String getDbQuerySummary(REQUEST request) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
these methods have same defaults in super interface, but we have comments to remove the defaults in the super interface at which time we'll still want them here
| extends DbClientCommonAttributesExtractor< | ||
| REQUEST, RESPONSE, DbClientAttributesGetter<REQUEST, RESPONSE>> { | ||
| implements AttributesExtractor<REQUEST, RESPONSE>, SpanKeyProvider { |
There was a problem hiding this comment.
this change is ok since DbClientCommonAttributesExtractor wasn't public
|
🔧 The result from spotlessApply was committed to the PR branch. |
4 participants
Semconv doesn't have this concept.
HttpCommonAttributesGetter makes sense b/c it's common attributes across both client and server.
But in semconv, Sql "extends" Db semantic conventions, and so I think our getter hierarchy should do the same.
In practice, this means that Sql getter now has things like getDbOperationName(), which I think is probably good, though we can default them to
nullin the Sql getter interface to make them not required.Related to #12608