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.

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

fix: Updated Shim to properly calculate the _moduleRoot on windows environments #2014

Merged
merged 1 commit into from Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 4 additions & 8 deletions lib/shim/shim.js
Expand Up @@ -14,6 +14,7 @@ const util = require('util')
const symbols = require('../symbols')
const { addCLMAttributes: maybeAddCLMAttributes } = require('../util/code-level-metrics')
const { makeId } = require('../util/hashes')
const { isBuiltin } = require('module')

// Some modules do terrible things, like change the prototype of functions. To
// avoid crashing things we'll use a cached copy of apply everywhere.
Expand Down Expand Up @@ -45,14 +46,9 @@ function Shim(agent, moduleName, resolvedName, shimName, pkgVersion) {
this.assignId(shimName)
this.pkgVersion = pkgVersion

// Determine the root directory of the module.
let moduleRoot = null
let next = resolvedName || '/'
do {
moduleRoot = next
next = path.dirname(moduleRoot)
} while (moduleRoot.length > 1 && !/node_modules(?:\/@[^/]+)?$/.test(next))
this._moduleRoot = moduleRoot
// Used in `shim.require`
// If this is a built-in the root is set as `.`
this._moduleRoot = isBuiltin(resolvedName || moduleName) ? '.' : resolvedName
}
module.exports = Shim

Expand Down
9 changes: 5 additions & 4 deletions lib/shimmer.js
Expand Up @@ -324,7 +324,7 @@ const shimmer = (module.exports = {
*/
registerCoreInstrumentation(agent) {
// Instrument global.
const globalShim = new shims.Shim(agent, 'globals', 'globals')
const globalShim = new shims.Shim(agent, 'globals', '.')
applyDebugState(globalShim, global, false)
const globalsFilepath = path.join(__dirname, 'instrumentation', 'core', 'globals.js')
_firstPartyInstrumentation(agent, globalsFilepath, globalShim, global, 'globals')
Expand All @@ -336,7 +336,7 @@ const shimmer = (module.exports = {
type: MODULE_TYPE.TRANSACTION,
agent,
moduleName: 'undici',
resolvedName: 'undici'
resolvedName: '.'
})
_firstPartyInstrumentation(agent, undiciPath, undiciShim)

Expand Down Expand Up @@ -778,8 +778,9 @@ function trackInstrumentationUsage(agent, shim, moduleName, metricPrefix) {
}

function tryGetVersion(shim) {
// Global module (i.e. domain) or finding root failed
if (shim._moduleRoot === '/') {
// Indicates it is a built-in where the version is not useful as
// it is `process.version`
if (shim._moduleRoot === '.') {
return
}

Expand Down
22 changes: 22 additions & 0 deletions test/integration/module-loading/module-loading.tap.js
Expand Up @@ -179,6 +179,28 @@ tap.test('Should create usage version metric onRequire', (t) => {
}
})

tap.test('Should create usage metric onRequire for built-in', (t) => {
const domainMetric = `${FEATURES.INSTRUMENTATION.ON_REQUIRE}/domain`
let agent = helper.instrumentMockedAgent()

t.teardown(() => {
helper.unloadAgent(agent)
agent = null
})

// eslint-disable-next-line node/no-deprecated-api
require('domain')

const onRequireMetric = agent.metrics._metrics.unscoped[domainMetric]

t.ok(onRequireMetric)
t.equal(onRequireMetric.callCount, 1)
const domainMetrics = Object.keys(agent.metrics._metrics.unscoped)
t.equal(domainMetrics.length, 1, 'should not log a version metric for a built-in')

t.end()
})

tap.test('should instrument a local package', (t) => {
let agent = helper.instrumentMockedAgent()

Expand Down
44 changes: 44 additions & 0 deletions test/unit/shim/shim.test.js
Expand Up @@ -3061,4 +3061,48 @@ tap.test('Shim', function (t) {
t.end()
})
})

t.test('_moduleRoot', (t) => {
t.beforeEach((t) => {
t.context.agent = helper.loadMockedAgent()
})

t.afterEach((t) => {
helper.unloadAgent(t.context.agent)
})

t.test('should set _moduleRoot to `.` if resolvedName is a built-in', (t) => {
const { agent } = t.context
const shim = new Shim(agent, 'http', 'http')
t.equal(shim._moduleRoot, '.')
t.end()
})

t.test(
'should set _moduleRoot to `.` if resolvedName is undefined but moduleName is a built-in',
(t) => {
const { agent } = t.context
const shim = new Shim(agent, 'http')
t.equal(shim._moduleRoot, '.')
t.end()
}
)

t.test('should set _moduleRoot to resolvedName not a built-in', (t) => {
const { agent } = t.context
const root = '/path/to/app/node_modules/rando-mod'
const shim = new Shim(agent, 'rando-mod', root)
t.equal(shim._moduleRoot, root)
t.end()
})

t.test('should properly resolve _moduleRoot as windows path', (t) => {
const { agent } = t.context
const root = `c:\\path\\to\\app\\node_modules\\@scope\\test`
const shim = new Shim(agent, '@scope/test', root)
t.equal(shim._moduleRoot, root)
t.end()
})
t.end()
})
})