Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Tracing for send API #11651
[Service Bus] Tracing for send API #11651
Changes from 9 commits
a8c1d5d
a73fd31
e46e805
4af1738
c7d3a14
b5f77a8
cd71e42
d631ee5
8375ed8
8038315
6fa23f7
a8b34fc
d04a38e
9742d97
f262e45
ba4fef7
e46deb4
fea2177
5c5795f
6b2409a
7de9b6f
d1af28e
060842e
3197fbc
84e48aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Feedback not for this PR, but a note for both Event Hubs & Service Bus for which we can log a generic issue on improvements and make changes for both packages at once in the future:
createMessageSpan()
is slightly misleading as it may indicate that I can use it at any time when a message is involved. In reality, this method is tied to the "producer" kind, so is usable only when sending 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.
Yes to the first two!
parentSpan is not a required parameter but feels primary, I believe that was the reason Chris made the config optional in event-hubs. I can make it the first param in both service-bus and event-hubs and make it required in a later 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.
Logged #11687
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.
I noticed in Event Hubs that
null
is not an option forparentSpan
. Why do we have this 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.
There are other places where we allow the span to be "null" which is a valid value and it signifies the rootSpan.
Moreover, I needed to allow this "null" since I'm calling getParentSpan() for the tryAdd over array of messages which may return a null value.
In event-hubs, this tracing for "array of messages" case is duplicated.
"null" is a valid value and event-hubs should also allow this.
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.
I'll log an issue for fixing it in event-hubs.
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.
Logged #11687