-
Notifications
You must be signed in to change notification settings - Fork 946
Implementation for the latest Messaging Semantic conventions. #13192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SemconvStability.java
…ll' and it results in span name with 'null' in the name.
…n where PropagatorBasedSpanLinksExtractor is used
… make the spans correctly connected and hierarchical instead of separeted.
… and new naming conventions
@cbos are you doing this for fun or do you intend to get this merged? |
@laurit |
@laurit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually it would be nice if we also had tests running with the stable semconvs. For db tests we use
tasks {
val testStableSemconv by registering(Test::class) {
jvmArgs("-Dotel.semconv-stability.opt-in=database")
}
check {
dependsOn(testStableSemconv)
}
}
I guess the tests can be added a bit later once the other parts of the PR are finished.
@@ -13,16 +13,33 @@ | |||
* that may be used in a messaging system. | |||
*/ | |||
public enum MessageOperation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javadoc of the class also links to https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md#operation-names that could be replaced with https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/messaging-spans.md#operation-types
*/ | ||
@Deprecated | ||
String operationName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a package private method it is not part of the public api and can be renamed without preserving the original method. If you wish to keep the original method then it would be better to make the old method class the new one.
internalSet(attributes, MESSAGING_MESSAGE_BODY_SIZE, getter.getMessageBodySize(request)); | ||
internalSet( | ||
attributes, MESSAGING_MESSAGE_ENVELOPE_SIZE, getter.getMessageEnvelopeSize(request)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you move these from start to end?
String getDestination(REQUEST request); | ||
default Long getBatchMessageCount(REQUEST request, @Nullable RESPONSE response) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rearranging methods like that complicates the review. It is hard to tell what methods have been added and what was just moved.
import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder; | ||
import javax.annotation.Nullable; | ||
|
||
public final class MessagingNetworkAttributesExtractor<REQUEST, RESPONSE> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have ServerAttributesExtractor
and NetworkAttributesExtractor
are you sure we should add a new messaging specific extractor?
* This class is internal and is hence not for public use. Its APIs are unstable and can change at | ||
* any time. | ||
*/ | ||
public class PropagatorBasedBaggageExtractor<REQUEST> implements ContextCustomizer<REQUEST> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required by the specification? If I understand correctly what this is supposed to do it seems to have a limited usefulness. Perhaps it would be better to introduce it in a separate PR?
MessageOperation operation = MessageOperation.PUBLISH; | ||
MessageOperation operation = | ||
emitStableMessagingSemconv() ? MessageOperation.SEND : MessageOperation.PUBLISH; | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line can be deleted
enum SqsAttributesGetter | ||
implements | ||
MessagingAttributesGetter<Request<?>, Response<?>>, | ||
MessagingNetworkAttributesGetter<Request<?>, Response<?>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not much point in adding the network attributes interface when no methods are implemented
SpanKindExtractor.alwaysConsumer(), | ||
toSqsRequestExtractors(attributesExtractors()), | ||
singletonList(messagingAttributeExtractor), | ||
messagingReceiveInstrumentationEnabled); | ||
messagingReceiveInstrumentationEnabled || SemconvStability.emitStableMessagingSemconv()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My advice would be to do the attribute changes in one PR and leave any potential changes to trace structure for followup PRs. Changes to trace structure will most likely require new tests and will make your PR much larger. Also as far as I can tell https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/#message-creation-context-as-parent-of-process-span hints that having the process
span as child of publish
span is still allowed. Having these in one trace seems to be preferred by users.
public static Timer onEnter() { | ||
public static Timer onEnter(@Advice.This MessageConsumer consumer) { | ||
|
||
if (JmsConfig.EXPERIMENTAL_CONSUMER_PROCESS_TELEMETRY_ENABLED) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change is not related to the semantic conventions consider submitting it as a separate PR.
@laurit |
Update Messaging span with the latest semantic conventions.
https://github.com/open-telemetry/semantic-conventions/blob/v1.30.0/docs/messaging/messaging-spans.md
By default nothing changes, it adheres to this: