Skip to content

Commit e146182

Browse files
authored
fix(dogstatsd): stop re-emitting stale gauges (#8153)
This fixes Datadog "no data" alerting being silently masked for custom metrics. The aggregator's flush loop never cleared the gauge tree, so it kept rebroadcasting the last value of every gauge every 10 seconds for the lifetime of the process. Histograms had the same masking effect once observed at least once: the count===0 fallback emitted a row of zeros forever. Runtime metrics re-set every gauge each interval, so dashboards using them stay identical. In addition, if being used with a high cardinality input, this would be a memory leak. Fixes: #7504
1 parent ddfe085 commit e146182

2 files changed

Lines changed: 78 additions & 8 deletions

File tree

packages/dd-trace/src/dogstatsd.js

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,8 @@ class MetricsAggregationClient {
266266
this._captureTree(this._gauges, (node, name, tags) => {
267267
this._client.gauge(name, node.value, tags)
268268
})
269+
270+
this._gauges.clear()
269271
}
270272

271273
_captureCounters () {
@@ -278,12 +280,7 @@ class MetricsAggregationClient {
278280

279281
_captureHistograms () {
280282
this._captureTree(this._histograms, (node, name, tags) => {
281-
let stats = node.value
282-
283-
// Stats can contain garbage data when a value was never recorded.
284-
if (stats.count === 0) {
285-
stats = { max: 0, min: 0, sum: 0, avg: 0, median: 0, p95: 0, count: 0 }
286-
}
283+
const stats = node.value
287284

288285
this._client.gauge(`${name}.min`, stats.min, tags)
289286
this._client.gauge(`${name}.max`, stats.max, tags)
@@ -293,9 +290,9 @@ class MetricsAggregationClient {
293290
this._client.increment(`${name}.count`, stats.count, tags)
294291
this._client.gauge(`${name}.median`, stats.median, tags)
295292
this._client.gauge(`${name}.95percentile`, stats.p95, tags)
296-
297-
node.value.reset()
298293
})
294+
295+
this._histograms.clear()
299296
}
300297

301298
_captureTree (tree, fn) {

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ describe('dogstatsd', () => {
1515
let client
1616
let DogStatsDClient
1717
let CustomMetrics
18+
let MetricsAggregationClient
1819
let dgram
1920
let udp4
2021
let udp6
@@ -76,6 +77,7 @@ describe('dogstatsd', () => {
7677
})
7778
DogStatsDClient = dogstatsd.DogStatsDClient
7879
CustomMetrics = dogstatsd.CustomMetrics
80+
MetricsAggregationClient = dogstatsd.MetricsAggregationClient
7981

8082
httpData = []
8183
statusCode = 200
@@ -659,4 +661,75 @@ describe('dogstatsd', () => {
659661
assert.strictEqual(udp4.send.firstCall.args[0].toString(), 'test.avg:10|g|#foo:bar|c:ci-1234\n')
660662
})
661663
})
664+
665+
describe('MetricsAggregationClient', () => {
666+
let aggregator
667+
let gaugeCalls
668+
let incrementCalls
669+
670+
beforeEach(() => {
671+
gaugeCalls = []
672+
incrementCalls = []
673+
const inner = {
674+
gauge: (name, value, tags) => gaugeCalls.push([name, value, tags?.slice()]),
675+
increment: (name, value, tags) => incrementCalls.push([name, value, tags?.slice()]),
676+
distribution: () => {},
677+
histogram: () => {},
678+
flush: () => {},
679+
}
680+
aggregator = new MetricsAggregationClient(inner)
681+
})
682+
683+
it('emits a gauge once and then stays silent until it is set again', () => {
684+
aggregator.gauge('test.avg', 5)
685+
aggregator.flush()
686+
687+
assert.deepStrictEqual(gaugeCalls, [['test.avg', 5, []]])
688+
689+
gaugeCalls.length = 0
690+
aggregator.flush()
691+
aggregator.flush()
692+
693+
assert.deepStrictEqual(gaugeCalls, [])
694+
})
695+
696+
it('re-emits a gauge on every flush when it is updated each cycle', () => {
697+
for (let i = 1; i <= 3; i++) {
698+
aggregator.gauge('test.avg', i)
699+
aggregator.flush()
700+
}
701+
702+
assert.deepStrictEqual(gaugeCalls, [
703+
['test.avg', 1, []],
704+
['test.avg', 2, []],
705+
['test.avg', 3, []],
706+
])
707+
})
708+
709+
it('does not re-emit a histogram once observations stop', () => {
710+
aggregator.histogram('test.hist', 10)
711+
aggregator.flush()
712+
713+
assert(gaugeCalls.length > 0 && incrementCalls.length > 0)
714+
715+
gaugeCalls.length = 0
716+
incrementCalls.length = 0
717+
aggregator.flush()
718+
aggregator.flush()
719+
720+
assert.deepStrictEqual(gaugeCalls, [])
721+
assert.deepStrictEqual(incrementCalls, [])
722+
})
723+
724+
it('drains all metric trees on flush so cardinality is bounded', () => {
725+
aggregator.gauge('test.avg', 5, ['t:1'])
726+
aggregator.histogram('test.hist', 10, ['t:1'])
727+
aggregator.increment('test.count', 1, ['t:1'])
728+
aggregator.flush()
729+
730+
assert.strictEqual(aggregator._gauges.size, 0)
731+
assert.strictEqual(aggregator._histograms.size, 0)
732+
assert.strictEqual(aggregator._counters.size, 0)
733+
})
734+
})
662735
})

0 commit comments

Comments
 (0)