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: update OTel deps #954

Closed
wants to merge 7 commits into from

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented Mar 24, 2022

Which problem is this PR solving?

Problem explained in open-telemetry/opentelemetry-js#2863
It's another experiment to see if we can get away without updating all the dependencies for now.

Short description of the changes

  1. Leave ranges the same because they usually don't need to be stricter.
  2. Update pinned API version in dev deps not to conflict with the updated SDK packages.

This is an alternative solution to #953

@github-actions github-actions bot requested a review from obecny March 24, 2022 11:12
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #954 (1c314cc) into main (acd6a6f) will decrease coverage by 3.88%.
The diff coverage is 93.54%.

❗ Current head 1c314cc differs from pull request most recent head 6c36108. Consider uploading reports for the commit 6c36108 to get more accurate results

@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   95.91%   92.02%   -3.89%     
==========================================
  Files          13      127     +114     
  Lines         856     5833    +4977     
  Branches      178     1122     +944     
==========================================
+ Hits          821     5368    +4547     
- Misses         35      465     +430     
Impacted Files Coverage Δ
...tor-docker/src/detectors/DockerCGroupV1Detector.ts 93.33% <93.33%> (ø)
...ry-resource-detector-docker/src/detectors/index.ts 100.00% <100.00%> (ø)
...er-extension-autoinjection/src/background/index.ts 0.00% <0.00%> (ø)
...ource-detector-aws/src/detectors/AwsEcsDetector.ts 100.00% <0.00%> (ø)
packages/opentelemetry-host-metrics/src/util.ts 91.66% <0.00%> (ø)
...ode/opentelemetry-instrumentation-net/src/utils.ts 90.47% <0.00%> (ø)
...aba-cloud/src/detectors/AlibabaCloudEcsDetector.ts 97.67% <0.00%> (ø)
...telemetry-instrumentation-restify/src/constants.ts 100.00% <0.00%> (ø)
...etry-instrumentation-bunyan/src/instrumentation.ts 97.82% <0.00%> (ø)
...metry-instrumentation-mysql/src/instrumentation.ts 93.27% <0.00%> (ø)
... and 104 more

@rauno56
Copy link
Member Author

rauno56 commented Mar 24, 2022

That's a good start. No incompatible packages anymore. I imagine api version range in peerDeps being invalid still for contrib packages. Will check those next.

@rauno56 rauno56 marked this pull request as ready for review March 25, 2022 15:11
@rauno56 rauno56 requested a review from a team March 25, 2022 15:11
@rauno56 rauno56 changed the title chore: update pinned API deps only chore: update OTel deps Mar 28, 2022
@@ -26,7 +26,7 @@
"@opentelemetry/api": "^1.0.2"
},
"devDependencies": {
"@opentelemetry/api": "1.0.2",
"@opentelemetry/api": "1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

below quite some deps are pinned to fixed versions (github doesn't allow to comment there). I think we should ranges there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed those as well.

@@ -38,7 +38,7 @@
"@opentelemetry/api": "^1.0.2"
},
"devDependencies": {
"@opentelemetry/api": "1.0.2",
"@opentelemetry/api": "1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

There are pinned versions in dependencies below.

Copy link
Member Author

Choose a reason for hiding this comment

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

And these.

@Flarna
Copy link
Member

Flarna commented Mar 28, 2022

There are still some inconsistencies in the ranges used, e.g. sometimes we have

"peerDependencies": {
  "@opentelemetry/api": "^1.0.4"
},
"dependencies": {
  "@opentelemetry/resources": "^1.0.1",
  "@opentelemetry/semantic-conventions": "^1.0.1"
}

but sometimes like this:

"peerDependencies": {
  "@opentelemetry/api": "^1.0.2"
},
"dependencies": {
  "@opentelemetry/core": "^1.0.0",
  "@opentelemetry/resources": "^1.0.0",
  "@opentelemetry/semantic-conventions": "^1.0.0"
}

I guess we should do some cleanup in this regard also but no need to do this in this PR.

@Flarna
Copy link
Member

Flarna commented Mar 28, 2022

Another point (most likely also for a followup) is the version range used by components implementing API (propagators). On core we use a reduced range but not here.

"@opentelemetry/sdk-trace-node": "^1.1.1",
"@opentelemetry/resources": "^1.1.1",
"@opentelemetry/sdk-trace-base": "^1.1.1",
"@opentelemetry/semantic-conventions": "^1.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Why is here ^1.0.1 but the others are at ^1.1.1?
At other places you moved to 1.1.x only for dep-dependencies but not for prod dependencies. So maybe it should be ^1.0.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only updated core packages that rely on api, semantic conventions does not.
Since it was 1.0.1 before, I didn't want to to downgrade the possible versions and just opened it up for upgrades.

@Flarna
Copy link
Member

Flarna commented Mar 30, 2022

seems long task is unhappy now because it uses 1.1.1 for sdk-trace-base but 1.0.1 for sdk-trace-web as latest tag was moved.

@rauno56
Copy link
Member Author

rauno56 commented Mar 31, 2022

We're going to redo this once the peerDependency ranges in core are fixed increasing lower bound on API in Trace SDK should've not done.

An alternative would be to bump everything to 1.1 in contrib, which we agreed to not to do for now in SIG.

But the point on propagators is valid - since propagators implement API they should have a similar mechanism to Core package and cap the full range of API versions they support. I'll fix that in another PR.

@rauno56 rauno56 closed this Mar 31, 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.

8 participants