feat(baseplugin): add internalfilesloader #240
Conversation
markwolff
requested review from
bg451,
danielkhan,
iredelmeier,
mayurkale22,
OlivierAlbertini and
rochdev
as code owners
packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts
Outdated
Show resolved
Hide resolved
| protected _moduleExports!: T; | ||
| protected _tracer!: Tracer; | ||
| protected _logger!: Logger; | ||
| // tslint:disable-next-line:no-any | ||
| protected _internalFilesExports!: { [module: string]: any }; // output for internalFilesExports |
There was a problem hiding this comment.
could we use unknown instead of any there ?
There was a problem hiding this comment.
@vmarchaud With this, the expectation would be to load the types for the internal file in each plugin. Those typings probably probably won't exist if it isn't a typescript library and so the plugin would have to cast it to any anyways in that case. I'm okay with this behavior of encouraging trying to use types and passing along the burden of any casting to each plugin. Changed to unknown and made additional cast in grpc plugin. WDYT?
| this._logger.debug('loading File %s', extraModulesList[moduleName]); | ||
| outExtraModules[moduleName] = require(path.join( | ||
| basedir, | ||
| extraModulesList[moduleName] |
There was a problem hiding this comment.
This won't work in webpack because of the static import checking it does for tree shaking (hardcoding the path name works, but not the joining of basedir and the filepath. The internal file import shows up as not found). Ideally, we don't want people to muck with their webpack configs just to get plugins working in their browser bundling env.
AFAIK we don't have any plugins that would run outside of node.js, which makes me think this PR should live in PluginLoader instead? Ideally this functionality would be universal (node + browser) for any future "pure javascript" library we provide a plugin for, but it does not seem trivial from my first go at it.
/cc @mayurkale22 @draffensperger WDYT?
There was a problem hiding this comment.
I had a few conversations about the plugin loader system for the browser - my opinion is that this approach of treating packages like a runtime object isn't a good fit for the browser, where people can compile code using webpack or even Rollup + Closure Compiler and other approaches.
My opinion is that in the browser we should be monkey-patching not packages, but the browser APIs themselves (like XMLHttpRequest, etc.). Zone.js actually exists purely to monkey-patch all async browser APIs and we used it effectively in OpenCensus Web. I'm open to being wrong on this, but it's my understanding and opinion right now.
So I'm with this this being Node-specific.
As far as what package to put it in .... I'm open. I think there was a challenge with putting it in node-sdk because then we get circular dependency issues. I'm fine with leaving it in core. Maybe just explain in a comment that this way of dealing with plugins is designed to be Node-specific.
|
ping, for more reviews. |
Which problem is this PR solving?
BasePluginimplementation.Short description of the changes