fix: prevent ESM loader from loading instrumentation multiple times (#…

…1729)
newrelic · Jul 18, 2023 · 962d172 · 962d172
1 parent 0b96de3
commit 962d172
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 1 deletion.
diff --git a/esm-loader.mjs b/esm-loader.mjs
@@ -66,9 +66,16 @@ export async function resolve(specifier, context, nextResolve) {
     const { url, format } = resolvedModule
     logger.debug(`Instrumentation exists for ${specifier} ${format} package.`)
 
-    if (format === 'commonjs') {
+    if (registeredSpecifiers.get(url)) {
+      logger.debug(
+        `Instrumentation already registered for ${specifier} under ${fileURLToPath(
+          url
+        )}, skipping resolve hook...`
+      )
+    } else if (format === 'commonjs') {
       // ES Modules translate import statements into fully qualified filepaths, so we create a copy of our instrumentation under this filepath
       const instrumentationDefinitionCopy = [...instrumentationDefinition]
+
       instrumentationDefinitionCopy.forEach((copy) => {
         // Stripping the prefix is necessary because the code downstream gets this url without it
         copy.moduleName = fileURLToPath(url)
@@ -80,6 +87,9 @@ export async function resolve(specifier, context, nextResolve) {
           `Registered CommonJS instrumentation for ${specifier} under ${copy.moduleName}`
         )
       })
+
+      // Keep track of what we've registered so we don't double register (see: https://github.com/newrelic/node-newrelic/issues/1646)
+      registeredSpecifiers.set(url, specifier)
     } else if (format === 'module') {
       registeredSpecifiers.set(url, specifier)
       const modifiedUrl = new URL(url)

diff --git a/test/unit/esm-loader.test.mjs b/test/unit/esm-loader.test.mjs
@@ -231,6 +231,52 @@ tap.test('ES Module Loader', { skip: !esmHelpers.supportedLoaderVersion() }, (t)
     }
   )
 
+  t.test('should register a CommonJS instrumentation only once', async (t) => {
+    fakeShimmer.getInstrumentationNameFromModuleName.returnsArg(0)
+    fakeShimmer.registeredInstrumentations['my-test-dep'] = [
+      {
+        moduleName: 'my-test-dep',
+        type: 'generic',
+        onRequire: sinon.stub()
+      }
+    ]
+    fakeNextResolve.returns({ url: 'file:///path/to/my-test-dep/index.js', format: 'commonjs' })
+
+    await loader.resolve(fakeSpecifier, fakeContext, fakeNextResolve)
+
+    t.equal(
+      fakeShimmer.registerInstrumentation.callCount,
+      1,
+      'should have registered instrumentation only once'
+    )
+    t.equal(
+      loader.registeredSpecifiers.get('file:///path/to/my-test-dep/index.js'),
+      'my-test-dep',
+      'should have added instrumentation to registered list'
+    )
+    t.ok(
+      fakeLoggerChild.debug.calledWith(
+        'Registered CommonJS instrumentation for my-test-dep under /path/to/my-test-dep/index.js'
+      ),
+      'should log debug about instrumentation registration'
+    )
+
+    await loader.resolve(fakeSpecifier, fakeContext, fakeNextResolve)
+
+    t.ok(
+      fakeLoggerChild.debug.calledWith(
+        'Instrumentation already registered for my-test-dep under /path/to/my-test-dep/index.js, skipping resolve hook...'
+      ),
+      'should log debug about not double instrumenting'
+    )
+
+    t.equal(
+      fakeShimmer.registerInstrumentation.callCount,
+      1,
+      'registerInstrumentation should not have been called again'
+    )
+  })
+
   t.test('should register CJS instrumentation if url has urlencoded characters', async (t) => {
     fakeShimmer.getInstrumentationNameFromModuleName.returnsArg(0)
     fakeShimmer.registeredInstrumentations['my-test-dep'] = [