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(instrumentation): use all provided filepatches #2963

Merged
merged 5 commits into from
Jun 22, 2022

Conversation

Ugzuzg
Copy link
Contributor

@Ugzuzg Ugzuzg commented May 12, 2022

Which problem is this PR solving?

Fixes the issue when only the first InstrumentationNodeModuleFile for each path is applied.

Fixes #2355

Short description of the changes

Filter and apply all InstrumentationNodeModuleFiles instead of just finding the first.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Added a unit test with multiple InstrumentationNodeModuleFile targeting the same file

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #2963 (87d0285) into main (cb642d7) will decrease coverage by 0.01%.
The diff coverage is 90.00%.

❗ Current head 87d0285 differs from pull request most recent head b96cdf0. Consider uploading reports for the commit b96cdf0 to get more accurate results

@@            Coverage Diff             @@
##             main    #2963      +/-   ##
==========================================
- Coverage   92.66%   92.65%   -0.02%     
==========================================
  Files         187      187              
  Lines        6174     6178       +4     
  Branches     1305     1304       -1     
==========================================
+ Hits         5721     5724       +3     
- Misses        453      454       +1     
Impacted Files Coverage Δ
...strumentation/src/platform/node/instrumentation.ts 63.73% <90.00%> (+0.51%) ⬆️

@Ugzuzg Ugzuzg force-pushed the fix/patch-multiple branch 2 times, most recently from 1728496 to 68dc7e9 Compare May 12, 2022 17:44
@Ugzuzg Ugzuzg marked this pull request as ready for review May 12, 2022 17:52
@Ugzuzg Ugzuzg requested a review from a team May 12, 2022 17:52
@Ugzuzg Ugzuzg force-pushed the fix/patch-multiple branch 2 times, most recently from 98defc5 to b185c29 Compare May 12, 2022 18:01
@dyladan
Copy link
Member

dyladan commented May 12, 2022

I reran the browser test and it still failed in the same way. weird because this doesn't touch any browser code.

@Ugzuzg
Copy link
Contributor Author

Ugzuzg commented May 12, 2022

@pichlermarc
Copy link
Member

I reran the browser test and it still failed in the same way. weird because this doesn't touch any browser code.

@dyladan the failing tests here seem to be related to #2852. It's predominantly the tests for instrumentation-fetch. I've seen it across multiple PRs.

@Ugzuzg
Copy link
Contributor Author

Ugzuzg commented May 13, 2022

I think, I have a fix for these tests here: #2969

@Ugzuzg Ugzuzg force-pushed the fix/patch-multiple branch 2 times, most recently from e6d96cb to c0d9629 Compare May 25, 2022 19:57
@dyladan dyladan merged commit b891509 into open-telemetry:main Jun 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.

Only the first InstrumentationNodeModuleFile for each path is applied
5 participants