search

open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.67k stars 569 forks source link

Allow support for protobuf5 #3931

Open Sovietaced opened 1 month ago

Sovietaced commented 1 month ago

Description

I was unable to use a more recent version of grpc-io due to this library's upper restriction on the version of protobuf. Protobuf 5 was released in March and newer versions of grpc rely on it so it would be nice if this library were more flexible.

How Has This Been Tested?

I added environments to fox in order to ensure the library builds in protobuf 5.

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:

Checklist:

Sovietaced commented 1 month ago

Looks like this is currently blocked on a bad googleapis-common-protos wheel: https://github.com/googleapis/python-api-common-protos/commit/28fc17a9208aa98782acc6bee6c40ec12b959706

It should support protobuf > 5 but doesn't...

aabmass commented 1 month ago

I was unable to use a more recent version of grpc-io due to this library's upper restriction on the version of protobuf. Protobuf 5 was released in March and newer versions of grpc rely on it so it would be nice if this library were more flexible.

I'm a little dubious this will work correctly. Protobuf versions make no guarantees that the generated code will work with even different minor versions of the protobuf library: https://github.com/protocolbuffers/protobuf/issues/11123.

We had a bit of a debacle when protobuf 4 came out with a partition in the community on which version to support. IIRC we found some magic version protoc compiler that works with both versions. I guess that is still the case here if the tests are passing, but do you have any insight on how the generated code is working across protobuf 3, 4, and 5?

Sovietaced commented 1 month ago

I guess that is still the case here if the tests are passing, but do you have any insight on how the generated code is working across protobuf 3, 4, and 5?

Admittedly I have no idea yet. I'm just a user of a library that has this in a long list of transitive dependency that can no longer upgrade to grpc-io > 1.62.0. Unfortunately the tests are unable to even pass yet because google's own common protos library release is botched. Once I can get CI running I think I'll cross that bridge and look deeper into the generated code. I appreciate the context around the changes though.

aabmass commented 1 month ago

@Sovietaced gotcha I appreciate the PR. I reached out internally regarding https://github.com/googleapis/python-api-common-protos/pull/221 and there is apparently more work needed to support protobuf 5 and should have a fix in the next couple of weeks.

aabmass commented 4 weeks ago

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

Sovietaced commented 4 weeks ago

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

Thanks for the ping. I wasn't subscribed to the releases. Will take a look today or tomorrow.

Sovietaced commented 4 weeks ago

Looks like the release as made and we can move this PR forward https://github.com/googleapis/python-api-common-protos/releases/tag/v1.63.1

Updated. Ran tox locally and it seems to work. Just need approval for CI to run.

aabmass commented 3 weeks ago

Related: https://github.com/open-telemetry/opentelemetry-python/issues/3932#issuecomment-2146072966

I'm going to bring this up in the SIG call but I'm nervous to move forward with this change even with CI passing without first re-generating the code and only supporting a single major version. From the protobuf website

Protobuf cross-version usages outside the guarantees are error-prone and not supported. Version skews can lead to flakes and undefined behaviors that are hard to diagnose, even if it can often seem to work as long as nothing has changed in a source-incompatible way. For Protobuf, the proliferation of tools and services that rely on using unsupported Protobuf language bindings prevents the protobuf team from updating the protobuf implementation in response to bug reports or security vulnerabilities.]

Sovietaced commented 3 weeks ago

I'm going to bring this up in the SIG call but I'm nervous to move forward with this change even with CI passing without first re-generating the code and only supporting a single major version. From the protobuf website

Makes sense. That does seem like a tricky problem to manage. And seems like the only way to safely handle that is to distribute different versions of these packages, one per protobuf major, which does not seem fun :/

aabmass commented 3 weeks ago

We didn't really come to a conclusion. I opened https://github.com/open-telemetry/opentelemetry-python/issues/3958 for discussion