Do not apply adaptive sampler to child spans #457
Conversation
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #457 +/- ##
==========================================
+ Coverage 87.39% 87.74% +0.34%
==========================================
Files 59 59
Lines 3476 3484 +8
==========================================
+ Hits 3038 3057 +19
+ Misses 325 315 -10
+ Partials 113 112 -1
Continue to review full report at Codecov.
|
sampler.go
Outdated
| return SamplingDecision{Sample: sampled, Retryable: true, Tags: tags} | ||
| } | ||
|
|
||
| // OnSetOperationName implements OnSetOperationName of SamplerV2. | ||
| func (s *AdaptiveSampler) OnSetOperationName(span *Span, operationName string) SamplingDecision { | ||
| samplerV1 := s.getSamplerForOperation(operationName) | ||
| sampled, tags := samplerV1.IsSampled(span.context.TraceID(), operationName) | ||
| sampled := false |
There was a problem hiding this comment.
shouldn't the logic be:
if root:
isSampled()
else
inherent from context?
There was a problem hiding this comment.
It's not the business of this sampler to "inherit from context", that would be done by the tracer when appropriate. If this sampler is called, it needs to make a decision based on the data it has.
sampler.go
Outdated
| @@ -365,14 +365,22 @@ func (s *AdaptiveSampler) IsSampled(id TraceID, operation string) (bool, []Tag) | |||
| func (s *AdaptiveSampler) OnCreateSpan(span *Span) SamplingDecision { | |||
| operationName := span.OperationName() | |||
| samplerV1 := s.getSamplerForOperation(operationName) | |||
| sampled, tags := samplerV1.IsSampled(span.context.TraceID(), operationName) | |||
| sampled := false | |||
There was a problem hiding this comment.
small nit: seems like some copy pasta here could be put into single method
| @@ -290,6 +293,7 @@ func (t *Tracer) startSpanWithOptions( | |||
| ctx.samplingState = parent.samplingState | |||
| if parent.remote { | |||
| ctx.samplingState.setFinal() | |||
| ctx.samplingState.localRootSpan = ctx.spanID | |||
There was a problem hiding this comment.
What would this do? I thought for remote, it's just final and there-fore no samplers should be called?
There was a problem hiding this comment.
for consistency of localRootSpan property the assignment makes sense, but doesn't affect anything today.
Signed-off-by: Yuri Shkuro <[email protected]>
sampler.go
Outdated
| @@ -361,18 +361,23 @@ func (s *AdaptiveSampler) IsSampled(id TraceID, operation string) (bool, []Tag) | |||
| return false, nil | |||
| } | |||
|
|
|||
| func (s *AdaptiveSampler) trySampling(span *Span, operationName string) (sampled bool, tags []Tag) { | |||
There was a problem hiding this comment.
nit: I don't see these named return values frequently in Jaeger. Why use them here?
There was a problem hiding this comment.
It is technically idiomatic Go, makes the code shorter, and the signature self-documenting.
But I can change to vars.
Signed-off-by: Yuri Shkuro <[email protected]>
Which problem is this PR solving?
Per-operation adaptive sampling should only apply to service endpoints, not to operation names of the inner/children spans. Otherwise it's a problem for adaptive sampling backend that only records operations from the root spans, meaning all child operations are getting the default probability, which may be higher than the calculated probabilities.
Short description of the changes