-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Service Bus] Prepare tracing methods for message processor and scheduleMessage #16524
Conversation
…TION_STRING" This reverts commit 0ce8d23
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Show resolved
Hide resolved
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Show resolved
Hide resolved
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Show resolved
Hide resolved
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Outdated
Show resolved
Hide resolved
@@ -253,4 +275,75 @@ private static TransactionalState getTransactionState(ByteBuffer transactionId, | |||
transactionalState.setOutcome(outcome); | |||
return transactionalState; | |||
} | |||
|
|||
public static ServiceBusMessage traceMessageSpan(ServiceBusMessage serviceBusMessage, | |||
Context messageContext, String hostname, String entityPath, TracerProvider tracerProvider) { |
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.
How does Kind fits into this ?, should that be passed here ?
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/core/azure-core/src/main/java/com/azure/core/util/tracing/ProcessKind.java
@samvaity Can you look into this PR ?
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.
ProcessKind.MESSAGE
should be used to create a message. Just like in EventHubs.
Line 158 in 914ab8b
Context eventSpanContext = tracerProvider.startSpan(AZ_TRACING_SERVICE_NAME, eventContext, |
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.
The ProcessKind helps the tracer apply specific attributes on the span. For example, in the case of MESSAGE, the tracer will attribute the spanType=Producer, here
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.
@samvaity is this javadoc misleading? It looks like MESSAGE is used for receiving.
/**
* Amqp message process call to receive data.
*/
MESSAGE,
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.
Yeah, I think we initially had this as RECEIVE and later updated to MESSAGE so the left over javadoc. But yes it should be updated to suit process kind "message". AMQP process kind for message spans.
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Show resolved
Hide resolved
…ure/messaging/servicebus/implementation/MessageUtils.java Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
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.
LGTM! Please request @samvaity also to take a peek at this PR to verify if all the conventions for tracing are followed for messaging services.
...ing-servicebus/src/main/java/com/azure/messaging/servicebus/implementation/MessageUtils.java
Show resolved
Hide resolved
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.
Tracing changes looking good.
Consider adding unit tests.
EventHubs has distributed tracing for sending out events and processing events. ServiceBus already has tracing for sending. This PR is to prepare the tracing utility functions for the message processor (to be added, feature requirement #16087).
It also separates method
traceMessageSpan
fromServiceBusMessageBatch
soscheduleMessage
will also be able to use it in the future.The full feature of distributed tracing for ServiceBus for all languages is still under discussion with the service team.
closes #15622