Skip to content

[Service Bus] Prepare tracing methods for message processor and scheduleMessage#16524

Merged
YijunXieMS merged 7 commits into
Azure:masterfrom
YijunXieMS:sb_tracing
Oct 28, 2020
Merged

[Service Bus] Prepare tracing methods for message processor and scheduleMessage#16524
YijunXieMS merged 7 commits into
Azure:masterfrom
YijunXieMS:sb_tracing

Conversation

@YijunXieMS

Copy link
Copy Markdown
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

}

public static ServiceBusMessage traceMessageSpan(ServiceBusMessage serviceBusMessage,
Context messageContext, String hostname, String entityPath, TracerProvider tracerProvider) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@YijunXieMS YijunXieMS Oct 27, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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,

@samvaity samvaity Oct 28, 2020

Copy link
Copy Markdown
Member

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>

@srnagar srnagar left a comment

Copy link
Copy Markdown
Member

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

@samvaity samvaity left a comment

Copy link
Copy Markdown
Member

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

YijunXieMS commented Oct 28, 2020

Copy link
Copy Markdown
Contributor Author

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