fix: Updated shimmer to allow registering instrumentation for different versions of the same module

THIRD_PARTY_NOTICES.md

@@ -26,6 +26,7 @@ code, the source code can be found at [https://github.com/newrelic/node-newrelic
 * [import-in-the-middle](#import-in-the-middle)
 * [json-bigint](#json-bigint)
 * [json-stringify-safe](#json-stringify-safe)
+* [module-details-from-path](#module-details-from-path)
 * [readable-stream](#readable-stream)
 * [require-in-the-middle](#require-in-the-middle)
 * [semver](#semver)
@@ -1332,6 +1333,35 @@ IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 ```
 
+### module-details-from-path
+
+This product includes source derived from [module-details-from-path](https://github.com/watson/module-details-from-path) ([v1.0.3](https://github.com/watson/module-details-from-path/tree/v1.0.3)), distributed under the [MIT License](https://github.com/watson/module-details-from-path/blob/v1.0.3/LICENSE):
+
+```
+The MIT License (MIT)
+
+Copyright (c) 2016 Thomas Watson Steen
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+
+```
+
 ### readable-stream
 
 This product includes source derived from [readable-stream](https://github.com/nodejs/readable-stream) ([v3.6.2](https://github.com/nodejs/readable-stream/tree/v3.6.2)), distributed under the [MIT License](https://github.com/nodejs/readable-stream/blob/v3.6.2/LICENSE):

api.js

@@ -29,6 +29,7 @@ const MODULE_TYPE = require('./lib/shim/constants').MODULE_TYPE
 const NAMES = require('./lib/metrics/names')
 const obfuscate = require('./lib/util/sql/obfuscate')
 const { DESTINATIONS } = require('./lib/config/attribute-filter')
+const parse = require('module-details-from-path')
 
 /*
  *
@@ -1495,8 +1496,9 @@ API.prototype.instrumentLoadedModule = function instrumentLoadedModule(moduleNam
 
   try {
     const resolvedName = require.resolve(moduleName)
+    const parsed = parse(resolvedName)
 
-    return shimmer.instrumentPostLoad(this.agent, module, moduleName, resolvedName)
+    return shimmer.instrumentPostLoad(this.agent, module, moduleName, parsed.basedir)
   } catch (error) {
     logger.error('instrumentLoadedModule encountered an error, module not instrumented: %s', error)
   }

lib/shimmer.js

@@ -10,7 +10,6 @@ const semver = require('semver')
 const fs = require('./util/unwrapped-core').fs
 const logger = require('./logger').child({ component: 'shimmer' })
 const INSTRUMENTATIONS = require('./instrumentations')()
-const properties = require('./util/properties')
 const shims = require('./shim')
 const { Hook } = require('require-in-the-middle')
 const IitmHook = require('import-in-the-middle')
@@ -92,7 +91,7 @@ const shimmer = (module.exports = {
    * Detects if the given function has already been wrapped.
    *
    * @param {Function} fn - The function to look for a wrapper on.
-   * @returns {bool} True if `fn` exists and has an attached original, else false.
+   * @returns {boolean} True if `fn` exists and has an attached original, else false.
    */
   isWrapped: function isWrapped(fn) {
     return !!(fn && fn[symbols.original])
@@ -183,7 +182,7 @@ const shimmer = (module.exports = {
    *                              helpful than you'd think.
    * @param {string}   property   The property to replace with the accessor.
    * @param {Function} options    Optional getter and setter to use for the accessor.
-   * @returns {object} The original value of the property.
+   * @returns {object|undefined} returns original function
    */
   wrapDeprecated: function wrapDeprecated(nodule, noduleName, property, options) {
     if (!property) {
@@ -396,6 +395,8 @@ const shimmer = (module.exports = {
       }
     }
 
+    opts[symbols.instrumented] = new Set()
+    opts[symbols.instrumentedErrored] = new Set()
     shimmer.registeredInstrumentations[opts.moduleName].push({ ...opts })
   },
 
@@ -418,8 +419,9 @@ const shimmer = (module.exports = {
    *
    * Use this to re-apply any applicable instrumentation.
    *
-   * @param agent
-   * @param modulePath
+   * @param {object} agent NR agent
+   * @param {string} modulePath path to module getting required
+   * @returns {object} exported module
    */
   reinstrument: function reinstrument(agent, modulePath) {
     return _postLoad(agent, require(modulePath), modulePath)
@@ -430,7 +432,8 @@ const shimmer = (module.exports = {
    * instrumentation.  These two things are usually, but not always,
    * the same.
    *
-   * @param moduleName
+   * @param {string} moduleName name of module getting instrumented
+   * @returns {string} name unless pg.js and then returns pg
    */
   getInstrumentationNameFromModuleName(moduleName) {
     // XXX When updating these special cases, also update `uninstrumented`.
@@ -447,12 +450,13 @@ const shimmer = (module.exports = {
    * only if every hook succeeded.
    *
    * @param {string} moduleName name of registered instrumentation
-   * @returns {boolean}
+   * @returns {boolean} if all instrumentation hooks ran for a given version
    */
   isInstrumented(moduleName) {
+    const pkgVersion = shimmer.getPackageVersion(moduleName)
     const instrumentations = shimmer.registeredInstrumentations[moduleName]
-    const didInstrument = instrumentations.filter(
-      (instrumentation) => instrumentation[symbols.instrumented]
+    const didInstrument = instrumentations.filter((instrumentation) =>
+      instrumentation[symbols.instrumented].has(pkgVersion)
     )
     return didInstrument.length === instrumentations.length
   },
@@ -463,6 +467,23 @@ const shimmer = (module.exports = {
     // previously it would just call instrumentation
     // and not check the result
     return returnModule ? result : shimmer.isInstrumented(moduleName)
+  },
+
+  /**
+   * Gets the version of a given package by parsing it from package.json
+   *
+   * @param {string} moduleName name of module
+   * @returns {string} version, defaults to Node.js version when it cannot parse
+   */
+  getPackageVersion(moduleName) {
+    try {
+      const { basedir } = shimmer.registeredInstrumentations[moduleName]
+      const pkg = require(path.resolve(basedir, 'package.json'))
+      return pkg.version
+    } catch (err) {
+      logger.debug('Failed to get version for `%s`, reason: %s', moduleName, err.message)
+      return process.version
+    }
   }
 })
 
@@ -486,23 +507,23 @@ function applyDebugState(shim, nodule, inEsm) {
  * initialization function that takes the agent and the module to be
  * instrumented.
  *
- * @param agent
- * @param nodule
- * @param moduleName
- * @param resolvedName
- * @param esmResolver
+ * @param {object} agent NR agent
+ * @param {object} nodule Class or module containing the function to wrap.
+ * @param {string} moduleName name of module
+ * @param {string} resolvedName fully qualified path to module
+ * @param {boolean} esmResolver indicates if it came from esm loader
+ * @returns {object|undefined} returns exported module unless already instrumented
  */
 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) => {
-    if (
-      properties.hasOwn(instrumentation, symbols.instrumented) ||
-      properties.hasOwn(instrumentation, symbols.instrumentedErrored)
-    ) {
-      logger.trace(
-        'Already instrumented or failed to instrument %s, skipping redundant instrumentation',
-        moduleName
-      )
+    const isInstrumented = instrumentation[symbols.instrumented].has(pkgVersion)
+    const failedInstrumentation = instrumentation[symbols.instrumentedErrored].has(pkgVersion)
+    if (isInstrumented || failedInstrumentation) {
+      const msg = isInstrumented ? 'Already instrumented' : 'Failed to instrument'
+      logger.trace(`${msg} ${moduleName}@${pkgVersion}, skipping registering instrumentation`)
       return
     }
 
@@ -530,38 +551,71 @@ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver
     // 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] = true
+      instrumentation[symbols.instrumented].add(pkgVersion)
       return
     }
 
-    try {
-      if (instrumentation.onRequire(shim, resolvedNodule, moduleName) !== false) {
-        nodule = shim.getExport(nodule)
-        instrumentation[symbols.instrumented] = true
-      }
-    } catch (instrumentationError) {
-      instrumentation[symbols.instrumentedErrored] = true
-      if (instrumentation.onError) {
-        try {
-          instrumentation.onError(instrumentationError)
-        } catch (e) {
-          logger.warn(
-            e,
-            instrumentationError,
-            'Custom instrumentation for %s failed, then the onError handler threw an error',
-            moduleName
-          )
-        }
-      } else {
+    nodule = loadInstrumentation({
+      shim,
+      resolvedNodule,
+      pkgVersion,
+      moduleName,
+      nodule,
+      instrumentation
+    })
+  })
+
+  return nodule
+}
+
+/**
+ * Attempts to execute an onRequire hook for a given module.
+ * If it fails it will call an onError hook and log warnings accordingly
+ *
+ * @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 {string} params.moduleName module name
+ * @param {object} params.instrumentation hooks for a give module
+ * @returns {object} updated xport module
+ */
+function loadInstrumentation({
+  shim,
+  resolvedNodule,
+  pkgVersion,
+  moduleName,
+  nodule,
+  instrumentation
+}) {
+  try {
+    if (instrumentation.onRequire(shim, resolvedNodule, moduleName) !== false) {
+      nodule = shim.getExport(nodule)
+      instrumentation[symbols.instrumented].add(pkgVersion)
+    }
+  } catch (instrumentationError) {
+    instrumentation[symbols.instrumentedErrored].add(pkgVersion)
+    if (instrumentation.onError) {
+      try {
+        instrumentation.onError(instrumentationError)
+      } catch (e) {
         logger.warn(
+          e,
           instrumentationError,
-          'Custom instrumentation for %s failed. Please report this to the ' +
-            'maintainers of the custom instrumentation.',
+          'Custom instrumentation for %s failed, then the onError handler threw an error',
           moduleName
         )
       }
+    } else {
+      logger.warn(
+        instrumentationError,
+        'Custom instrumentation for %s failed. Please report this to the ' +
+          'maintainers of the custom instrumentation.',
+        moduleName
+      )
     }
-  })
+  }
 
   return nodule
 }
@@ -589,10 +643,12 @@ function _postLoad(agent, nodule, name, resolvedName, esmResolver) {
   const hasPostLoadInstrumentation =
     registeredInstrumentation &&
     registeredInstrumentation.length &&
-    registeredInstrumentation.filter((instrumentation) => instrumentation.onRequire).length
+    registeredInstrumentation.filter((hook) => hook.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
     logger.trace('Instrumenting %s with onRequire (module loaded) hook.', name)
     return instrumentPostLoad(agent, nodule, instrumentation, resolvedName, esmResolver)
   }

package.json

@@ -165,9 +165,9 @@
     "versioned-tests": "./bin/run-versioned-tests.sh",
     "update-changelog-version": "node ./bin/update-changelog-version",
     "checkout-external-versioned": "node ./test/versioned-external/checkout-external-tests.js",
-    "versioned:internal:major": "VERSIONED_MODE=--major npm run versioned:internal", 
+    "versioned:internal:major": "VERSIONED_MODE=--major npm run versioned:internal",
     "versioned:internal": "npm run prepare-test && EXTERNAL_MODE=none time ./bin/run-versioned-tests.sh",
-    "versioned:external:major": "VERSIONED_MODE=--major npm run versioned:external", 
+    "versioned:external:major": "VERSIONED_MODE=--major npm run versioned:external",
     "versioned:external": "npm run checkout-external-versioned && SKIP_C8=true EXTERNAL_MODE=only time ./bin/run-versioned-tests.sh",
     "versioned:major": "VERSIONED_MODE=--major npm run versioned",
     "versioned": "npm run checkout-external-versioned && npm run prepare-test && time ./bin/run-versioned-tests.sh",
@@ -193,6 +193,7 @@
     "import-in-the-middle": "^1.4.2",
     "json-bigint": "^1.0.0",
     "json-stringify-safe": "^5.0.0",
+    "module-details-from-path": "^1.0.3",
     "readable-stream": "^3.6.1",
     "require-in-the-middle": "^7.2.0",
     "semver": "^7.5.2",

test/integration/module-loading/module-loading.tap.js

@@ -15,6 +15,7 @@ const { FEATURES } = require('../../../lib/metrics/names')
 
 const CUSTOM_MODULE = 'customTestPackage'
 const CUSTOM_MODULE_PATH = `./node_modules/${CUSTOM_MODULE}`
+const CUSTOM_MODULE_PATH_SUB = `./node_modules/subPkg/node_modules/${CUSTOM_MODULE}`
 const EXPECTED_REQUIRE_METRIC_NAME = `${FEATURES.INSTRUMENTATION.ON_REQUIRE}/${CUSTOM_MODULE}`
 
 tap.test('Should properly track module paths to enable shim.require()', function (t) {
@@ -32,9 +33,6 @@ tap.test('Should properly track module paths to enable shim.require()', function
     onRequire: () => {}
   })
 
-  // As of node 11, this path is being cached, and will not hit our resolve hooks for
-  // subsequent calls.  This extra require call ensures we cover the cached case.
-  require(CUSTOM_MODULE_PATH)
   const mycustomPackage = require(CUSTOM_MODULE_PATH)
 
   const shim = mycustomPackage[symbols.shim]
@@ -48,6 +46,31 @@ tap.test('Should properly track module paths to enable shim.require()', function
   t.equal(shimLoadedCustom.name, 'customFunction', 'Should grab correct module')
 })
 
+tap.test('should instrument multiple versions of the same package', function (t) {
+  t.autoend()
+
+  let agent = helper.instrumentMockedAgent()
+
+  t.teardown(() => {
+    helper.unloadAgent(agent)
+    agent = null
+  })
+
+  const instrumentation = {
+    moduleName: CUSTOM_MODULE,
+    onRequire: () => {}
+  }
+
+  shimmer.registerInstrumentation(instrumentation)
+
+  const pkg1 = require(CUSTOM_MODULE_PATH)
+  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'))
+})
+
 tap.test('should only log supportability metric for tracking type instrumentation', function (t) {
   t.autoend()