From b4462d6ac0f38565314e9f57bb2d531edfd3edcf Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Thu, 20 Jun 2024 16:43:07 -0700 Subject: [PATCH 1/4] refactor: 'module' is a terribly overloaded var name for the InstrumentationModuleDefinition --- .../src/platform/node/instrumentation.ts | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 0cd181a1ca5..4864b13739f 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -169,6 +169,7 @@ export abstract class InstrumentationBase< } private _extractPackageVersion(baseDir: string): string | undefined { + console.log('XXX _extractPackageVersion(baseDir=%s)', baseDir); // curious if we are doing this unnecessarily try { const json = readFileSync(path.join(baseDir, 'package.json'), { encoding: 'utf8', @@ -183,58 +184,58 @@ export abstract class InstrumentationBase< } private _onRequire( - module: InstrumentationModuleDefinition, + imd: InstrumentationModuleDefinition, exports: T, name: string, baseDir?: string | void ): T { if (!baseDir) { - if (typeof module.patch === 'function') { - module.moduleExports = exports; + if (typeof imd.patch === 'function') { + imd.moduleExports = exports; if (this._enabled) { this._diag.debug( 'Applying instrumentation patch for nodejs core module on require hook', { - module: module.name, + module: imd.name, } ); - return module.patch(exports); + return imd.patch(exports); } } return exports; } const version = this._extractPackageVersion(baseDir); - module.moduleVersion = version; - if (module.name === name) { + imd.moduleVersion = version; + if (imd.name === name) { // main module if ( - isSupported(module.supportedVersions, version, module.includePrerelease) + isSupported(imd.supportedVersions, version, imd.includePrerelease) ) { - if (typeof module.patch === 'function') { - module.moduleExports = exports; + if (typeof imd.patch === 'function') { + imd.moduleExports = exports; if (this._enabled) { this._diag.debug( 'Applying instrumentation patch for module on require hook', { - module: module.name, - version: module.moduleVersion, + module: imd.name, + version: imd.moduleVersion, baseDir, } ); - return module.patch(exports, module.moduleVersion); + return imd.patch(exports, imd.moduleVersion); } } } return exports; } // internal file - const files = module.files ?? []; + const files = imd.files ?? []; const normalizedName = path.normalize(name); const supportedFileInstrumentations = files .filter(f => f.name === normalizedName) .filter(f => - isSupported(f.supportedVersions, version, module.includePrerelease) + isSupported(f.supportedVersions, version, imd.includePrerelease) ); return supportedFileInstrumentations.reduce((patchedExports, file) => { file.moduleExports = patchedExports; @@ -242,15 +243,15 @@ export abstract class InstrumentationBase< this._diag.debug( 'Applying instrumentation patch for nodejs module file on require hook', { - module: module.name, - version: module.moduleVersion, + module: imd.name, + version: imd.moduleVersion, fileName: file.name, baseDir, } ); // patch signature is not typed, so we cast it assuming it's correct - return file.patch(patchedExports, module.moduleVersion) as T; + return file.patch(patchedExports, imd.moduleVersion) as T; } return patchedExports; }, exports); From e1533091560128f2f5dad95d74bf555a77ec2564 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 21 Jun 2024 16:11:39 -0700 Subject: [PATCH 2/4] feat: support 'otel:bundle:load' diagnostics_channel message for instrumentation in bundles --- .../src/platform/node/instrumentation.ts | 48 +++++++++++++++++-- .../src/types_internal.ts | 18 +++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 4864b13739f..dca8f9e66ab 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -16,6 +16,7 @@ import * as types from '../../types'; import * as path from 'path'; +import * as diagch from 'diagnostics_channel'; // XXX added v14.17. Do we need to conditionally import? import { types as utilTypes } from 'util'; import { satisfies } from 'semver'; import { wrap, unwrap, massWrap, massUnwrap } from 'shimmer'; @@ -30,6 +31,7 @@ import { InstrumentationConfig, InstrumentationModuleDefinition, } from '../../types'; +import { DiagChSubscribe, OTelBundleLoadMessage } from '../../types_internal'; import { diag } from '@opentelemetry/api'; import type { OnRequireFn } from 'require-in-the-middle'; import { Hook as HookRequire } from 'require-in-the-middle'; @@ -169,7 +171,6 @@ export abstract class InstrumentationBase< } private _extractPackageVersion(baseDir: string): string | undefined { - console.log('XXX _extractPackageVersion(baseDir=%s)', baseDir); // curious if we are doing this unnecessarily try { const json = readFileSync(path.join(baseDir, 'package.json'), { encoding: 'utf8', @@ -187,9 +188,10 @@ export abstract class InstrumentationBase< imd: InstrumentationModuleDefinition, exports: T, name: string, - baseDir?: string | void + baseDir?: string | void, + version?: string ): T { - if (!baseDir) { + if (!version && !baseDir) { if (typeof imd.patch === 'function') { imd.moduleExports = exports; if (this._enabled) { @@ -205,7 +207,9 @@ export abstract class InstrumentationBase< return exports; } - const version = this._extractPackageVersion(baseDir); + if (!version && baseDir) { + version = this._extractPackageVersion(baseDir); + } imd.moduleVersion = version; if (imd.name === name) { // main module @@ -294,6 +298,8 @@ export abstract class InstrumentationBase< } this._warnOnPreloadedModules(); + + const imdFromHookPath: Map = new Map(); for (const module of this._modules) { const hookFn: HookFn = (exports, name, baseDir) => { return this._onRequire(module, exports, name, baseDir); @@ -316,6 +322,40 @@ export abstract class InstrumentationBase< hookFn ); this._hooks.push(esmHook); + + imdFromHookPath.set(module.name, module); + for (let file of module.files) { + imdFromHookPath.set(file.name, module); + } + } + + // `diagch.subscribe` was added in Node.js v18.7.0, v16.17.0. + const subscribe: DiagChSubscribe = (diagch as any).subscribe; + if (typeof subscribe === 'function') { + // A bundler plugin, e.g. `@opentelemetry/esbuild-plugin`, can pass + // a loaded module to this instrumentation via the well-known + // `otel:bundle:load` diagnostics channel message. The message includes + // the module exports, that can be patched in-place. + subscribe("otel:bundle:load", (message_, name) => { + const message = message_ as OTelBundleLoadMessage; // XXX TS advice + if (typeof message.name !== 'string' || typeof message.version !== 'string') { + this._diag.debug('skipping invalid "otel:bundle:load" diagch message'); + return + } + const imd = imdFromHookPath.get(message.name); + if (!imd) { + // This loaded module is not relevant for this instrumentation. + return; + } + const patchedExports = this._onRequire( + imd, + message.exports, + message.name, + undefined, + message.version // Package version was determined at bundle-time. + ); + message.exports = patchedExports; + }); } } diff --git a/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts b/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts index 6d678fea907..5b4a20fd4f4 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts @@ -28,3 +28,21 @@ export interface AutoLoaderOptions { meterProvider?: MeterProvider; loggerProvider?: LoggerProvider; } + +/** + * A subset of types for Node.js `diagnostics_channel`. + * `diagnostics_channel.subscribe` was added in Node.js v18.7.0, v16.17.0. + * The current `@types/node` dependency is for an earlier version (v14) of + * Node.js + */ +type DiagChChannelListener = (message: unknown, name: string | symbol) => void; +export type DiagChSubscribe = (name: string | symbol, onMessage: DiagChChannelListener) => void; + +/** + * The shape of a `otel:bundle:load` diagnostics_channel message. + */ +export type OTelBundleLoadMessage = { + name: string; + version: string; + exports: any; +}; From aa624ee4a1f2163e846526c25e69995cadeb067b Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 21 Jun 2024 16:16:04 -0700 Subject: [PATCH 3/4] undo the s/module/imd/ refactor from an earlier commit to keep this PR smaller --- .../src/platform/node/instrumentation.ts | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index dca8f9e66ab..d484bdcec12 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -185,23 +185,23 @@ export abstract class InstrumentationBase< } private _onRequire( - imd: InstrumentationModuleDefinition, + module: InstrumentationModuleDefinition, exports: T, name: string, baseDir?: string | void, version?: string ): T { if (!version && !baseDir) { - if (typeof imd.patch === 'function') { - imd.moduleExports = exports; + if (typeof module.patch === 'function') { + module.moduleExports = exports; if (this._enabled) { this._diag.debug( 'Applying instrumentation patch for nodejs core module on require hook', { - module: imd.name, + module: module.name, } ); - return imd.patch(exports); + return module.patch(exports); } } return exports; @@ -210,36 +210,36 @@ export abstract class InstrumentationBase< if (!version && baseDir) { version = this._extractPackageVersion(baseDir); } - imd.moduleVersion = version; - if (imd.name === name) { + module.moduleVersion = version; + if (module.name === name) { // main module if ( - isSupported(imd.supportedVersions, version, imd.includePrerelease) + isSupported(module.supportedVersions, version, module.includePrerelease) ) { - if (typeof imd.patch === 'function') { - imd.moduleExports = exports; + if (typeof module.patch === 'function') { + module.moduleExports = exports; if (this._enabled) { this._diag.debug( 'Applying instrumentation patch for module on require hook', { - module: imd.name, - version: imd.moduleVersion, + module: module.name, + version: module.moduleVersion, baseDir, } ); - return imd.patch(exports, imd.moduleVersion); + return module.patch(exports, module.moduleVersion); } } } return exports; } // internal file - const files = imd.files ?? []; + const files = module.files ?? []; const normalizedName = path.normalize(name); const supportedFileInstrumentations = files .filter(f => f.name === normalizedName) .filter(f => - isSupported(f.supportedVersions, version, imd.includePrerelease) + isSupported(f.supportedVersions, version, module.includePrerelease) ); return supportedFileInstrumentations.reduce((patchedExports, file) => { file.moduleExports = patchedExports; @@ -247,15 +247,15 @@ export abstract class InstrumentationBase< this._diag.debug( 'Applying instrumentation patch for nodejs module file on require hook', { - module: imd.name, - version: imd.moduleVersion, + module: module.name, + version: module.moduleVersion, fileName: file.name, baseDir, } ); // patch signature is not typed, so we cast it assuming it's correct - return file.patch(patchedExports, imd.moduleVersion) as T; + return file.patch(patchedExports, module.moduleVersion) as T; } return patchedExports; }, exports); From c3d4ba29bd22c55aacf261e7d0d1339981000446 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 21 Jun 2024 16:16:45 -0700 Subject: [PATCH 4/4] lint:fix --- .../src/platform/node/instrumentation.ts | 18 ++++++++++++------ .../src/types_internal.ts | 5 ++++- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index d484bdcec12..2d692367b41 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -299,7 +299,8 @@ export abstract class InstrumentationBase< this._warnOnPreloadedModules(); - const imdFromHookPath: Map = new Map(); + const imdFromHookPath: Map = + new Map(); for (const module of this._modules) { const hookFn: HookFn = (exports, name, baseDir) => { return this._onRequire(module, exports, name, baseDir); @@ -324,7 +325,7 @@ export abstract class InstrumentationBase< this._hooks.push(esmHook); imdFromHookPath.set(module.name, module); - for (let file of module.files) { + for (const file of module.files) { imdFromHookPath.set(file.name, module); } } @@ -336,11 +337,16 @@ export abstract class InstrumentationBase< // a loaded module to this instrumentation via the well-known // `otel:bundle:load` diagnostics channel message. The message includes // the module exports, that can be patched in-place. - subscribe("otel:bundle:load", (message_, name) => { + subscribe('otel:bundle:load', message_ => { const message = message_ as OTelBundleLoadMessage; // XXX TS advice - if (typeof message.name !== 'string' || typeof message.version !== 'string') { - this._diag.debug('skipping invalid "otel:bundle:load" diagch message'); - return + if ( + typeof message.name !== 'string' || + typeof message.version !== 'string' + ) { + this._diag.debug( + 'skipping invalid "otel:bundle:load" diagch message' + ); + return; } const imd = imdFromHookPath.get(message.name); if (!imd) { diff --git a/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts b/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts index 5b4a20fd4f4..5b176b3948e 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/types_internal.ts @@ -36,7 +36,10 @@ export interface AutoLoaderOptions { * Node.js */ type DiagChChannelListener = (message: unknown, name: string | symbol) => void; -export type DiagChSubscribe = (name: string | symbol, onMessage: DiagChChannelListener) => void; +export type DiagChSubscribe = ( + name: string | symbol, + onMessage: DiagChChannelListener +) => void; /** * The shape of a `otel:bundle:load` diagnostics_channel message.