feat: support 'otel:bundle:load' diagnostics_channel message for instrumentation in bundles #4818
Conversation
…ntationModuleDefinition
…rumentation in bundles
…nstrumentation (More details to come.) Refs: open-telemetry#1856 Refs: open-telemetry/opentelemetry-js#4818
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
There was a problem hiding this comment.
👋 Thanks for your awesome work on this. I was able to try it out on our single-file server build and it worked out great for the most part, but I found one issue with instrumentations that supported multiple versions with the same file path. Only the last one specified would be found in the imdFromHookPath map.
I figured it was probably safe to turn the value type of that map into an array, since calling ._onRequire for a instrumentation file definition that doesn't match the version is essentially a no-op.
I've added the changes I made as suggestions on this PR, hopefully you find it useful. It's working for me and I'm finally able to get trace_id and span_id in my winston logs 💪
Again, thank you so much for this work! Hopefully it makes it into the package proper soon 👍
PS: I also made some performance tweaks to the related esbuild plugin, I'll see about commenting on that PR with those changes later today.
| @@ -293,6 +298,9 @@ export abstract class InstrumentationBase< | |||
| } | |||
|
|
|||
| this._warnOnPreloadedModules(); | |||
|
|
|||
| const imdFromHookPath: Map<string, InstrumentationModuleDefinition> = | |||
There was a problem hiding this comment.
| const imdFromHookPath: Map<string, InstrumentationModuleDefinition> = | |
| const imdsFromHookPath: Map<string, InstrumentationModuleDefinition[]> = |
| const imd = imdFromHookPath.get(message.name); | ||
| if (!imd) { |
There was a problem hiding this comment.
| const imd = imdFromHookPath.get(message.name); | |
| if (!imd) { | |
| const imds = imdsFromHookPath.get(message.name); | |
| if (!imds) { |
| const patchedExports = this._onRequire<typeof exports>( | ||
| imd, | ||
| message.exports, | ||
| message.name, | ||
| undefined, | ||
| message.version // Package version was determined at bundle-time. | ||
| ); | ||
| message.exports = patchedExports; |
There was a problem hiding this comment.
| const patchedExports = this._onRequire<typeof exports>( | |
| imd, | |
| message.exports, | |
| message.name, | |
| undefined, | |
| message.version // Package version was determined at bundle-time. | |
| ); | |
| message.exports = patchedExports; | |
| for (const imd of imds) { | |
| const patchedExports = this._onRequire<typeof exports>( | |
| imd, | |
| message.exports, | |
| message.name, | |
| undefined, | |
| message.version // Package version was determined at bundle-time. | |
| ); | |
| message.exports = patchedExports; | |
| } |
| imdFromHookPath.set(module.name, module); | ||
| for (const file of module.files) { | ||
| imdFromHookPath.set(file.name, module); | ||
| } |
There was a problem hiding this comment.
| imdFromHookPath.set(module.name, module); | |
| for (const file of module.files) { | |
| imdFromHookPath.set(file.name, module); | |
| } | |
| const imdsByModuleName = imdsFromHookPath.get(module.name) ?? []; | |
| imdsFromHookPath.set(module.name, imdsByModuleName); | |
| imdsByModuleName.push(module); | |
| for (const file of module.files) { | |
| const imdsByFileName = imdsFromHookPath.get(file.name) ?? []; | |
| imdsFromHookPath.set(file.name, imdsByFileName); | |
| imdsByFileName.push(module); | |
| } |
|
@trentm will you be able to incorporate these comments? I'd like to push this along in any way possible |
|
@trentm any word here? I've been using my own fork of open-telemetry/opentelemetry-js-contrib#1856 in production for about a year now with great results. I really want to get this incorporated upstream to let others leverage it. Once this gets in I'd be happy to take a stab at plugins for other bundlers too |
(This is the core-repo part of a proposed
diagnostics_channel-based approach to supporting instrumentation in bundles. There will be a coming contrib-repo PR to add an esbuild-plugin package. More details coming.)checklist