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(node): allow passing in plugin loader instead of a module path #1854

Closed
wants to merge 1 commit into from

Conversation

dobesv
Copy link
Contributor

@dobesv dobesv commented Jan 20, 2021

In environments like Yarn PnP you cannot call require to load arbitrary modules,
you can only load modules that your module explicitly lists in its dependencies.

This change allows developers in these environments to pass in a function that does
the require operation within the context of a module that does declare the
appropriate dependency.

Note: this is a bit of a proposal at this point. If you think it's not a good idea, we
can discard it. If it's good but in need of tests, some tips on how you would test
this would be great.

In environments like Yarn PnP you cannot call `require` to load arbitrary modules,
you can only load modules that your module explicitly lists in its dependencies.

This change allows developers in these environments to pass in a function that does
the require operation within the context of a module that does declare the
appropriate dependency.
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #1854 (6ec7660) into master (2fcb76d) will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
- Coverage   92.63%   92.60%   -0.04%     
==========================================
  Files         174      174              
  Lines        6054     6056       +2     
  Branches     1288     1290       +2     
==========================================
  Hits         5608     5608              
- Misses        446      448       +2     
Impacted Files Coverage Δ
...telemetry-node/src/instrumentation/PluginLoader.ts 92.20% <66.66%> (-1.13%) ⬇️
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@dyladan
Copy link
Member

dyladan commented Jan 20, 2021

Note: this is a bit of a proposal at this point. If you think it's not a good idea, we
can discard it. If it's good but in need of tests, some tips on how you would test
this would be great.

If that is the case, marking it as Draft might be a good idea.

FWIW we were planning on removing the plugin loading system in favor modules which inherit from the InstrumentationBase class. These Instrumentation based modules call the API directly and are simple to enable without a loader mechanism.

@dobesv
Copy link
Contributor Author

dobesv commented Jan 20, 2021

If that is the case, marking it as Draft might be a good idea.

I can't change the labels on this PR from what I can tell. Is that what you mean by marking it as draft? I mean, if you're OK with this to merge as-is, go for it, but otherwise I won't be bothered if you say this is not the way.

FWIW we were planning on removing the plugin loading system

Makes sense. This is a quick stopgap solution for anyone running into this problem. But if you think the migration to instrumentation will be done soon enough, feel free to close this one.

@dyladan dyladan marked this pull request as draft January 20, 2021 20:08
@obecny
Copy link
Member

obecny commented Jan 21, 2021

PluginLoader is going to be removed from node package here . It was already moved to instrumentation package and then it will be removed once all plugins are converted to instrumentations. Can you please verify how would similar approach works for instrumentations and if this is still something that could be improved ?

@dobesv dobesv closed this Jan 22, 2021
@dobesv dobesv deleted the plugin-dep-module branch January 22, 2021 17:31
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 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.

None yet

3 participants