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): support multiple module definitions with different versions #2120

Merged
merged 3 commits into from
Apr 20, 2021

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Apr 19, 2021

Which problem is this PR solving?

Fixes #2119

Short description of the changes

For the detailed problem description refer to the linked issue.

In short: don't iterate over all module definitions at onRequire hook when checking for supported library versions. The hook already has the module definition. isSupported might otherwise find another module and report the supported status back falsely.

@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #2120 (a987527) into main (27f64d9) will increase coverage by 0.13%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2120      +/-   ##
==========================================
+ Coverage   92.67%   92.80%   +0.13%     
==========================================
  Files         140      140              
  Lines        4982     4976       -6     
  Branches     1030     1028       -2     
==========================================
+ Hits         4617     4618       +1     
+ Misses        365      358       -7     
Impacted Files Coverage Δ
...strumentation/src/platform/node/instrumentation.ts 24.28% <0.00%> (+1.91%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 93.75% <0.00%> (+6.25%) ⬆️

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

I like that this also makes isSupported a function. Better for minification and less risk of leaking impl details to end users.

@dyladan dyladan added the bug Something isn't working label Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instrumentation: respect multiple instrumentation module definitions with different supported versions
4 participants