-
Notifications
You must be signed in to change notification settings - Fork 661
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
Compatible workaround to support both protobuf <=3.20 and >=4.21 #2954
Conversation
This PR would be helpful for us at Jina AI, since open telemetry version cap in protobuf restricts our protobuf version. |
@gen-xu thank you for the PR. Does the fix mentioned in #2880 (comment) (along with loosening the dependencies) solve this issue as well? It seems like a good middle ground until we are ready to drop Protobuf 3. |
Yes this should support both protobuf 3 and 4, but afaik there are some other protos (jaeger etc.) in the contrib repo might also need to get regenerated? |
@gen-xu I saw your comment here #2880 (comment), where did you see that "old protobuf library version won't be able to use newer generated protobuf code"? Which version is the cutoff? I've opened protocolbuffers/protobuf#11123 to get some clarity on this as I couldn't find it documented.
👍 |
Basically this line in the generated code, the from google.protobuf.internal import builder as _builder |
@gen-xu right I know we need newer protoc to work with upb. I'm a bit confused because I generated the protobufs with latest Is there a case you've seen where the latest version of protoc does not work with edit see protocolbuffers/protobuf#11123 (comment), I think latest version should work OK for now. |
if you use protoc from google.protobuf.internal import builder as _builder I believe the reason why it passes all tests is that you are using |
You're right, I update my PR to actually test with ~=3.19.0 which resolves to 3.19.6. It works now with protos generated with grpcio-tools==1.48.1 (which provides protoc 3.19.4). Thanks for talking this through. Does #3070 look good to you now as a solution for now? |
Yup that looks good to me! I am closing this PR since #3070 works. |
Description
This is a work around for #2880
By checking the api implementation before define anything in the generated code, it is able to have both versions of generated protobuf and so it allows the
opentelemetry-proto
to work with bothprotobuf<=3.20
andprotobuf>=4.21
Type of change
How Has This Been Tested?
Tested with both protobuf~=3.19.4 and protobuf==4.21.6 by importing any generated modules and it should not throw any error
Does This PR Require a Contrib Repo Change?
Answer the following question based on these examples of changes that would require a Contrib Repo Change:
The OTel specification has changed which prompted this PR to update the method interfaces of
opentelemetry-api/
oropentelemetry-sdk/
The method interfaces of
test/util
have changedScripts in
scripts/
that were copied over to the Contrib repo have changedConfiguration files that were copied over to the Contrib repo have changed (when consistency between repositories is applicable) such as in
pyproject.toml
isort.cfg
.flake8
When a new
.github/CODEOWNER
is addedMajor changes to project information, such as in:
README.md
CONTRIBUTING.md
Yes. - Link to PR: Update dependency fo open-telemetry/opentelemetry-python#2954 opentelemetry-python-contrib#1363
No.
Checklist: