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(otlp-proto-exporter-base): update protobufjs to 7.1.2 #3433

Merged

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Nov 21, 2022

Relax protobufjs versioning and update it to the latest version. This avoids pulling in potentially duplicate protobufjs packages.

@seemk seemk requested a review from a team November 21, 2022 10:46
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #3433 (06a4969) into main (c9a3494) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3433      +/-   ##
==========================================
+ Coverage   93.28%   93.29%   +0.01%     
==========================================
  Files         247      247              
  Lines        7352     7352              
  Branches     1512     1512              
==========================================
+ Hits         6858     6859       +1     
+ Misses        494      493       -1     
Impacted Files Coverage Δ
...-trace-base/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

@@ -65,7 +65,7 @@
"dependencies": {
"@opentelemetry/core": "1.8.0",
"@opentelemetry/otlp-exporter-base": "0.34.0",
"protobufjs": "7.1.1"
"protobufjs": "^7.1.2"
Copy link
Member

Choose a reason for hiding this comment

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

Dependabot will just open a PR to pin this if you don't change the config. Also, it was an intentional choice to pin dependencies so that our users can effectively pin.

Copy link
Contributor Author

@seemk seemk Nov 21, 2022

Choose a reason for hiding this comment

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

Currently installing @opentelemetry/exporter-trace-otlp-grpc and @opentelemetry/exporter-trace-otlp-proto side by side brings in 2 different protobufjs versions: 7.1.1 and 7.1.2.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like the problem here is that we can not effectively pin indirect dependencies like @grpc/grpc-js -> @grpc/proto-loader -> protobufjs ^7.0.0.

I noticed that the dependencies on @grpc/grpc-js are declared as caret range, e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/exporter-trace-otlp-grpc/package.json#L71. So I'd find it should be fine for us to apply caret range for protobufjs too.

Copy link
Member

Choose a reason for hiding this comment

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

usually we pin only dev-dependencies and the otel dependencies which are from same lerna project (and therefore released together).
e.g. @opentelemetry/exporter-jaeger depends on "jaeger-client": "^3.15.0", @opentelemetry/instrumentation depends on "require-in-the-middle": "^5.0.3", "semver": "^7.3.2" and "shimmer": "^1.2.1".

@@ -65,7 +65,7 @@
"dependencies": {
"@opentelemetry/core": "1.8.0",
"@opentelemetry/otlp-exporter-base": "0.34.0",
"protobufjs": "7.1.1"
"protobufjs": "^7.1.2"
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the problem here is that we can not effectively pin indirect dependencies like @grpc/grpc-js -> @grpc/proto-loader -> protobufjs ^7.0.0.

I noticed that the dependencies on @grpc/grpc-js are declared as caret range, e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/exporter-trace-otlp-grpc/package.json#L71. So I'd find it should be fine for us to apply caret range for protobufjs too.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

OK sounds good

@dyladan dyladan merged commit 3290b25 into open-telemetry:main Nov 22, 2022
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.

5 participants