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

Fix: multiple calls to end of span #75

Merged
merged 9 commits into from Feb 25, 2020

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Feb 14, 2020

Fixes #71

@googlebot googlebot added the cla: yes label Feb 14, 2020
@@ -892,7 +892,7 @@ private PooledSession take() throws SpannerException {
return s.session;
}
} catch (Exception e) {
TraceUtil.endSpanWithFailure(tracer.getCurrentSpan(), e);
Copy link
Member Author

@mayurkale22 mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

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

tracer.getCurrentSpan() is called outside of scope, which means endSpanWithFailure will execute on either READ_WRITE_TRANSACTION or READ_ONLY_TRANSACTION span instead of WAIT_FOR_SESSION span.

@mayurkale22 mayurkale22 requested review from olavloite and skuruppu Feb 14, 2020
@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 14, 2020

@rghetia Could you please review PR?

span.setStatus(Status.INTERNAL.withDescription(e.getMessage()));
}
}

static void endSpanWithFailure(Span span, Throwable e) {
Copy link

@rghetia rghetia Feb 14, 2020

Choose a reason for hiding this comment

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

is endSpanWithFailure used anywhere now? If not then please remove it.

Copy link
Member Author

@mayurkale22 mayurkale22 Feb 14, 2020

Choose a reason for hiding this comment

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

This has been used in a few places (ex: singleUse) where span does not end except when there is a failure.

See: https://github.com/googleapis/google-cloud-java/pull/2677/files#r157323309 for original reason.

@mayurkale22 mayurkale22 marked this pull request as ready for review Feb 18, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

Thank you so much @mayurkale22 and @olavloite for reproducing the problem and working on the fix. I really appreciate it. Great work.

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 19, 2020

@olavloite could you please help me to fix the tests? Here is the issue: When I ran SpanTest and SpannerGaxRetryTest separately, they work as expected but saw tests failures when running together (mvn test). I suspect it is due to reflection that we used to set the test tracer in SpanTest. I am trying to solve it locally, no luck so far.

@olavloite
Copy link
Contributor

@olavloite olavloite commented Feb 20, 2020

@olavloite could you please help me to fix the tests? Here is the issue: When I ran SpanTest and SpannerGaxRetryTest separately, they work as expected but saw tests failures when running together (mvn test). I suspect it is due to reflection that we used to set the test tracer in SpanTest. I am trying to solve it locally, no luck so far.

@mayurkale22
mayurkale22#2 should do the trick.

@googlebot
Copy link

@googlebot googlebot commented Feb 20, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Feb 20, 2020
@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 20, 2020

mayurkale22#2 should do the trick.

Thanks for the help.

the reason that other test cases are failing could be an indication that there are still conditions possible that could trigger a span to be ended multiple times. It would therefore be interesting to investigate those specific failures and see if you can reproduce them in the SpanTest class.

I managed to repro-d the issue in SpannerGaxRetryTest, will try to fix it soon.

@mayurkale22 mayurkale22 added cla: yes and removed cla: no labels Feb 20, 2020
@googlebot
Copy link

@googlebot googlebot commented Feb 20, 2020

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

@googlebot googlebot commented Feb 20, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Feb 20, 2020
@mayurkale22 mayurkale22 force-pushed the handle_span_end branch 2 times, most recently from 55f9dcb to 02563ec Compare Feb 21, 2020
@mayurkale22 mayurkale22 changed the title Attempt to fix multiple call to end of span Fix: multiple calls to end of span Feb 21, 2020
@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 21, 2020

@olavloite I have added a few more tests and attempted to fix remaining issues, PTAL c0689bd when you get a chance.

Copy link
Contributor

@olavloite olavloite left a comment

LGTM.
I don't quite get where the dependency problem is coming from. It can be fixed by adding the following to the <ignoredDependencies> tag in the pom.xml: com.google.errorprone:error_prone_annotations

@mayurkale22 mayurkale22 added the kokoro:force-run label Feb 21, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 21, 2020
@mayurkale22 mayurkale22 added cla: yes and removed cla: no labels Feb 21, 2020
@googlebot
Copy link

@googlebot googlebot commented Feb 21, 2020

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 21, 2020

LGTM.
I don't quite get where the dependency problem is coming from. It can be fixed by adding the following to the <ignoredDependencies> tag in the pom.xml: com.google.errorprone:error_prone_annotations

Great. Finally, build is passed.

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 21, 2020

@rghetia would like to get your eyes on OpenCensus related changes. Please review when you get a chance.

@mayurkale22
Copy link
Member Author

@mayurkale22 mayurkale22 commented Feb 25, 2020

@skuruppu I think this is good to merge now.

@skuruppu skuruppu merged commit 3f32f51 into googleapis:master Feb 25, 2020
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants