opentelemetry-python icon indicating copy to clipboard operation
opentelemetry-python copied to clipboard

Allow support for protobuf5

Open Sovietaced opened this issue 1 year ago • 10 comments

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:

  • The OTel specification has changed which prompted this PR to update the method interfaces of opentelemetry-api/ or opentelemetry-sdk/

  • The method interfaces of test/util have changed

  • Scripts in scripts/ that were copied over to the Contrib repo have changed

  • Configuration 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 added

  • Major changes to project information, such as in:

    • README.md
    • CONTRIBUTING.md
  • [ ] Yes. - Link to PR:

  • [x] No.

Checklist:

  • [ ] Followed the style guidelines of this project
  • [x] Changelogs have been updated
  • [x] Unit tests have been added
  • [ ] Documentation has been updated

Sovietaced avatar May 24 '24 22:05 Sovietaced

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...

Sovietaced avatar May 28 '24 18:05 Sovietaced

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?

aabmass avatar May 29 '24 16:05 aabmass

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.

Sovietaced avatar May 29 '24 16:05 Sovietaced

@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 avatar May 29 '24 19:05 aabmass

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

aabmass avatar Jun 03 '24 19:06 aabmass

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 avatar Jun 03 '24 20:06 Sovietaced

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.

Sovietaced avatar Jun 04 '24 03:06 Sovietaced

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.]

aabmass avatar Jun 06 '24 16:06 aabmass

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 :/

Sovietaced avatar Jun 06 '24 17:06 Sovietaced

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

aabmass avatar Jun 06 '24 20:06 aabmass

@Sovietaced thanks again for your patience, do you still have time to work on this? I think we have reached a decision to move forward with only supporting protobuf 5+ and get this into the next release.

aabmass avatar Sep 27 '24 16:09 aabmass

@Sovietaced thanks again for your patience, do you still have time to work on this? I think we have reached a decision to move forward with only supporting protobuf 5+ and get this into the next release.

I can rework the build to support that. If it requires more than that I probably wouldn't be a good candidate since I'm a Python hobbyist. I can at least and fix up this pull request.

Sovietaced avatar Sep 29 '24 23:09 Sovietaced

I can rework the build to support that. If it requires more than that I probably wouldn't be a good candidate since I'm a Python hobbyist. I can at least and fix up this pull request.

I took a look a things have changed a bit. I'm not sure I'll have to time to get it fully working so feel free to have someone else take a look.

Sovietaced avatar Sep 30 '24 00:09 Sovietaced

No worries @Sovietaced appreciate your work on this so far. This PR helped move the discussion forward. Since we are scoping down, I will probably open a separate PR

aabmass avatar Sep 30 '24 14:09 aabmass

Closing as support for protobuf5 was merged 🚀 #4206

emdneto avatar Oct 14 '24 19:10 emdneto