From 1f77a84169decc6d1777d9bf72f8b94dd9393171 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 14:27:47 +0100 Subject: [PATCH 1/6] fix(instrumentation): normalize paths for internal files in scoped packages --- .../scoped-test-module/package.json | 5 + .../scoped-test-module/src/index.js | 7 ++ .../scoped-test-module/src/internal.js | 1 + .../scoped-package-instrumentation.test.ts | 105 ++++++++++++++++++ 4 files changed, 118 insertions(+) create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/package.json create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/index.js create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/internal.js create mode 100644 experimental/packages/opentelemetry-instrumentation/test/node/scoped-package-instrumentation.test.ts diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/package.json b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/package.json new file mode 100644 index 00000000000..2863978928d --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/package.json @@ -0,0 +1,5 @@ +{ + "name": "@opentelemetry/scoped-test-module", + "version": "0.1.0", + "main": "./src/index.js" +} diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/index.js b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/index.js new file mode 100644 index 00000000000..939163a6b2d --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/index.js @@ -0,0 +1,7 @@ +const { testString } = require('./internal'); + +exports.getString = function () { + return "from index.js: " + testString; +} + +exports.propertyOnMainModule = 'string in main module'; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/internal.js b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/internal.js new file mode 100644 index 00000000000..9e591ef0dde --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/node_modules/@opentelemetry/scoped-test-module/src/internal.js @@ -0,0 +1 @@ +exports.testString = "internal string"; diff --git a/experimental/packages/opentelemetry-instrumentation/test/node/scoped-package-instrumentation.test.ts b/experimental/packages/opentelemetry-instrumentation/test/node/scoped-package-instrumentation.test.ts new file mode 100644 index 00000000000..b3478cfe46b --- /dev/null +++ b/experimental/packages/opentelemetry-instrumentation/test/node/scoped-package-instrumentation.test.ts @@ -0,0 +1,105 @@ +/* + * 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, + InstrumentationConfig, + InstrumentationNodeModuleDefinition, + InstrumentationNodeModuleFile, +} from '../../src'; + +class TestInstrumentationSimple extends InstrumentationBase { + constructor(config: InstrumentationConfig) { + super('test-scoped-package-instrumentation', '0.0.1', config); + } + init() { + return new InstrumentationNodeModuleDefinition( + '@opentelemetry/scoped-test-module', + ['*'], + moduleExports => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + moduleExports.propertyOnMainModule = 'modified string in main module'; + return moduleExports; + }, + moduleExports => { + return moduleExports; + }, + [ + new InstrumentationNodeModuleFile( + '@opentelemetry/scoped-test-module/src/internal.js', + ['*'], + moduleExports => { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore no types + moduleExports.testString = 'modified internal string'; + return moduleExports; + }, + moduleExports => { + return moduleExports; + } + ), + ] + ); + } +} + +describe('instrumenting a scoped package', function () { + /** + * On Windows, instrumentation would fail on internal files of scoped packages. + * The reason: the path would include a '/' separator in the package name: + * - actual: @opentelemetry/scoped-test-module\src\internal.js + * - expected: @opentelemetry\scoped-test-module\src\internal.js + * This resulted in internal files of scoped packages not being instrumented. + * + * See https://github.com/open-telemetry/opentelemetry-js/issues/4436 + */ + it('should patch internal file and main module', function () { + const instrumentation = new TestInstrumentationSimple({ + enabled: false, + }); + instrumentation.enable(); + + const { getString } = require('@opentelemetry/scoped-test-module'); + + assert.strictEqual(getString(), 'from index.js: modified internal string'); + }); + + /** + * Normalizing everything passed to onRequire() from RequireInTheMiddleSingleton would cause main modules from a + * scoped package not to be instrumented. + * The reason: we'd check: + * '@opentelemetry\scoped-test-module' === '@opentelemetry/scoped-test-module' + * + * then determine that since this evaluates to false, this is not the main module, and we'd never instrument it. + * + * Therefore, the fix to the above test (internal files) is not to normalize everything passed to onRequire() + */ + it('should patch main module', function () { + const instrumentation = new TestInstrumentationSimple({ + enabled: false, + }); + instrumentation.enable(); + + const { + propertyOnMainModule, + } = require('@opentelemetry/scoped-test-module'); + + assert.strictEqual(propertyOnMainModule, 'modified string in main module'); + }); +}); From d367f06f4301bb4cac446a923c57486d3f86a4ba Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 14:37:36 +0100 Subject: [PATCH 2/6] fix(instrumentation): normalize name passed to onRequire in RequireInTheMiddleSingleton --- .../src/platform/node/RequireInTheMiddleSingleton.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index ca74699e90e..70680fd061f 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -77,7 +77,7 @@ export class RequireInTheMiddleSingleton { }); for (const { onRequire } of matches) { - exports = onRequire(exports, name, basedir); + exports = onRequire(exports, path.normalize(name), basedir); } return exports; From 0877d3857fd23209a6a719e29536143d01ca3422 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 14:53:34 +0100 Subject: [PATCH 3/6] fix(instrumentation): apply normalization during filtering internal files --- .../src/platform/node/RequireInTheMiddleSingleton.ts | 2 +- .../src/platform/node/instrumentation.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts index 70680fd061f..ca74699e90e 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts @@ -77,7 +77,7 @@ export class RequireInTheMiddleSingleton { }); for (const { onRequire } of matches) { - exports = onRequire(exports, path.normalize(name), basedir); + exports = onRequire(exports, name, basedir); } return exports; diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index a4c11498c9c..ef48a926fbb 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -205,7 +205,7 @@ export abstract class InstrumentationBase // internal file const files = module.files ?? []; const supportedFileInstrumentations = files - .filter(f => f.name === name) + .filter(f => f.name === path.normalize(name)) .filter(f => isSupported(f.supportedVersions, version, module.includePrerelease) ); From bc4f973ba8296c025278538ce9b464e7310b4813 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 16:10:21 +0100 Subject: [PATCH 4/6] fix(changelog): add changelog entry --- experimental/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index a9bd8d8d166..01b1b3d6881 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -16,6 +16,8 @@ All notable changes to experimental packages in this project will be documented ### :bug: (Bug Fix) * fix(sdk-node): allow using samplers when the exporter is defined in the environment [#4394](https://github.com/open-telemetry/opentelemetry-js/pull/4394) @JacksonWeber +* fix(instrumentation): normalize paths for internal files in scoped packages [#4467](https://github.com/open-telemetry/opentelemetry-js/pull/4467) @pichlermarc + * Fixes a bug where, on Windows, internal files on scoped packages would not be instrumented. ### :books: (Refine Doc) From d32212393374faaf38adf2f23d7d450a75c6c9a8 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 16:41:40 +0100 Subject: [PATCH 5/6] fix: normalize before filtering --- .../src/platform/node/instrumentation.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index ef48a926fbb..07c69410e12 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -204,8 +204,9 @@ export abstract class InstrumentationBase } // internal file const files = module.files ?? []; + const normalizedName = path.normalize(name) const supportedFileInstrumentations = files - .filter(f => f.name === path.normalize(name)) + .filter(f => f.name === normalizedName) .filter(f => isSupported(f.supportedVersions, version, module.includePrerelease) ); From e42f184123d6d44a3dc09024f0d4760f702aaa86 Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Thu, 8 Feb 2024 16:46:35 +0100 Subject: [PATCH 6/6] fix: lint --- .../src/platform/node/instrumentation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 07c69410e12..c639bc8bd48 100644 --- a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -204,7 +204,7 @@ export abstract class InstrumentationBase } // internal file const files = module.files ?? []; - const normalizedName = path.normalize(name) + const normalizedName = path.normalize(name); const supportedFileInstrumentations = files .filter(f => f.name === normalizedName) .filter(f =>