[Service Bus] Tracing for send API #11651
Conversation
ghost
added
the
…rentSpanOptions and TryAddOptions that extends it
HarshaNalluru
requested review from
bterlson,
chradek,
ramya-rao-a,
richardpark-msft and
xirzec
as code owners
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| * @internal | ||
| * @ignore | ||
| */ | ||
| export function createMessageSpan( |
There was a problem hiding this comment.
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:
- The name
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. - Also, jsdoc for this method would be helpful
- Why is config optional here? I would imagine that we would always have a ConnectionConfig
There was a problem hiding this comment.
Yes to the first two!
Why is config optional here? I would imagine that we would always have a ConnectionConfig
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.
sdk/servicebus/service-bus/src/modelsToBeSharedWithEventHubs.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * The `Span` or `SpanContext` to use as the `parent` of any spans created while calling operations that make a request to the service. | ||
| */ | ||
| parentSpan?: Span | SpanContext | null; |
There was a problem hiding this comment.
I noticed in Event Hubs that null is not an option for parentSpan. Why do we have this here?
There was a problem hiding this comment.
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.
I'll log an issue for fixing it in event-hubs.
Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
|
/azp run js - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // No cleanup: | ||
| // "Closing the link" cleanup would step over new link initializations | ||
| // and can possibly clear the link once created, hence we do the cleanup | ||
| // at the time of new link creation |
There was a problem hiding this comment.
Take the case of a disconnected event firing and user making no more send requests. In that case, the loop to keep renewing the token on the sender link does not get cleared and we will have a rogue loop on our hands
There was a problem hiding this comment.
I realize that we can argue that in the normal case of user stopping to make any send requests and no disconnected event, our loop will still keep trying to renew the lock. In that case, at some point the service will close the link because it has been idle for too long. That results in an "amqp close" event in which case we should clean up the timers and assets from our end. In this PR, we are removing those as well
There was a problem hiding this comment.
Upon discussion, reverted this in favour of the proposal for cleanup with detached where onDetached would be called before the refreshConnection() is invoked.
Triggered live tests https://dev.azure.com/azure-sdk/internal/_build/results?buildId=567200&view=results
| // For the cases such as connection is disconnected, onAmqpClose and onSessionClose events, | ||
| // we don't clear the link for sender. To compensate for the above, | ||
| // we are calling close on the link to cleanup before creating the new one. | ||
| await this.link.close(); |
There was a problem hiding this comment.
Why close() and not closeLink()?
The former sets the _wasClosedPermanently to true and removes the link from the context cache which may have other repercussions
There was a problem hiding this comment.
.close() is different from .link.close(), we are not calling .close() here.
closeLink would try acquiring a lock but the initialization already has it, leading into a deadlock. Hence closeLink shouldn't be called. Alternatively, closeLinkImpl can be called which is a combination of clear-timeout and link.close(). Timers are handled in ensureTokenRenewal anyway, so link.close() makes sense here.
There was a problem hiding this comment.
Anyways, reverted this in favour of the proposal for cleanup with detached where onDetached would be called before the refreshConnection() is invoked.
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
/azp run js - servicebus - tests |
|
No pipelines are associated with this pull request. |
|
Skipped and logged an issue for the flaky test #11718 |
|
/check-enforcer override |
Tracing for the send related APIs - inspired from event-hubs support for the same though with minor differences #5207