Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(instrumentation-node): recursively discover package.json #2206

Closed
wants to merge 2 commits into from
Closed

fix(instrumentation-node): recursively discover package.json #2206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1137b9c
fix(instrumentation-node): recursively load package.json from module …
adambartholomew May 13, 2021
159eb90
fix(instrumentation-node): refactor methods to private with ts-expect…
adambartholomew May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 26 additions & 2 deletions packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ export abstract class InstrumentationBase<T = any>
}
}

private _onRequire<T>(
protected _onRequire<T>(
module: InstrumentationModuleDefinition<T>,
exports: T,
name: string,
Expand All @@ -73,7 +73,7 @@ export abstract class InstrumentationBase<T = any>
return exports;
}

const version = require(path.join(baseDir, 'package.json')).version;
const version = this.getModuleVersion(baseDir)
module.moduleVersion = version;
if (module.name === name) {
// main module
Expand Down Expand Up @@ -102,6 +102,30 @@ export abstract class InstrumentationBase<T = any>
return exports;
}

protected getModuleVersion(directory: string): string {
const modulePackage = this.findModulePackage(directory);
if (!modulePackage) {
return '0.0.0';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we should return a invalid version if the version isn't found, i believe module.moduleVersion can already be undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version is used further down with isSupported and ultimately semver.satisfies(). I'm not familiar enough to determine the behaviour if we were to let undefined into these methods.

}
return modulePackage.version;
}

protected findModulePackage(directory: string): any {
const parentDirectory = path.dirname(directory)
if (directory === parentDirectory) { return; }
let contents = null
try {
contents = this.loadModulePackage(directory)
} catch (err) {
}
if (contents) { return contents }
return this.findModulePackage(parentDirectory)
}

protected loadModulePackage(directory: string): any {
adambartholomew marked this conversation as resolved.
Show resolved Hide resolved
return require(path.join(directory, 'package.json'))
}

public enable() {
if (this._enabled) {
return;
Expand Down
70 changes: 69 additions & 1 deletion packages/opentelemetry-instrumentation/test/common/Instrumentation.test.ts
View file
Open in desktop
Expand Up @@ -15,7 +15,8 @@
*/

import * as assert from 'assert';
import { Instrumentation, InstrumentationBase } from '../../src';
import sinon = require('sinon');
import { Instrumentation, InstrumentationBase, InstrumentationModuleDefinition } from '../../src';

class TestInstrumentation extends InstrumentationBase {
constructor() {
Expand All @@ -24,6 +25,23 @@ class TestInstrumentation extends InstrumentationBase {
enable() {}
disable() {}
init() {}
_onRequire<T>(
module: InstrumentationModuleDefinition<T>,
exports: T,
name: string,
baseDir?: string
): T {
return super._onRequire(module, exports, name, baseDir)
}
getModuleVersion(directory: string): string {
adambartholomew marked this conversation as resolved.
Show resolved Hide resolved
return super.getModuleVersion(directory)
}
findModulePackage(directory: string): any {
return super.findModulePackage(directory)
}
loadModulePackage(directory: string): any {
return super.loadModulePackage(directory)
}
}

describe('BaseInstrumentation', () => {
Expand Down Expand Up @@ -56,4 +74,54 @@ describe('BaseInstrumentation', () => {
assert.strictEqual(called, true);
});
});

describe('_onRequire', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should not be added here but in node test only. Common is run for browser and for node

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explains the CI failure. The browser tests won't run on my machine. I'll move these tests. Apologies for being new, appreciate all the help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to apology, we are here to help :), in tests you have folders: browser, node, common, just move your tests to folder node. This way your tests will be run only in node environment

it('loads package.json recursively returning 0.0.0 when undiscovered', () => {
const instrumentation = new TestInstrumentation()
const versionSpy = sinon.spy(instrumentation, 'getModuleVersion');
const findSpy = sinon.spy(instrumentation, 'findModulePackage');
const loadSpy = sinon.spy(instrumentation, 'loadModulePackage')

const moduleDefinition = {} as InstrumentationModuleDefinition<unknown>
instrumentation._onRequire<unknown>(
moduleDefinition,
{} as unknown,
'test-module',
'/foo/bar/baz'
)

sinon.assert.calledOnceWithExactly(versionSpy, '/foo/bar/baz')
sinon.assert.calledWith(findSpy, '/foo/bar/baz')
sinon.assert.calledWith(findSpy, '/foo/bar')
sinon.assert.calledWith(findSpy, '/foo')
sinon.assert.calledThrice(loadSpy)
assert.strictEqual(moduleDefinition.moduleVersion, '0.0.0')
})

it('loads package.json recursively returning version when discovered', () => {
const instrumentation = new TestInstrumentation()
const versionSpy = sinon.spy(instrumentation, 'getModuleVersion');
const findSpy = sinon.spy(instrumentation, 'findModulePackage');
const loadStub = sinon.stub(instrumentation, 'loadModulePackage')

loadStub.withArgs('/foo/bar').returns({
name: 'Test Package',
version: '1.0.2'
})

const moduleDefinition = {} as InstrumentationModuleDefinition<unknown>
instrumentation._onRequire<unknown>(
moduleDefinition,
{} as unknown,
'test-module',
'/foo/bar/baz'
)

sinon.assert.calledOnceWithExactly(versionSpy, '/foo/bar/baz')
sinon.assert.calledWith(findSpy, '/foo/bar/baz')
sinon.assert.calledWith(findSpy, '/foo/bar')
sinon.assert.calledTwice(loadStub)
assert.strictEqual(moduleDefinition.moduleVersion, '1.0.2')
})
})
});