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

Update protobuf dependency to 4.x #2880

Closed
markfickett opened this issue Aug 19, 2022 · 17 comments · Fixed by #3070
Closed

Update protobuf dependency to 4.x #2880

markfickett opened this issue Aug 19, 2022 · 17 comments · Fixed by #3070

Comments

@markfickett
Copy link

Is your feature request related to a problem?
As other libraries upgrade to protobuf major version 4, it introduces a dependency conflict with opentelemetry still pinned to 3. In my case, I need to upgrade pulumi to 3.38.0 which uses protobuf 4, and that conflicts with OTel.

Describe the solution you'd like
It would be great if everyone can use the same major version of protobuf, upgrading to 4.

Describe alternatives you've considered
For my case, I tried running pulumi using pipx, but although that keeps its executable dependencies separate, it still needs to import parts of my application code in my virtual environment, so there's a conflict. I'm using infrastructure as code relating to monitoring dashboards and alerts, so the IaC is somewhat tightly coupled with my OTel code; I couldn't split them into separate projects with distinct dependencies.

Even though I can override the proto version in my virtual env, if I lock protobuf 4 in my requirements.txt then I can't install a fresh dev environment without errors (as is required for CircleCI, as well as new developers).

Additional context
OTel originally had not pinned protobuf 3, but the first release of protobuf 4.21.0 caused issues. As a result, OTel specifies protobuf~=3.13.0 in a few modules.

However, protobuf is now at 4.21.5 and if I install that version locally I am able to use OTel with the GRPC OLTP exporter.

@gen-xu
Copy link
Contributor

gen-xu commented Aug 22, 2022

This is a major breaking change afaik as old protobuf library version won't be able to use newer generated protobuf code, and it would force user to upgrade newer protobuf version, which can lead extra work in other people's code base. As protobuf is used by many other libraries too. Upgrade with a major change may not be feasible at this point

That said, there is indeed a workaround so that the generated code supports both protobuf>=4.21 and <=3.20 as shown below

from google.protobuf.internal.api_implementation import Type
if Type() == "upb":  # or int(google.protobuf.__version__.split(".")[0]) >=4
  # generated code by protoc >= 3.21
else:
  # generated code by protoc <= 3.20

This requires extra post-process of generated code though, and it is bit hacky.
I am using this workaround myself to deal with the breaking change so same opentelemetry-proto can be used along with whichever version the protobuf installed in the running environment.
If this workaround is acceptable I can create PR for this.

@jblakeney
Copy link

+1

Im currently working on a project that is using a newer protobuf (we cant downgrade) but im unable to use the opentelemetry libraries due to this issue.

@Raz-Hemo
Copy link

+1

This is super annoying and preventing us from moving forward with a project which needs google-cloud-bigquery, which depends on protobuf 4, unless we ditch opentelemetry.

@gen-xu
Copy link
Contributor

gen-xu commented Sep 29, 2022

I have created a work around for this #2954
Not sure if we want to accept PR since I would also agree if anyone think this is an issue of protobuf itself.

@ljodal
Copy link

ljodal commented Sep 30, 2022

I'm not really familiar with either protobuf or the open telemetry libraries, but it seems like Google themselves has started moving their libraries over to use protobuf 4. At least this is currently blocking us from updating a few of Google's python libraries to the newest version. So one way or another it would be great to have protobuf 4 support, and I assume most users will hit this at some point if they user other libraries that use protobuf 4

@Alphasite
Copy link

The latest version of mypy-protobuf also switched to 4.x, is there anything I can do to help with this effort?

@psigen
Copy link

psigen commented Nov 14, 2022

@gen-xu : From the original report, it seems like the correct thing to do is just regenerate the protobuf files using protobuf 3.19, and they should be forward and backwards compatible with both 3.x and 4.x versions.

Then we shouldn't need to switch between them at all.

@ocelotl
Copy link
Contributor

ocelotl commented Nov 16, 2022

@gen-xu : From the original report, it seems like the correct thing to do is just regenerate the protobuf files using protobuf 3.19, and they should be forward and backwards compatible with both 3.x and 4.x versions.

Then we shouldn't need to switch between them at all.

Hello, I have regenerated the code with protobuf 3.20.3 (also updated to use proto v0.19.0) at #3040.

@gen-xu @markfickett @jblakeney @psigen @Alphasite @ljodal @Raz-Hemo can you please try this one and let me know if it works for you?

Thanks!

@markfickett
Copy link
Author

Thanks @ocelotl !

I'm still getting a pip dependency conflict between pulumi and opentelemetry-exporter-otlp-proto-grpc. The latter directly has relaxed requirements, but it requires opentelemetry-proto which requires protobuf~=3.13, which conflicts with a 4.x requirement elsewhere.

I have recently found pipdeptree useful for sleuthing out these sorts of conflicts:

$ pip install --upgrade pipdeptree opentelemetry-exporter-otlp-proto-grpc
$ pipdeptree
opentelemetry-exporter-otlp-proto-grpc==1.14.0
  - backoff [required: >=1.10.0,<3.0.0, installed: 2.2.1]
  - googleapis-common-protos [required: ~=1.52, installed: 1.57.0]
    - protobuf [required: >=3.19.5,<5.0.0dev,!=4.21.5,!=4.21.4,!=4.21.3,!=4.21.2,!=4.21.1,!=3.20.1,!=3.20.0, installed: 3.20.3]
  - grpcio [required: >=1.0.0,<2.0.0, installed: 1.50.0]
    - six [required: >=1.5.2, installed: 1.16.0]
  - opentelemetry-api [required: ~=1.12, installed: 1.14.0]
    - deprecated [required: >=1.2.6, installed: 1.2.13]
      - wrapt [required: >=1.10,<2, installed: 1.14.1]
    - setuptools [required: >=16.0, installed: 44.0.0]
  - opentelemetry-proto [required: ==1.14.0, installed: 1.14.0]
    - protobuf [required: ~=3.13, installed: 3.20.3]
  - opentelemetry-sdk [required: ~=1.12, installed: 1.14.0]
    - opentelemetry-api [required: ==1.14.0, installed: 1.14.0]
      - deprecated [required: >=1.2.6, installed: 1.2.13]
        - wrapt [required: >=1.10,<2, installed: 1.14.1]
      - setuptools [required: >=16.0, installed: 44.0.0]
    - opentelemetry-semantic-conventions [required: ==0.35b0, installed: 0.35b0]
    - setuptools [required: >=16.0, installed: 44.0.0]
    - typing-extensions [required: >=3.7.4, installed: 4.4.0]
pip==20.0.2
pipdeptree==2.3.3
pkg-resources==0.0.0

@JoanFM
Copy link

JoanFM commented Nov 23, 2022

+1 this would be a major benefit for Jina https://github.com/jina-ai/jina

@psigen
Copy link

psigen commented Nov 23, 2022

[...] @psigen [...] can you please try this one and let me know if it works for you?

Thanks @ocelotl, it seems to be working for me for the components I am using!

@psigen
Copy link

psigen commented Nov 23, 2022

@ocelotl : I think you want to generate the protos using 3.19 (keeping that version locked in requirements.txt), and then relax the runtime version dependency to include 4.x (because now those proto files should work in both 3.1x and 4.1x -- that would fix @markficket's problem.

The key thing is that the library version in opentelemetry-proto is going to be used by clients of the library, and the point of this change is to allow the clients to be able to use protobuf 4.x at runtime. :)

Here is the line that should be expanded to allow protobuf 4.x:

@aabmass
Copy link
Member

aabmass commented Dec 1, 2022

Hey folks, I'm taking over #3040 to try to get this fixed ASAP for the next release.

I'd like to get a read of the room so we can decide if/when to drop protobuf 3 support. Please vote:

  • ❤️ if you care about Protobuf 3
  • 🚀 if you care about Protobuf 4
  • 👀 if you care about supporting BOTH 3 and 4

If 👀, please share how long you would like to see both supported.

@cretz
Copy link

cretz commented Dec 1, 2022

If 👀, please share how long you would like to see both supported.

I work on a library that optionally integrates opentelemetry. My library also has other users that use Pulumi which requires 4.x, but other may-never-go-to-4.x libraries such as ONNX and
Streamlit. I originally only supported 4.x until all the complaints kept coming.

I just merged a PR to support both where I basically gen code w/ 3.x but run CI tests w/ both. I am afraid it's the only practical way forward for libraries that are expected to be used along side other libraries that use proto. I think you have to support both until you can't.

@psigen
Copy link

psigen commented Dec 2, 2022

@cretz : I think the current form of #3040 does exactly what you did in temporal 👍

@musicinmybrain
Copy link
Contributor

I’m the maintainer of the python-opentelemetry package in Fedora Linux. It is a dependency for grpc via python-xds-protos. (I maintain those as well.)

Updating to the current grpc version will require switching to Protobuf 4, and I’m told that a lot of the current versions of Python bindings for various Google APIs also need Protobuf 4. We need to update protobuf in Fedora Linux from 3 to 4, and all the dependent packages are going to have to be able to build with Protobuf 4. There are a lot of simultaneous moving parts to this update, but we would like to do it for Fedora 38 if we can.

I can apply a PR as a downstream patch, or perhaps re-generate the bindings with Protobuf 4 myself, but things will be a lot easier if there is Protobuf 4 support in an official opentelemetry release.

In conclusion, I’m firmly in the 🚀 camp.

@aabmass
Copy link
Member

aabmass commented Dec 3, 2022

#3070 keeps compatibility with Protobuf 3 and 4. I'll open an issue for considering dropping protobuf 3 but we'll support both for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.