update tests for metrics

Author: jsumners-nr

lib/shimmer.js

@@ -306,7 +306,7 @@ const shimmer = (module.exports = {
   registerHooks(agent) {
     // add the packages from the subscriber based instrumentation
     // this is only added to add tracking metrics
-    pkgsToHook.push(...Object.keys(subscriptions), ...trackingPkgs)
+    Array.prototype.push.apply(pkgsToHook, trackingPkgs)
     this._ritm = new Hook(pkgsToHook, function onHook(exports, name, basedir) {
       return _postLoad(agent, exports, name, basedir)
     })

lib/subscribers/base.js

@@ -30,36 +30,41 @@ const ArrayPrototypeSplice = makeCall(Array.prototype.splice)
 
 /**
  * @property {object} agent A New Relic Node.js agent instance.
- * @property {object} logger An agent logger instance.
+ * @property {TracingChannel} channel The tracing channel instance this subscriber will be monitoring.
+ * @property {string} channelName A unique name for the diagnostics channel
+ * that will be registered.
  * @property {object} config The agent configuration object.
+ * @property {string} id A unique identifier for the subscriber, combining the prefix, package
+ * name, and channel name.
+ * @property {object} logger An agent logger instance.
  * @property {string} packageName The name of the module being instrumented.
  * This is the same string one would pass to the `require` function.
- * @property {string} channelName A unique name for the diagnostics channel
- * that will be registered.
+ * @property {AsyncLocalStorage} store The async local storage instance used for context management.
+ * @property {number} [callback=null] Position of callback if it needs to be wrapped for instrumentation.
+ * -1 means last argument.
  * @property {string[]} [events=[]] Set of tracing channel event names to
  * register handlers for. For any name in the set, a corresponding method
  * must exist on the subscriber instance. The method will be passed the
  * event object. Possible event names are `start`, `end`, `asyncStart`,
  * `asyncEnd`, and `error`.
- *
- *  See {@link https://nodejs.org/api/diagnostics_channel.html#class-tracingchannel}
- * @property {boolean} [opaque=false] If true, any children segments will not be created.
+ * See {@link https://nodejs.org/api/diagnostics_channel.html#class-tracingchannel}
  * @property {boolean} [internal=false] If true, any children segments from the same library
  * will not be created.
+ * @property {boolean} [opaque=false] If true, any children segments will not be created.
  * @property {string} [prefix='orchestrion:'] String to prepend to diagnostics
  * channel event names. This provides a namespace for the events we are
  * injecting into a module.
- * @property {boolean} [requireActiveTx=true] If true, the subscriber will only handle events
- * when there is an active transaction.
  * @property {boolean} [propagateContext=false] If true, it will bind `asyncStart` to the store
  * and re-propagate the active context. It will also attach the `transaction` to the event in
  * `start.bindStore`. This is used for functions that queue async code and context is lost.
- * @property {string} id A unique identifier for the subscriber, combining the prefix, package
- * name, and channel name.
- * @property {TracingChannel} channel The tracing channel instance this subscriber will be monitoring.
- * @property {AsyncLocalStorage} store The async local storage instance used for context management.
- * @property {number} [callback=null] Position of callback if it needs to be wrapped for instrumentation.
- * -1 means last argument.
+ * @property {boolean} [requireActiveTx=true] If true, the subscriber will only handle events
+ * when there is an active transaction.
+ * @property {object} [targetModuleMeta] Defines the target module's name and
+ * version string, i.e. is an object `{ name, version }`. This is only necessary
+ * when target instrumentation can surface an unexpected name for the
+ * `packageName` property. For example, `express` uses multiple modules to
+ * compose its core functionality. We want to track things under the `express`
+ * name, but `packageName` will be set to `router` is most cases.
  */
 class Subscriber {
   #usageMetricRecorded = false
@@ -239,10 +244,11 @@ class Subscriber {
      */
     const handler = (data) => {
       if (this.#usageMetricRecorded === false) {
+        const meta = this.targetModuleMeta
         recordSupportabilityMetric({
           agent: this.agent,
-          moduleName: this.packageName,
-          moduleVersion: data.moduleVersion
+          moduleName: meta?.name || this.packageName,
+          moduleVersion: meta?.version || data.moduleVersion
         })
         this.#usageMetricRecorded = true
       }

lib/subscribers/express/base.js

@@ -4,6 +4,7 @@
  */
 
 const MiddlewareSubscriber = require('../middleware')
+const resolvePackageVersion = require('../resolve-package-version')
 
 /**
  * Special error handler to ignore if the error passed to next handler
@@ -18,9 +19,24 @@ function errorHandler(err) {
 }
 
 class ExpressSubscriber extends MiddlewareSubscriber {
+  #resolvedMeta
+
   constructor({ agent, logger, packageName = 'express', channelName }) {
     super({ agent, logger, packageName, channelName, system: 'Expressjs', errorHandler })
   }
+
+  get targetModuleMeta() {
+    if (this.#resolvedMeta !== undefined) {
+      return this.#resolvedMeta
+    }
+
+    const version = resolvePackageVersion('express')
+    this.#resolvedMeta = {
+      name: 'express',
+      version
+    }
+    return this.#resolvedMeta
+  }
 }
 
 module.exports = ExpressSubscriber

test/lib/custom-assertions/assert-pkg-tracking-metrics.js

@@ -4,9 +4,18 @@
  */
 
 'use strict'
-const NAMES = require('#agentlib/metrics/names.js')
-const assertMetrics = require('./assert-metrics')
+
 const semver = require('semver')
+const assertMetrics = require('./assert-metrics')
+
+const {
+  FEATURES: {
+    INSTRUMENTATION: {
+      SUBSCRIBER_USED,
+      ON_REQUIRE
+    }
+  }
+} = require('#agentlib/metrics/names.js')
 
 /**
  * assertion to verify tracking metrics for a given
@@ -16,19 +25,29 @@ const semver = require('semver')
  * @param {string} params.pkg name of package
  * @param {string} params.version version of package
  * @param {Agent} params.agent agent instance
+ * @param {boolean} params.subscriberType When true, the metrics are expected
+ * to have been generated from a subscriber based instrumentation. Otherwise,
+ * the metrics are expected to be generated from a shimmer based
+ * instrumentation.
  * @param {object} [deps] Injected dependencies.
  * @param {object} [deps.assert] Assertion library to use.
  */
 module.exports = function assertPackageMetrics(
-  { pkg, version, agent },
+  { pkg, version, agent, subscriberType = false },
   { assert = require('node:assert') } = {}
 ) {
-  const metrics = [
-    [{ name: `${NAMES.FEATURES.INSTRUMENTATION.ON_REQUIRE}/${pkg}` }]
-  ]
+  const metrics = []
+  const prefix = subscriberType === true
+    ? `${SUBSCRIBER_USED}/${pkg}`
+    : `${ON_REQUIRE}/${pkg}`
 
+  metrics.push([{ name: prefix }])
   if (version) {
-    metrics.push([{ name: `${NAMES.FEATURES.INSTRUMENTATION.ON_REQUIRE}/${pkg}/Version/${semver.major(version)}` }])
+    const major = semver.major(version)
+    const suffix = subscriberType === true
+      ? `/${major}`
+      : `/Version/${major}`
+    metrics.push([{ name: `${prefix}${suffix}` }])
   }
 
   assertMetrics(agent.metrics, metrics, false, false, { assert })

test/versioned/amqplib/callback.test.js

@@ -62,7 +62,7 @@ test('amqplib callback instrumentation', async function (t) {
 
   await t.test('should log tracking metrics', function(t) {
     const { agent } = t.nr
-    assertPackageMetrics({ agent, pkg: 'amqplib', version })
+    assertPackageMetrics({ agent, pkg: 'amqplib', version, subscriberType: true })
   })
 
   await t.test('connect in a transaction', function (t, end) {

test/versioned/amqplib/promises.test.js

@@ -55,7 +55,7 @@ test('amqplib promise instrumentation', async function (t) {
 
   await t.test('should log tracking metrics', function(t) {
     const { agent } = t.nr
-    assertPackageMetrics({ agent, pkg: 'amqplib', version })
+    assertPackageMetrics({ agent, pkg: 'amqplib', version, subscriberType: true })
   })
 
   await t.test('connect in a transaction', async function (t) {

test/versioned/bunyan/bunyan.test.js

@@ -51,9 +51,10 @@ test('logging enabled/disabled', async (t) => {
 
   await t.test('should log tracking metrics', function(t) {
     setup(t.nr, { application_logging: { enabled: true } })
-    const { agent } = t.nr
+    const { agent, bunyan } = t.nr
     const { version } = require('bunyan/package.json')
-    assertPackageMetrics({ agent, pkg: 'bunyan', version })
+    bunyan.createLogger({ name: 'test-logger' })
+    assertPackageMetrics({ agent, pkg: 'bunyan', version, subscriberType: true })
   })
 
   await t.test('logging enabled', (t) => {

test/versioned/elastic/elasticsearch.test.js

@@ -407,7 +407,7 @@ test('Elasticsearch instrumentation', async (t) => {
       expected['Datastore/instance/ElasticSearch/' + HOST_ID] = 5
       checkMetrics(unscoped, expected)
       const pkgName = '@elastic/elasticsearch'
-      assertPackageMetrics({ agent, pkg: pkgName, version: pkgVersion })
+      assertPackageMetrics({ agent, pkg: pkgName, version: pkgVersion, subscriberType: true })
     })
   })
 

test/versioned/express/app-use.test.js

@@ -39,7 +39,6 @@ test('app should be at top of stack when mounted', async function (t) {
 
   const express = require('express')
   const { version } = require('express/package.json')
-  assertPackageMetrics({ agent, pkg: 'express', version })
   const main = express()
   const app = express()
   const app2 = express()
@@ -67,6 +66,7 @@ test('app should be at top of stack when mounted', async function (t) {
   // store finished transactions
   const finishedTransactions = {}
   agent.on('transactionFinished', function (tx) {
+    assertPackageMetrics({ agent, pkg: 'express', version, subscriberType: true })
     finishedTransactions[tx.id] = tx
   })
 

test/versioned/express/ignoring.test.js

@@ -8,45 +8,41 @@
 const test = require('node:test')
 const helper = require('../../lib/agent_helper')
 const API = require('../../../api')
-const tsplan = require('@matteo.collina/tspl')
+const promiseResolvers = require('../../lib/promise-resolvers')
 const { setup, teardown } = require('./utils')
 
-test.beforeEach(async (ctx) => {
-  await setup(ctx)
-})
-
-test.afterEach(teardown)
-
 test('ignoring an Express route', async function (t) {
-  const { agent, app, port, isExpress5 } = t.nr
-  const plan = tsplan(t, { plan: 8 })
+  t.plan(8)
+
+  await setup(t)
+  t.after(() => { teardown(t) })
 
+  const { agent, app, port } = t.nr
+  const { promise, resolve } = promiseResolvers()
   const api = new API(agent)
 
   agent.on('transactionFinished', function (transaction) {
-    plan.equal(
+    t.assert.equal(
       transaction.name,
       'WebTransaction/Expressjs/GET//polling/:id',
       'transaction has expected name even on error'
     )
 
-    plan.ok(transaction.ignore, 'transaction is ignored')
+    t.assert.ok(transaction.ignore, 'transaction is ignored')
 
-    plan.ok(!agent.traces.trace, 'should have no transaction trace')
+    t.assert.ok(!agent.traces.trace, 'should have no transaction trace')
 
-    const metrics = agent.metrics._metrics.unscoped
-    // loading k2 adds instrumentation metrics for things it loads
-    let expectedMetrics = isExpress5 ? 10 : 8
-    if (helper.isSecurityAgentEnabled(agent) === true) {
-      expectedMetrics = isExpress5 ? 21 : 19
-    }
-    plan.equal(
-      Object.keys(metrics).length,
-      expectedMetrics,
+    const metrics = Object.keys(agent.metrics._metrics.unscoped).filter(
+      (k) => k.startsWith('Supportability/') === false
+    )
+    t.assert.equal(
+      metrics.length,
+      0,
       'only supportability metrics added to agent collection'
     )
+
     const errors = agent.errors.traceAggregator.errors
-    plan.equal(errors.length, 0, 'no errors noticed')
+    t.assert.equal(errors.length, 0, 'no errors noticed')
   })
 
   app.get('/polling/:id', function (req, res) {
@@ -57,9 +53,14 @@ test('ignoring an Express route', async function (t) {
 
   const url = 'http://localhost:' + port + '/polling/31337'
   helper.makeGetRequest(url, function (error, res, body) {
-    plan.ifError(error)
-    plan.equal(res.statusCode, 400, 'got expected error')
-    plan.deepEqual(body, { status: 'pollpollpoll' }, 'got expected response')
+    t.assert.ifError(error)
+    t.assert.equal(res.statusCode, 400, 'got expected error')
+    t.assert.deepEqual(body, { status: 'pollpollpoll' }, 'got expected response')
+
+    // The request finished callback is invoked after the transactionFinished
+    // callback. So we must trigger the end of the test here.
+    resolve()
   })
-  await plan.completed
+
+  await promise
 })