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

feat: bump google_protobuf >=3.18, < 5.a #1645

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

colinbendell
Copy link
Contributor

The current version of google_protobuf is 4.27.1. The approximate equal dependency causes compat issues for consuming libraries. Since protobuf now uses the serialized descriptor instead of the DSL, the contained _pb.rb files need to be updated. This causes the min compat version to be v3.18 (Sep 2021) as per https://protobuf.dev/news/2023-04-20/

Copy link

linux-foundation-easycla bot commented Jun 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@arielvalentin
Copy link
Contributor

@colinbendell thank you for your contribution. I would qualify this as a feat as opposed to a chore: since it includes changes to generated protobuf files and trigger a minor version bump that includes these changes. That will alert our users that a big change is coming their way.

At GitHub we would not be able to upgrade to use newer protoc definitions. we are pinned to an earlier version of protobufs and since it will not allow mixed definitions in 3.14 with errors like this, when mixing definitions generated from different protoc versions it yields errors like this:

  • undefined method add_serialized_file' for an instance of Google::Protobuf::DescriptorPool (NoMethodError)
    pool.add_serialized_file(descriptor_data)`
  • Google::Protobuf::TypeError: Unable to build file to DescriptorPool: duplicate file name (xyz.proto)
  • add_file': Unable to add defs to DescriptorPool: out of memory (Google::Protobuf::TypeError)

@open-telemetry/ruby-maintainers I presume other language sigs are bundling generated protobuf source?

Is there a way to generate the source during installation time instead of bundled in the gem itself that way we do not run into Ruby DSL compatibility issues with generated source files?

@colinbendell colinbendell changed the title chore: bump google_protobuf >=3.18, < 5.0 feat: bump google_protobuf >=3.18, < 5.a Jun 12, 2024
@robertlaurin
Copy link
Contributor

@arielvalentin

Is there a way to generate the source during installation time instead of bundled in the gem itself that way we do not run into Ruby DSL compatibility issues with generated source files?

Don't have an answer for this one yet.

But just putting out some other proposals to unblock this work.

  • We could simply make this change with the recompiled protos and backport any changes needed to support users unable to upgrade.
  • We could decouple the actual encoding and protos from the export gem itself using this unpublished gem which would in theory allow someone to pin the encoding while allowing the exporter version itself to move forward freely.

@arielvalentin
Copy link
Contributor

We could decouple the actual encoding and protos from the export gem itself using this unpublished gem which would in theory allow someone to pin the encoding while allowing the exporter version itself to move forward freely.

If I understand what you are saying correctly, we would create separate gems for specific protoc generated versions and those gems would reference the "common" gem being the code used across all versions?

@robertlaurin
Copy link
Contributor

robertlaurin commented Jun 12, 2024

If I understand what you are saying correctly, we would create separate gems for specific protoc generated versions and those gems would reference the "common" gem being the code used across all versions?

No I mistyped. I meant to suggest we could decouple encoding from the export. So that you could pin to an older version of the encoding gem (with the compatible protos). Then we could create releases of the encoding gem with the new style. The exporter would simply depend on the encoding gem, not on any one specific version of it.

@arielvalentin
Copy link
Contributor

Reiterating once again...

The exporters would have a transitive dependency on the protoc generated source code, which are published in separate gems.

The min version would be permissive allowing users to pin to a version of the protoc generated code compatible with their protobuf version.

I get it right this time?

@robertlaurin
Copy link
Contributor

I get it right this time?

Yup.

We could simply make this change with the recompiled protos and backport any changes needed to support users unable to upgrade.

I'd prefer this option out of all of them as it seems to be the least effort overall.

@colinbendell
Copy link
Contributor Author

What is the path forward here?

  1. bump to require >= 3.18 (Sept-2021) which supports the descriptor syntax. Generate the _pb.rb files with descriptor syntax. Clients that need the dsl syntax will have to pin to an older version of the opentelemetry gem.
  2. create a dual pipeline that supports protobuf < 3.18 and bundles the generated _pb.rb files using the dsl v the descriptor syntax

I'll be honest, {2} sounds like a lot of work for a dependency from prior to 2021. (3.18 was released in Sept-2021). In the mean time this is holding up the rest of us who are trying to use functionality in the 4.x series (performance, security, etc).

@robertlaurin
Copy link
Contributor

bump to require >= 3.18 (Sept-2021) which supports the descriptor syntax. Generate the _pb.rb files with descriptor syntax. Clients that need the dsl syntax will have to pin to an older version of the opentelemetry gem.

We're going with this option

Co-authored-by: Francis Bogsanyi <francis.bogsanyi@shopify.com>
@robertlaurin robertlaurin merged commit 289b4aa into open-telemetry:main Jun 19, 2024
56 checks passed
@colinbendell colinbendell deleted the protobuf_3_18 branch June 28, 2024 16:14
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

4 participants