Skip to content
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

Merged
merged 7 commits into from
Oct 28, 2020

Conversation

YijunXieMS
Copy link
Contributor

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 from ServiceBusMessageBatch so scheduleMessage 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

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@YijunXieMS YijunXieMS Oct 27, 2020

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.

Context eventSpanContext = tracerProvider.startSpan(AZ_TRACING_SERVICE_NAME, eventContext,

Copy link
Member

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

Copy link
Contributor Author

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,

Copy link
Member

@samvaity samvaity Oct 28, 2020

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.

…ure/messaging/servicebus/implementation/MessageUtils.java

Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
Copy link
Member

@srnagar srnagar left a 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.

@srnagar srnagar requested a review from samvaity October 27, 2020 23:44
Copy link
Member

@samvaity samvaity left a 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.

@YijunXieMS
Copy link
Contributor Author

YijunXieMS commented Oct 28, 2020

Tracing changes looking good.
Consider adding unit tests.

@samvaity I created an issue to follow the unit test code. Will discuss with you later when I have time.
#16941

@YijunXieMS YijunXieMS merged commit 44c0103 into Azure:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review Distributed Tracing features in the new Service Bus library
4 participants