Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

added conditional calling of unary_unary etc functions in _InterceptorChannel #2484

Merged

Conversation

VamsiKrishna1211
Copy link
Contributor

@VamsiKrishna1211 VamsiKrishna1211 commented Apr 30, 2024

Fixes #2483

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Installing the grpc contrib package along with grpc>=1.63.0 version.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Apr 30, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@VamsiKrishna1211 VamsiKrishna1211 marked this pull request as ready for review April 30, 2024 19:57
@xrmx
Copy link
Contributor

xrmx commented May 2, 2024

@VamsiKrishna1211 care to add a test with updated test requirements so we can actually test it in CI?

@VamsiKrishna1211
Copy link
Contributor Author

@xrmx Yes sure.
This is my first contribution to otel-contrib repository.
So please let me know if I have to make any other changes in the PR. :)

@xrmx
Copy link
Contributor

xrmx commented May 2, 2024

@xrmx Yes sure. This is my first contribution to otel-contrib repository. So please let me know if I have to make any other changes in the PR. :)

Cool! tests and an entry in CHANGELOG should be enough

@VamsiKrishna1211
Copy link
Contributor Author

Cool! tests and an entry in CHANGELOG should be enough

@xrmx The changes in the PR pertain to instrumentation-grpc only, and I need to test with multiple versions of grpcio package. More specifically grpcio>=1.63.0 and grpcio<1.63.0. Because there is a breaking change introduced in the methods of Channel class in latest release of grpcio==1.63.0.

Is there any standard way already you guys follow to test these kind of scenarios?

@xrmx
Copy link
Contributor

xrmx commented May 3, 2024

@VamsiKrishna1211 you can take a look at opentelemetry-instrumentation-httpx both in tox.ini (grep for httpx) and in the instrumentation package dir

@VamsiKrishna1211 VamsiKrishna1211 requested a review from a team May 3, 2024 20:00
@VamsiKrishna1211 VamsiKrishna1211 marked this pull request as draft May 3, 2024 20:01
@VamsiKrishna1211 VamsiKrishna1211 force-pushed the bugfix/registered-method-error branch from 1defe6b to f982fb5 Compare May 3, 2024 20:08
@VamsiKrishna1211 VamsiKrishna1211 marked this pull request as ready for review May 3, 2024 20:08
@VamsiKrishna1211
Copy link
Contributor Author

@xrmx Thanks for correcting the _registered_method argument, I think missed it by accident.

@xrmx xrmx requested a review from aabmass May 17, 2024 07:15
@xrmx
Copy link
Contributor

xrmx commented May 20, 2024

@VamsiKrishna1211 could you please add a CHANGELOG entry? And fix lint and CLA please

@VamsiKrishna1211 VamsiKrishna1211 force-pushed the bugfix/registered-method-error branch from 68faaab to 56f2bb0 Compare June 8, 2024 08:32
VamsiKrishna1211 and others added 6 commits June 18, 2024 14:40
…emetry/instrumentation/grpc/grpcext/_interceptor.py

Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
…emetry/instrumentation/grpc/grpcext/_interceptor.py

Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
@VamsiKrishna1211 VamsiKrishna1211 force-pushed the bugfix/registered-method-error branch from 92bfbaa to fb9bcc1 Compare June 18, 2024 15:10
@VamsiKrishna1211
Copy link
Contributor Author

@VamsiKrishna1211 could you please add a CHANGELOG entry? And fix lint and CLA please

@xrmx Apologies for the delayed changes, was caught up with my full-time work for a while.
Added the changes to CHANGELOG and fixed the CLA, please check and let me know for any other issues.

@xrmx xrmx self-requested a review June 18, 2024 15:24
@VamsiKrishna1211
Copy link
Contributor Author

@xrmx Updated the repo with the changes.

@VamsiKrishna1211
Copy link
Contributor Author

@aabmass Can you look into the MR? We're currently trying to update our packages to their latest version (Official nvidia-triton-client package mainly) while also using open-telemetry. But is causing issues because of the issue attached to this MR.

@lzchen lzchen merged commit 1c8d8ef into open-telemetry:main Jul 30, 2024
389 checks passed
xrmx pushed a commit to xrmx/opentelemetry-python-contrib that referenced this pull request Jan 24, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with the latest grpc version (v1.63.0)
3 participants