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

fix(deps): update otel core experimental to ^0.38.0 #1468

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Apr 17, 2023

Which problem is this PR solving?

Short description of the changes

  • Updated experimental packages to ^0.38.0
  • added droppedAttributesCount: 0 to span assertion in cassandra instrumentation tests

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #1468 (10b1f99) into main (d2d8288) will decrease coverage by 2.73%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1468      +/-   ##
==========================================
- Coverage   96.13%   93.41%   -2.73%     
==========================================
  Files          14       98      +84     
  Lines         906     4706    +3800     
  Branches      197      885     +688     
==========================================
+ Hits          871     4396    +3525     
- Misses         35      310     +275     

see 84 files with indirect coverage changes

@pichlermarc
Copy link
Member Author

Not exactly sure why the coverage step is failing. Investigating...

@Flarna
Copy link
Member

Flarna commented Apr 21, 2023

Is there any chance to get this merged soon?

My local build files with following error:

test/InstanaAgentDetectorIntegrationTest.test.ts:62:7 - error TS2322: Type 'import("c:/workspaces/GitHubForks/opentelemetry-js-contrib/node_modules/@opentelemetry/resources/build/src/Resource").Resource' is not assignable to type 'import("c:/workspaces/GitHubForks/opentelemetry-js-contrib/node_modules/@opentelemetry/sdk-node/node_modules/@opentelemetry/resources/build/src/Resource").Resource'.
  Types have separate declarations of a private property '_syncAttributes'.

62       resource: globalResource,

I assume the build in #1471 fails for the same reason (no useful logs in CI for whatever reason).

Above typescript error indicates that more then one version of @opentelemetry/resources is there which is likely because instana detector moved to 0.37 of @opentelemetry/sdk-node which in turn pulls in 0.37 of other experimental packages and maybe other version of GA core packages.
No idea why main broke without a PR. Maybe because OTel core was released and GA packages are selected via ^here.

@pichlermarc pichlermarc force-pushed the renovate/otel-core-experimental branch from 2b94e87 to e82964f Compare April 21, 2023 09:11
@pichlermarc
Copy link
Member Author

pichlermarc commented Apr 21, 2023

Maybe because OTel core was released and GA packages are selected via ^here.

I think this is most likely the issue.

I pushed the last commit again to re-run the codecov step, it does not seem to update the comment/check-status even when re-running the pipeline to upload new reports. Looks like the previous failing step was a false positive, now it's reporting the expected coverage. EDIT: looks like still the same problem. 🙁

There is a flaky test in the instrumentation-nestjs-core that is triggered by the limited compute resources in CI, which makes the test run into the timeout. I'll create an issue to follow up on this but I don't think this should prevent this PR from merging.
It kept failing, so I increased the timeout to 5000ms in 10b1f99.

@pichlermarc pichlermarc marked this pull request as ready for review April 21, 2023 10:10
@pichlermarc pichlermarc requested a review from a team April 21, 2023 10:10
@pichlermarc pichlermarc force-pushed the renovate/otel-core-experimental branch from b91b7ed to 10b1f99 Compare April 21, 2023 10:34
@pichlermarc
Copy link
Member Author

pichlermarc commented Apr 24, 2023

I was not able to reproduce the coverage problem locally. This seems to be a problem with codecov not merging the coverage correctly on main, where it only takes web package coverage into account - which seems to have a higher percentage of covered lines than the node packages:

https://app.codecov.io/gh/open-telemetry/opentelemetry-js-contrib/tree/main
https://app.codecov.io/gh/open-telemetry/opentelemetry-js-contrib/tree/renovate%2Fotel-core-experimental

I believe this should be okay to merge even with the codecov step failing.
It's out of scope for this PR to fix it but I'll create an issue to follow up on this. Edit: bug issue I created to follow up on this: #1473

This branch's coverage report (detecting 4706 lines and including detectors)
image

main branch's coverage report (detecting only 906 lines)
image

@pichlermarc pichlermarc merged commit 565a2b2 into open-telemetry:main Apr 24, 2023
@pichlermarc pichlermarc deleted the renovate/otel-core-experimental branch April 24, 2023 14:18
@dyladan dyladan mentioned this pull request Apr 24, 2023
@dyladan dyladan mentioned this pull request Feb 13, 2024
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.

10 participants