From 9c7bfcd1ed9185ae1046ccb84c1086ce6d2361b2 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 30 Oct 2022 18:51:27 +0100 Subject: [PATCH 1/3] feat(instrumentation): add support for esm module --- CHANGELOG.md | 2 + .../opentelemetry-instrumentation/README.md | 4 ++ .../package.json | 5 +- .../node/RequireInTheMiddleSingleton.ts | 41 +++++++++------- .../test/node/esmInstrumentation.test.mjs | 47 +++++++++++++++++++ .../test/node/node_modules/.gitkeep | 0 .../node_modules/my-esm-module/package.json | 6 +++ .../node_modules/my-esm-module/src/index.mjs | 6 +++ 8 files changed, 93 insertions(+), 18 deletions(-) create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/esmInstrumentation.test.mjs create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/.gitkeep create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/package.json create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/src/index.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 545e002cd8..ba0acf3c1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * feat(sdk-trace): re-export sdk-trace-base in sdk-trace-node and web [#3319](https://github.com/open-telemetry/opentelemetry-js/pull/3319) @legendecas * feat: enable tree shaking [#3329](https://github.com/open-telemetry/opentelemetry-js/pull/3329) @pkanal +* feat(instrumentation): add support for esm module [#2846](https://github.com/open-telemetry/opentelemetry-js/pull/2846) @vmarchaud + ### :bug: (Bug Fix) diff --git a/experimental/packages/opentelemetry-instrumentation/README.md b/experimental/packages/opentelemetry-instrumentation/README.md index 26e9f8c696..74756af785 100644 --- a/experimental/packages/opentelemetry-instrumentation/README.md +++ b/experimental/packages/opentelemetry-instrumentation/README.md @@ -263,6 +263,10 @@ If nothing is specified the global registered provider is used. Usually this is There might be usecase where someone has the need for more providers within an application. Please note that special care must be takes in such setups to avoid leaking information from one provider to the other because there are a lot places where e.g. the global `ContextManager` or `Propagator` is used. +## ESM within Node.JS + +As the module loading mechanism for ESM is different than CJS, you need to select a custom loader so instrumentation can load hook on the esm module it want to patch, to do so you need to provide `--loader=import-in-the-middle/hook.mjs` to the `node` binary. This only works for Node.JS > 12. + ## License Apache 2.0 - See [LICENSE][license-url] for more information. diff --git a/experimental/packages/opentelemetry-instrumentation/package.json b/experimental/packages/opentelemetry-instrumentation/package.json index 29b186d510..61c998ba7a 100644 --- a/experimental/packages/opentelemetry-instrumentation/package.json +++ b/experimental/packages/opentelemetry-instrumentation/package.json @@ -47,7 +47,9 @@ "tdd": "npm run tdd:node", "tdd:node": "npm run test -- --watch-extensions ts --watch", "tdd:browser": "karma start", - "test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'", + "test:cjs": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'", + "test:esm": "nyc node --loader=import-in-the-middle/hook.mjs node_modules/.bin/_mocha test/node/*.test.mjs", + "test": "npm run test:cjs && npm run test:esm", "test:browser": "nyc karma start --single-run", "version": "node ../../../scripts/version-update.js", "watch": "tsc --build --watch tsconfig.all.json", @@ -68,6 +70,7 @@ "url": "https://github.com/open-telemetry/opentelemetry-js/issues" }, "dependencies": { + "import-in-the-middle": "^1.3.4", "require-in-the-middle": "^5.0.3", "semver": "^7.3.2", "shimmer": "^1.2.1" diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index 812db52b68..1f03fdec05 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -15,6 +15,7 @@ */ import * as RequireInTheMiddle from 'require-in-the-middle'; +import * as ImportInTheMiddle from 'import-in-the-middle'; import * as path from 'path'; import { ModuleNameTrie, ModuleNameSeparator } from './ModuleNameTrie'; @@ -23,6 +24,12 @@ export type Hooked = { onRequire: RequireInTheMiddle.OnRequireFn }; +/** + * We are forced to re-type there because ImportInTheMiddle is exported as normal CJS + * in the JS files but transpiled ESM (with a default export) in its typing. + */ + const ESMHook = ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default; + /** * Whether Mocha is running in this process * Inspired by https://github.com/AndreasPizsa/detect-mocha @@ -35,12 +42,12 @@ const isMocha = ['afterEach','after','beforeEach','before','describe','it'].ever }); /** - * Singleton class for `require-in-the-middle` + * Singleton class for `require-in-the-middle` and `import-in-the-middle` * Allows instrumentation plugins to patch modules with only a single `require` patch - * WARNING: Because this class will create its own `require-in-the-middle` (RITM) instance, + * WARNING: Because this class will create its own RITM and IITM instance, * we should minimize the number of new instances of this class. * Multiple instances of `@opentelemetry/instrumentation` (e.g. multiple versions) in a single process - * will result in multiple instances of RITM, which will have an impact + * will result in multiple instances of RITM/ITTM, which will have an impact * on the performance of instrumentation hooks being applied. */ export class RequireInTheMiddleSingleton { @@ -52,23 +59,23 @@ export class RequireInTheMiddleSingleton { } private _initialize() { - RequireInTheMiddle( - // Intercept all `require` calls; we will filter the matching ones below - null, - { internals: true }, - (exports, name, basedir) => { - // For internal files on Windows, `name` will use backslash as the path separator - const normalizedModuleName = normalizePathSeparators(name); + // + const onHook = (exports: any, name: string, basedir: string | undefined | void) => { + // For internal files on Windows, `name` will use backslash as the path separator + const normalizedModuleName = normalizePathSeparators(name); - const matches = this._moduleNameTrie.search(normalizedModuleName, { maintainInsertionOrder: true }); + const matches = this._moduleNameTrie.search(normalizedModuleName, { maintainInsertionOrder: true }); - for (const { onRequire } of matches) { - exports = onRequire(exports, name, basedir); - } - - return exports; + for (const { onRequire } of matches) { + exports = onRequire(exports, name, basedir ? basedir : undefined); } - ); + + return exports; + } + // Intercept all `require` calls; we will filter the matching ones below + RequireInTheMiddle(null, { internals: true }, onHook); + // We can give no module to patch but this signature isn't exposed in typings + new ESMHook(null as any, { internals: true }, onHook) } /** diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/esmInstrumentation.test.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/esmInstrumentation.test.mjs new file mode 100644 index 0000000000..a80341368d --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/esmInstrumentation.test.mjs @@ -0,0 +1,47 @@ +/* + * Copyright The OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert' +import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '../../build/src/index.js'; + +describe('when loading esm module', () => { + it('should patch module file', async () => { + class TestInstrumentation extends InstrumentationBase { + constructor(onPatch, onUnpatch) { + super('my-esm-instrumentation', '0.1.0'); + } + + init() { + return [ + new InstrumentationNodeModuleDefinition( + 'my-esm-module', + ['*'], + (exports, version) => { + exports.myConstant = 43; + exports.myFunction = () => 'another'; + } + ) + ]; + } + } + + const instrumentation = new TestInstrumentation(); + instrumentation.enable(); + const exported = await import('my-esm-module'); + assert.deepEqual(exported.myConstant, 43); + assert.deepEqual(exported.myFunction(), 'another'); + }); +}); diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/.gitkeep b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/.gitkeep new file mode 100644 index 0000000000..e69de29bb2 diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/package.json b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/package.json new file mode 100644 index 0000000000..70e551be04 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/package.json @@ -0,0 +1,6 @@ +{ + "name": "my-esm-module", + "version": "0.1.0", + "main": "./src/index.mjs", + "type": "module" +} diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/src/index.mjs b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/src/index.mjs new file mode 100644 index 0000000000..9e38381828 --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/my-esm-module/src/index.mjs @@ -0,0 +1,6 @@ + +export const myFunction = () => { + return 'test'; +}; + +export const myConstant = 42; From 1cf08c671c05699ed2e57bbafad34df9dad180a9 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 30 Oct 2022 18:57:01 +0100 Subject: [PATCH 2/3] chore: fix windows + lint --- CHANGELOG.md | 1 - .../packages/opentelemetry-instrumentation/package.json | 2 +- .../src/platform/node/RequireInTheMiddleSingleton.ts | 7 +++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ba0acf3c1f..54e2f21ce9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,7 +32,6 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ * feat: enable tree shaking [#3329](https://github.com/open-telemetry/opentelemetry-js/pull/3329) @pkanal * feat(instrumentation): add support for esm module [#2846](https://github.com/open-telemetry/opentelemetry-js/pull/2846) @vmarchaud - ### :bug: (Bug Fix) * fix(sdk-trace): enforce consistent span durations diff --git a/experimental/packages/opentelemetry-instrumentation/package.json b/experimental/packages/opentelemetry-instrumentation/package.json index 61c998ba7a..c2710d8dac 100644 --- a/experimental/packages/opentelemetry-instrumentation/package.json +++ b/experimental/packages/opentelemetry-instrumentation/package.json @@ -48,7 +48,7 @@ "tdd:node": "npm run test -- --watch-extensions ts --watch", "tdd:browser": "karma start", "test:cjs": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'", - "test:esm": "nyc node --loader=import-in-the-middle/hook.mjs node_modules/.bin/_mocha test/node/*.test.mjs", + "test:esm": "nyc node --experimental-loader=import-in-the-middle/hook.mjs ../../../node_modules/mocha/bin/mocha 'test/node/*.test.mjs' test/node/*.test.mjs", "test": "npm run test:cjs && npm run test:esm", "test:browser": "nyc karma start --single-run", "version": "node ../../../scripts/version-update.js", diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index 1f03fdec05..303b3b312f 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -28,7 +28,7 @@ export type Hooked = { * We are forced to re-type there because ImportInTheMiddle is exported as normal CJS * in the JS files but transpiled ESM (with a default export) in its typing. */ - const ESMHook = ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default; +const ESMHook = ImportInTheMiddle as unknown as typeof ImportInTheMiddle.default; /** * Whether Mocha is running in this process @@ -59,7 +59,6 @@ export class RequireInTheMiddleSingleton { } private _initialize() { - // const onHook = (exports: any, name: string, basedir: string | undefined | void) => { // For internal files on Windows, `name` will use backslash as the path separator const normalizedModuleName = normalizePathSeparators(name); @@ -71,11 +70,11 @@ export class RequireInTheMiddleSingleton { } return exports; - } + }; // Intercept all `require` calls; we will filter the matching ones below RequireInTheMiddle(null, { internals: true }, onHook); // We can give no module to patch but this signature isn't exposed in typings - new ESMHook(null as any, { internals: true }, onHook) + new ESMHook(null as any, { internals: true }, onHook); } /** From f6384aaa8c6f4f21f4de92eda897c60bc3a86077 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 30 Oct 2022 21:04:07 +0100 Subject: [PATCH 3/3] chore: documentation --- .../packages/opentelemetry-instrumentation/README.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/README.md b/experimental/packages/opentelemetry-instrumentation/README.md index 74756af785..f31674bdd0 100644 --- a/experimental/packages/opentelemetry-instrumentation/README.md +++ b/experimental/packages/opentelemetry-instrumentation/README.md @@ -263,9 +263,10 @@ If nothing is specified the global registered provider is used. Usually this is There might be usecase where someone has the need for more providers within an application. Please note that special care must be takes in such setups to avoid leaking information from one provider to the other because there are a lot places where e.g. the global `ContextManager` or `Propagator` is used. -## ESM within Node.JS +## Instrumentation for ESM Module In NodeJS (experimental) -As the module loading mechanism for ESM is different than CJS, you need to select a custom loader so instrumentation can load hook on the esm module it want to patch, to do so you need to provide `--loader=import-in-the-middle/hook.mjs` to the `node` binary. This only works for Node.JS > 12. +As the module loading mechanism for ESM is different than CJS, you need to select a custom loader so instrumentation can load hook on the esm module it want to patch. To do so, you must provide the `--experimental-loader=import-in-the-middle/hook.mjs` flag to the `node` binary, alternatively you can set the `NODE_OPTIONS` environment variable to `--experimental-loader=import-in-the-middle/hook.mjs`. +As the ESM module loader from NodeJS is experimental, so is our support for it. Feel free to provide feedback or report issues about it. ## License