-
Notifications
You must be signed in to change notification settings - Fork 647
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
upgrade opentelemetry-exporter-prometheus-remote-write to use protobuf 5.26 #3219
base: main
Are you sure you want to change the base?
upgrade opentelemetry-exporter-prometheus-remote-write to use protobuf 5.26 #3219
Conversation
…_prometheus_remote_write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey: thanks for the PR. I think we should at least add a test using protobuf 5, although tests pass and there is no guarantee for cross-version runtime. What we did in opentelemetry-proto was to support only one major version, protobuf 5.
@aabmass do you think we should wait for the protobuf Q12025 release for this kind of issue?
It's not arbitrary like @emdneto mentioned, protobuf makes it very clear that this is not officially supported. This PR lets the user decide but I fear people will come here when things break and expect us to fix it. I would prefer to do what @emdneto mentioned and just support protobuf 5. It's been out for almost a year now. Is there a not a newer streamlit version that works with protobuf 5? |
I am completely fine with just having 5.x supported. steamlit isn't a specific issue for me but just something I ran into when testing in a few different environments. I'll update this to be only protobuf 5.x. |
https://protobuf.dev/support/version-support/#python picked 5.26 based on this page. |
@emdneto more than happy to write some tests although I am not sure exactly what kind of test to write here to improve coverage. I ran I do see that we are loading the protobufs in the tests |
If we're just doing protobuf 5 I think his comment is resolved. Current tests should be ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think we need to regenerate the protobuf code too?
...pentelemetry-exporter-prometheus-remote-write/tests/test_prometheus_remote_write_exporter.py
Outdated
Show resolved
Hide resolved
Just regenerated with protoc 29.3 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with Changelog entry
Was thinking to add this under Breaking changes in the main CHANGELOG.md since upon update in the case someone has pinned protobuf to a specific version it will cause issues... But this is also true for any dependency update so I'm not 100% sure if this should go under another area. |
These small PRs always turn out to have the most foot guns 😄 I was just testing some more locally to make sure I don't break anything for people and was running the tests with different versions of presumably supported protobuf versions as defined in the requirements (~=5.26). Because I generated with 29.3 protobuf will blow up with anything that isn't python protobuf version 5.29.3 or later. Although we could just say this is the min version we should likely be nice to people since they are all still supported. I'll regenerate using v26 of protoc so that we actually support ~=5.26. Learning on the go here although i have used protobuf before I haven't had to care much about versions since I controled client and server and generate them at the same time. With the PR as it currently is installing 5.26 python protobuf throws the following.
|
Yeah. In that case, if we want to support starting from 5.26 we must re-generate it with protoc 26. You can also add a second test-requirements.txt file with protobuf 5.29, which should also work. With that we are sure 5.26 and newer runtime versions are covered (which is guaranteed according to this) |
Done, updated docs and change log and regenerated protos with v26. |
Description
This change for my case it to allow us to use this in a project that needs to use protobuf 5.29. I have tested in that project that everything still works as expected so the version limit is only based on the restriction in the produced pypi package not on any code limitation.
I originally was looking at saying the package uses =~5.29 however this conflicts with another random package I had installed
streamlit 1.30.0 requires protobuf<5,>=3.20, but you have protobuf 5.29.3 which is incompatible
. Which made me think it might be nice to let people use all known working versions of protobuf with this package instead of arbitrarily deciding what one is correct.Fixes #3179
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I tested locally by running
Does This PR Require a Core Repo Change?
I don't think so?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.