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: blanks span for session keepAlive traces #797

Merged

Conversation

thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Jan 14, 2021

The keepAlive traces were being tracked along with the parent span set by the client, which clutters the tracing stack. Here, we set the tracing to blank before issuing the keepAlive query.

The keepAlive traces were being tracked along with the parent span set
by the client, which clutters the tracing stack. Here, we set the
tracing to blank before issueing the keepAlive query.
@thiagotnunes thiagotnunes requested a review from as a code owner Jan 14, 2021
@google-cla google-cla bot added the cla: yes label Jan 14, 2021
@product-auto-label product-auto-label bot added the api: spanner label Jan 14, 2021
@@ -1472,11 +1473,15 @@ public void prepareReadWriteTransaction() {

private void keepAlive() {
markUsed();
final Span previousSpan = delegate.getCurrentSpan();
Copy link
Contributor Author

@thiagotnunes thiagotnunes Jan 14, 2021

Choose a reason for hiding this comment

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

We could, instead of doing this, set the current span on the delegate to blank during close. I think that this is the least intrusive approach, that is why I went for it.

@codecov
Copy link

@codecov codecov bot commented Jan 14, 2021

Codecov Report

Merging #797 (1c5f34f) into master (1a71e50) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #797      +/-   ##
============================================
- Coverage     85.01%   85.01%   -0.01%     
  Complexity     2562     2562              
============================================
  Files           143      143              
  Lines         14015    14019       +4     
  Branches       1341     1341              
============================================
+ Hits          11915    11918       +3     
- Misses         1537     1538       +1     
  Partials        563      563              
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/cloud/spanner/SessionImpl.java 85.38% <0.00%> (-0.51%) 30.00 <0.00> (ø)
...ain/java/com/google/cloud/spanner/SessionPool.java 88.85% <100.00%> (+0.03%) 71.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a71e50...1c5f34f. Read the comment docs.

c24t
c24t approved these changes Jan 14, 2021
Copy link

@c24t c24t left a comment

LGTM, and suppressing tracing for keep-alive calls seems like a clear improvement. It may be worth adding a test to check that this works as intended, and we don't export spans for these calls.

Any risk that we'll fail to trace a real executeQuery call (e.g. from another thread) in between calls to setCurrentSpan in keepAlive?

@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Jan 14, 2021

@c24t good question. So, when the pool maintainer is running the keepAlive method, it removes the session from the pool temporarily. Because of this, no other thread can be using the same Session, until the keepAlive has finished.

@thiagotnunes thiagotnunes requested a review from olavloite Jan 14, 2021
@thiagotnunes thiagotnunes merged commit 1a86e4f into googleapis:master Jan 14, 2021
17 of 19 checks passed
@thiagotnunes thiagotnunes deleted the keep-alive-open-telemetry branch Jan 14, 2021
thiagotnunes added a commit that referenced this issue Jan 22, 2021
The keepAlive traces were being tracked along with the parent span set
by the client, which clutters the tracing stack. Here, we set the
tracing to blank before issueing the keepAlive query.
@thiagotnunes thiagotnunes restored the keep-alive-open-telemetry branch Feb 8, 2021
thiagotnunes added a commit that referenced this issue May 6, 2021
The keepAlive traces were being tracked along with the parent span set
by the client, which clutters the tracing stack. Here, we set the
tracing to blank before issueing the keepAlive query.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants