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: Correct recording values in opencensus measureMap in HeaderInterceptor #1726

Merged
merged 2 commits into from
Mar 22, 2022

Conversation

KiranmayiB
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@KiranmayiB KiranmayiB requested a review from a team as a code owner March 3, 2022 05:46
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Mar 3, 2022
@KiranmayiB KiranmayiB changed the title Bugfix fix: Correct recording values in opencensus measureMap in HeaderInterceptor Mar 3, 2022
@rajatbhatta rajatbhatta self-requested a review March 3, 2022 05:50
@rajatbhatta rajatbhatta added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 3, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 3, 2022
@thiagotnunes thiagotnunes added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 3, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 3, 2022
@thiagotnunes
Copy link
Contributor

@KiranmayiB can you explain how does this change solve the issue we are seeing?

@KiranmayiB
Copy link
Contributor Author

KiranmayiB commented Mar 3, 2022

@KiranmayiB can you explain how does this change solve the issue we are seeing?
@thiagotnunes
Here, the code puts a value into the measureMap twice and says record twice. That is causing the crash as we cannot put anything into the measureMap after we say record. So i think the second time when it tried to put a value after the record, it could not internally find the bucket causing an NPE. This is what i gathered from the bug.

@thiagotnunes
Copy link
Contributor

@KiranmayiB is there any (manual) testing we can do for this (to simulate the bug and have the fix correct it)?

@KiranmayiB
Copy link
Contributor Author

@KiranmayiB is there any (manual) testing we can do for this (to simulate the bug and have the fix correct it)?

@thiagotnunes I dont know how to. Locally running a module dependent on this is looking okay. It worked fine the last time too. I don't know how to simulate the bug yet. I will follow-up on the bug.

@rajatbhatta rajatbhatta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 3, 2022
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 3, 2022
@thiagotnunes
Copy link
Contributor

@KiranmayiB @ansh0l any updates?

@rajatbhatta
Copy link
Contributor

@thiagotnunes: We shared the jar with the changes in this PR with the end user, and they confirmed that they're not facing the issue anymore. They're waiting for an official release to be available to use it. I think we can merge this PR.

@thiagotnunes
Copy link
Contributor

Sounds good, thanks for checking!

@KiranmayiB KiranmayiB merged commit bdb2b89 into googleapis:main Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants