test: refactor to move knowledge for in-process Agent re-use into Agent methods (#2305)

Move away from using the test/_agent.js wrapper in tests, in favour of
using the real Agent class directly and agent.destroy() at the end of a
test to clean up.  Agent#destroy() has been improved and
Instrumentation#stop() added to properly handle agent clean-up (within
reason) such that tests can serially create multiple instances of the
Agent.

Author: trentm

lib/agent.js

@@ -71,8 +71,46 @@ Object.defineProperty(Agent.prototype, 'currentTraceIds', {
   }
 })
 
+// Destroy this agent. This prevents any new agent processing, communication
+// with APM server, and resets changed global state *as much as is possible*.
+//
+// In the typical uses case -- a singleton Agent running for the full process
+// lifetime -- it is *not* necessary to call `agent.destroy()`.  It is used
+// for some testing.
+//
+// Limitations:
+// - Patching/wrapping of functions for instrumentation *is* undone, but
+//   references to the wrapped versions can remain.
+// - There may be in-flight tasks (in ins.addEndedSpan() and
+//   agent.captureError() for example) that will complete after this destroy
+//   completes. They should have no impact other than CPU/resource use.
 Agent.prototype.destroy = function () {
-  if (this._transport) this._transport.destroy()
+  if (this._transport && this._transport.destroy) {
+    this._transport.destroy()
+  }
+  // So in-flight tasks in ins.addEndedSpan() and agent.captureError() do
+  // not use the destroyed transport.
+  this._transport = null
+
+  // So in-flight tasks do not call user-added filters after the agent has
+  // been destroyed.
+  this._errorFilters = new Filters()
+  this._transactionFilters = new Filters()
+  this._spanFilters = new Filters()
+
+  if (this._uncaughtExceptionListener) {
+    process.removeListener('uncaughtException', this._uncaughtExceptionListener)
+  }
+  this._metrics.stop()
+  this._instrumentation.stop()
+
+  // Allow a new Agent instance to `.start()`. Typically this is only relevant
+  // for tests that may use multiple Agent instances in a single test process.
+  global[symbols.agentInitialized] = null
+
+  if (Error.stackTraceLimit === this._conf.stackTraceLimit) {
+    Error.stackTraceLimit = this._origStackTraceLimit
+  }
 }
 
 // These are metrics about the agent itself -- separate from the metrics
@@ -215,6 +253,7 @@ Agent.prototype.start = function (opts) {
   this._instrumentation.start()
   this._metrics.start()
 
+  this._origStackTraceLimit = Error.stackTraceLimit
   Error.stackTraceLimit = this._conf.stackTraceLimit
   if (this._conf.captureExceptions) this.handleUncaughtExceptions()
 

lib/config.js

@@ -22,13 +22,6 @@ if (packageName === 'elastic-apm-node') {
 }
 var userAgent = `${packageName}/${version}`
 
-config.INTAKE_STRING_MAX_SIZE = 1024
-config.CAPTURE_ERROR_LOG_STACK_TRACES_NEVER = 'never'
-config.CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES = 'messages'
-config.CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS = 'always'
-
-module.exports = config
-
 let confFile = loadConfigFile()
 
 let serviceName, serviceVersion
@@ -38,6 +31,11 @@ try {
   serviceVersion = version
 } catch (err) {}
 
+const INTAKE_STRING_MAX_SIZE = 1024
+const CAPTURE_ERROR_LOG_STACK_TRACES_NEVER = 'never'
+const CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES = 'messages'
+const CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS = 'always'
+
 var DEFAULTS = {
   abortedErrorThreshold: '25s',
   active: true,
@@ -47,7 +45,7 @@ var DEFAULTS = {
   asyncHooks: true,
   breakdownMetrics: true,
   captureBody: 'off',
-  captureErrorLogStackTraces: config.CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES,
+  captureErrorLogStackTraces: CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES,
   captureExceptions: true,
   captureHeaders: true,
   captureSpanStackTraces: true,
@@ -562,8 +560,8 @@ function normalizeBools (opts, logger) {
 }
 
 function truncateOptions (opts) {
-  if (opts.serviceVersion) opts.serviceVersion = truncate(String(opts.serviceVersion), config.INTAKE_STRING_MAX_SIZE)
-  if (opts.hostname) opts.hostname = truncate(String(opts.hostname), config.INTAKE_STRING_MAX_SIZE)
+  if (opts.serviceVersion) opts.serviceVersion = truncate(String(opts.serviceVersion), INTAKE_STRING_MAX_SIZE)
+  if (opts.hostname) opts.hostname = truncate(String(opts.hostname), INTAKE_STRING_MAX_SIZE)
 }
 
 function bytes (input) {
@@ -685,7 +683,7 @@ function getBaseClientConfig (conf, agent) {
     environment: conf.environment,
 
     // Sanitize conf
-    truncateKeywordsAt: config.INTAKE_STRING_MAX_SIZE,
+    truncateKeywordsAt: INTAKE_STRING_MAX_SIZE,
     truncateErrorMessagesAt: conf.errorMessageMaxLength,
 
     // HTTP conf
@@ -720,3 +718,11 @@ function getBaseClientConfig (conf, agent) {
     cloudMetadataFetcher: (new CloudMetadata(cloudProvider, conf.logger, conf.serviceName))
   }
 }
+
+// Exports.
+module.exports = config
+module.exports.INTAKE_STRING_MAX_SIZE = INTAKE_STRING_MAX_SIZE
+module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_NEVER = CAPTURE_ERROR_LOG_STACK_TRACES_NEVER
+module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES = CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES
+module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS = CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS
+module.exports.DEFAULTS = DEFAULTS

lib/instrumentation/async-hooks.js

@@ -3,20 +3,12 @@
 const asyncHooks = require('async_hooks')
 const shimmer = require('./shimmer')
 
-// FOR INTERNAL TESTING PURPOSES ONLY!
-const resettable = process.env._ELASTIC_APM_ASYNC_HOOKS_RESETTABLE === 'true'
-let _asyncHook
-
 module.exports = function (ins) {
   const asyncHook = asyncHooks.createHook({ init, before, destroy })
-  const contexts = new WeakMap()
-
-  if (resettable) {
-    if (_asyncHook) _asyncHook.disable()
-    _asyncHook = asyncHook
-  }
-
+  const activeSpans = new Map()
   const activeTransactions = new Map()
+  let contexts = new WeakMap()
+
   Object.defineProperty(ins, 'currentTransaction', {
     get () {
       const asyncId = asyncHooks.executionAsyncId()
@@ -32,7 +24,6 @@ module.exports = function (ins) {
     }
   })
 
-  const activeSpans = new Map()
   Object.defineProperty(ins, 'activeSpan', {
     get () {
       const asyncId = asyncHooks.executionAsyncId()
@@ -63,6 +54,18 @@ module.exports = function (ins) {
     }
   })
 
+  shimmer.wrap(ins, 'stop', function (origStop) {
+    return function wrappedStop () {
+      asyncHook.disable()
+      activeTransactions.clear()
+      activeSpans.clear()
+      contexts = new WeakMap()
+      shimmer.unwrap(ins, 'addEndedTransaction')
+      shimmer.unwrap(ins, 'stop')
+      return origStop.call(this)
+    }
+  })
+
   asyncHook.enable()
 
   function init (asyncId, type, triggerAsyncId, resource) {

lib/instrumentation/index.js

@@ -86,6 +86,25 @@ function Instrumentation (agent) {
   }
 }
 
+// Stop active instrumentation and reset global state *as much as possible*.
+//
+// Limitations: Removing and re-applying 'require-in-the-middle'-based patches
+// has no way to update existing references to patched or unpatched exports from
+// those modules.
+Instrumentation.prototype.stop = function () {
+  // Reset context tracking.
+  this.currentTransaction = null
+  this.bindingSpan = null
+  this.activeSpan = null
+
+  // Reset patching.
+  this._started = false
+  if (this._hook) {
+    this._hook.unhook()
+    this._hook = null
+  }
+}
+
 Object.defineProperty(Instrumentation.prototype, 'ids', {
   get () {
     const current = this.currentSpan || this.currentTransaction

test/_agent.js

@@ -1,5 +1,17 @@
 'use strict'
 
+// DEPRECATED: New tests should not use this wrapper. Instead using the
+// real Agent directly, and its `agent.destroy()` method to clean up state
+// and the end of tests. E.g.:
+//
+// const Agent = require('.../lib/agent')
+// test('test name', t => {
+//   const agent = new Agent().start({ ... })
+//   ...
+//   agent.destroy()
+//   t.end()
+// })
+
 var Agent = require('../lib/agent')
 var symbols = require('../lib/symbols')
 

test/_apm_server.js

@@ -6,6 +6,8 @@
 //      // Test code using `serverUrl`...
 //      // - Events received on the intake API will be on `server.events`.
 //      // - Raw request data is on `server.requests`.
+//      // - Use `server.clear()` to clear `server.events` and `server.requests`
+//      //   for re-use of the mock server in multiple test cases.
 //      // - Call `server.close()` when done.
 //    })
 
@@ -15,12 +17,16 @@ const zlib = require('zlib')
 
 class MockAPMServer {
   constructor () {
-    this.events = []
-    this.requests = []
+    this.clear()
     this.serverUrl = null // set in .start()
     this._http = http.createServer(this._onRequest.bind(this))
   }
 
+  clear () {
+    this.events = []
+    this.requests = []
+  }
+
   _onRequest (req, res) {
     var parsedUrl = new URL(req.url, this.serverUrl)
     var instream = req