fix duplicate custom metrics when multiple tags are used (#5491)

Author: rochdev

packages/dd-trace/src/dogstatsd.js

@@ -194,7 +194,6 @@ class DogStatsDClient {
   }
 }
 
-// TODO: Handle arrays of tags and tags translation.
 class MetricsAggregationClient {
   constructor (client) {
     this._client = client
@@ -211,98 +210,132 @@ class MetricsAggregationClient {
   }
 
   reset () {
-    this._counters = {}
-    this._gauges = {}
-    this._histograms = {}
+    this._counters = new Map()
+    this._gauges = new Map()
+    this._histograms = new Map()
   }
 
-  distribution (name, value, tag) {
-    this._client.distribution(name, value, tag && [tag])
+  // TODO: Aggerate with a histogram and send the buckets to the client.
+  distribution (name, value, tags) {
+    this._client.distribution(name, value, tags)
   }
 
-  boolean (name, value, tag) {
-    this.gauge(name, value ? 1 : 0, tag)
+  boolean (name, value, tags) {
+    this.gauge(name, value ? 1 : 0, tags)
   }
 
-  histogram (name, value, tag) {
-    this._histograms[name] = this._histograms[name] || new Map()
+  histogram (name, value, tags) {
+    const node = this._ensureTree(this._histograms, name, tags, null)
 
-    if (!this._histograms[name].has(tag)) {
-      this._histograms[name].set(tag, new Histogram())
+    if (!node.value) {
+      node.value = new Histogram()
     }
 
-    this._histograms[name].get(tag).record(value)
+    node.value.record(value)
   }
 
-  count (name, count, tag, monotonic = true) {
-    if (typeof tag === 'boolean') {
-      monotonic = tag
-      tag = undefined
+  count (name, count, tags = [], monotonic = true) {
+    if (typeof tags === 'boolean') {
+      monotonic = tags
+      tags = []
     }
 
-    const map = monotonic ? this._counters : this._gauges
+    const container = monotonic ? this._counters : this._gauges
+    const node = this._ensureTree(container, name, tags, 0)
 
-    map[name] = map[name] || new Map()
-
-    const value = map[name].get(tag) || 0
-
-    map[name].set(tag, value + count)
+    node.value = node.value + count
   }
 
-  gauge (name, value, tag) {
-    this._gauges[name] = this._gauges[name] || new Map()
-    this._gauges[name].set(tag, value)
+  gauge (name, value, tags) {
+    const node = this._ensureTree(this._gauges, name, tags, 0)
+
+    node.value = value
   }
 
-  increment (name, count = 1, tag) {
-    this.count(name, count, tag)
+  increment (name, count = 1, tags) {
+    this.count(name, count, tags)
   }
 
-  decrement (name, count = 1, tag) {
-    this.count(name, -count, tag)
+  decrement (name, count = 1, tags) {
+    this.count(name, -count, tags)
   }
 
   _captureGauges () {
-    Object.keys(this._gauges).forEach(name => {
-      this._gauges[name].forEach((value, tag) => {
-        this._client.gauge(name, value, tag && [tag])
-      })
+    this._captureTree(this._gauges, (node, name, tags) => {
+      this._client.gauge(name, node.value, tags)
     })
   }
 
   _captureCounters () {
-    Object.keys(this._counters).forEach(name => {
-      this._counters[name].forEach((value, tag) => {
-        this._client.increment(name, value, tag && [tag])
-      })
+    this._captureTree(this._counters, (node, name, tags) => {
+      this._client.increment(name, node.value, tags)
     })
 
-    this._counters = {}
+    this._counters.clear()
   }
 
   _captureHistograms () {
-    Object.keys(this._histograms).forEach(name => {
-      this._histograms[name].forEach((stats, tag) => {
-        const tags = tag && [tag]
+    this._captureTree(this._histograms, (node, name, tags) => {
+      let stats = node.value
 
-        // Stats can contain garbage data when a value was never recorded.
-        if (stats.count === 0) {
-          stats = { max: 0, min: 0, sum: 0, avg: 0, median: 0, p95: 0, count: 0, reset: stats.reset }
-        }
+      // Stats can contain garbage data when a value was never recorded.
+      if (stats.count === 0) {
+        stats = { max: 0, min: 0, sum: 0, avg: 0, median: 0, p95: 0, count: 0 }
+      }
 
-        this._client.gauge(`${name}.min`, stats.min, tags)
-        this._client.gauge(`${name}.max`, stats.max, tags)
-        this._client.increment(`${name}.sum`, stats.sum, tags)
-        this._client.increment(`${name}.total`, stats.sum, tags)
-        this._client.gauge(`${name}.avg`, stats.avg, tags)
-        this._client.increment(`${name}.count`, stats.count, tags)
-        this._client.gauge(`${name}.median`, stats.median, tags)
-        this._client.gauge(`${name}.95percentile`, stats.p95, tags)
+      this._client.gauge(`${name}.min`, stats.min, tags)
+      this._client.gauge(`${name}.max`, stats.max, tags)
+      this._client.increment(`${name}.sum`, stats.sum, tags)
+      this._client.increment(`${name}.total`, stats.sum, tags)
+      this._client.gauge(`${name}.avg`, stats.avg, tags)
+      this._client.increment(`${name}.count`, stats.count, tags)
+      this._client.gauge(`${name}.median`, stats.median, tags)
+      this._client.gauge(`${name}.95percentile`, stats.p95, tags)
 
-        stats.reset()
-      })
+      node.value.reset()
     })
   }
+
+  _captureTree (tree, fn) {
+    for (const [name, root] of tree) {
+      this._captureNode(root, name, [], fn)
+    }
+  }
+
+  _captureNode (node, name, tags, fn) {
+    if (node.touched) {
+      fn(node, name, tags)
+    }
+
+    for (const [tag, next] of node.nodes) {
+      this._captureNode(next, name, tags.concat(tag), fn)
+    }
+  }
+
+  _ensureTree (tree, name, tags, value) {
+    tags = tags ? [].concat(tags) : []
+
+    let node = this._ensureNode(tree, name, value)
+
+    for (const tag of tags) {
+      node = this._ensureNode(node.nodes, tag, value)
+    }
+
+    node.touched = true
+
+    return node
+  }
+
+  _ensureNode (container, key, value) {
+    let node = container.get(key)
+
+    if (!node) {
+      node = { nodes: new Map(), touched: false, value }
+      container.set(key, node)
+    }
+
+    return node
+  }
 }
 
 /**
@@ -324,45 +357,29 @@ class CustomMetrics {
   }
 
   increment (stat, value = 1, tags) {
-    for (const tag of this._normalizeTags(tags)) {
-      this._client.increment(stat, value, tag)
-    }
+    this._client.increment(stat, value, CustomMetrics.tagTranslator(tags))
   }
 
   decrement (stat, value = 1, tags) {
-    for (const tag of this._normalizeTags(tags)) {
-      this._client.decrement(stat, value, tag)
-    }
+    this._client.decrement(stat, value, CustomMetrics.tagTranslator(tags))
   }
 
   gauge (stat, value, tags) {
-    for (const tag of this._normalizeTags(tags)) {
-      this._client.gauge(stat, value, tag)
-    }
+    this._client.gauge(stat, value, CustomMetrics.tagTranslator(tags))
   }
 
   distribution (stat, value, tags) {
-    for (const tag of this._normalizeTags(tags)) {
-      this._client.distribution(stat, value, tag)
-    }
+    this._client.distribution(stat, value, CustomMetrics.tagTranslator(tags))
   }
 
   histogram (stat, value, tags) {
-    for (const tag of this._normalizeTags(tags)) {
-      this._client.histogram(stat, value, tag)
-    }
+    this._client.histogram(stat, value, CustomMetrics.tagTranslator(tags))
   }
 
   flush () {
     return this._client.flush()
   }
 
-  _normalizeTags (tags) {
-    tags = CustomMetrics.tagTranslator(tags)
-
-    return tags.length === 0 ? [undefined] : tags
-  }
-
   /**
    * Exposing { tagName: 'tagValue' } to the end user
    * These are translated into [ 'tagName:tagValue' ] for internal use

packages/dd-trace/src/histogram.js

@@ -7,39 +7,28 @@ class Histogram {
     this.reset()
   }
 
-  get min () { return this._min }
-  get max () { return this._max }
-  get avg () { return this._count === 0 ? 0 : this._sum / this._count }
-  get sum () { return this._sum }
-  get count () { return this._count }
+  get min () { return this._sketch.count === 0 ? 0 : this._sketch.min }
+  get max () { return this._sketch.count === 0 ? 0 : this._sketch.max }
+  get avg () { return this._sketch.count === 0 ? 0 : this._sketch.sum / this._sketch.count }
+  get sum () { return this._sketch.sum }
+  get count () { return this._sketch.count }
   get median () { return this.percentile(50) }
   get p95 () { return this.percentile(95) }
 
   percentile (percentile) {
-    return this._histogram.getValueAtQuantile(percentile / 100) || 0
+    return this._sketch.getValueAtQuantile(percentile / 100) || 0
   }
 
-  record (value) {
-    if (this._count === 0) {
-      this._min = this._max = value
-    } else {
-      this._min = Math.min(this._min, value)
-      this._max = Math.max(this._max, value)
-    }
-
-    this._count++
-    this._sum += value
+  merge (histogram) {
+    return this._sketch.merge(histogram._sketch)
+  }
 
-    this._histogram.accept(value)
+  record (value) {
+    this._sketch.accept(value)
   }
 
   reset () {
-    this._min = 0
-    this._max = 0
-    this._sum = 0
-    this._count = 0
-
-    this._histogram = new DDSketch()
+    this._sketch = new DDSketch()
   }
 }
 

packages/dd-trace/test/dogstatsd.spec.js

@@ -374,6 +374,21 @@ describe('dogstatsd', () => {
       expect(udp4.send.firstCall.args[0].toString()).to.equal('test.avg:10|g|#foo:bar\n')
     })
 
+    it('.gauge() with tags', () => {
+      client = new CustomMetrics({ dogstatsd: {} })
+
+      client.gauge('test.avg', 10, { foo: 'bar' })
+      client.gauge('test.avg', 10, { foo: 'bar', baz: 'qux' })
+      client.gauge('test.avg', 20, { foo: 'bar', baz: 'qux' })
+      client.flush()
+
+      expect(udp4.send).to.have.been.called
+      expect(udp4.send.firstCall.args[0].toString()).to.equal([
+        'test.avg:10|g|#foo:bar',
+        'test.avg:20|g|#foo:bar,baz:qux'
+      ].join('\n') + '\n')
+    })
+
     it('.increment()', () => {
       client = new CustomMetrics({ dogstatsd: {} })
 
@@ -396,6 +411,21 @@ describe('dogstatsd', () => {
       expect(udp4.send.firstCall.args[0].toString()).to.equal('test.count:2|c\n')
     })
 
+    it('.increment() with tags', () => {
+      client = new CustomMetrics({ dogstatsd: {} })
+
+      client.increment('test.count', 10, { foo: 'bar' })
+      client.increment('test.count', 10, { foo: 'bar', baz: 'qux' })
+      client.increment('test.count', 10, { foo: 'bar', baz: 'qux' })
+      client.flush()
+
+      expect(udp4.send).to.have.been.called
+      expect(udp4.send.firstCall.args[0].toString()).to.equal([
+        'test.count:10|c|#foo:bar',
+        'test.count:20|c|#foo:bar,baz:qux'
+      ].join('\n') + '\n')
+    })
+
     it('.decrement()', () => {
       client = new CustomMetrics({ dogstatsd: {} })
 
@@ -449,6 +479,35 @@ describe('dogstatsd', () => {
       ].join('\n') + '\n')
     })
 
+    it('.histogram() with tags', () => {
+      client = new CustomMetrics({ dogstatsd: {} })
+
+      client.histogram('test.histogram', 10, { foo: 'bar' })
+      client.histogram('test.histogram', 10, { foo: 'bar', baz: 'qux' })
+      client.histogram('test.histogram', 10, { foo: 'bar', baz: 'qux' })
+      client.flush()
+
+      expect(udp4.send).to.have.been.called
+      expect(udp4.send.firstCall.args[0].toString()).to.equal([
+        'test.histogram.min:10|g|#foo:bar',
+        'test.histogram.max:10|g|#foo:bar',
+        'test.histogram.sum:10|c|#foo:bar',
+        'test.histogram.total:10|c|#foo:bar',
+        'test.histogram.avg:10|g|#foo:bar',
+        'test.histogram.count:1|c|#foo:bar',
+        'test.histogram.median:10.074696689511441|g|#foo:bar',
+        'test.histogram.95percentile:10.074696689511441|g|#foo:bar',
+        'test.histogram.min:10|g|#foo:bar,baz:qux',
+        'test.histogram.max:10|g|#foo:bar,baz:qux',
+        'test.histogram.sum:20|c|#foo:bar,baz:qux',
+        'test.histogram.total:20|c|#foo:bar,baz:qux',
+        'test.histogram.avg:10|g|#foo:bar,baz:qux',
+        'test.histogram.count:2|c|#foo:bar,baz:qux',
+        'test.histogram.median:10.074696689511441|g|#foo:bar,baz:qux',
+        'test.histogram.95percentile:10.074696689511441|g|#foo:bar,baz:qux'
+      ].join('\n') + '\n')
+    })
+
     it('should flush via interval', () => {
       const clock = sinon.useFakeTimers()