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 grpc supported version #2845

Merged
merged 8 commits into from
Sep 10, 2024
Merged

fix grpc supported version #2845

merged 8 commits into from
Sep 10, 2024

Conversation

JaredTan95
Copy link
Member

@JaredTan95 JaredTan95 commented Sep 3, 2024

Description

Our python application uses grpcio==1.34.0 (greater than version 1.27 as described in the documentation)

The opentelemetry-instrumentation-grpc 1.27.0 plugin conflicts with grpcio==1.34.0 , resulting in the following error:

/olet-auto-instrumentation-python/opentetelemetry/instrumerflation/grpe/__inft__:ру , timhe 402, In server
return self._original_func(*args, **kwargs)
tile "/usr/local/lib/python3.9/site-packages/grpc/aio/server.ny", line 206, in server
return Server(migration_thread_pool, () if handlersis is None else handlers,
File "/usr/local/lib/python3.9/site-packages/grpc/aio/_server.py", line 53, in __init__
self._server = cygrpc.AioServer(
File "src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi", line 813, in grpc._cython.cygrpc.AioServer.__init__
TypeError: Expected tuple, got list

This has been fixed for grpc in version 1.42.0 3 years ago: grpc/grpc@9699580

instrumentation/README.md Outdated Show resolved Hide resolved
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

Given you are the first to report I think we don't have many users with older grpc then

@JaredTan95
Copy link
Member Author

@xrmx can you help add an skipchanlog label?

@xrmx xrmx self-requested a review September 3, 2024 08:21
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

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

I think you need to add the changelog and also change the code in instrumentation/opentelemetry-instrumentation-grpc/package.py to match documentation and then run python scripts/generate_instrumentation_bootstrap.py to regenerate bootstrap_gen.py

@JaredTan95
Copy link
Member Author

JaredTan95 commented Sep 9, 2024

$ python3 scripts/generate_instrumentation_bootstrap.py
>>> black --config github/opentelemetry-python-contrib/pyproject.toml .
reformatted github/opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py

All done! ✨ 🍰 ✨
1 file reformatted, 12 files left unchanged.
>>> isort --settings-path github/opentelemetry-python-contrib/.isort.cfg --profile black .
INFO:instrumentation_list_generator:generatedgithub/opentelemetry-python-contrib/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py

There seems to be no changes in bootstrap_gen.py

@xrmx
Copy link
Contributor

xrmx commented Sep 9, 2024

Yeah to the list of things to update I forgot the package pyproject.toml

@JaredTan95 JaredTan95 requested a review from a team September 9, 2024 09:29
@JaredTan95
Copy link
Member Author

Yeah to the list of things to update I forgot the package pyproject.toml

thx, done~

@xrmx
Copy link
Contributor

xrmx commented Sep 9, 2024

@JaredTan95 Please add the changelog and we are good I guess

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

And also at the end run tox -e generate

@lzchen lzchen merged commit 6f1a17d into open-telemetry:main Sep 10, 2024
521 checks passed
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.

4 participants