-
Notifications
You must be signed in to change notification settings - Fork 648
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
Add support to protobuf5 for opentelemetry-proto and regenerate proto files #4206
Conversation
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
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.
Thanks Emidio!
opentelemetry-proto/src/opentelemetry/proto/collector/logs/v1/logs_service_pb2_grpc.py
Show resolved
Hide resolved
Signed-off-by: emdneto <[email protected]>
@emdneto regarding the docs CI failure
Should we just do the workaround since it's only happening as a result of docs needing everyhting installed in one virtualenv? |
Signed-off-by: emdneto <[email protected]>
exporter/opentelemetry-exporter-otlp-proto-common/test-requirements.txt
Outdated
Show resolved
Hide resolved
The |
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, as already noted we have a few tests-requirements-{0,1}.txt to remove though
…ove unused dependencies from tox.ini Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
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.
I am trying to check if users who are stuck on protobuf <5 will be able to continue getting API/SDK updates by using older exporter version. It looks like no because we have this dep specifier in both the proto-http and proto-grpc exporters:
"opentelemetry-sdk ~= 1.27.0",
which is equivalent
>= 1.27.0, == 1.27.*
i.e. no new minor versions of the SDK. Does this concern you @emdneto @xrmx? I don't think there is any specific reason for this, we should probably fix it.
One other option. Since the newly generated protos didn't require any code changes in their usage sites, we could relax the |
Signed-off-by: emdneto <[email protected]>
I think we bump this in every release no? So I suppose that's why is 1.27.0 - to match the release script?. I'm also concerned about this:
Since in this version they re-generated the pb2 files to support protobuf5 and also this
Because we re-generated the files with grpcio-tools 1.63.2. If we have grpcio < 1.63.2 user will get the warning:
|
As discussed in the 10/10/2024 SIG,
@aabmass after you had to drop, we do recognize that this is a valid use case and we have gotten issues before where users could not use new versions of protobuf but also wanted to use the latest versions of the sdk. We don't want users to be in this "stuck" position, especially if we are considering "fixing this bug" (loosening the version dependency on the sdk in the exporter) AND releasing protobuf 5 at the same release. I think for the sake of speed and to not disrupt the momentum of this PR, we can go ahead with this change and release at the same time, and then we can always retroactively patch the previous release version if we ever encounter users who are in need of using sdk >= 1.27 to loosen the restriction. wdyt? |
Yes that's what I was going to suggest too. SGTM! |
I'm OK to set a stricter constraint in this PR, wdyt? Re googleapis-common-protos, ideally they would have set proper dependency constraints and the pip resolver could figure it out. |
Signed-off-by: emdneto <[email protected]>
@aabmass Added constraint to grpc>=1.63.2 for otlp grpc exporter |
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.
Thanks, LGTM!
Description
Follow-up of this thread #3958 and continue the work from this PR #3931
We are adding support to only one major version of protobuf. In that case, 5
Main changes
Steps to generate
grpcio-tools==1.63.2
Issues
--prefer-stubs yes
parameter for lint in the exporters that use protoPROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python
environment variable.