fix: blanks span for session keepAlive traces #797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: spanner
thiagotnunes
commented
Jan 14, 2021
| @@ -1472,11 +1473,15 @@ public void prepareReadWriteTransaction() { | |||
|
|
|||
| private void keepAlive() { | |||
| markUsed(); | |||
| final Span previousSpan = delegate.getCurrentSpan(); | |||
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
c24t
approved these changes
Jan 14, 2021
There was a problem hiding this 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?
|
@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. |
olavloite
approved these changes
Jan 14, 2021
thiagotnunes
added a commit
that referenced
this pull request
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
added a commit
that referenced
this pull request
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.
ansh0l
pushed a commit
to ansh0l/java-spanner
that referenced
this pull request
Nov 10, 2022
This is an auto-generated regeneration of the .pb.go files by cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will update the corresponding PR to depend on the newer version of go-genproto, and assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot will not create any more regeneration PRs. If all regen PRs are closed, gapicgen will create a new set of regeneration PRs once per night. If you have been assigned to review this PR, please: - Ensure that CI is passing. If it's failing, it requires your manual attention. - Approve and submit this PR if you believe it's ready to ship. That will prompt genbot to assign reviewers to the google-cloud-go PR. Corresponding google-cloud-go PR: googleapis/google-cloud-go#5920 Changes: feat: Google Chat API logging proto for documentation PiperOrigin-RevId: 443146889 Source-Link: googleapis/googleapis@824ea38 chore: regenerate API index Source-Link: googleapis/googleapis@ba7f434 feat(eventarc/publishing): Add publishing methods for channel resources PiperOrigin-RevId: 442858558 Source-Link: googleapis/googleapis@726dac2 chore: regenerate API index Source-Link: googleapis/googleapis@0d5e4e2 feat: Update Notebooks API for clients libraries PiperOrigin-RevId: 442854284 Source-Link: googleapis/googleapis@38b542a chore: regenerate API index Source-Link: googleapis/googleapis@b1e784c feat: initial generation of APIs This is the first release of the Backup for GKE client APIs. PiperOrigin-RevId: 442847686 Source-Link: googleapis/googleapis@1900ca3 feat:Add a call log type for handled exceptions feat:Add a structured format for logged call arguments PiperOrigin-RevId: 442805938 Source-Link: googleapis/googleapis@acc1642 fix(dialogflow)!: correct broken ConversationModelEvaluation resource pattern PiperOrigin-RevId: 442646533 Source-Link: googleapis/googleapis@b62c562 chore: Update disco-to-proto3-converter PiperOrigin-RevId: 442604967 Source-Link: googleapis/googleapis@bd6f2c8 chore(bazel): update rules_gapic to v0.12.1 Changes include: - build_gen includes service_yaml on py_gapic_library PiperOrigin-RevId: 442594351 Source-Link: googleapis/googleapis@4923ca5 chore: regenerate API index Source-Link: googleapis/googleapis@5dcdea0 feat(securitycenter): Add connection and description field to finding's list of attributes PiperOrigin-RevId: 442589635 Source-Link: googleapis/googleapis@50fc834 docs(dialogflow/cx): minor wording update PiperOrigin-RevId: 442267541 Source-Link: googleapis/googleapis@740f072 docs(dialogflow/cx): minor wording update PiperOrigin-RevId: 442267451 Source-Link: googleapis/googleapis@4445d18 feat: add kernel_spec for libraries PiperOrigin-RevId: 442115593 Source-Link: googleapis/googleapis@4e5ef95 feat: add kernel_spec for libraries PiperOrigin-RevId: 442088600 Source-Link: googleapis/googleapis@c56ae2a feat: Require google-cloud-compute-v1 version 1.3 Source-Link: googleapis/googleapis@487c2cb
rajatbhatta
pushed a commit
to rajatbhatta/java-spanner
that referenced
this pull request
Nov 17, 2022
Refactors the integration tests to fix a number of problems: 1. Too much state was kept in the abstract base class for all integration tests. This did not work properly with parameterized tests, as some state leaked from one parameter value (dialect) to another. 2. The emulator cannot use an existing instance for testing, as it always starts without any existing instances. This is now fixed by forcing tests to used a separate instance for each test class when running on the emulator, and by fixing creation and database cleanup before/after tests. In addition the integration tests were not executed by Kokoro, as they were skipped in the default test execution. All integration tests are now executed by default. @ansh0l @mpeddada1 The integration tests are (probably) still failing, but that is because the default test instance (projects/gcloud-devel/instances/spanner-testing-east1) is clogged with old (?) test databases that have not been cleaned up. Some test cases therefore currently fail with an error that they cannot create another database, as the max of 100 databases per instance has been reached. I don't have access to the affected instance, so I cannot clean it up. Hopefully one of you has access to it. I've verified that the integration tests work on a different instance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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.