From 01348e6fbc73646703c64f85c623406503692ebc Mon Sep 17 00:00:00 2001 From: Marc Pichler Date: Mon, 12 Feb 2024 13:09:15 +0100 Subject: [PATCH] fix(instrumentation): normalize paths for internal files in scoped packages (#4467) * fix(instrumentation): normalize paths for internal files in scoped packages * fix(instrumentation): normalize name passed to onRequire in RequireInTheMiddleSingleton * fix(instrumentation): apply normalization during filtering internal files * fix(changelog): add changelog entry * fix: normalize before filtering * fix: lint --- experimental/CHANGELOG.md | 2 + .../src/platform/node/instrumentation.ts | 3 +- .../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 ++++++++++++++++++ 6 files changed, 122 insertions(+), 1 deletion(-) 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/CHANGELOG.md b/experimental/CHANGELOG.md index a9bd8d8d1..01b1b3d68 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) diff --git a/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index a4c11498c..c639bc8bd 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 === name) + .filter(f => f.name === normalizedName) .filter(f => isSupported(f.supportedVersions, version, module.includePrerelease) ); 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 000000000..286397892 --- /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 000000000..939163a6b --- /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 000000000..9e591ef0d --- /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 000000000..b3478cfe4 --- /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'); + }); +});