Skip to content

Commit

Permalink
addressed my own comments :)
Browse files Browse the repository at this point in the history
  • Loading branch information
gbbr committed Dec 24, 2018
1 parent 0130d02 commit 06041ae
Show file tree
Hide file tree
Showing 7 changed files with 24 additions and 64 deletions.
4 changes: 0 additions & 4 deletions ddtrace/tracer/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ type config struct {
// sampler specifies the sampler that will be used for sampling traces.
sampler Sampler

// prioritySampling will be non-nil when priority sampling is enabled.
prioritySampling *prioritySampler

// agentAddr specifies the hostname and of the agent where the traces
// are sent to.
agentAddr string
Expand All @@ -50,7 +47,6 @@ func defaults(c *config) {
c.serviceName = filepath.Base(os.Args[0])
c.sampler = NewAllSampler()
c.agentAddr = defaultAddress
c.prioritySampling = newPrioritySampler()
}

// WithPrioritySampling is deprecated, and priority sampling is enabled by default.
Expand Down
13 changes: 0 additions & 13 deletions ddtrace/tracer/sampler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,19 +167,6 @@ func TestRateSampler(t *testing.T) {
assert.False(NewRateSampler(1).Sample(internal.NoopSpan{}))
}

func TestRateSamplerFinishedSpan(t *testing.T) {
rs := NewRateSampler(0.9999)
tracer := newTracer(WithSampler(rs)) // high probability of sampling
span := newBasicSpan("test")
span.finished = true
tracer.sample(span)
if !rs.Sample(span) {
t.Skip("wasn't sampled") // no flaky tests
}
_, ok := span.Metrics[sampleRateMetricKey]
assert.False(t, ok)
}

func TestRateSamplerSetting(t *testing.T) {
assert := assert.New(t)
rs := NewRateSampler(1)
Expand Down
6 changes: 2 additions & 4 deletions ddtrace/tracer/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,7 @@ func TestSpanSetMetric(t *testing.T) {
span.SetTag("bytes", 1024.42)
assert.Equal(3, len(span.Metrics))
assert.Equal(1024.42, span.Metrics["bytes"])
var ok bool
_, ok = span.Metrics[samplingPriorityKey]
_, ok := span.Metrics[samplingPriorityKey]
assert.True(ok)
_, ok = span.Metrics[samplingPriorityRateKey]
assert.True(ok)
Expand Down Expand Up @@ -302,8 +301,7 @@ func TestSpanSamplingPriority(t *testing.T) {
tracer := newTracer(withTransport(newDefaultTransport()))

span := tracer.newRootSpan("my.name", "my.service", "my.resource")
var ok bool
_, ok = span.Metrics[samplingPriorityKey]
_, ok := span.Metrics[samplingPriorityKey]
assert.True(ok)
_, ok = span.Metrics[samplingPriorityRateKey]
assert.True(ok)
Expand Down
6 changes: 1 addition & 5 deletions ddtrace/tracer/spancontext.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,7 @@ func (c *spanContext) setSamplingPriority(p int) {
defer c.mu.Unlock()
c.priority = p
c.hasPriority = true
if p == ext.PriorityAutoKeep || p == ext.PriorityUserKeep {
c.sampled = true
} else {
c.sampled = false
}
c.sampled = p == ext.PriorityAutoKeep || p == ext.PriorityUserKeep
}

func (c *spanContext) samplingPriority() int {
Expand Down
3 changes: 1 addition & 2 deletions ddtrace/tracer/textmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ func (p *propagator) extractTextMap(reader TextMapReader) (ddtrace.SpanContext,
return ErrSpanContextCorrupted
}
case p.cfg.PriorityHeader:
var priority int
priority, err = strconv.Atoi(v)
priority, err := strconv.Atoi(v)
if err != nil {
return ErrSpanContextCorrupted
}
Expand Down
51 changes: 20 additions & 31 deletions ddtrace/tracer/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ type tracer struct {
// a synchronous (blocking) operation, meaning that it will only return after
// the trace has been fully processed and added onto the payload.
syncPush chan struct{}

// prioritySampling holds an instance of the priority sampler.
prioritySampling *prioritySampler
}

const (
Expand Down Expand Up @@ -118,15 +121,16 @@ func newTracer(opts ...StartOption) *tracer {
c.propagator = NewPropagator(nil)
}
t := &tracer{
config: c,
payload: newPayload(),
flushAllReq: make(chan chan<- struct{}),
flushTracesReq: make(chan struct{}, 1),
flushErrorsReq: make(chan struct{}, 1),
exitReq: make(chan struct{}),
payloadQueue: make(chan []*span, payloadQueueSize),
errorBuffer: make(chan error, errorBufferSize),
stopped: make(chan struct{}),
config: c,
payload: newPayload(),
flushAllReq: make(chan chan<- struct{}),
flushTracesReq: make(chan struct{}, 1),
flushErrorsReq: make(chan struct{}, 1),
exitReq: make(chan struct{}),
payloadQueue: make(chan []*span, payloadQueueSize),
errorBuffer: make(chan error, errorBufferSize),
stopped: make(chan struct{}),
prioritySampling: newPrioritySampler(),
}

go t.worker()
Expand Down Expand Up @@ -307,8 +311,8 @@ func (t *tracer) flushTraces() {
if err != nil {
t.pushError(&dataLossError{context: err, count: count})
}
if err == nil && t.config.prioritySampling != nil {
t.config.prioritySampling.readRatesJSON(rc) // TODO: handle error?
if err == nil {
t.prioritySampling.readRatesJSON(rc) // TODO: handle error?
}
t.payload.reset()
}
Expand Down Expand Up @@ -357,33 +361,18 @@ const sampleRateMetricKey = "_sample_rate"

// Sample samples a span with the internal sampler.
func (t *tracer) sample(span *span) {
// Apply priority sampling
if t.config.prioritySampling != nil {
// Only apply if a priority hasn't been set yet.
// It may have been set by tags during initialization.
if !span.context.hasPriority {
t.config.prioritySampling.apply(span)
}
return
}

// TODO: Decide if this code and related tests should be removed.
sampler := t.config.sampler
sampled := sampler.Sample(span)
span.context.sampled = sampled
if !sampled {
return
}

if rs, ok := sampler.(RateSampler); ok && rs.Rate() < 1 {
// the span was sampled using a rate sampler which wasn't all permissive,
// so we make note of the sampling rate.
span.Lock()
defer span.Unlock()
if span.finished {
// we don't touch finished span as they might be flushing
return
}
span.Metrics[sampleRateMetricKey] = rs.Rate()
}
if !span.context.hasPriority {
// no priority was set by the user, so we make our own decision
// using our priority sampler.
t.prioritySampling.apply(span)
}
}
5 changes: 0 additions & 5 deletions ddtrace/tracer/tracer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,8 +358,6 @@ func TestTracerSampler(t *testing.T) {
withTransport(newDefaultTransport()),
WithSampler(sampler),
)
// override the default (priority sampling) within this test
tracer.config.prioritySampling = nil

span := tracer.newRootSpan("pylons.request", "pylons", "/")

Expand Down Expand Up @@ -448,15 +446,12 @@ func TestTracerEdgeSampler(t *testing.T) {
withTransport(newDefaultTransport()),
WithSampler(NewRateSampler(0)),
)
// avoid using priority sampling for this test
tracer0.config.prioritySampling = nil
defer stop()
// a sample rate of 1 should sample everything
tracer1, _, stop := startTestTracer(
withTransport(newDefaultTransport()),
WithSampler(NewRateSampler(1)),
)
tracer1.config.prioritySampling = nil
defer stop()

count := payloadQueueSize / 3
Expand Down

0 comments on commit 06041ae

Please sign in to comment.