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

feat: update experimental deps to ^0.34.0, core deps to ^1.8.0, api to ^1.3.0 #1278

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

mhassan1
Copy link
Contributor

@mhassan1 mhassan1 commented Nov 10, 2022

Which problem is this PR solving?

This PR updates experimental dependencies to ^0.34.0 across all packages (except @opentelemetry/instrumentation-aws-lambda, which is not yet compatible with @opentelemetry/instrumentation@^0.34.0, see #1285). It also upgrades @opentelemetry/api peer dependencies to ^1.3.0 in order to satisfy ^0.34.0.

Short description of the changes

This PR updates package.json files across the repository for the following dependencies (to ^0.34.0):

  • @opentelemetry/instrumentation
  • @opentelemetry/instrumentation-fetch
  • @opentelemetry/instrumentation-xml-http-request
  • @opentelemetry/instrumentation-http
  • @opentelemetry/instrumentation-grpc
  • @opentelemetry/sdk-node
  • @opentelemetry/exporter-trace-otlp-http

It also updates those package.json files with upgraded @opentelemetry/api peer dependencies (to ^1.3.0) to satisfy the experimental dependency upgrades (#3340) , which led to upgrades of the following dependencies (to ^1.8.0, see #3340):

  • @opentelemetry/core
  • @opentelemetry/context-async-hooks
  • @opentelemetry/context-zone-peer-dep
  • @opentelemetry/resources
  • @opentelemetry/sdk-trace-base
  • @opentelemetry/sdk-trace-node
  • @opentelemetry/sdk-trace-web

@Flarna
Copy link
Member

Flarna commented Nov 11, 2022

I think the build failure here is in packages/opentelemetry-host-metrics:

test/metric.test.ts:36:10 - error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'MetricReader'.

36   public selectAggregationTemporality(): AggregationTemporality {

Maybe pull in the changes from #1281?

Edit: There is more, e.g.

test/InstanaAgentDetectorIntegrationTest.test.ts:61:31 - error TS2554: Expected 0 arguments, but got 1.
     await sdk.detectResources({
       detectors: [envDetector, processDetector, instanaAgentDetector],
     });

excludes `@opentelemetry/instrumentation-aws-lambda`,
which is not yet compatible with `@opentelemetry/instrumentation@^0.34.0`
@mhassan1 mhassan1 force-pushed the bump-instrumentation branch from 90d483e to 1da1280 Compare November 11, 2022 19:24
@mhassan1
Copy link
Contributor Author

@Flarna Where did you see the error in test/metric.test.ts? I don't see it in the workflow run, and I couldn't replicate it on my machine.

This should be ready for re-run.

@dyladan
Copy link
Member

dyladan commented Nov 11, 2022

Please avoid force pushing. It makes it harder to follow history and also I believe if you don't force push it will allow the workflows to keep running without re-approval

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #1278 (7dfafe3) into main (d39595a) will decrease coverage by 3.58%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1278      +/-   ##
==========================================
- Coverage   96.08%   92.49%   -3.59%     
==========================================
  Files          14      118     +104     
  Lines         893     6158    +5265     
  Branches      191     1222    +1031     
==========================================
+ Hits          858     5696    +4838     
- Misses         35      462     +427     
Impacted Files Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 90.16% <0.00%> (ø)
...nstrumentation-fastify/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...urce-detector-alibaba-cloud/src/detectors/index.ts 100.00% <0.00%> (ø)
...ode/opentelemetry-instrumentation-net/src/utils.ts 92.00% <0.00%> (ø)
...opentelemetry-instrumentation-graphql/src/utils.ts 93.17% <0.00%> (ø)
...lemetry-instrumentation-net/src/instrumentation.ts 98.09% <0.00%> (ø)
...metry-resource-detector-aws/src/detectors/index.ts 100.00% <0.00%> (ø)
...etry-instrumentation-bunyan/src/instrumentation.ts 97.82% <0.00%> (ø)
...try-instrumentation-express/src/instrumentation.ts 99.28% <0.00%> (ø)
...instrumentation-nestjs-core/src/instrumentation.ts 95.65% <0.00%> (ø)
... and 94 more

@blumamir
Copy link
Member

@mhassan1 the title of the PR is "update experimental deps to ^0.34.0."

Your PR also updates core dependencies and peer dependencies on @opentelemetry/api to latest.
Any reason for that? those dependencies are deliberately set to the lowest supported version and not the latest.

You can use #1143 as reference.

@mhassan1
Copy link
Contributor Author

mhassan1 commented Nov 12, 2022

I put that explanation in the PR description. We need to upgrade the api explicitly because ^1.0.0 no longer satisfies the peer dependency requirement of the experimental packages, which has been bumped to ^1.3.0 in #3340. Similarly, the core dependencies need to be upgraded because earlier versions have an api peer dependency range that doesn't include ^1.0.0 (#3340).

@mhassan1 mhassan1 changed the title feat: update experimental deps to ^0.34.0 feat: update experimental deps to ^0.34.0, core deps to ^1.8.0, api to ^1.3.0 Nov 12, 2022
@Flarna
Copy link
Member

Flarna commented Nov 14, 2022

@Flarna Where did you see the error in test/metric.test.ts? I don't see it in the workflow run, and I couldn't replicate it on my machine.

Maybe I mixed something up during working on #1281. CI seems to be green here so no need to care about.

@blumamir
Copy link
Member

I put that explanation in the PR description. We need to upgrade the api explicitly because ^1.0.0 no longer satisfies the peer dependency requirement of the experimental packages, which has been bumped to ^1.3.0 in #3340. Similarly, the core dependencies need to be upgraded because earlier versions have an api peer dependency range that doesn't include ^1.0.0 (#3340).

Thanks for the explanation.

Now the codebase mixes core package versions. some are at ^1.0.0 - semantic-conventions, resources, and some are bumped to ^1.8.0.
I see why it's not mandatory to upgrade those, but wonder how much value we will get from it. Maybe we should just align all core packages to the same version range ^1.8.0? That will decrease the cognitive load of managing these dependencies

@Flarna
Copy link
Member

Flarna commented Nov 14, 2022

I created open-telemetry/opentelemetry-js#3408 as this mix is not only here.

@Flarna
Copy link
Member

Flarna commented Nov 14, 2022

except @opentelemetry/instrumentation-aws-lambda, which is not yet compatible with @opentelemetry/instrumentation@^0.34.0

Any details? Maybe best to create a new issue and tag the AWS lambda instrumentation owners about this.

@mhassan1
Copy link
Contributor Author

Now the codebase mixes core package versions. some are at ^1.0.0 - semantic-conventions, resources, and some are bumped to ^1.8.0. I see why it's not mandatory to upgrade those, but wonder how much value we will get from it. Maybe we should just align all core packages to the same version range ^1.8.0? That will decrease the cognitive load of managing these dependencies

@blumamir I was trying to keep this pull request focused on getting experimental packages to ^0.34.0, but I am open to it.

except @opentelemetry/instrumentation-aws-lambda, which is not yet compatible with @opentelemetry/instrumentation@^0.34.0

Any details? Maybe best to create a new issue and tag the AWS lambda instrumentation owners about this.

@Flarna See #1285.

@mhassan1
Copy link
Contributor Author

I see codecov/project is failing. Do I need to address that? I didn't make any real code changes in this PR, so I'm not sure how coverage could have gone down, and I don't see any coverage diffs in the report.

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this, adding my moral support 🙂

@dyladan
Copy link
Member

dyladan commented Nov 15, 2022

The coverage report doesn't really make sense. This may be the result of uploading reports from multiple test branches (web and node). In any case this PR didn't change coverage so i think it's ok.

@SimenB
Copy link
Contributor

SimenB commented Nov 15, 2022

Yay! Is there a time estimate for when this'll get released? 🙂

@mhassan1 mhassan1 deleted the bump-instrumentation branch November 17, 2022 18:49
@dyladan dyladan mentioned this pull request Jan 2, 2023
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.