Skip to content

Commit b44467e

Browse files
rochdevrochdev
authored
fix sampling priority applied incorrectly (#1266)
1 parent ded7242 commit b44467e

File tree

6 files changed

+87
-13
lines changed

6 files changed

+87
-13
lines changed

‎packages/dd-trace/src/format.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ const id = require('./id')
77

88
const SAMPLING_PRIORITY_KEY = constants.SAMPLING_PRIORITY_KEY
99
const ANALYTICS_KEY = constants.ANALYTICS_KEY
10+
const SAMPLING_RULE_DECISION = constants.SAMPLING_RULE_DECISION
11+
const SAMPLING_LIMIT_DECISION = constants.SAMPLING_LIMIT_DECISION
12+
const SAMPLING_AGENT_DECISION = constants.SAMPLING_AGENT_DECISION
1013
const ANALYTICS = tags.ANALYTICS
1114
const MEASURED = tags.MEASURED
1215
const ORIGIN_KEY = constants.ORIGIN_KEY
@@ -22,6 +25,7 @@ function format (span) {
2225
const formatted = formatSpan(span)
2326

2427
extractError(formatted, span)
28+
extractRootTags(formatted, span)
2529
extractTags(formatted, span)
2630
extractAnalytics(formatted, span)
2731

@@ -96,6 +100,18 @@ function extractTags (trace, span) {
96100
addTag(trace.meta, trace.metrics, HOSTNAME_KEY, hostname)
97101
}
98102

103+
function extractRootTags (trace, span) {
104+
const context = span.context()
105+
const isLocalRoot = span === context._trace.started[0]
106+
const parentId = context._parentId
107+
108+
if (!isLocalRoot || (parentId && parentId.toString(10) !== '0')) return
109+
110+
addTag({}, trace.metrics, SAMPLING_RULE_DECISION, context._trace[SAMPLING_RULE_DECISION])
111+
addTag({}, trace.metrics, SAMPLING_LIMIT_DECISION, context._trace[SAMPLING_LIMIT_DECISION])
112+
addTag({}, trace.metrics, SAMPLING_AGENT_DECISION, context._trace[SAMPLING_AGENT_DECISION])
113+
}
114+
99115
function extractError (trace, span) {
100116
const error = span.context()._tags['error']
101117

‎packages/dd-trace/src/opentracing/span.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ class DatadogSpan extends Span {
113113

114114
_addTags (keyValuePairs) {
115115
tagger.add(this._spanContext._tags, keyValuePairs)
116+
117+
this._prioritySampler.sample(this, false)
116118
}
117119

118120
_finish (finishTime) {

‎packages/dd-trace/src/priority_sampler.js

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class PrioritySampler {
4141
: this._isSampledByAgent(context)
4242
}
4343

44-
sample (span) {
44+
sample (span, auto = true) {
4545
if (!span) return
4646

4747
const context = this._getContext(span)
@@ -55,7 +55,9 @@ class PrioritySampler {
5555
return
5656
}
5757

58-
context._sampling.priority = this.isSampled(span) ? AUTO_KEEP : AUTO_REJECT
58+
if (auto) {
59+
context._sampling.priority = this.isSampled(span) ? AUTO_KEEP : AUTO_REJECT
60+
}
5961
}
6062

6163
update (rates) {
@@ -106,15 +108,15 @@ class PrioritySampler {
106108
}
107109

108110
_isSampledByRule (context, rule) {
109-
context._tags[SAMPLING_RULE_DECISION] = rule.sampleRate
111+
context._trace[SAMPLING_RULE_DECISION] = rule.sampleRate
110112

111113
return rule.sampler.isSampled(context)
112114
}
113115

114116
_isSampledByRateLimit (context) {
115117
const allowed = this._limiter.isAllowed()
116118

117-
context._tags[SAMPLING_LIMIT_DECISION] = this._limiter.effectiveRate()
119+
context._trace[SAMPLING_LIMIT_DECISION] = this._limiter.effectiveRate()
118120

119121
return allowed
120122
}
@@ -123,7 +125,7 @@ class PrioritySampler {
123125
const key = `service:${context._tags[SERVICE_NAME]},env:${this._env}`
124126
const sampler = this._samplers[key] || this._samplers[DEFAULT_KEY]
125127

126-
context._tags[SAMPLING_AGENT_DECISION] = sampler.rate()
128+
context._trace[SAMPLING_AGENT_DECISION] = sampler.rate()
127129

128130
return sampler.isSampled(context)
129131
}

‎packages/dd-trace/test/format.spec.js

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ const ANALYTICS = tags.ANALYTICS
1010
const MEASURED = tags.MEASURED
1111
const ORIGIN_KEY = constants.ORIGIN_KEY
1212
const HOSTNAME_KEY = constants.HOSTNAME_KEY
13+
const SAMPLING_AGENT_DECISION = constants.SAMPLING_AGENT_DECISION
14+
const SAMPLING_LIMIT_DECISION = constants.SAMPLING_LIMIT_DECISION
15+
const SAMPLING_RULE_DECISION = constants.SAMPLING_RULE_DECISION
1316

1417
const spanId = id('0234567812345678')
1518

@@ -27,7 +30,9 @@ describe('format', () => {
2730
_tags: {},
2831
_metrics: {},
2932
_sampling: {},
30-
_trace: {},
33+
_trace: {
34+
started: []
35+
},
3136
_name: 'operation'
3237
}
3338

@@ -40,6 +45,8 @@ describe('format', () => {
4045
_duration: 100
4146
}
4247

48+
spanContext._trace.started.push(span)
49+
4350
format = require('../src/format')
4451
})
4552

@@ -84,6 +91,35 @@ describe('format', () => {
8491
expect(trace.resource).to.equal('resource')
8592
})
8693

94+
it('should extract Datadog specific root tags', () => {
95+
spanContext._parentId = null
96+
spanContext._trace[SAMPLING_AGENT_DECISION] = 0.8
97+
spanContext._trace[SAMPLING_LIMIT_DECISION] = 0.2
98+
spanContext._trace[SAMPLING_RULE_DECISION] = 0.5
99+
100+
trace = format(span)
101+
102+
expect(trace.metrics).to.include({
103+
[SAMPLING_AGENT_DECISION]: 0.8,
104+
[SAMPLING_LIMIT_DECISION]: 0.2,
105+
[SAMPLING_RULE_DECISION]: 0.5
106+
})
107+
})
108+
109+
it('should not extract Datadog specific root tags from non-root spans', () => {
110+
spanContext._trace[SAMPLING_AGENT_DECISION] = 0.8
111+
spanContext._trace[SAMPLING_LIMIT_DECISION] = 0.2
112+
spanContext._trace[SAMPLING_RULE_DECISION] = 0.5
113+
114+
trace = format(span)
115+
116+
expect(trace.metrics).to.not.have.keys(
117+
SAMPLING_AGENT_DECISION,
118+
SAMPLING_LIMIT_DECISION,
119+
SAMPLING_RULE_DECISION
120+
)
121+
})
122+
87123
it('should discard user-defined tags with name HOSTNAME_KEY by default', () => {
88124
spanContext._tags[HOSTNAME_KEY] = 'some_hostname'
89125

‎packages/dd-trace/test/opentracing/span.spec.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,14 @@ describe('Span', () => {
250250

251251
expect(tagger.add).to.have.been.calledWith(span.context()._tags, tags)
252252
})
253+
254+
it('should sample based on the tags', () => {
255+
const tags = { foo: 'bar' }
256+
257+
span.addTags(tags)
258+
259+
expect(prioritySampler.sample).to.have.been.calledWith(span, false)
260+
})
253261
})
254262

255263
describe('finish', () => {

‎packages/dd-trace/test/priority_sampler.spec.js

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ describe('PrioritySampler', () => {
2222
beforeEach(() => {
2323
context = {
2424
_tags: {},
25-
_sampling: {}
25+
_sampling: {},
26+
_trace: {
27+
started: []
28+
}
2629
}
2730

2831
span = {
@@ -262,7 +265,7 @@ describe('PrioritySampler', () => {
262265
it('should add metrics for agent sample rate', () => {
263266
prioritySampler.sample(span)
264267

265-
expect(context._tags).to.have.property('_dd.agent_psr', 1)
268+
expect(context._trace).to.have.property('_dd.agent_psr', 1)
266269
})
267270

268271
it('should add metrics for rule sample rate', () => {
@@ -271,8 +274,8 @@ describe('PrioritySampler', () => {
271274
})
272275
prioritySampler.sample(span)
273276

274-
expect(context._tags).to.have.property('_dd.rule_psr', 0)
275-
expect(context._tags).to.not.have.property('_dd.limit_psr')
277+
expect(context._trace).to.have.property('_dd.rule_psr', 0)
278+
expect(context._trace).to.not.have.property('_dd.limit_psr')
276279
})
277280

278281
it('should add metrics for rate limiter sample rate', () => {
@@ -282,14 +285,21 @@ describe('PrioritySampler', () => {
282285
})
283286
prioritySampler.sample(span)
284287

285-
expect(context._tags).to.have.property('_dd.rule_psr', 0.5)
286-
expect(context._tags).to.have.property('_dd.limit_psr', 1)
288+
expect(context._trace).to.have.property('_dd.rule_psr', 0.5)
289+
expect(context._trace).to.have.property('_dd.limit_psr', 1)
287290
})
288291

289292
it('should ignore empty span', () => {
293+
expect(() => {
294+
prioritySampler.sample()
295+
}).to.not.throw()
290296
prioritySampler.sample()
297+
})
298+
299+
it('should support manual only sampling', () => {
300+
prioritySampler.sample(span, false)
291301

292-
expect(context._sampling).to.not.have.property('priority')
302+
expect(context._sampling.priority).to.be.undefined
293303
})
294304
})
295305

0 commit comments

Comments
 (0)