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 asynchonous unary call traces #536

Merged
merged 15 commits into from
Jul 12, 2021
Merged

Conversation

sengjea
Copy link
Contributor

@sengjea sengjea commented Jun 7, 2021

Description

When calling the GRPC client with future(), the client fails as the context object is re-entered during the asynchonous callback. See test_unary_unary_future. This PR fixes that by using lower level function calls to control the re-entry of context during the synchronous and asynchonous bits.

Type of change

Bug fix

How Has This Been Tested?

  • tox -e test-instrumentation-grpc test_client_interceptor.py::TestClientProto::test_unary_unary_future

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Unit tests have been added

@sengjea sengjea requested a review from a team as a code owner June 7, 2021 08:14
@sengjea sengjea requested review from codeboten and srikanthccv and removed request for a team June 7, 2021 08:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

@sengjea sengjea requested a review from aabmass June 20, 2021 17:42
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Nice LGTM overall, but would be great if someone else can also take a look.

intercept_unary() and intercept_stream() seem to duplicate a lot of code, would it be reasonable to refactor them to share it?

@sengjea
Copy link
Contributor Author

sengjea commented Jun 25, 2021

Nice LGTM overall, but would be great if someone else can also take a look.

intercept_unary() and intercept_stream() seem to duplicate a lot of code, would it be reasonable to refactor them to share it?

ok i've refactored some of the code but I think it's getting into can-o-worms territory. There's stuff like RpcInfo which seems like an overhang from the opentracing era where span decorator functions could be passed into the instrumentor for additional span processing. Will do a bigger refactor in another PR.

@aabmass
Copy link
Member

aabmass commented Jun 25, 2021

ok i've refactored some of the code but I think it's getting into can-o-worms territory. There's stuff like RpcInfo which seems like an overhang from the opentracing era where span decorator functions could be passed into the instrumentor for additional span processing. Will do a bigger refactor in another PR.

Nice thank you for the contribution!

@sengjea
Copy link
Contributor Author

sengjea commented Jul 7, 2021

the docs check is failing because it was unable to reach some endpoint.

@codeboten codeboten merged commit 2ee2cf3 into open-telemetry:main Jul 12, 2021
ocelotl pushed a commit to ocelotl/opentelemetry-python-contrib that referenced this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants