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

chore(deps): Allow google-protobuf ~> 3.14 #1500

Merged
merged 2 commits into from Jul 27, 2023

Conversation

arielvalentin
Copy link
Contributor

@arielvalentin arielvalentin commented Jul 27, 2023

GitHub still uses legacy versions of the protobuf gem. This makes it challenging to
upgrade to newer versions of the OTLP Exporter which only supports versions ~> 3.19.

This change loosens the restrictions to allow GitHub to adopt newer versions of the protobuf
definitions using an older verson of the library.

This change explicitly skips 3.15 due to bugs like protocolbuffers/protobuf#8337

@simi
Copy link
Contributor

simi commented Jul 27, 2023

I'm not sure it is open-telemetry responsibility to prevent dependencies with bugs. If anyone runs 3.15 version of protobuf and is happy about, there is no need to block it. If 3.15 doesn't work at all (AFAIK it works, just with minor problems at some point), it should be considered yanked.

@arielvalentin
Copy link
Contributor Author

@simi I did this out of an abundance of caution based on our experience with 3.15 at GitHub and Shopify.

I think the use case we are mostly concerned with is where the application isn't using protobuf and for some reason the buggy 3.15 is pulled in then the application suffers from performance problem and runs into bugs as a result of the Exporter bringing in a bad dependency.

That being said, I do see your point about how 3.15 had additional bug fix releases and it will prevent users that are happy with it from using the gem. For those reasons I am going to change the constraints and allow 3.15 installs.

exporter/otlp/Appraisals Outdated Show resolved Hide resolved
exporter/otlp/Appraisals Outdated Show resolved Hide resolved
exporter/otlp/opentelemetry-exporter-otlp.gemspec Outdated Show resolved Hide resolved
arielvalentin added a commit to arielvalentin/opentelemetry-ruby that referenced this pull request Jul 27, 2023
@arielvalentin arielvalentin self-assigned this Jul 27, 2023
@arielvalentin arielvalentin marked this pull request as ready for review July 27, 2023 20:02
@arielvalentin arielvalentin marked this pull request as draft July 27, 2023 20:04
@robertlaurin
Copy link
Contributor

robertlaurin commented Jul 27, 2023

I'm not sure it is open-telemetry responsibility to prevent dependencies with bugs. If anyone runs 3.15 version of protobuf and is happy about, there is no need to block it. If 3.15 doesn't work at all (AFAIK it works, just with minor problems at some point), it should be considered yanked.

I agree with that point of view. We (Shopify) saw a host of issues with memory pressure as a result of that release. While we don't want this gem to be positioned as responsible for making sure no one pulls in potentially problematic releases of its dependencies, we saw a large amounts of reports coming in internally around "OpenTelemetry has a memory leak", when in fact it did not.

We're really just trying to do the right thing in terms of potentially protecting downstream users of this package (ourselves included) and preventing support burden from misidentified memory leaks.

Edit: I had this PR opened from earlier today and did not see Ariels response. I'm fine with removing that constraint, we'll just add it internally and hope the community doesn't run into those issues 😄

GitHub still uses legacy versions of the protobuf gem. This makes it challenging to
upgrade to newer versions of the OTLP Exporter which only supports versions `~> 3.19`.

This change loosens the restrictions to allow GitHub to adopt newer versions of the protobuf
definitions using an older verson of the library.

This change explicitly skips 3.15 due to bugs like protocolbuffers/protobuf#8337
@arielvalentin arielvalentin marked this pull request as ready for review July 27, 2023 20:07
@arielvalentin
Copy link
Contributor Author

@robertlaurin @simi et.al. I rebased main onto this branch so it should be a clean review now.

Copy link
Contributor

@robertlaurin robertlaurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just waiting on CI now

@robertlaurin robertlaurin merged commit 08aec4b into open-telemetry:main Jul 27, 2023
46 checks passed
@arielvalentin
Copy link
Contributor Author

@robertlaurin thank you!!! This website where I opened this PR also thanks you!!!

@arielvalentin arielvalentin deleted the google-protobuf-3.14 branch August 1, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants