From c9f30390da0bc5897bcb9addaa9f30ca9870accd Mon Sep 17 00:00:00 2001 From: James Sumners Date: Mon, 18 Mar 2024 10:21:36 -0400 Subject: [PATCH 1/6] chore: Refactored module instrumentation tracking to use a tracker class --- .github/workflows/ci-workflow.yml | 14 +- jsdoc-conf.json | 26 ---- jsdoc-conf.jsonc | 38 ++++++ lib/instrumentation-descriptor.js | 10 ++ lib/shimmer.js | 121 ++++++++++-------- lib/symbols.js | 2 - package-lock.json | 8 +- package.json | 4 +- .../module-loading/module-loading.tap.js | 14 +- test/integration/shimmer/module-load.tap.js | 90 ------------- test/lib/agent_helper.js | 3 +- test/unit/shimmer.test.js | 5 +- 12 files changed, 147 insertions(+), 188 deletions(-) delete mode 100644 jsdoc-conf.json create mode 100644 jsdoc-conf.jsonc delete mode 100644 test/integration/shimmer/module-load.tap.js diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index d70533926d..e4a486975e 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -35,7 +35,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run Linting run: npm run lint - name: Inspect Lockfile @@ -58,7 +58,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run CI Script Unit Tests run: npm run unit:scripts @@ -79,7 +79,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run Unit Tests run: npm run unit - name: Archive Unit Test Coverage @@ -104,7 +104,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run Docker Services run: npm run services - name: Run Integration Tests @@ -132,7 +132,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run Docker Services run: npm run services - name: Run Versioned Tests @@ -161,7 +161,7 @@ jobs: with: name: logs-${{ matrix.node-version }}.tgz path: ./logs-${{ matrix.node-version }}.tgz - + # There is no coverage for external as that's tracked in their respective repos versioned-external: needs: skip_if_release @@ -180,7 +180,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci + run: npm ci --verbose - name: Run Versioned Tests run: TEST_CHILD_TIMEOUT=600000 npm run versioned:external env: diff --git a/jsdoc-conf.json b/jsdoc-conf.json deleted file mode 100644 index 603fe6f5d1..0000000000 --- a/jsdoc-conf.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "opts": { - "destination": "out/", - "readme": "./README.md", - "template": "./node_modules/clean-jsdoc-theme", - "theme_opts": { - "search": false, - "shouldRemoveScrollbarStyle": true - } - }, - "source": { - "exclude": ["node_modules", "test"], - "includePattern": ".+\\.js(doc)?$" - }, - "plugins": [ - "plugins/markdown" - ], - "templates": { - "cleverLinks": true, - "showInheritedInNav": false - }, - "markdown": { - "hardwrap": false, - "idInHeadings": true - } -} diff --git a/jsdoc-conf.jsonc b/jsdoc-conf.jsonc new file mode 100644 index 0000000000..a9e3d77a51 --- /dev/null +++ b/jsdoc-conf.jsonc @@ -0,0 +1,38 @@ +{ + "opts": { + "destination": "out/", + "readme": "./README.md", + "template": "./node_modules/clean-jsdoc-theme", + "theme_opts": { + "search": false, + "shouldRemoveScrollbarStyle": true + }, + "tutorials": "examples/shim", + "recurse": true + }, + "source": { + "exclude": ["node_modules", "test"], + "includePattern": ".+\\.js(doc)?$", + // Note: we cannot add patterns to the "include" strings. They must be + // explicit strings that point to either specific files or directories. + // Recursion is enabled for the `jsdoc` command, thus any directories + // specified here will be processed recursively. So make sure any files in + // these directories should be included in our public docs. + "include": [ + "lib/shim/", + "lib/transaction/handle.js", + "lib/instrumentation-descriptor.js" + ] + }, + "plugins": [ + "plugins/markdown" + ], + "templates": { + "cleverLinks": true, + "showInheritedInNav": false + }, + "markdown": { + "hardwrap": false, + "idInHeadings": true + } +} diff --git a/lib/instrumentation-descriptor.js b/lib/instrumentation-descriptor.js index 357ed82b36..ce53142083 100644 --- a/lib/instrumentation-descriptor.js +++ b/lib/instrumentation-descriptor.js @@ -30,6 +30,7 @@ const idGen = new IdGen() * @property {string} absolutePath * @property {string} module * @property {string} moduleName + * @property {string} shimName * @property {InstrumentationOnError} onError * @property {InstrumentationOnRequire} onRequire * @property {string} resolvedName @@ -117,6 +118,14 @@ class InstrumentationDescriptor { */ moduleName + /** + * Used when instrumenting a module to determine if a module has already + * been wrapped by a specific shim instance. It is used in conjunction with + * the `shim.id` value. + * @type {string} + */ + shimName + /** * The absolute path to the module to instrument. This should only be set * when the module being instrumented does not reside in a `node_modules` @@ -163,6 +172,7 @@ class InstrumentationDescriptor { this.absolutePath = params.absolutePath this.module = params.module this.moduleName = params.moduleName + this.shimName = params.shimName this.onError = params.onError this.onRequire = params.onRequire this.resolvedName = params.resolvedName diff --git a/lib/shimmer.js b/lib/shimmer.js index 77eda61d0a..cb7e1f9d6b 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -14,6 +14,8 @@ const shims = require('./shim') const { Hook } = require('@newrelic/ritm') const IitmHook = require('import-in-the-middle') const { nrEsmProxy } = require('./symbols') +const InstrumentationDescriptor = require('./instrumentation-descriptor') +const InstrumentationTracker = require('./instrumentation-tracker') let pkgsToHook = [] const NAMES = require('./metrics/names') @@ -361,6 +363,7 @@ const shimmer = (module.exports = { _firstPartyInstrumentation(agent, filePath, shim, uninstrumented, mojule) } }, + registerHooks(agent) { this._ritm = new Hook(pkgsToHook, function onHook(exports, name, basedir) { return _postLoad(agent, exports, name, basedir) @@ -369,6 +372,7 @@ const shimmer = (module.exports = { return _postLoad(agent, exports, name, basedir, true) }) }, + removeHooks() { if (this._ritm) { this._ritm.unhook() @@ -394,10 +398,11 @@ const shimmer = (module.exports = { return } - const registeredInstrumentation = shimmer.registeredInstrumentations[opts.moduleName] + const registeredInstrumentation = shimmer.registeredInstrumentations.getAllByName( + opts.moduleName + ) 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 @@ -412,12 +417,13 @@ const shimmer = (module.exports = { } } - opts[symbols.instrumented] = new Set() - opts[symbols.instrumentedErrored] = new Set() - shimmer.registeredInstrumentations[opts.moduleName].push({ ...opts }) + shimmer.registeredInstrumentations.track( + opts.moduleName, + new InstrumentationDescriptor({ ...opts }) + ) }, - registeredInstrumentations: Object.create(null), + registeredInstrumentations: new InstrumentationTracker(), /** * NOT FOR USE IN PRODUCTION CODE @@ -467,15 +473,18 @@ const shimmer = (module.exports = { * only if every hook succeeded. * * @param {string} moduleName name of registered instrumentation - * @returns {boolean} if all instrumentation hooks ran for a given version + * @param {object} resolvedName the fully resolve path to the module + * @returns {boolean} if all instrumentation hooks successfully ran for a + * module */ - isInstrumented(moduleName) { - const pkgVersion = shimmer.getPackageVersion(moduleName) - const instrumentations = shimmer.registeredInstrumentations[moduleName] - const didInstrument = instrumentations.filter((instrumentation) => - instrumentation[symbols.instrumented].has(pkgVersion) - ) - return didInstrument.length === instrumentations.length + isInstrumented(moduleName, resolvedName) { + const items = shimmer.registeredInstrumentations + .getAllByName(moduleName) + .filter( + (item) => + item.instrumentation.resolvedName === resolvedName && item.meta.instrumented === true + ) + return items.length > 0 }, instrumentPostLoad(agent, module, moduleName, resolvedName, returnModule = false) { @@ -483,7 +492,7 @@ const shimmer = (module.exports = { // This is to not break the public API // previously it would just call instrumentation // and not check the result - return returnModule ? result : shimmer.isInstrumented(moduleName) + return returnModule ? result : shimmer.isInstrumented(moduleName, resolvedName) }, /** @@ -494,8 +503,15 @@ const shimmer = (module.exports = { */ getPackageVersion(moduleName) { try { - const { basedir } = shimmer.registeredInstrumentations[moduleName] - const pkg = require(path.resolve(basedir, 'package.json')) + const trackedItems = shimmer.registeredInstrumentations.getAllByName(moduleName) + if (trackedItems === undefined) { + throw Error(`no tracked items for module '${moduleName}'`) + } + const item = trackedItems.find((item) => item.instrumentation.resolvedName !== undefined) + if (item === undefined) { + return process.version + } + const pkg = require(path.resolve(item.instrumentation.resolvedName, 'package.json')) return pkg.version } catch (err) { logger.debug('Failed to get version for `%s`, reason: %s', moduleName, err.message) @@ -534,16 +550,17 @@ function applyDebugState(shim, nodule, inEsm) { function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver) { // default to Node.js version, this occurs for core libraries const pkgVersion = resolvedName ? shimmer.getPackageVersion(moduleName) : process.version - const instrumentations = shimmer.registeredInstrumentations[moduleName] - instrumentations.forEach((instrumentation) => { - const isInstrumented = instrumentation[symbols.instrumented].has(pkgVersion) - const failedInstrumentation = instrumentation[symbols.instrumentedErrored].has(pkgVersion) - if (isInstrumented || failedInstrumentation) { + const trackedInstrumentations = shimmer.registeredInstrumentations.getAllByName(moduleName) + trackedInstrumentations.forEach((trackedInstrumentation) => { + const isInstrumented = trackedInstrumentation.meta.instrumented === true + const failedInstrumentation = trackedInstrumentation.meta.didError === true + if (isInstrumented === true || failedInstrumentation === true) { const msg = isInstrumented ? 'Already instrumented' : 'Failed to instrument' logger.trace(`${msg} ${moduleName}@${pkgVersion}, skipping registering instrumentation`) return } + const { instrumentation } = trackedInstrumentation const resolvedNodule = resolveNodule({ nodule, instrumentation, esmResolver }) const shim = shims.createShimFromType({ type: instrumentation.type, @@ -555,18 +572,13 @@ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver }) applyDebugState(shim, resolvedNodule, esmResolver) - trackInstrumentationUsage( - agent, - shim, - instrumentation.specifier || moduleName, - NAMES.FEATURES.INSTRUMENTATION.ON_REQUIRE - ) + trackInstrumentationUsage(agent, shim, moduleName, NAMES.FEATURES.INSTRUMENTATION.ON_REQUIRE) // Tracking instrumentation is only used to add the supportability metrics // that occur directly above this. No reason to attempt to load instrumentation // as it does not exist. if (instrumentation.type === MODULE_TYPE.TRACKING) { - instrumentation[symbols.instrumented].add(pkgVersion) + shimmer.registeredInstrumentations.setHookSuccess(trackedInstrumentation) return } @@ -644,27 +656,20 @@ function resolveNodule({ nodule, instrumentation, esmResolver }) { * @param {object} params wrapping object to function * @param {*} params.shim The instance of the shim used to instrument the module. * @param {object} params.nodule Class or module containing the function to wrap. - * @param {object} params.resolvedNodule returns xport of the default property - * @param {string} params.pkgVersion version of module + * @param {object} params.resolvedNodule returns export of the default property * @param {string} params.moduleName module name - * @param {object} params.instrumentation hooks for a give module - * @returns {object} updated xport module + * @param {InstrumentationDescriptor} params.instrumentation hooks for a give module + * @returns {object} updated export module */ -function loadInstrumentation({ - shim, - resolvedNodule, - pkgVersion, - moduleName, - nodule, - instrumentation -}) { +function loadInstrumentation({ shim, resolvedNodule, moduleName, nodule, instrumentation }) { + const trackedItem = shimmer.registeredInstrumentations.getTrackedItem(moduleName, instrumentation) try { if (instrumentation.onRequire(shim, resolvedNodule, moduleName) !== false) { + shimmer.registeredInstrumentations.setHookSuccess(trackedItem) nodule = shim.getExport(nodule) - instrumentation[symbols.instrumented].add(pkgVersion) } } catch (instrumentationError) { - instrumentation[symbols.instrumentedErrored].add(pkgVersion) + shimmer.registeredInstrumentations.setHookFailure(trackedItem) if (instrumentation.onError) { try { instrumentation.onError(instrumentationError) @@ -706,20 +711,34 @@ function _firstPartyInstrumentation(agent, fileName, shim, nodule, moduleName) { } } +/** + * Invoked directly after a module is loaded via `require` (or `import`). This + * is the first opportunity for us to work with the newly loaded module. Prior + * to this, the only information we have is the simple name (the string used + * to load the module), as well as the `onRequire` and `onError` hooks to + * attach to the module. + * + * @param {object} agent + * @param {object} nodule The newly loaded module. + * @param {string} name The simple name used to load the module. + * @param {string} resolvedName The full file system path to the module. + * @param {object} [esmResolver] + * @returns {*|Object|undefined} + * @private + */ function _postLoad(agent, nodule, name, resolvedName, esmResolver) { - const instrumentation = shimmer.getInstrumentationNameFromModuleName(name) - const registeredInstrumentation = shimmer.registeredInstrumentations[instrumentation] + const simpleName = shimmer.getInstrumentationNameFromModuleName(name) + const registeredInstrumentations = shimmer.registeredInstrumentations.getAllByName(simpleName) const hasPostLoadInstrumentation = - registeredInstrumentation && - registeredInstrumentation.length && - registeredInstrumentation.filter((hook) => hook.onRequire).length + registeredInstrumentations && + registeredInstrumentations.length && + registeredInstrumentations.filter((ri) => ri.instrumentation.onRequire).length // Check if this is a known instrumentation and then run it. if (hasPostLoadInstrumentation) { - // Add the basedir to the instrumentation to be used later to parse version from package.json - registeredInstrumentation.basedir = resolvedName + shimmer.registeredInstrumentations.setResolvedName(simpleName, resolvedName) logger.trace('Instrumenting %s with onRequire (module loaded) hook.', name) - return instrumentPostLoad(agent, nodule, instrumentation, resolvedName, esmResolver) + return instrumentPostLoad(agent, nodule, simpleName, resolvedName, esmResolver) } return nodule diff --git a/lib/symbols.js b/lib/symbols.js index 5caf048dac..8d48933cf6 100644 --- a/lib/symbols.js +++ b/lib/symbols.js @@ -11,8 +11,6 @@ module.exports = { databaseName: Symbol('databaseName'), disableDT: Symbol('Disable distributed tracing'), // description for backwards compatibility executorContext: Symbol('executorContext'), - instrumented: Symbol('instrumented'), - instrumentedErrored: Symbol('instrumentedErrored'), name: Symbol('name'), onceExecuted: Symbol('onceExecuted'), offTheRecord: Symbol('offTheRecord'), diff --git a/package-lock.json b/package-lock.json index af1e7164d7..28925cb1d6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -33,7 +33,7 @@ "devDependencies": { "@newrelic/eslint-config": "^0.3.0", "@newrelic/newrelic-oss-cli": "^0.1.2", - "@newrelic/test-utilities": "^8.1.0", + "@newrelic/test-utilities": "^8.4.0", "@octokit/rest": "^18.0.15", "@slack/bolt": "^3.7.0", "ajv": "^6.12.6", @@ -997,9 +997,9 @@ } }, "node_modules/@newrelic/test-utilities": { - "version": "8.2.0", - "resolved": "https://registry.npmjs.org/@newrelic/test-utilities/-/test-utilities-8.2.0.tgz", - "integrity": "sha512-t5QcfnmTkVWn0tMasNdy+H4DLbx2zJo4HSEDlhmw07kYCVFWjkzfWNSa5+Sp6NhIRyyuNjC3fT5uZhVs41Mkpw==", + "version": "8.4.0", + "resolved": "https://registry.npmjs.org/@newrelic/test-utilities/-/test-utilities-8.4.0.tgz", + "integrity": "sha512-WCr/zGAwZs2B1mI/Kf0aR+XofM61Vb0pPjCyS7adyLFBd82Ccb0kPRGiycc07ITQKvtmd6LdQnl3a2K+qaKA5g==", "dev": true, "dependencies": { "@smithy/eventstream-codec": "^2.1.1", diff --git a/package.json b/package.json index 79da6cf169..9606adb777 100644 --- a/package.json +++ b/package.json @@ -161,7 +161,7 @@ "lint": "eslint ./*.{js,mjs} lib test bin examples", "lint:fix": "eslint --fix, ./*.{js,mjs} lib test bin examples", "lint:lockfile": "lockfile-lint --path package-lock.json --type npm --allowed-hosts npm --validate-https --validate-integrity", - "public-docs": "jsdoc -c ./jsdoc-conf.json --tutorials examples/shim api.js lib/shim/* lib/shim/specs/params/* lib/transaction/handle.js && cp examples/shim/*.png out/", + "public-docs": "jsdoc -c ./jsdoc-conf.jsonc && cp examples/shim/*.png out/", "publish-docs": "./bin/publish-docs.sh", "services": "docker compose up -d --wait", "smoke": "npm run ssl && time tap test/smoke/**/**/*.tap.js --timeout=180 --no-coverage", @@ -217,7 +217,7 @@ "devDependencies": { "@newrelic/eslint-config": "^0.3.0", "@newrelic/newrelic-oss-cli": "^0.1.2", - "@newrelic/test-utilities": "^8.1.0", + "@newrelic/test-utilities": "^8.4.0", "@octokit/rest": "^18.0.15", "@slack/bolt": "^3.7.0", "ajv": "^6.12.6", diff --git a/test/integration/module-loading/module-loading.tap.js b/test/integration/module-loading/module-loading.tap.js index 704fbd5b14..3f20850b39 100644 --- a/test/integration/module-loading/module-loading.tap.js +++ b/test/integration/module-loading/module-loading.tap.js @@ -69,8 +69,14 @@ tap.test('should instrument multiple versions of the same package', function (t) const pkg2 = require(CUSTOM_MODULE_PATH_SUB) t.ok(pkg1[symbols.shim], 'should wrap first package') t.ok(pkg2[symbols.shim], 'should wrap sub package of same name, different version') - t.ok(instrumentation[symbols.instrumented].has('3.0.0')) - t.ok(instrumentation[symbols.instrumented].has('1.0.0')) + + const trackedItems = shimmer.registeredInstrumentations.getAllByName(CUSTOM_MODULE) + t.equal(trackedItems.length, 2) + t.equal(trackedItems[0].instrumentation.resolvedName.includes(CUSTOM_MODULE_PATH.slice(1)), true) + t.equal( + trackedItems[1].instrumentation.resolvedName.includes(CUSTOM_MODULE_PATH_SUB.slice(1)), + true + ) }) tap.test('should only log supportability metric for tracking type instrumentation', function (t) { @@ -92,7 +98,9 @@ tap.test('should only log supportability metric for tracking type instrumentatio t.equal(knexOnRequiredMetric.callCount, 1, `should record ${PKG}`) const knexVersionMetric = agent.metrics._metrics.unscoped[PKG_VERSION] t.equal(knexVersionMetric.callCount, 1, `should record ${PKG_VERSION}`) - t.ok(shimmer.isInstrumented('knex'), 'should mark tracking modules as instrumented') + // eslint-disable-next-line node/no-extraneous-require + const modPath = path.dirname(require.resolve('knex')) + t.ok(shimmer.isInstrumented('knex', modPath), 'should mark tracking modules as instrumented') t.end() }) diff --git a/test/integration/shimmer/module-load.tap.js b/test/integration/shimmer/module-load.tap.js deleted file mode 100644 index ed066f85e6..0000000000 --- a/test/integration/shimmer/module-load.tap.js +++ /dev/null @@ -1,90 +0,0 @@ -/* - * Copyright 2020 New Relic Corporation. All rights reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -'use strict' - -const tap = require('tap') -const helper = require('../../lib/agent_helper') -const shimmer = require('../../../lib/shimmer') -const symbols = require('../../../lib/symbols') - -tap.test('Test Module Instrumentation Loading', (t) => { - t.autoend() - let agent = null - - t.beforeEach(() => { - agent = helper.loadMockedAgent() - }) - - t.afterEach(() => { - helper.unloadAgent(agent) - agent = null - }) - - t.test('symbols.instrumented set correctly', (t) => { - // path to our module fixture from this file - const modulePathLocal = './module-load-fixture' - - // path to our module fixture from the shimmer -- - // needed since `reinstrument` results in a call - // to require _from_ from the shimmer - const modulePathFromShimmer = '../test/integration/shimmer/module-load-fixture' - - // register our instrumentation == onRequire will be - // the code that's normally in the "instrument" function - // that a instrumentation module exports - const instrumentation = { - moduleName: modulePathFromShimmer, - type: null, - onRequire: () => {} - } - - shimmer.registerInstrumentation(instrumentation) - - // use reinstrument helper method to - // manually instrument our module - shimmer.reinstrument(agent, modulePathFromShimmer) - - const module = require(modulePathLocal) - - t.ok(module, 'loaded module') - t.equal(module(), 'hello world', 'module behaves as expected') - t.ok(instrumentation[symbols.instrumented].has(process.version)) - t.end() - }) - - t.test('symbols.instrumentedErrored set correctly', (t) => { - // path to our module fixture from this file - const modulePathLocal = './module-load-fixture-errored' - - // path to our module fixture from the shimmer -- - // needed since `reinstrument` results in a call - // to require _from_ from the shimmer - const modulePathFromShimmer = '../test/integration/shimmer/module-load-fixture-errored' - - // register our instrumentation == onRequire will be - // the code that's normally in the "instrument" function - // that a instrumentation module exports - const instrumentation = { - moduleName: modulePathFromShimmer, - type: null, - onRequire: () => { - throw new Error('our instrumentation errors') - } - } - shimmer.registerInstrumentation(instrumentation) - - // use reinstrument helper method to - // manually instrument our module - shimmer.reinstrument(agent, modulePathFromShimmer) - - const module = require(modulePathLocal) - - t.ok(module, 'loaded module') - t.equal(module(), 'hello world', 'module behaves as expected') - t.ok(instrumentation[symbols.instrumentedErrored].has(process.version)) - t.end() - }) -}) diff --git a/test/lib/agent_helper.js b/test/lib/agent_helper.js index 1c3fb1e44c..fb4ddbb3d9 100644 --- a/test/lib/agent_helper.js +++ b/test/lib/agent_helper.js @@ -16,6 +16,7 @@ const { defaultAttributeConfig } = require('./fixtures') const { EventEmitter } = require('events') const Transaction = require('../../lib/transaction') const symbols = require('../../lib/symbols') +const InstrumentationTracker = require('../../lib/instrumentation-tracker') const http = require('http') const https = require('https') const semver = require('semver') @@ -237,7 +238,7 @@ helper.unloadAgent = (agent, shimmer = require('../../lib/shimmer')) => { agent.emit('unload') shimmer.removeHooks() shimmer.unwrapAll() - shimmer.registeredInstrumentations = Object.create(null) + shimmer.registeredInstrumentations = new InstrumentationTracker() shimmer.debug = false helper.maybeUnloadSecurityAgent(agent) diff --git a/test/unit/shimmer.test.js b/test/unit/shimmer.test.js index d3f8720d30..6c9b7fcb64 100644 --- a/test/unit/shimmer.test.js +++ b/test/unit/shimmer.test.js @@ -68,7 +68,8 @@ function makeModuleTests({ moduleName, relativePath, throwsError }, t) { }) t.test('should construct a DatastoreShim if the type is "datastore"', function (t) { - shimmer.registeredInstrumentations[moduleName][0].type = 'datastore' + shimmer.registeredInstrumentations.getAllByName(moduleName)[0].instrumentation.type = + 'datastore' require(relativePath) const { onRequireArgs } = t.context t.ok(onRequireArgs[0] instanceof shims.DatastoreShim) @@ -887,7 +888,7 @@ tap.test('Shimmer with logger mock', (t) => { t.same(loggerMock.debug.args[0], [ 'Failed to get version for `%s`, reason: %s', 'bogus', - `Cannot destructure property 'basedir' of 'shimmer.registeredInstrumentations[moduleName]' as it is undefined.` + `no tracked items for module 'bogus'` ]) t.end() } From ba0eae9142cd0696249eeeeea1fe15917cd6be88 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Tue, 2 Apr 2024 16:11:40 -0400 Subject: [PATCH 2/6] refactor shim constants --- api.js | 2 +- lib/instrumentation-descriptor.js | 19 ++++++++++ lib/instrumentations.js | 60 +++++++++++++++---------------- lib/shim/conglomerate-shim.js | 29 +++++++-------- lib/shim/constants.js | 42 ---------------------- lib/shim/index.js | 18 +++++----- lib/shimmer.js | 28 +++++++-------- 7 files changed, 86 insertions(+), 112 deletions(-) delete mode 100644 lib/shim/constants.js diff --git a/api.js b/api.js index 719ebc69f6..338cf46d40 100644 --- a/api.js +++ b/api.js @@ -26,7 +26,7 @@ const { const LlmFeedbackMessage = require('./lib/llm-events/feedback-message') const ATTR_DEST = require('./lib/config/attribute-filter').DESTINATIONS -const MODULE_TYPE = require('./lib/shim/constants').MODULE_TYPE +const MODULE_TYPE = require('./lib/instrumentation-descriptor').TYPES const NAMES = require('./lib/metrics/names') const obfuscate = require('./lib/util/sql/obfuscate') const { DESTINATIONS } = require('./lib/config/attribute-filter') diff --git a/lib/instrumentation-descriptor.js b/lib/instrumentation-descriptor.js index ce53142083..0a1b8c19a7 100644 --- a/lib/instrumentation-descriptor.js +++ b/lib/instrumentation-descriptor.js @@ -194,3 +194,22 @@ class InstrumentationDescriptor { } module.exports = InstrumentationDescriptor + +// This export is for backward compatibility in the public API. The +// public API object simply re-exports this object that was originally +// in a `constants.js` file prior to the creation of the +// `InstrumentationDescriptor`. +module.exports.TYPES = { + GENERIC: InstrumentationDescriptor.TYPE_GENERIC, + + DATASTORE: InstrumentationDescriptor.TYPE_DATASTORE, + MESSAGE: InstrumentationDescriptor.TYPE_MESSAGE, + PROMISE: InstrumentationDescriptor.TYPE_PROMISE, + + WEB_FRAMEWORK: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK, + TRACKING: InstrumentationDescriptor.TYPE_TRACKING, + /** @private */ + CONGLOMERATE: InstrumentationDescriptor.TYPE_CONGLOMERATE, + /** @private */ + TRANSACTION: InstrumentationDescriptor.TYPE_TRANSACTION +} diff --git a/lib/instrumentations.js b/lib/instrumentations.js index f2f644a1b1..71eae6b966 100644 --- a/lib/instrumentations.js +++ b/lib/instrumentations.js @@ -5,44 +5,44 @@ 'use strict' -const MODULE_TYPE = require('./shim/constants').MODULE_TYPE +const InstrumentationDescriptor = require('./instrumentation-descriptor') // Return a new copy of this array every time we're called module.exports = function instrumentations() { return { 'aws-sdk': { module: '@newrelic/aws-sdk' }, 'amqplib': { module: './instrumentation/amqplib' }, - 'cassandra-driver': { type: MODULE_TYPE.DATASTORE }, - 'connect': { type: MODULE_TYPE.WEB_FRAMEWORK }, - 'bluebird': { type: MODULE_TYPE.PROMISE }, - 'bunyan': { type: MODULE_TYPE.GENERIC }, - 'director': { type: MODULE_TYPE.WEB_FRAMEWORK }, - '@elastic/elasticsearch': { type: MODULE_TYPE.DATASTORE }, - 'express': { type: MODULE_TYPE.WEB_FRAMEWORK }, - 'fastify': { type: MODULE_TYPE.WEB_FRAMEWORK }, - 'generic-pool': { type: MODULE_TYPE.GENERIC }, + 'cassandra-driver': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + 'connect': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + 'bluebird': { type: InstrumentationDescriptor.TYPE_PROMISE }, + 'bunyan': { type: InstrumentationDescriptor.TYPE_GENERIC }, + 'director': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + '@elastic/elasticsearch': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + 'express': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + 'fastify': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + 'generic-pool': { type: InstrumentationDescriptor.TYPE_GENERIC }, '@grpc/grpc-js': { module: './instrumentation/grpc-js' }, - '@hapi/hapi': { type: MODULE_TYPE.WEB_FRAMEWORK }, - 'ioredis': { type: MODULE_TYPE.DATASTORE }, + '@hapi/hapi': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + 'ioredis': { type: InstrumentationDescriptor.TYPE_DATASTORE }, 'koa': { module: '@newrelic/koa' }, 'langchain': { module: './instrumentation/langchain' }, - 'memcached': { type: MODULE_TYPE.DATASTORE }, - 'mongodb': { type: MODULE_TYPE.DATASTORE }, + 'memcached': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + 'mongodb': { type: InstrumentationDescriptor.TYPE_DATASTORE }, 'mysql': { module: './instrumentation/mysql' }, - 'openai': { type: MODULE_TYPE.GENERIC }, - '@nestjs/core': { type: MODULE_TYPE.WEB_FRAMEWORK }, - '@prisma/client': { type: MODULE_TYPE.DATASTORE }, + 'openai': { type: InstrumentationDescriptor.TYPE_GENERIC }, + '@nestjs/core': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, + '@prisma/client': { type: InstrumentationDescriptor.TYPE_DATASTORE }, 'pino': { module: './instrumentation/pino' }, - 'pg': { type: MODULE_TYPE.DATASTORE }, + 'pg': { type: InstrumentationDescriptor.TYPE_DATASTORE }, 'q': { type: null }, - 'redis': { type: MODULE_TYPE.DATASTORE }, - '@node-redis/client': { type: MODULE_TYPE.DATASTORE }, - '@redis/client': { type: MODULE_TYPE.DATASTORE }, - 'restify': { type: MODULE_TYPE.WEB_FRAMEWORK }, + 'redis': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + '@node-redis/client': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + '@redis/client': { type: InstrumentationDescriptor.TYPE_DATASTORE }, + 'restify': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, 'superagent': { module: '@newrelic/superagent' }, - '@hapi/vision': { type: MODULE_TYPE.WEB_FRAMEWORK }, + '@hapi/vision': { type: InstrumentationDescriptor.TYPE_WEB_FRAMEWORK }, 'when': { module: './instrumentation/when' }, - 'winston': { type: MODULE_TYPE.GENERIC }, + 'winston': { type: InstrumentationDescriptor.TYPE_GENERIC }, /** * The modules below are listed here purely to take * advantage of the Supportability/Features/onRequire/ @@ -52,11 +52,11 @@ module.exports = function instrumentations() { * Libraries that have OpenTelemetry instrumentation we want to register * or have already registered. */ - 'loglevel': { type: MODULE_TYPE.TRACKING }, - 'npmlog': { type: MODULE_TYPE.TRACKING }, - 'fancy-log': { type: MODULE_TYPE.TRACKING }, - 'knex': { type: MODULE_TYPE.TRACKING }, - '@azure/openai': { type: MODULE_TYPE.TRACKING }, - '@langchain/community/llms/bedrock': { type: MODULE_TYPE.TRACKING } + 'loglevel': { type: InstrumentationDescriptor.TYPE_TRACKING }, + 'npmlog': { type: InstrumentationDescriptor.TYPE_TRACKING }, + 'fancy-log': { type: InstrumentationDescriptor.TYPE_TRACKING }, + 'knex': { type: InstrumentationDescriptor.TYPE_TRACKING }, + '@azure/openai': { type: InstrumentationDescriptor.TYPE_TRACKING }, + '@langchain/community/llms/bedrock': { type: InstrumentationDescriptor.TYPE_TRACKING } } } diff --git a/lib/shim/conglomerate-shim.js b/lib/shim/conglomerate-shim.js index 7c61cad097..a29650c482 100644 --- a/lib/shim/conglomerate-shim.js +++ b/lib/shim/conglomerate-shim.js @@ -8,14 +8,14 @@ const logger = require('../logger').child({ component: 'ConglomerateShim' }) const Shim = require('./shim') -const { MODULE_TYPE } = require('./constants') +const InstrumentationDescriptor = require('../instrumentation-descriptor') const SHIM_CLASSES = { - [MODULE_TYPE.GENERIC]: Shim, - [MODULE_TYPE.DATASTORE]: require('./datastore-shim'), - [MODULE_TYPE.MESSAGE]: require('./message-shim'), - [MODULE_TYPE.PROMISE]: require('./promise-shim'), - [MODULE_TYPE.TRANSACTION]: require('./transaction-shim'), - [MODULE_TYPE.WEB_FRAMEWORK]: require('./webframework-shim') + [InstrumentationDescriptor.TYPE_GENERIC]: Shim, + [InstrumentationDescriptor.TYPE_DATASTORE]: require('./datastore-shim'), + [InstrumentationDescriptor.TYPE_MESSAGE]: require('./message-shim'), + [InstrumentationDescriptor.TYPE_PROMISE]: require('./promise-shim'), + [InstrumentationDescriptor.TYPE_TRANSACTION]: require('./transaction-shim'), + [InstrumentationDescriptor.TYPE_WEB_FRAMEWORK]: require('./webframework-shim') } /** @@ -37,29 +37,30 @@ class ConglomerateShim extends Shim { } get GENERIC() { - return MODULE_TYPE.GENERIC + return InstrumentationDescriptor.TYPE_GENERIC } get DATASTORE() { - return MODULE_TYPE.DATASTORE + return InstrumentationDescriptor.TYPE_DATASTORE } get MESSAGE() { - return MODULE_TYPE.MESSAGE + return InstrumentationDescriptor.TYPE_MESSAGE } get PROMISE() { - return MODULE_TYPE.PROMISE + return InstrumentationDescriptor.TYPE_PROMISE } get TRANSACTION() { - return MODULE_TYPE.TRANSACTION + return InstrumentationDescriptor.TYPE_TRANSACTION } get WEB_FRAMEWORK() { - return MODULE_TYPE.WEB_FRAMEWORK + return InstrumentationDescriptor.TYPE_WEB_FRAMEWORK } /** * Constructs a new `Shim` of the specified type for instrumenting submodules * of the conglomerate module. * - * @param {MODULE_TYPE} type - The type of shim to construct. + * @param {string} type - The type of shim to construct. Utilize the static + * type fields on {@link InstrumentationDescriptor}. * @param {string} submodule - The name of the submodule this will instrument. * @returns {Shim} A new shim of the given type. */ diff --git a/lib/shim/constants.js b/lib/shim/constants.js deleted file mode 100644 index 8fa34ead9a..0000000000 --- a/lib/shim/constants.js +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright 2020 New Relic Corporation. All rights reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -'use strict' - -/** - * Enumeration of module instrumentation types. - * - * @private - * @readonly - * @enum {string} - */ -exports.MODULE_TYPE = { - /** Utility/generic module, such as pooling libraries. */ - GENERIC: 'generic', - - /** @private */ - CONGLOMERATE: 'conglomerate', - - /** Database module, such as the MongoDB or MySQL drivers. */ - DATASTORE: 'datastore', - - /** Messaging module, such as AMQP */ - MESSAGE: 'message', - - /** Promise module, such as Bluebird */ - PROMISE: 'promise', - - /** @private */ - TRANSACTION: 'transaction', - - /** Web server framework module, such as Express or Restify. */ - WEB_FRAMEWORK: 'web-framework', - - /** - * Used to load supportability metrics on installed verisions of packages - * that the Node.js agent does not instrument(i.e. - otel instrumentation or top logging libraries) - */ - TRACKING: 'tracking' -} diff --git a/lib/shim/index.js b/lib/shim/index.js index 0a6da77b8c..572627fdf0 100644 --- a/lib/shim/index.js +++ b/lib/shim/index.js @@ -5,8 +5,6 @@ 'use strict' -const constants = require('./constants') - const Shim = require('./shim') const ConglomerateShim = require('./conglomerate-shim') const DatastoreShim = require('./datastore-shim') @@ -15,14 +13,15 @@ const PromiseShim = require('./promise-shim') const TransactionShim = require('./transaction-shim') const WebFrameworkShim = require('./webframework-shim') const properties = require('../util/properties') +const InstrumentationDescriptor = require('../instrumentation-descriptor') const SHIM_TYPE_MAP = Object.create(null) -SHIM_TYPE_MAP[constants.MODULE_TYPE.GENERIC] = Shim -SHIM_TYPE_MAP[constants.MODULE_TYPE.CONGLOMERATE] = ConglomerateShim -SHIM_TYPE_MAP[constants.MODULE_TYPE.DATASTORE] = DatastoreShim -SHIM_TYPE_MAP[constants.MODULE_TYPE.MESSAGE] = MessageShim -SHIM_TYPE_MAP[constants.MODULE_TYPE.PROMISE] = PromiseShim -SHIM_TYPE_MAP[constants.MODULE_TYPE.TRANSACTION] = TransactionShim -SHIM_TYPE_MAP[constants.MODULE_TYPE.WEB_FRAMEWORK] = WebFrameworkShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_GENERIC] = Shim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_CONGLOMERATE] = ConglomerateShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_DATASTORE] = DatastoreShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_MESSAGE] = MessageShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_PROMISE] = PromiseShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_TRANSACTION] = TransactionShim +SHIM_TYPE_MAP[InstrumentationDescriptor.TYPE_WEB_FRAMEWORK] = WebFrameworkShim /** * @@ -46,7 +45,6 @@ function createShimFromType({ type, agent, moduleName, resolvedName, shimName, p return shim } -exports.constants = constants exports.Shim = Shim exports.ConglomerateShim = ConglomerateShim exports.DatastoreShim = DatastoreShim diff --git a/lib/shimmer.js b/lib/shimmer.js index cb7e1f9d6b..c92d4a99f1 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -22,51 +22,49 @@ const NAMES = require('./metrics/names') const symbols = require('./symbols') const { unsubscribe } = require('./instrumentation/undici') -const MODULE_TYPE = shims.constants.MODULE_TYPE - const CORE_INSTRUMENTATION = { child_process: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'child_process.js' }, crypto: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'crypto.js' }, // domain: { // XXX Do not include domains in this list! The - // type: MODULE_TYPE.GENERIC, // core instrumentations are run at startup by + // type: InstrumentationDescriptor.TYPE_GENERIC, // core instrumentations are run at startup by // file: 'domain.js' // requiring each of their modules. Loading // }, // `domain` has side effects that we try to avoid. dns: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'dns.js' }, fs: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'fs.js' }, http: { - type: MODULE_TYPE.TRANSACTION, + type: InstrumentationDescriptor.TYPE_TRANSACTION, file: 'http.js' }, https: { - type: MODULE_TYPE.TRANSACTION, + type: InstrumentationDescriptor.TYPE_TRANSACTION, file: 'http.js' }, inspector: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'inspector.js' }, net: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'net.js' }, timers: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'timers.js' }, zlib: { - type: MODULE_TYPE.GENERIC, + type: InstrumentationDescriptor.TYPE_GENERIC, file: 'zlib.js' } } @@ -335,7 +333,7 @@ const shimmer = (module.exports = { // We register this as core and it'll work for both fetch and undici const undiciPath = path.join(__dirname, 'instrumentation', 'undici.js') const undiciShim = shims.createShimFromType({ - type: MODULE_TYPE.TRANSACTION, + type: InstrumentationDescriptor.TYPE_TRANSACTION, agent, moduleName: 'undici', resolvedName: '.' @@ -577,7 +575,7 @@ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver // Tracking instrumentation is only used to add the supportability metrics // that occur directly above this. No reason to attempt to load instrumentation // as it does not exist. - if (instrumentation.type === MODULE_TYPE.TRACKING) { + if (instrumentation.type === InstrumentationDescriptor.TYPE_TRACKING) { shimmer.registeredInstrumentations.setHookSuccess(trackedInstrumentation) return } From f599f14b00a933a6557fbd9ebf67ac7025f37ff8 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 3 Apr 2024 08:41:31 -0400 Subject: [PATCH 3/6] remove unnecessary ternary --- lib/shimmer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/shimmer.js b/lib/shimmer.js index c92d4a99f1..c084a447c5 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -547,7 +547,7 @@ function applyDebugState(shim, nodule, inEsm) { */ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver) { // default to Node.js version, this occurs for core libraries - const pkgVersion = resolvedName ? shimmer.getPackageVersion(moduleName) : process.version + const pkgVersion = shimmer.getPackageVersion(moduleName) const trackedInstrumentations = shimmer.registeredInstrumentations.getAllByName(moduleName) trackedInstrumentations.forEach((trackedInstrumentation) => { const isInstrumented = trackedInstrumentation.meta.instrumented === true From 1438e538b68fd163a042316348c4502242a20b00 Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 3 Apr 2024 08:48:41 -0400 Subject: [PATCH 4/6] fix isInstrumented --- lib/shimmer.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/shimmer.js b/lib/shimmer.js index c084a447c5..74e8c83afd 100644 --- a/lib/shimmer.js +++ b/lib/shimmer.js @@ -467,7 +467,7 @@ const shimmer = (module.exports = { }, /** - * Checks all registered instrumentation for a module and returns true + * Checks all registered instrumentations for a module and returns true * only if every hook succeeded. * * @param {string} moduleName name of registered instrumentation @@ -476,13 +476,12 @@ const shimmer = (module.exports = { * module */ isInstrumented(moduleName, resolvedName) { - const items = shimmer.registeredInstrumentations - .getAllByName(moduleName) - .filter( - (item) => - item.instrumentation.resolvedName === resolvedName && item.meta.instrumented === true - ) - return items.length > 0 + const allItems = shimmer.registeredInstrumentations.getAllByName(moduleName) + const items = allItems.filter( + (item) => + item.instrumentation.resolvedName === resolvedName && item.meta.instrumented === true + ) + return items.length === allItems.length }, instrumentPostLoad(agent, module, moduleName, resolvedName, returnModule = false) { From 01e54dee3c0441d9e2d0a05a4a3eab187aa87eca Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 3 Apr 2024 10:00:43 -0400 Subject: [PATCH 5/6] add in original replication as a test --- .../shimmer/module-load-fixture-errored.js | 9 --- .../shimmer/module-load-fixture.js | 9 --- test/integration/shimmer/reinstrument.tap.js | 56 +++++++++++++++++++ test/integration/shimmer/testdata/.gitignore | 1 + test/integration/shimmer/testdata/index.js | 11 ++++ .../testdata/node_modules/person/index.js | 21 +++++++ .../person/node_modules/human/index.js | 18 ++++++ .../human/node_modules/test-logger/index.js | 17 ++++++ .../node_modules/test-logger/package.json | 3 + .../person/node_modules/test-logger/index.js | 17 ++++++ .../node_modules/test-logger/package.json | 3 + 11 files changed, 147 insertions(+), 18 deletions(-) delete mode 100644 test/integration/shimmer/module-load-fixture-errored.js delete mode 100644 test/integration/shimmer/module-load-fixture.js create mode 100644 test/integration/shimmer/reinstrument.tap.js create mode 100644 test/integration/shimmer/testdata/.gitignore create mode 100644 test/integration/shimmer/testdata/index.js create mode 100644 test/integration/shimmer/testdata/node_modules/person/index.js create mode 100644 test/integration/shimmer/testdata/node_modules/person/node_modules/human/index.js create mode 100644 test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/index.js create mode 100644 test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/package.json create mode 100644 test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/index.js create mode 100644 test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/package.json diff --git a/test/integration/shimmer/module-load-fixture-errored.js b/test/integration/shimmer/module-load-fixture-errored.js deleted file mode 100644 index 16964d89ed..0000000000 --- a/test/integration/shimmer/module-load-fixture-errored.js +++ /dev/null @@ -1,9 +0,0 @@ -/* - * Copyright 2020 New Relic Corporation. All rights reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -'use strict' -module.exports = () => { - return 'hello world' -} diff --git a/test/integration/shimmer/module-load-fixture.js b/test/integration/shimmer/module-load-fixture.js deleted file mode 100644 index 16964d89ed..0000000000 --- a/test/integration/shimmer/module-load-fixture.js +++ /dev/null @@ -1,9 +0,0 @@ -/* - * Copyright 2020 New Relic Corporation. All rights reserved. - * SPDX-License-Identifier: Apache-2.0 - */ - -'use strict' -module.exports = () => { - return 'hello world' -} diff --git a/test/integration/shimmer/reinstrument.tap.js b/test/integration/shimmer/reinstrument.tap.js new file mode 100644 index 0000000000..7831f17259 --- /dev/null +++ b/test/integration/shimmer/reinstrument.tap.js @@ -0,0 +1,56 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +const tap = require('tap') +const helper = require('../../lib/agent_helper') + +tap.beforeEach((t) => { + t.context.agent = helper.instrumentMockedAgent({}) +}) + +tap.afterEach((t) => { + helper.unloadAgent(t.context.agent) +}) + +tap.test('can instrument the same module from multiple installs', (t) => { + t.plan(3) + + const { agent } = t.context + agent.start(() => { + const api = helper.getAgentApi() + + let instrumentedCount = 0 + api.instrument('test-logger', (shim, mod) => { + shim.wrap(mod.prototype, 'info', (shim, fn) => { + return function wrappedInfo() { + instrumentedCount += 1 + return fn.apply(this, arguments) + } + }) + }) + + const { Writable } = require('stream') + const Person = require('./testdata/index') + + const lines = [] + const dest = new Writable({ + write(chunk, _, cb) { + lines.push(chunk.toString()) + cb() + } + }) + const person = new Person(dest) + t.equal(person.isHuman, true) + t.same(lines, ['human constructed\n', 'person constructed\n']) + + // We loaded the same module from two different installed paths. + // Thus, we should have two instrumentations. + t.equal(instrumentedCount, 2) + + t.end() + }) +}) diff --git a/test/integration/shimmer/testdata/.gitignore b/test/integration/shimmer/testdata/.gitignore new file mode 100644 index 0000000000..cf4bab9ddd --- /dev/null +++ b/test/integration/shimmer/testdata/.gitignore @@ -0,0 +1 @@ +!node_modules diff --git a/test/integration/shimmer/testdata/index.js b/test/integration/shimmer/testdata/index.js new file mode 100644 index 0000000000..947219665b --- /dev/null +++ b/test/integration/shimmer/testdata/index.js @@ -0,0 +1,11 @@ +/* + * Copyright 2024 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +'use strict' + +// eslint-disable-next-line node/no-extraneous-require +const Person = require('person') + +module.exports = Person diff --git a/test/integration/shimmer/testdata/node_modules/person/index.js b/test/integration/shimmer/testdata/node_modules/person/index.js new file mode 100644 index 0000000000..ce1272e3e8 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/index.js @@ -0,0 +1,21 @@ +'use strict' + +const Logger = require('test-logger') +const Human = require('human') + +class Person extends Human { + #log + + constructor(logStream) { + super(logStream) + this.#log = new Logger(logStream) + + this.#log.info('person constructed') + } + + get isPerson() { + return true + } +} + +module.exports = Person diff --git a/test/integration/shimmer/testdata/node_modules/person/node_modules/human/index.js b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/index.js new file mode 100644 index 0000000000..d719c7e651 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/index.js @@ -0,0 +1,18 @@ +'use strict' + +const Logger = require('test-logger') + +class Human { + #log + + constructor(logStream) { + this.#log = new Logger(logStream) + this.#log.info('human constructed') + } + + get isHuman() { + return true + } +} + +module.exports = Human diff --git a/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/index.js b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/index.js new file mode 100644 index 0000000000..af10bcab03 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/index.js @@ -0,0 +1,17 @@ +'use strict' + +class Logger { + #stream = process.stdout + + constructor(stream) { + if (stream) { + this.#stream = stream + } + } + + info(msg) { + this.#stream.write(msg + "\n") + } +} + +module.exports = Logger diff --git a/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/package.json b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/package.json new file mode 100644 index 0000000000..7d6b068325 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/node_modules/human/node_modules/test-logger/package.json @@ -0,0 +1,3 @@ +{ + "version": "999.0.0" +} diff --git a/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/index.js b/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/index.js new file mode 100644 index 0000000000..af10bcab03 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/index.js @@ -0,0 +1,17 @@ +'use strict' + +class Logger { + #stream = process.stdout + + constructor(stream) { + if (stream) { + this.#stream = stream + } + } + + info(msg) { + this.#stream.write(msg + "\n") + } +} + +module.exports = Logger diff --git a/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/package.json b/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/package.json new file mode 100644 index 0000000000..7d6b068325 --- /dev/null +++ b/test/integration/shimmer/testdata/node_modules/person/node_modules/test-logger/package.json @@ -0,0 +1,3 @@ +{ + "version": "999.0.0" +} From 78e384c839f376c2ba8b1fc77a78fb573eb0722a Mon Sep 17 00:00:00 2001 From: James Sumners Date: Wed, 3 Apr 2024 10:01:40 -0400 Subject: [PATCH 6/6] revert unnecessary workflow changes --- .github/workflows/ci-workflow.yml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci-workflow.yml b/.github/workflows/ci-workflow.yml index e4a486975e..d70533926d 100644 --- a/.github/workflows/ci-workflow.yml +++ b/.github/workflows/ci-workflow.yml @@ -35,7 +35,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run Linting run: npm run lint - name: Inspect Lockfile @@ -58,7 +58,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run CI Script Unit Tests run: npm run unit:scripts @@ -79,7 +79,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run Unit Tests run: npm run unit - name: Archive Unit Test Coverage @@ -104,7 +104,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run Docker Services run: npm run services - name: Run Integration Tests @@ -132,7 +132,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run Docker Services run: npm run services - name: Run Versioned Tests @@ -161,7 +161,7 @@ jobs: with: name: logs-${{ matrix.node-version }}.tgz path: ./logs-${{ matrix.node-version }}.tgz - + # There is no coverage for external as that's tracked in their respective repos versioned-external: needs: skip_if_release @@ -180,7 +180,7 @@ jobs: with: node-version: ${{ matrix.node-version }} - name: Install Dependencies - run: npm ci --verbose + run: npm ci - name: Run Versioned Tests run: TEST_CHILD_TIMEOUT=600000 npm run versioned:external env: