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 instrumentation registration to allow for instrumenting of a local file that does not exist within node_modules. You must pass in absolutePath with the absolute path to the file that is being instrumented along with the moduleName which in this case is just the file name without the extension #1974

Merged
merged 1 commit into from
Jan 25, 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
10 changes: 5 additions & 5 deletions api.js
Original file line number Diff line number Diff line change
Expand Up @@ -1312,7 +1312,7 @@
*
* @param {string|object} moduleName The module name given to require to load the module, or the instrumentation specification
* @param {string} moduleName.moduleName The module name given to require to load the module
* @param {Function} moduleName.onResolved The function to call prior to module load after the filepath has been resolved
* @param {string} [moduleName.absolutePath] Must provide absolute path to module if it does not exist within node_modules. This is used to instrument a file within the same application.
* @param {Function} moduleName.onRequire The function to call when the module has been loaded
* @param {Function} [moduleName.onError] If provided, should `onRequire` throw an error, the error will be passed to
* @param {Function} onRequire The function to call when the module has been loaded
Expand Down Expand Up @@ -1343,7 +1343,7 @@
*
* @param {string|object} moduleName The module name given to require to load the module, or the instrumentation specification
* @param {string} moduleName.moduleName The module name given to require to load the module
* @param {Function} moduleName.onResolved The function to call prior to module load after the filepath has been resolved
* @param {string} [moduleName.absolutePath] Must provide absolute path to module if it does not exist within node_modules. This is used to instrument a file within the same application.
* @param {Function} moduleName.onRequire The function to call when the module has been loaded
* @param {Function} [moduleName.onError] If provided, should `onRequire` throw an error, the error will be passed to
* @param {Function} onRequire The function to call when the module has been loaded
Expand Down Expand Up @@ -1375,7 +1375,7 @@
*
* @param {string|object} moduleName The module name given to require to load the module, or the instrumentation specification
* @param {string} moduleName.moduleName The module name given to require to load the module
* @param {Function} moduleName.onResolved The function to call prior to module load after the filepath has been resolved
* @param {string} [moduleName.absolutePath] Must provide absolute path to module if it does not exist within node_modules. This is used to instrument a file within the same application.
* @param {Function} moduleName.onRequire The function to call when the module has been loaded
* @param {Function} [moduleName.onError] If provided, should `onRequire` throw an error, the error will be passed to
* @param {Function} onRequire The function to call when the module has been loaded
Expand Down Expand Up @@ -1408,7 +1408,7 @@
*
* @param {string|object} moduleName The module name given to require to load the module, or the instrumentation specification
* @param {string} moduleName.moduleName The module name given to require to load the module
* @param {Function} moduleName.onResolved The function to call prior to module load after the filepath has been resolved
* @param {string} [moduleName.absolutePath] Must provide absolute path to module if it does not exist within node_modules. This is used to instrument a file within the same application.
* @param {Function} moduleName.onRequire The function to call when the module has been loaded
* @param {Function} [moduleName.onError] If provided, should `onRequire` throw an error, the error will be passed to
* @param {Function} onRequire The function to call when the module has been loaded
Expand Down Expand Up @@ -1445,7 +1445,7 @@
*
* @param {string|object} moduleName The module name given to require to load the module, or the instrumentation specification
* @param {string} moduleName.moduleName The module name given to require to load the module
* @param {Function} moduleName.onResolved The function to call prior to module load after the filepath has been resolved
* @param {string} [moduleName.absolutePath] Must provide absolute path to module if it does not exist within node_modules. This is used to instrument a file within the same application.
* @param {Function} moduleName.onRequire The function to call when the module has been loaded
* @param {Function} [moduleName.onError] If provided, should `onRequire` throw an error, the error will be passed to
* @param {Function} onRequire The function to call when the module has been loaded
Expand Down Expand Up @@ -1544,7 +1544,7 @@
* @param {object} params Input parameters.
* @param {string} params.responseId The LLM generated identifier for the
* response.
* @returns {LlmTrackedIds|undefined} The tracked identifiers.

Check warning on line 1547 in api.js

View workflow job for this annotation

GitHub Actions / lint (lts/*)

The type 'LlmTrackedIds' is undefined
*/
API.prototype.getLlmMessageIds = function getLlmMessageIds({ responseId } = {}) {
this.agent.metrics
Expand Down
10 changes: 8 additions & 2 deletions lib/shimmer.js
Original file line number Diff line number Diff line change
Expand Up @@ -398,11 +398,17 @@ const shimmer = (module.exports = {

if (!registeredInstrumentation) {
shimmer.registeredInstrumentations[opts.moduleName] = []
// In cases where a customer is trying to instrument a file
// that is not within node_modules, they must provide the absolutePath
// so require-in-the-middle can call our callback. the moduleName
// still needs to be the resolved name so we can look up our instrumentation correctly
const pkgHook = opts.absolutePath || opts.moduleName
bizob2828 marked this conversation as resolved.
Show resolved Hide resolved

// not using a set because this is shared by reference
// to allow custom instrumentation to be loaded after the
// agent is bootstrapped
if (!pkgsToHook.includes(opts.moduleName)) {
pkgsToHook.push(opts.moduleName)
if (!pkgsToHook.includes(pkgHook)) {
pkgsToHook.push(pkgHook)
}
}

Expand Down
8 changes: 8 additions & 0 deletions test/integration/module-loading/local-package.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* Copyright 2020 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

module.exports = () => ({ hello: 'world' })
32 changes: 32 additions & 0 deletions test/integration/module-loading/module-loading.tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const shimmer = require('../../../lib/shimmer')
const symbols = require('../../../lib/symbols')
const { FEATURES } = require('../../../lib/metrics/names')

const LOCAL_MODULE = 'local-package'
const LOCAL_MODULE_PATH = require.resolve('./local-package')
const CUSTOM_MODULE = 'customTestPackage'
const CUSTOM_MODULE_PATH = `./node_modules/${CUSTOM_MODULE}`
const CUSTOM_MODULE_PATH_SUB = `./node_modules/subPkg/node_modules/${CUSTOM_MODULE}`
Expand Down Expand Up @@ -177,6 +179,36 @@ tap.test('Should create usage version metric onRequire', (t) => {
}
})

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

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

shimmer.registerInstrumentation({
moduleName: LOCAL_MODULE,
absolutePath: LOCAL_MODULE_PATH,
onRequire: onRequireHandler
})

require('./local-package')

function onRequireHandler(shim, localPkg, name) {
t.equal(
shim.pkgVersion,
process.version,
'defaults to node version for pkgVersion as this is not a package'
)
t.ok(shim.id)
t.equal(name, LOCAL_MODULE)
const result = localPkg()
t.same(result, { hello: 'world' })
t.end()
}
})

function simulateTestLoadAndUnload() {
const agent = helper.instrumentMockedAgent()

Expand Down
64 changes: 64 additions & 0 deletions test/unit/api/api-instrument-messages.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright 2020 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

'use strict'

const tap = require('tap')
const API = require('../../../api')
const helper = require('../../lib/agent_helper')
const sinon = require('sinon')
const shimmer = require('../../../lib/shimmer')

tap.test('Agent API - instrumentMessages', (t) => {
t.autoend()

let agent = null
let api = null

t.beforeEach(() => {
agent = helper.loadMockedAgent()
api = new API(agent)

sinon.spy(shimmer, 'registerInstrumentation')
})

t.afterEach(() => {
helper.unloadAgent(agent)
agent = null

shimmer.registerInstrumentation.restore()
})

t.test('should register the instrumentation with shimmer', (t) => {
const opts = {
moduleName: 'foobar',
absolutePath: `${__dirname}/foobar`,
onRequire: function () {}
}
api.instrumentMessages(opts)

t.ok(shimmer.registerInstrumentation.calledOnce)
const args = shimmer.registerInstrumentation.getCall(0).args
const [actualOpts] = args

t.same(actualOpts, opts)
t.equal(actualOpts.type, 'message')

t.end()
})

t.test('should convert separate args into an options object', (t) => {
function onRequire() {}
function onError() {}
api.instrumentMessages('foobar', onRequire, onError)

const opts = shimmer.registerInstrumentation.getCall(0).args[0]
t.equal(opts.moduleName, 'foobar')
t.equal(opts.onRequire, onRequire)
t.equal(opts.onError, onError)

t.end()
})
})