From 18057be088c00bb56ef198cf9215729f53387928 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Wed, 1 Sep 2021 21:38:51 +0300 Subject: [PATCH 1/5] feat: handle missing version case --- .../src/platform/node/instrumentation.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index d03afadb93..54dc60582e 100644 --- a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -17,6 +17,7 @@ import * as types from '../../types'; import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; +import { existsSync } from 'fs'; import { satisfies } from 'semver'; import { InstrumentationAbstract } from '../../instrumentation'; import { InstrumentationModuleDefinition } from './types'; @@ -73,8 +74,9 @@ export abstract class InstrumentationBase return exports; } + const packageJsonPath = path.join(baseDir, 'package.json'); // eslint-disable-next-line @typescript-eslint/no-var-requires - const version = require(path.join(baseDir, 'package.json')).version; + const version = existsSync(packageJsonPath) ? require(packageJsonPath).version : undefined; module.moduleVersion = version; if (module.name === name) { // main module @@ -167,8 +169,13 @@ export abstract class InstrumentationBase } } -function isSupported(supportedVersions: string[], version: string, includePrerelease?: boolean): boolean { +function isSupported(supportedVersions: string[], version?: string, includePrerelease?: boolean): boolean { return supportedVersions.some(supportedVersion => { + if (typeof version === 'undefined') { + // If we don't have the version, accept the wildcard case only + return supportedVersion === '*'; + } + return satisfies(version, supportedVersion, { includePrerelease }); }); } From d8052216c6a5c236ddcdb00bebea03df16907ae8 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Fri, 3 Sep 2021 09:37:10 +0300 Subject: [PATCH 2/5] feat: add test cases --- .../src/platform/node/instrumentation.ts | 11 +- .../test/node/InstrumentationBase.test.ts | 155 ++++++++++++++++++ 2 files changed, 160 insertions(+), 6 deletions(-) create mode 100644 packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts diff --git a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 54dc60582e..3fe00d6809 100644 --- a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -81,7 +81,6 @@ export abstract class InstrumentationBase if (module.name === name) { // main module if ( - typeof version === 'string' && isSupported(module.supportedVersions, version, module.includePrerelease) ) { if (typeof module.patch === 'function') { @@ -170,12 +169,12 @@ export abstract class InstrumentationBase } function isSupported(supportedVersions: string[], version?: string, includePrerelease?: boolean): boolean { - return supportedVersions.some(supportedVersion => { - if (typeof version === 'undefined') { - // If we don't have the version, accept the wildcard case only - return supportedVersion === '*'; - } + if (typeof version === 'undefined') { + // If we don't have the version, accept the wildcard case only + return supportedVersions.includes('*'); + } + return supportedVersions.some(supportedVersion => { return satisfies(version, supportedVersion, { includePrerelease }); }); } diff --git a/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts new file mode 100644 index 0000000000..5dd427455c --- /dev/null +++ b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -0,0 +1,155 @@ +/* + * 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 * as sinon from 'sinon'; +import { InstrumentationBase, InstrumentationModuleDefinition } from '../../src'; + +const MODULE_NAME = 'test-module'; +const MODULE_FILE_NAME = 'test-module-file'; +const MODULE_VERSION = '0.1.0'; +const WILDCARD_VERSION = '*'; +const MODULE_DIR = '/random/dir'; + +class TestInstrumentation extends InstrumentationBase { + constructor() { + super(MODULE_NAME, MODULE_VERSION); + } + + init() {} +} + +describe('InstrumentationBase', () => { + describe('_onRequire - module version is not available', () => { + let instrumentation: TestInstrumentation; + let modulePatchSpy: sinon.SinonSpy; + + beforeEach(() => { + instrumentation = new TestInstrumentation(); + // @ts-expect-error access internal property for testing + instrumentation._enabled = true; + modulePatchSpy = sinon.spy(); + }); + + describe('when patching a module', () => { + it('should not patch module when there is no wildcard supported version', () => { + const moduleExports = {}; + const instrumentationModule = { + supportedVersions: [`^${MODULE_VERSION}`], + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, undefined); + sinon.assert.notCalled(modulePatchSpy); + }); + + it('should patch module when there is a wildcard supported version', () => { + const moduleExports = {}; + const instrumentationModule = { + supportedVersions: [`^${MODULE_VERSION}`, WILDCARD_VERSION], + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, moduleExports); + sinon.assert.calledOnceWithExactly(modulePatchSpy, moduleExports, undefined); + }); + }); + + describe('when patching module files', () => { + let filePatchSpy: sinon.SinonSpy; + + beforeEach(() => { + filePatchSpy = sinon.spy(); + }) + + it('should not patch module file when there is no wildcard supported version', () => { + const moduleExports = {}; + const supportedVersions = [`^${MODULE_VERSION}`]; + const instrumentationModule = { + supportedVersions, + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + files: [{ + name: MODULE_FILE_NAME, + supportedVersions, + patch: filePatchSpy as unknown + }] + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_FILE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, undefined); + sinon.assert.notCalled(modulePatchSpy); + sinon.assert.notCalled(filePatchSpy); + }); + + it('should patch module file when there is a wildcard supported version', () => { + const moduleExports = {}; + const supportedVersions = [`^${MODULE_VERSION}`, WILDCARD_VERSION]; + const instrumentationModule = { + supportedVersions, + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + files: [{ + name: MODULE_FILE_NAME, + supportedVersions, + patch: filePatchSpy as unknown + }] + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_FILE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.files[0].moduleExports, moduleExports); + sinon.assert.notCalled(modulePatchSpy); + sinon.assert.calledOnceWithExactly(filePatchSpy, moduleExports, undefined); + }); + }); + }); +}); From b9dfc184311930519cea28bb705bf213afcbab5d Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Fri, 3 Sep 2021 11:34:42 +0300 Subject: [PATCH 3/5] fix: cr comment --- .../src/platform/node/instrumentation.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts index 3fe00d6809..f631f001ee 100644 --- a/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts +++ b/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts @@ -17,7 +17,6 @@ import * as types from '../../types'; import * as path from 'path'; import * as RequireInTheMiddle from 'require-in-the-middle'; -import { existsSync } from 'fs'; import { satisfies } from 'semver'; import { InstrumentationAbstract } from '../../instrumentation'; import { InstrumentationModuleDefinition } from './types'; @@ -60,6 +59,18 @@ export abstract class InstrumentationBase } } + private _extractPackageVersion(baseDir: string): string | undefined { + try { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const version = require(path.join(baseDir, 'package.json')).version; + return typeof version === 'string' ? version : undefined; + } catch (error) { + diag.warn('Failed extracting version', baseDir); + } + + return undefined; + } + private _onRequire( module: InstrumentationModuleDefinition, exports: T, @@ -74,9 +85,7 @@ export abstract class InstrumentationBase return exports; } - const packageJsonPath = path.join(baseDir, 'package.json'); - // eslint-disable-next-line @typescript-eslint/no-var-requires - const version = existsSync(packageJsonPath) ? require(packageJsonPath).version : undefined; + const version = this._extractPackageVersion(baseDir); module.moduleVersion = version; if (module.name === name) { // main module From 12c58fca349fcadaff4f8e71035d4e08fddc79f7 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Mon, 6 Sep 2021 12:39:30 +0300 Subject: [PATCH 4/5] fix: add comment to test --- .../test/node/InstrumentationBase.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts index 5dd427455c..2d4fbf056c 100644 --- a/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts +++ b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -34,6 +34,9 @@ class TestInstrumentation extends InstrumentationBase { describe('InstrumentationBase', () => { describe('_onRequire - module version is not available', () => { + // For all of these cases, there is no indication of the actual module version, + // so we require there to be a wildcard supported version. + let instrumentation: TestInstrumentation; let modulePatchSpy: sinon.SinonSpy; From 8bc0f667d58a3d2cb253adcde6cf78ba870c0725 Mon Sep 17 00:00:00 2001 From: Ran Nozik Date: Wed, 8 Sep 2021 11:48:42 +0300 Subject: [PATCH 5/5] fix: tests description --- .../test/node/InstrumentationBase.test.ts | 186 +++++++++--------- 1 file changed, 97 insertions(+), 89 deletions(-) diff --git a/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts index 2d4fbf056c..e1c42681d3 100644 --- a/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts +++ b/packages/opentelemetry-instrumentation/test/node/InstrumentationBase.test.ts @@ -48,49 +48,53 @@ describe('InstrumentationBase', () => { }); describe('when patching a module', () => { - it('should not patch module when there is no wildcard supported version', () => { - const moduleExports = {}; - const instrumentationModule = { - supportedVersions: [`^${MODULE_VERSION}`], - name: MODULE_NAME, - patch: modulePatchSpy as unknown, - } as InstrumentationModuleDefinition; - - // @ts-expect-error access internal property for testing - instrumentation._onRequire( - instrumentationModule, - moduleExports, - MODULE_NAME, - MODULE_DIR - ); - - assert.strictEqual(instrumentationModule.moduleVersion, undefined); - assert.strictEqual(instrumentationModule.moduleExports, undefined); - sinon.assert.notCalled(modulePatchSpy); + describe('AND there is no wildcard supported version', () => { + it('should not patch module', () => { + const moduleExports = {}; + const instrumentationModule = { + supportedVersions: [`^${MODULE_VERSION}`], + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, undefined); + sinon.assert.notCalled(modulePatchSpy); + }); }); - it('should patch module when there is a wildcard supported version', () => { - const moduleExports = {}; - const instrumentationModule = { - supportedVersions: [`^${MODULE_VERSION}`, WILDCARD_VERSION], - name: MODULE_NAME, - patch: modulePatchSpy as unknown, - } as InstrumentationModuleDefinition; - - // @ts-expect-error access internal property for testing - instrumentation._onRequire( - instrumentationModule, - moduleExports, - MODULE_NAME, - MODULE_DIR - ); - - assert.strictEqual(instrumentationModule.moduleVersion, undefined); - assert.strictEqual(instrumentationModule.moduleExports, moduleExports); - sinon.assert.calledOnceWithExactly(modulePatchSpy, moduleExports, undefined); + describe('AND there is a wildcard supported version', () => { + it('should patch module', () => { + const moduleExports = {}; + const instrumentationModule = { + supportedVersions: [`^${MODULE_VERSION}`, WILDCARD_VERSION], + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, moduleExports); + sinon.assert.calledOnceWithExactly(modulePatchSpy, moduleExports, undefined); + }); }); }); - + describe('when patching module files', () => { let filePatchSpy: sinon.SinonSpy; @@ -98,60 +102,64 @@ describe('InstrumentationBase', () => { filePatchSpy = sinon.spy(); }) - it('should not patch module file when there is no wildcard supported version', () => { - const moduleExports = {}; - const supportedVersions = [`^${MODULE_VERSION}`]; - const instrumentationModule = { - supportedVersions, - name: MODULE_NAME, - patch: modulePatchSpy as unknown, - files: [{ - name: MODULE_FILE_NAME, + describe('AND there is no wildcard supported version', () => { + it('should not patch module file', () => { + const moduleExports = {}; + const supportedVersions = [`^${MODULE_VERSION}`]; + const instrumentationModule = { supportedVersions, - patch: filePatchSpy as unknown - }] - } as InstrumentationModuleDefinition; - - // @ts-expect-error access internal property for testing - instrumentation._onRequire( - instrumentationModule, - moduleExports, - MODULE_FILE_NAME, - MODULE_DIR - ); - - assert.strictEqual(instrumentationModule.moduleVersion, undefined); - assert.strictEqual(instrumentationModule.moduleExports, undefined); - sinon.assert.notCalled(modulePatchSpy); - sinon.assert.notCalled(filePatchSpy); + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + files: [{ + name: MODULE_FILE_NAME, + supportedVersions, + patch: filePatchSpy as unknown + }] + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_FILE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.moduleExports, undefined); + sinon.assert.notCalled(modulePatchSpy); + sinon.assert.notCalled(filePatchSpy); + }); }); - it('should patch module file when there is a wildcard supported version', () => { - const moduleExports = {}; - const supportedVersions = [`^${MODULE_VERSION}`, WILDCARD_VERSION]; - const instrumentationModule = { - supportedVersions, - name: MODULE_NAME, - patch: modulePatchSpy as unknown, - files: [{ - name: MODULE_FILE_NAME, + describe('AND there is a wildcard supported version', () => { + it('should patch module file', () => { + const moduleExports = {}; + const supportedVersions = [`^${MODULE_VERSION}`, WILDCARD_VERSION]; + const instrumentationModule = { supportedVersions, - patch: filePatchSpy as unknown - }] - } as InstrumentationModuleDefinition; - - // @ts-expect-error access internal property for testing - instrumentation._onRequire( - instrumentationModule, - moduleExports, - MODULE_FILE_NAME, - MODULE_DIR - ); - - assert.strictEqual(instrumentationModule.moduleVersion, undefined); - assert.strictEqual(instrumentationModule.files[0].moduleExports, moduleExports); - sinon.assert.notCalled(modulePatchSpy); - sinon.assert.calledOnceWithExactly(filePatchSpy, moduleExports, undefined); + name: MODULE_NAME, + patch: modulePatchSpy as unknown, + files: [{ + name: MODULE_FILE_NAME, + supportedVersions, + patch: filePatchSpy as unknown + }] + } as InstrumentationModuleDefinition; + + // @ts-expect-error access internal property for testing + instrumentation._onRequire( + instrumentationModule, + moduleExports, + MODULE_FILE_NAME, + MODULE_DIR + ); + + assert.strictEqual(instrumentationModule.moduleVersion, undefined); + assert.strictEqual(instrumentationModule.files[0].moduleExports, moduleExports); + sinon.assert.notCalled(modulePatchSpy); + sinon.assert.calledOnceWithExactly(filePatchSpy, moduleExports, undefined); + }); }); }); });