fix: Updated shimmer to handle instrumenting named and default exports of CommonJS modules in ESM

bin/macos-firewall.sh

@@ -0,0 +1,26 @@
+#!/bin/sh
+# Script that adds rules to Mac OS X Socket Firewall to avoid
+# popups asking to accept incoming network connections when
+# running tests.
+#
+# Originally from https://github.com/nodejs/node/blob/5398cb55ca10d34ed7ba5592f95b3b9f588e5754/tools/macos-firewall.sh
+
+SFW="/usr/libexec/ApplicationFirewall/socketfilterfw"
+NODE_PATH=$(realpath $(which node))
+
+add_and_unblock () {
+  if [ -e "$1" ]
+  then
+    echo Processing "$1"
+    $SFW --remove "$1" >/dev/null
+    $SFW --add "$1"
+    $SFW --unblock "$1"
+  fi
+}
+
+if [ -f $SFW ];
+then
+  add_and_unblock "$NODE_PATH"
+else
+  echo "SocketFirewall not found in location: $SFW"
+fi

documentation/developer-setup.md

@@ -43,3 +43,23 @@ There are a variety of ways to get the latest changes into your local branches.
 
 1. `git checkout main`
 2. `git pull upstream main`
+
+### Testing
+
+In addition to basic unit tests that can be run with `npm run unit`, we have
+a set of "versioned" integration style tests. See the [package.json](../package.json)
+for the full set of scripts, but a short list of common scenarios is:
+
++ `npm run versioned:internal` - to run all versioned tests across all supported
+versions of each instrumented module.
++ `npm run versioned:internal:major` - to run all versioned tests for each
+current major release of each instrumented module.
++ `npm run versioned:internal:major foo` - to run a specific versioned test
+for the latest major version of the instrumented module named "foo" (as an
+example).
+
+Note: when running the versioned test suite on a macOS system, the application
+firewall is likely to issue multiple requests to authorize the `node` binary
+for incoming connections. This can be avoided by running the
+[macos-firewall.sh](../bin/macos-firewall.sh) script to prime the application
+firewall with a rule to allow the connections.

lib/instrumentation/pg.js

@@ -5,6 +5,8 @@
 
 'use strict'
 
+const { nrEsmProxy } = require('../symbols')
+
 function getQuery(shim, original, name, args) {
   const config = args[0]
   let statement
@@ -78,27 +80,52 @@ module.exports = function initialize(agent, pgsql, moduleName, shim) {
     })
   }
 
-  // The pg module defines "native" getter which sets up the native client lazily
-  // (only when called).  We replace the getter, so that we can instrument the native
-  // client.  The original getter replaces itself with the instance of the native
-  // client, so only instrument if the getter exists (otherwise assume already
-  // instrumented).
-  const origGetter = pgsql.__lookupGetter__('native')
-  if (origGetter) {
-    delete pgsql.native
-    pgsql.__defineGetter__('native', function getNative() {
-      const temp = origGetter()
-      if (temp != null) {
-        instrumentPGNative(temp)
-      }
-      return temp
-    })
-  }
+  updateNative(pgsql, instrumentPGNative)
 
   // wrapping for JS
   shim.recordQuery(pgsql && pgsql.Client && pgsql.Client.prototype, 'query', wrapJSClientQuery)
 }
 
+/**
+ * Determines if the `pg` module is a plain CJS module or a CJS module that
+ * has been imported via ESM's import. After making the determination, it will
+ * update the `native` export of the module to be instrumented.
+ *
+ * @param {object} pg The module to inspect.
+ * @param {Function} instrumentPGNative A function that will apply instrumentation
+ * to the `native` export.
+ */
+function updateNative(pg, instrumentPGNative) {
+  if (pg[nrEsmProxy] === true) {
+    // When pg is imported via an ESM import statement, then our proxy will
+    // make our non-ESM native getter wrapper not work correctly. Basically,
+    // the getter will get evaluated by the proxy, and we never gain access to
+    // replace the getter with our own implementation. Luckily, we get to
+    // simplify in this scenario.
+    const native = pg.default.native
+    if (native !== null) {
+      instrumentPGNative(native)
+    }
+  } else {
+    // The pg module defines a "native" getter which sets up the native client lazily
+    // (only when called).  We replace the getter, so that we can instrument the native
+    // client.  The original getter replaces itself with the instance of the native
+    // client, so only instrument if the getter exists (otherwise assume already
+    // instrumented).
+    const origGetter = pg.__lookupGetter__('native')
+    if (origGetter) {
+      delete pg.native
+      pg.__defineGetter__('native', function getNative() {
+        const temp = origGetter()
+        if (temp != null) {
+          instrumentPGNative(temp)
+        }
+        return temp
+      })
+    }
+  }
+}
+
 function getInstanceParameters(shim, client) {
   return {
     host: client.host || null,

lib/instrumentation/winston.js

@@ -101,7 +101,12 @@ function registerFormatter({ opts, agent, winston }) {
   if (opts.format) {
     opts.format = winston.format.combine(instrumentedFormatter(), opts.format)
   } else {
-    opts.format = instrumentedFormatter()
+    // The default formatter for Winston is the JSON formatter. If the user
+    // has not provided a formatter through opts.format, we must emulate the
+    // default. Otherwise, the message symbol will not get attached to log
+    // messages and transports, e.g. the "Console" transport, will not be able
+    // to output logs correctly.
+    opts.format = winston.format.combine(instrumentedFormatter(), winston.format.json())
   }
 }
 

lib/shimmer.js

@@ -13,6 +13,7 @@ const INSTRUMENTATIONS = require('./instrumentations')()
 const shims = require('./shim')
 const { Hook } = require('require-in-the-middle')
 const IitmHook = require('import-in-the-middle')
+const { nrEsmProxy } = require('./symbols')
 let pkgsToHook = []
 
 const NAMES = require('./metrics/names')
@@ -537,10 +538,7 @@ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver
       return
     }
 
-    // We only return the .default when we're a CJS module in the import-in-the-middle(ESM)
-    // callback hook
-    const resolvedNodule = instrumentation.isEsm || !esmResolver ? nodule : nodule.default
-
+    const resolvedNodule = resolveNodule({ nodule, instrumentation, esmResolver })
     const shim = shims.createShimFromType({
       type: instrumentation.type,
       agent,
@@ -579,6 +577,60 @@ function instrumentPostLoad(agent, nodule, moduleName, resolvedName, esmResolver
   return nodule
 }
 
+/**
+ * If a module that is being shimmed was loaded via a typical `require` then
+ * we can use it as normal. If was loaded via an ESM import, then we need to
+ * handle it with some extra care. This function inspects the module,
+ * the conditions around its loading, and returns an appropriate object for
+ * subsequent shimming methods.
+ *
+ * @param {object} params Input parameters.
+ * @param {object} params.nodule The nodule being instrumented.
+ * @param {object} params.instrumentation The configuration for the nodule
+ * to be instrumented.
+ * @param {boolean|null} params.esmResolver Indicates if the nodule was loaded
+ * via an ESM import.
+ * @returns {object} The nodule or a Proxy.
+ */
+function resolveNodule({ nodule, instrumentation, esmResolver }) {
+  if (instrumentation.isEsm === true || !esmResolver) {
+    return nodule
+  }
+
+  // We have a CJS module wrapped by import-in-the-middle having been
+  // imported through ESM syntax. Due to the way CJS modules are parsed by
+  // ESM's import, we can have the same "export" attached to the `default`
+  // export and as a top-level named export. In order to shim things such
+  // that our users don't need to know to access `something.default.foo`
+  // when they have done `import * as something from 'something'`, we need
+  // to proxy the proxy in order to set our wrappers on both instances.
+  const noduleDefault = nodule.default
+  const origNodule = nodule
+  return new Proxy(
+    { origNodule, noduleDefault },
+    {
+      get(target, name) {
+        if (name === nrEsmProxy) {
+          return true
+        }
+        if (target.noduleDefault[name]) {
+          return target.noduleDefault[name]
+        }
+        return target.origNodule[name]
+      },
+      set(target, name, value) {
+        if (target.origNodule[name]) {
+          target.origNodule[name] = value
+        }
+        if (target.noduleDefault[name]) {
+          target.noduleDefault[name] = value
+        }
+        return true
+      }
+    }
+  )
+}
+
 /**
  * Attempts to execute an onRequire hook for a given module.
  * If it fails it will call an onError hook and log warnings accordingly

lib/symbols.js

@@ -34,5 +34,10 @@ module.exports = {
   unwrapPool: Symbol('unwrapPool'),
   clusterOf: Symbol('clusterOf'),
   createPoolCluster: Symbol('createPoolCluster'),
-  wrappedPoolConnection: Symbol('wrappedPoolConnection')
+  wrappedPoolConnection: Symbol('wrappedPoolConnection'),
+
+  // Used to "decorate" the proxy returned during ESM importing so that
+  // instrumentations can determine if the module looks like an ESM module
+  // or not.
+  nrEsmProxy: Symbol('nr-esm-proxy')
 }

test/versioned/winston-esm/common.mjs

@@ -0,0 +1,13 @@
+/*
+ * Copyright 2023 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import Transport from 'winston-transport'
+export class Sink extends Transport {
+  loggedLines = []
+  log(data, done) {
+    this.loggedLines.push(data)
+    done()
+  }
+}

test/versioned/winston-esm/fixtures/named-import.mjs

@@ -0,0 +1,13 @@
+/*
+ * Copyright 2023 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import winston from 'winston'
+
+export function doLog(sink) {
+  const logger = winston.createLogger({
+    transports: sink
+  })
+  logger.warn('import winston from winston')
+}

test/versioned/winston-esm/fixtures/star-import.mjs

@@ -0,0 +1,13 @@
+/*
+ * Copyright 2023 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as winston from 'winston'
+
+export function doLog(sink) {
+  const logger = winston.createLogger({
+    transports: sink
+  })
+  logger.warn('import * as winston from winston')
+}

test/versioned/winston-esm/newrelic.cjs

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+
+exports.config = {
+  app_name: ['New Relic for Node.js tests'],
+  license_key: 'license key here',
+  logging: {
+    level: 'trace',
+    filepath: '../../../newrelic_agent.log'
+  },
+  utilization: {
+    detect_aws: false,
+    detect_pcf: false,
+    detect_azure: false,
+    detect_gcp: false,
+    detect_docker: false
+  },
+  distributed_tracing: {
+    enabled: true
+  },
+  transaction_tracer: {
+    enabled: true
+  }
+}

test/versioned/winston-esm/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "winston-esm-tests",
+  "version": "0.0.0",
+  "type": "module",
+  "private": true,
+  "tests": [
+    {
+      "engines": {
+        "node": ">=16"
+      },
+      "dependencies": {
+        "winston": ">=3",
+        "winston-transport": ">=4"
+      },
+      "files": [
+        "winston.tap.mjs"
+      ]
+    }
+  ]
+}