Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Do not apply adaptive sampler to child spans #457

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions propagation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,11 @@ func TestSpanPropagator(t *testing.T) {
t.Fatalf("%d: ParentID %d does not match expectation %d", i, a, e)
} else {
// Prepare for comparison.
sp.context.spanID, sp.context.parentID = exp.context.SpanID(), 0
sp.duration, sp.startTime = exp.duration, exp.startTime
}
assert.Equal(t, exp.context, sp.context, formatName)
assert.Equal(t, exp.context.traceID, sp.context.traceID, formatName)
assert.Equal(t, exp.context.Flags(), sp.context.Flags(), formatName)
assert.Equal(t, exp.context.baggage, sp.context.baggage, formatName)
assert.Equal(t, "span.kind", sp.tags[0].key)
assert.Equal(t, expTags, sp.tags[1:] /*skip span.kind tag*/, formatName)
assert.Empty(t, sp.logs, formatName)
Expand Down
15 changes: 10 additions & 5 deletions sampler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't see these named return values frequently in Jaeger. Why use them here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is technically idiomatic Go, makes the code shorter, and the signature self-documenting.

But I can change to vars.

samplerV1 := s.getSamplerForOperation(operationName)
if span.context.samplingState.isLocalRootSpan(span.context.spanID) {
sampled, tags = samplerV1.IsSampled(span.context.TraceID(), operationName)
}
return
}

// OnCreateSpan implements OnCreateSpan of SamplerV2.
func (s *AdaptiveSampler) OnCreateSpan(span *Span) SamplingDecision {
operationName := span.OperationName()
samplerV1 := s.getSamplerForOperation(operationName)
sampled, tags := samplerV1.IsSampled(span.context.TraceID(), operationName)
sampled, tags := s.trySampling(span, span.OperationName())
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, tags := s.trySampling(span, operationName)
return SamplingDecision{Sample: sampled, Retryable: false, Tags: tags}
}

Expand Down
53 changes: 48 additions & 5 deletions sampler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"sync"
"testing"

"github.com/opentracing/opentracing-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -273,6 +274,48 @@ func TestMaxOperations(t *testing.T) {
assert.Equal(t, testProbabilisticExpectedTags, decision.Tags)
}

func TestAdaptiveSamplerDoesNotApplyToChildrenSpans(t *testing.T) {
strategies := &sampling.PerOperationSamplingStrategies{
DefaultSamplingProbability: 0,
DefaultLowerBoundTracesPerSecond: 0,
PerOperationStrategies: []*sampling.OperationSamplingStrategy{
{
Operation: "op1",
ProbabilisticSampling: &sampling.ProbabilisticSamplingStrategy{SamplingRate: 0.0},
},
{
Operation: "op2",
ProbabilisticSampling: &sampling.ProbabilisticSamplingStrategy{SamplingRate: 1.0},
},
},
}

sampler, err := NewAdaptiveSampler(strategies, 1)
assert.NoError(t, err)
tracer, closer := NewTracer("service", sampler, NewNullReporter())
defer closer.Close()

_ = tracer.StartSpan("op1") // exhaust lower bound sampler once
span1 := tracer.StartSpan("op1")
assert.False(t, span1.Context().(SpanContext).IsSampled(), "op1 should not be sampled on root span")
span1.SetOperationName("op2")
assert.True(t, span1.Context().(SpanContext).IsSampled(), "op2 should be sampled on root span")

span2 := tracer.StartSpan("op2")
assert.True(t, span2.Context().(SpanContext).IsSampled(), "op2 should be sampled on root span")

parent := tracer.StartSpan("op1")
assert.False(t, parent.Context().(SpanContext).IsSampled(), "parent span should not be sampled")
assert.False(t, parent.Context().(SpanContext).IsSamplingFinalized(), "parent span should not be finalized")

child := tracer.StartSpan("op2", opentracing.ChildOf(parent.Context()))
assert.False(t, child.Context().(SpanContext).IsSampled(), "child span should not be sampled even with op2")
assert.False(t, child.Context().(SpanContext).IsSamplingFinalized(), "child span should not be finalized")
child.SetOperationName("op2")
assert.False(t, child.Context().(SpanContext).IsSampled(), "op2 should not be sampled on the child span")
assert.True(t, child.Context().(SpanContext).IsSamplingFinalized(), "child span should be finalized after setOperationName")
}

func TestAdaptiveSampler_lockRaceCondition(t *testing.T) {
agent, remoteSampler, _ := initAgent(t)
defer agent.Close()
Expand All @@ -290,18 +333,18 @@ func TestAdaptiveSampler_lockRaceCondition(t *testing.T) {
// Overwrite the sampler with an adaptive sampler
remoteSampler.sampler = adaptiveSampler

tracer, closer := NewTracer("service", remoteSampler, NewNullReporter())
defer closer.Close()

var wg sync.WaitGroup
defer wg.Wait()
wg.Add(2)

isSampled := func(t *testing.T, remoteSampler *RemotelyControlledSampler, numOperations int, operationNamePrefix string) {
for i := 0; i < numOperations; i++ {
runtime.Gosched()
span := &Span{
operationName: fmt.Sprintf("%s%d", operationNamePrefix, i),
}
decision := remoteSampler.OnCreateSpan(span)
assert.True(t, decision.Sample)
span := tracer.StartSpan(fmt.Sprintf("%s%d", operationNamePrefix, i))
assert.True(t, span.Context().(SpanContext).IsSampled())
}
}

Expand Down
7 changes: 7 additions & 0 deletions span_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,18 @@ type samplingState struct {
// like SetOperationName / SetTag, and the spans will remain writable.
final atomic.Bool

// localRootSpan stores the SpanID of the first span created in this process for a given trace.
localRootSpan SpanID

// extendedState allows samplers to keep intermediate state.
// The keys and values in this map are completely opaque: interface{} -> interface{}.
extendedState sync.Map
}

func (s *samplingState) isLocalRootSpan(id SpanID) bool {
return id == s.localRootSpan
}

func (s *samplingState) setFlag(newFlag int32) {
swapped := false
for !swapped {
Expand Down
6 changes: 5 additions & 1 deletion tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type Tracer struct {
// NewTracer creates Tracer implementation that reports tracing to Jaeger.
// The returned io.Closer can be used in shutdown hooks to ensure that the internal
// queue of the Reporter is drained and all buffered spans are submitted to collectors.
// TODO (breaking change) return *Tracer only, without closer.
func NewTracer(
serviceName string,
sampler Sampler,
Expand Down Expand Up @@ -272,7 +273,9 @@ func (t *Tracer) startSpanWithOptions(
}
ctx.spanID = SpanID(ctx.traceID.Low)
ctx.parentID = 0
ctx.samplingState = &samplingState{}
ctx.samplingState = &samplingState{
localRootSpan: ctx.spanID,
}
if hasParent && parent.isDebugIDContainerOnly() && t.isDebugAllowed(operationName) {
ctx.samplingState.setDebugAndSampled()
internalTags = append(internalTags, Tag{key: JaegerDebugHeader, value: parent.debugID})
Expand All @@ -290,6 +293,7 @@ func (t *Tracer) startSpanWithOptions(
ctx.samplingState = parent.samplingState
if parent.remote {
ctx.samplingState.setFinal()
ctx.samplingState.localRootSpan = ctx.spanID

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would this do? I thought for remote, it's just final and there-fore no samplers should be called?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency of localRootSpan property the assignment makes sense, but doesn't affect anything today.

}
}
if hasParent {
Expand Down