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.

Sign up for GitHub

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

Jump to bottom

[storage] Fix spans that were not waiting for child subspans #5780

Merged
merged 5 commits into from
Nov 6, 2019
Merged

[storage] Fix spans that were not waiting for child subspans #5780

merged 5 commits into from
Nov 6, 2019

Conversation

xirzec
Copy link
Member

@xirzec xirzec commented Oct 24, 2019 •
edited
Loading

For iterators, we weren't properly waiting for the underlying promise before closing spans.

Fixes #5776

@xirzec xirzec added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) labels Oct 24, 2019
@xirzec xirzec self-assigned this Oct 24, 2019
@jeremymeng
Copy link
Member

jeremymeng commented Oct 24, 2019 via email

@HarshaNalluru
Copy link
Member

We always thought awaiting on a return statement is redundant. It looks like a many functions are affected by the same issue.

Isn't it, @xirzec?

Example

azure-sdk-for-js/sdk/storage/storage-blob/src/BlobClient.ts

Lines 1395 to 1410 in 27910f7

public async syncCopyFromURL(
copySource: string,
options: BlobSyncCopyFromURLOptions = {}
): Promise<BlobCopyFromURLResponse> {
const { span, spanOptions } = createSpan("BlobClient-syncCopyFromURL", options.spanOptions);
options.conditions = options.conditions || {};
options.sourceConditions = options.sourceConditions || {};
try {
return this.blobContext.copyFromURL(copySource, {
abortSignal: options.abortSignal,
metadata: options.metadata,
leaseAccessConditions: options.conditions,
modifiedAccessConditions: options.conditions,
sourceModifiedAccessConditions: {
sourceIfMatch: options.sourceConditions.ifMatch,

@xirzec
Copy link
Member Author

xirzec commented Oct 24, 2019

We always thought awaiting on a return statement is redundant.

Normally, it is redundant, but now that we are essentially wanting to then() off the returned promise, it is not.

Many more functions in clients return Promises directly. Will they have the same issue?

It looks like a many functions are affected by the same issue. Isn't it, @xirzec?

Yes, it looks like this bites us in many places. I'll have to go through the spans more thoroughly.

@ramya-rao-a ramya-rao-a changed the title [storage] Fix spans that were not waiting for child subspans [do not merge][storage] Fix spans that were not waiting for child subspans Oct 27, 2019
@ramya-rao-a ramya-rao-a changed the title [do not merge][storage] Fix spans that were not waiting for child subspans [WIP][storage] Fix spans that were not waiting for child subspans Oct 27, 2019
@ramya-rao-a
Copy link
Contributor

Updated title to include WIP as we have to review many more scenarios

@xirzec xirzec changed the title [WIP][storage] Fix spans that were not waiting for child subspans [storage] Fix spans that were not waiting for child subspans Oct 28, 2019
@xirzec
Copy link
Member Author

xirzec commented Oct 28, 2019

@HarshaNalluru I think this new version takes care of all the remaining spans

HarshaNalluru
HarshaNalluru approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@HarshaNalluru HarshaNalluru left a comment

Choose a reason for hiding this comment

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

Damn!

@HarshaNalluru
Copy link
Member

What about the SDKs of other services, they might have the same problem?
If so, please log an issue for now. :)

@HarshaNalluru
Copy link
Member

Waiting for @XiaoningLiu's review.

@xirzec
Copy link
Member Author

xirzec commented Oct 31, 2019

What about the SDKs of other services, they might have the same problem?
If so, please log an issue for now. :)

I reviewed all the other Spans we're creating in other packages. I found only one issue in Identity (which I added to this PR since it's a 2 line change to add await.)

I did also notice that we're instrumenting some list* operations in a questionable way in several of the other packages, so I updated the notes of #5607 to mention that.

@HarshaNalluru
Copy link
Member

Waiting for @XiaoningLiu and @jiacfan

@HarshaNalluru HarshaNalluru merged commit e18b7be into Azure:master Nov 6, 2019
@xirzec xirzec deleted the fixNominalSpans branch November 6, 2019 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XiaoningLiu XiaoningLiu XiaoningLiu approved these changes

@HarshaNalluru HarshaNalluru HarshaNalluru approved these changes

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

@jiacfan jiacfan Awaiting requested review from jiacfan

@vinjiang vinjiang Awaiting requested review from vinjiang

@daviwil daviwil Awaiting requested review from daviwil

@sophiajt sophiajt Awaiting requested review from sophiajt

Assignees

@xirzec xirzec

Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Distributed Tracing][Storage]: Some storage calls end their spans right away
5 participants
@xirzec @jeremymeng @HarshaNalluru @ramya-rao-a @XiaoningLiu