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: use Otel SDK 1.2/0.28 #984

Merged
merged 6 commits into from
May 2, 2022
Merged

feat: use Otel SDK 1.2/0.28 #984

merged 6 commits into from
May 2, 2022

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Apr 27, 2022

Which problem is this PR solving?

fixes #983

Short description of the changes

use ^0.28.0 in dependencies to SDK experimental packages
use 1.2.0 in dev-dependencies

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #984 (49d9ca7) into main (cb915cf) will decrease coverage by 3.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #984      +/-   ##
==========================================
- Coverage   95.91%   92.23%   -3.68%     
==========================================
  Files          13      129     +116     
  Lines         856     5926    +5070     
  Branches      178     1144     +966     
==========================================
+ Hits          821     5466    +4645     
- Misses         35      460     +425     
Impacted Files Coverage Δ
...opentelemetry-instrumentation-graphql/src/utils.ts 93.40% <0.00%> (ø)
plugins/node/instrumentation-amqplib/src/utils.ts 95.38% <0.00%> (ø)
...etry-instrumentation-bunyan/src/instrumentation.ts 97.82% <0.00%> (ø)
...metry-instrumentation-mysql/src/instrumentation.ts 93.27% <0.00%> (ø)
...e/opentelemetry-instrumentation-redis/src/utils.ts 93.10% <0.00%> (ø)
...er-extension-autoinjection/src/background/index.ts 0.00% <0.00%> (ø)
...pentelemetry-instrumentation-knex/src/constants.ts 100.00% <0.00%> (ø)
...lemetry-instrumentation-dns/src/instrumentation.ts 76.38% <0.00%> (ø)
...instrumentation-nestjs-core/src/instrumentation.ts 95.65% <0.00%> (ø)
...ource-detector-aws/src/detectors/AwsEksDetector.ts 91.25% <0.00%> (ø)
... and 106 more

@Flarna
Copy link
Member Author

Flarna commented Apr 27, 2022

I tried to keep the changes low here. I have not adapted the host-metrics package as I'm not that familiar with metrics API.

There are still some dependencies to SDK packages like "@opentelemetry/core": "^1.0.1" which work but I'm not sure if we should keep them that way. Is see two options:

  • stay at minimum version and allow newer (e.g. ^1.0.0) like we do it with API
  • depend on at latest SDK or even pin the minor version

@blumamir
Copy link
Member

  • stay at minimum version and allow newer (e.g. ^1.0.0) like we do it with API

I prefer this as it will allow users to update the SDK (patch / minor) without updating each instrumentation they use.

  • depend on at latest SDK or even pin the minor version

I think that if an instrumentation does not use any feature from a new minor version, it is cleaner to allow it to use old SDK so it's less hassel for users to update SDK version.

@Flarna
Copy link
Member Author

Flarna commented Apr 27, 2022

In general I'm fine with allowing older SDKs. Main issue is that there is no test actually verifying that it works as we install newest in our tests so it's easy to miss.

@dyladan
Copy link
Member

dyladan commented Apr 27, 2022

In general I'm fine with allowing older SDKs. Main issue is that there is no test actually verifying that it works as we install newest in our tests so it's easy to miss.

We can use test all versions but the CI will take a very long time. We should do it at least for releases though

@legendecas
Copy link
Member

I've updated the host-metrics at #990.

@Flarna
Copy link
Member Author

Flarna commented Apr 29, 2022

Is it ok to merge this or do we require approval of owners of all effected packages?

@vmarchaud
Copy link
Member

I think if @dyladan is fine with it we can merge

@Flarna Flarna added the dependencies Pull requests that update a dependency file label May 2, 2022
Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

That's awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

contrib instrumentations are not compatible with SDK 1.2.0/0.28.0
10 participants