fix: ensure metrics collection is actually disabled with metricsInterval=0s (#2330)

Also guard against:
- `metricsInterval:null` resulting in metrics being enabled without any
  log message, which is perhaps slightly surprising.
- `metricsInterval:-1s` (any negative value) resulting in metrics being
  collected *as fast as possible* via
  `setInterval(collectAllTheMetrics, -1000)`.

An invalid value for this config var, and the other "POSTIVE_TIME_OPTS",
will result in (a) a log.warning and (b) falling back to the default
value.

A slight semantic change on the internal Metrics object means that the
`getOrCreate...()` metric functions now return undefined when metrics
are disabled via `metricsInterval`. This doesn't affect any public API.

Author: trentm

CHANGELOG.asciidoc

@@ -29,6 +29,29 @@ Notes:
 === Node.js Agent version 3.x
 
 
+==== Unreleased
+
+[float]
+===== Breaking changes
+
+[float]
+===== Features
+
+[float]
+===== Bug fixes
+
+* Guard against a negative value of `metricsInterval`, which can lead to
+  high CPU usage as metrics are collected as fast as possible. Also ensure
+  no metrics collection can happen if `metricsInterval="0s"` as intended.
+  Before this change it was possible for some metric collection to still
+  happen, even though none would be reported. ({pull}2330[#2330])
++
+This change also guards against negative and invalid values in the following
+configuration options: `abortedErrorThreshold`, `apiRequestTime`, and
+`serverTimeout`. If an invalid value is given, then will fallback to their
+default value.
+
+
 [[release-notes-3.21.1]]
 ==== 3.21.1 2021/09/16
 
@@ -51,16 +74,16 @@ Notes:
   Specific transaction/span/error fields (see the list below) will be truncated
   at this number of unicode characters. ({pull}2193[#2193], {issues}1921[#1921])
 +
-  The `errorMessageMaxLength` configuration option is now *deprecated*, but
-  still supported. Users should switch to using `longFieldMaxLength. If
-  `errorMessageMaxLength` is not specified, truncation of error messages will
-  now use the `longFieldMaxLength` value.
+The `errorMessageMaxLength` configuration option is now *deprecated*, but
+still supported. Users should switch to using `longFieldMaxLength`. If
+`errorMessageMaxLength` is not specified, truncation of error messages will
+now use the `longFieldMaxLength` value.
 +
-  Note that ultimately the maximum length of any tracing field is limited by the
-  {apm-server-ref-v}/configuration-process.html#max_event_size[`max_event_size`]
-  configured for the receiving APM server.
+Note that ultimately the maximum length of any tracing field is limited by the
+{apm-server-ref-v}/configuration-process.html#max_event_size[`max_event_size`]
+configured for the receiving APM server.
 +
-  The fields affected by `longFieldMaxLength` are:
+The fields affected by `longFieldMaxLength` are:
 +
 ** `transaction.context.request.body`, `error.context.request.body` - Before
    this change these fields were not truncated.

docs/configuration.asciidoc

@@ -948,15 +948,14 @@ it will be parsed out of the `/proc/self/cgroup` file.
 ==== `metricsInterval`
 
 * *Type:* String
-* *Default:* `30s`
+* *Default:* `"30s"`
 * *Env:* `ELASTIC_APM_METRICS_INTERVAL`
 
 Specify the interval for reporting metrics to APM Server.
 The interval should be in seconds,
 or should include a time suffix.
 
-To disable metrics reporting,
-set the interval to `0`.
+To disable metrics reporting, set the interval to `"0s"`.
 
 [[metrics-limit]]
 ==== `metricsLimit`

lib/config.js

@@ -197,11 +197,14 @@ var NUM_OPTS = [
   'transactionSampleRate'
 ]
 
-var TIME_OPTS = [
+var POSITIVE_TIME_OPTS = [
   'abortedErrorThreshold',
   'apiRequestTime',
   'metricsInterval',
-  'serverTimeout',
+  'serverTimeout'
+]
+// Time options allowing negative values.
+var TIME_OPTS = [
   'spanFramesMinDuration'
 ]
 
@@ -400,6 +403,7 @@ function normalize (opts, logger) {
   normalizeNumbers(opts)
   normalizeBytes(opts)
   normalizeArrays(opts)
+  normalizePositiveTime(opts, logger)
   normalizeTime(opts)
   normalizeBools(opts, logger)
   normalizeIgnoreOptions(opts)
@@ -512,9 +516,24 @@ function normalizeBytes (opts) {
   }
 }
 
+function normalizePositiveTime (opts, logger) {
+  for (const key of POSITIVE_TIME_OPTS) {
+    if (key in opts) {
+      let val = secondsFromTimeStr(String(opts[key]))
+      if (val === null) {
+        const def = DEFAULTS[key]
+        logger.warn('invalid time value "%s" for "%s" config option: using default "%s"',
+          opts[key], key, def)
+        val = secondsFromTimeStr(def)
+      }
+      opts[key] = val
+    }
+  }
+}
+
 function normalizeTime (opts) {
   for (const key of TIME_OPTS) {
-    if (key in opts) opts[key] = toSeconds(String(opts[key]))
+    if (key in opts) opts[key] = secondsFromTimeStr(String(opts[key]), true)
   }
 }
 
@@ -594,14 +613,19 @@ function bytes (input) {
   }
 }
 
-function toSeconds (value) {
-  var matches = /^(-)?(\d+)(m|ms|s)?$/.exec(value)
-  if (!matches) return null
+function secondsFromTimeStr (value, allowNegative) {
+  let matches
+  if (allowNegative) {
+    matches = /^(-?\d+)(m|ms|s)?$/.exec(value)
+  } else {
+    matches = /^(\d+)(m|ms|s)?$/.exec(value)
+  }
+  if (!matches) {
+    return null
+  }
 
-  var negate = matches[1]
-  var amount = Number(matches[2])
-  if (negate) amount = -amount
-  var scale = matches[3]
+  var amount = Number(matches[1])
+  var scale = matches[2]
 
   if (scale === 'm') {
     amount *= 60
@@ -738,4 +762,7 @@ 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
+
+// The following are exported for tests.
 module.exports.DEFAULTS = DEFAULTS
+module.exports.secondsFromTimeStr = secondsFromTimeStr

lib/instrumentation/modules/aws-sdk/sqs.js

@@ -92,6 +92,9 @@ function recordMetrics (queueName, data, agent) {
   }
   if (!queueMetrics.get(queueName)) {
     const collector = agent._metrics.createQueueMetricsCollector(queueName)
+    if (!collector) {
+      return
+    }
     queueMetrics.set(queueName, collector)
   }
   const metrics = queueMetrics.get(queueName)

lib/metrics/index.js

@@ -24,14 +24,16 @@ class Metrics {
   start (refTimers) {
     const metricsInterval = this[agentSymbol]._conf.metricsInterval
     const enabled = metricsInterval !== 0 && !this[agentSymbol]._conf.disableSend
-    this[registrySymbol] = new MetricsRegistry(this[agentSymbol], {
-      reporterOptions: {
-        defaultReportingIntervalInSeconds: metricsInterval,
-        enabled: enabled,
-        unrefTimers: !refTimers,
-        logger: new NoopLogger()
-      }
-    })
+    if (enabled) {
+      this[registrySymbol] = new MetricsRegistry(this[agentSymbol], {
+        reporterOptions: {
+          defaultReportingIntervalInSeconds: metricsInterval,
+          enabled: enabled,
+          unrefTimers: !refTimers,
+          logger: new NoopLogger()
+        }
+      })
+    }
   }
 
   stop () {

test/config.test.js

@@ -354,14 +354,66 @@ bytesValues.forEach(function (key) {
   })
 })
 
-var timeValues = [
+var POSITIVE_TIME_OPTS = [
   'abortedErrorThreshold',
   'apiRequestTime',
   'metricsInterval',
   'serverTimeout'
 ]
+// Time options allowing negative values.
+var TIME_OPTS = [
+  'spanFramesMinDuration'
+]
+
+POSITIVE_TIME_OPTS.forEach(function (key) {
+  test(key + ' should guard against a negative time', function (t) {
+    var agent = new Agent()
+    var logger = new CaptureLogger()
+    agent.start(Object.assign(
+      {},
+      agentOptsNoopTransport,
+      {
+        [key]: '-1s',
+        logger
+      }
+    ))
+
+    t.strictEqual(agent._conf[key], config.secondsFromTimeStr(config.DEFAULTS[key]),
+      'fell back to default value')
+    const warning = logger.calls[0]
+    t.equal(warning.type, 'warn', 'got a log.warn')
+    t.ok(warning.message.indexOf('-1s') !== -1, 'warning message includes the invalid value')
+    t.ok(warning.message.indexOf(key) !== -1, 'warning message includes the invalid key')
+
+    agent.destroy()
+    t.end()
+  })
+
+  test(key + ' should guard against a bogus non-time', function (t) {
+    var agent = new Agent()
+    var logger = new CaptureLogger()
+    agent.start(Object.assign(
+      {},
+      agentOptsNoopTransport,
+      {
+        [key]: 'bogusvalue',
+        logger
+      }
+    ))
+
+    t.strictEqual(agent._conf[key], config.secondsFromTimeStr(config.DEFAULTS[key]),
+      'fell back to default value')
+    const warning = logger.calls[0]
+    t.equal(warning.type, 'warn', 'got a log.warn')
+    t.ok(warning.message.indexOf('bogusvalue') !== -1, 'warning message includes the invalid value')
+    t.ok(warning.message.indexOf(key) !== -1, 'warning message includes the invalid key')
+
+    agent.destroy()
+    t.end()
+  })
+})
 
-timeValues.forEach(function (key) {
+POSITIVE_TIME_OPTS.concat(TIME_OPTS).forEach(function (key) {
   test(key + ' should convert minutes to seconds', function (t) {
     var agent = new Agent()
     var opts = {}