fix: remove zero count metrics from registry (#2163)

* feat: no longer sends counting metrics with a count of zero and removes them from the metrics cache/registry

Closes: #1977

CHANGELOG.asciidoc

@@ -28,6 +28,19 @@ Notes:
 [[release-notes-3.x]]
 === Node.js Agent version 3.x
 
+==== Unreleased
+
+[float]
+===== Breaking changes
+
+[float]
+===== Features
+
+[float]
+===== Bug fixes
+
+* The agent will no longer report counting metrics with a value of zero, and will
+  remove these metrics from the registry. ({pull}2163[#2163])
 
 [[release-notes-3.18.0]]
 ==== 3.18.0 2021/07/20

lib/metrics/reporter.js

@@ -30,6 +30,12 @@ class MetricsReporter extends Reporter {
         // Due to limitations in measured-reporting, metrics dropped
         // due to `metricsLimit` leave empty slots in the list.
         if (!metric) continue
+
+        if (this.isStaleMetric(metric)) {
+          this.removeMetricFromRegistry(metric, this._registry)
+          continue
+        }
+
         const data = seen.ensure(metric.dimensions, () => {
           const metricData = unflattenBreakdown(metric.dimensions)
           const merged = Object.assign({ samples: {} }, baseDimensions, metricData)
@@ -55,6 +61,23 @@ class MetricsReporter extends Reporter {
       collector.collect(next())
     }
   }
+
+  isStaleMetric (metric) {
+    // if a metric is a counting metric and that count is
+    // zero, then the metric is considered stale
+    if (metric.metricImpl && metric.metricImpl._count === 0) {
+      return true
+    }
+    return false
+  }
+
+  removeMetricFromRegistry (metric) {
+    if (!this._registry || !this._registry._metrics) {
+      return
+    }
+    const key = this._registry._generateStorageKey(metric.name, metric.dimensions)
+    return this._registry._metrics.delete(key)
+  }
 }
 
 module.exports = MetricsReporter

test/metrics/breakdown.test.js

@@ -44,7 +44,11 @@ const metrics = {
     'transaction.breakdown.count'
   ],
   'transaction span': spanMetrics,
-  span: spanMetrics
+  span: spanMetrics,
+  'transaction not sampling': [
+    'transaction.duration.count',
+    'transaction.duration.sum.us'
+  ]
 }
 
 function nullableEqual (a, b) {
@@ -66,6 +70,9 @@ const finders = {
       if (!nullableEqual(subtype, span.subtype)) return false
       return true
     })
+  },
+  'transaction not sampling' (metricsets) {
+    return metricsets.find(metricset => metricset.transaction && !metricset.span)
   }
 }
 
@@ -91,6 +98,14 @@ const expectations = {
         type: span.type
       }
     })
+  },
+  'transaction not sampling' (transaction) {
+    return {
+      transaction: {
+        name: transaction.name,
+        type: transaction.type
+      }
+    }
   }
 }
 
@@ -144,7 +159,7 @@ test('does not include breakdown when not sampling', t => {
 
     const { metricsets } = data
 
-    assertMetricSet(t, 'transaction', metricsets, {
+    assertMetricSet(t, 'transaction not sampling', metricsets, {
       transaction
     })
 

test/metrics/stale.test.js

@@ -0,0 +1,57 @@
+const tape = require('tape')
+const MetricsReporter = require('../../lib/metrics/reporter')
+
+function createMockRegistry (fnGenerateStorageKey, fnDelete) {
+  return {
+    _generateStorageKey: fnGenerateStorageKey,
+    _metrics: {
+      delete: fnDelete
+    }
+  }
+}
+
+tape.test('Unit Tests: isStaleMetric and removeMetricFromRegistry', function (t) {
+  t.test('isStaleMetric', function (t) {
+    const reg = new MetricsReporter({})
+
+    t.ok(reg.isStaleMetric({ metricImpl: { _count: 0 } }, '_count 0 is stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: {} }, 'no _count property is not stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: { _count: 1 } }, 'non-zero values not stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: { _count: false } }, 'non-zero values not stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: { _count: '' } }, 'non-zero values not stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: { _count: null } }, 'non-zero values not stale'))
+    t.ok(!reg.isStaleMetric({ metricImpl: { _count: undefined } }, 'non-zero values not stale'))
+    t.ok(!reg.isStaleMetric({}), 'value without metricImpl does not explode')
+
+    t.end()
+  })
+
+  t.test('removeMetricFromRegistry functionality', function (t) {
+    const reg = new MetricsReporter({})
+    reg._registry = createMockRegistry(
+      function generateStorageKey (name, dimensions) {
+        return JSON.stringify(name) + JSON.stringify(dimensions)
+      },
+      function metricsDelete (key) {
+        t.equals(key, '"foo"["bar"]', 'requested deletion of correct key')
+        t.end()
+      }
+    )
+
+    reg.removeMetricFromRegistry({
+      name: 'foo',
+      dimensions: ['bar']
+    })
+  })
+
+  t.test('removeMetricFromRegistry invalid types', function (t) {
+    const reg = new MetricsReporter({})
+
+    t.ok(!reg.removeMetricFromRegistry(), 'call with empty object does not explode')
+
+    reg._registry = {}
+    t.ok(!reg.removeMetricFromRegistry(), 'call with invalid registry does not explode')
+
+    t.end()
+  })
+})