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

Regenerate opentelemetry-proto to be compatible with protobuf 3 and 4 #3070

Merged
merged 5 commits into from
Dec 3, 2022

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Dec 1, 2022

Fixes #2880
Fixes #3050

I pinned grpcio-tools==1.48.1 dev dependency which provides protoc 3.19.4. This version appears to generate code compatible with both protobuf 3.x and 4.x. Then I regenerated the protobufs, and added tox envs to test the relevant packages against both versions of protobuf.

Finally I widened the protobuf version specifier in the opentelemetry-proto package.

@aabmass aabmass changed the title Pin a version of grpcio-tools with protoc compatible with protobuf 3 and 4 Regenerate opentelemetry-proto that is compatible with protobuf 3 and 4 Dec 2, 2022
@aabmass aabmass changed the title Regenerate opentelemetry-proto that is compatible with protobuf 3 and 4 Regenerate opentelemetry-proto to be compatible with protobuf 3 and 4 Dec 2, 2022
@aabmass aabmass force-pushed the protoc-for-protobuf3-and-4 branch from 309a4c3 to 8ac8e5c Compare December 2, 2022 02:20
@aabmass aabmass force-pushed the protoc-for-protobuf3-and-4 branch from 8ac8e5c to d4869e0 Compare December 2, 2022 02:30
@aabmass aabmass added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Dec 2, 2022
@aabmass aabmass marked this pull request as ready for review December 2, 2022 02:34
@aabmass aabmass requested a review from a team December 2, 2022 02:34
@aabmass aabmass mentioned this pull request Dec 2, 2022
@aabmass
Copy link
Member Author

aabmass commented Dec 2, 2022

This code actually passes with the latest protoc version (from the latest grpcio-tools package). I may bump the version again. See protocolbuffers/protobuf#11123 (comment)

I tested it out and it actually failed for protobuf 3.19.. Leaving it as is and I updated the tox file to test specifically against protobuf 3.19.x. I think this is ready to go

Copy link
Contributor

@gen-xu gen-xu left a comment

Choose a reason for hiding this comment

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

LGTM! I have also done some benchmark locally myself with this PR and confirmed that the upb is properly loaded and brings performance improvements for protobuf serialization.

@srikanthccv
Copy link
Member

Thanks, @aabmass!

@tmc
Copy link

tmc commented Dec 4, 2022

A new release that includes these changes would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support protobuf 4.x in opentelemetry alongside protobuf 3.x Update protobuf dependency to 4.x
5 participants