From 4d83d5b57190f433ca268ce8d78832c67f543189 Mon Sep 17 00:00:00 2001 From: ET Date: Thu, 3 Sep 2020 07:28:01 -0700 Subject: [PATCH] Change name of ProbabilitySampler to TraceIdRatioBased (#1115) * Change name of ProbabilitySampler to TraceIdRatioBased * Modify behavior to ignore parent span * Add test for inclusivity property on TraceIdRatioBased sampler * Modify tests in `trace_test.go` to reflect change in parent span behavior * Add to CHANGELOG * Satisfy golint * Update CHANGELOG.md Co-authored-by: Tyler Yahn Co-authored-by: Tyler Yahn --- CHANGELOG.md | 4 ++ example/zipkin/main.go | 4 +- sdk/trace/sampling.go | 29 ++++++-------- sdk/trace/sampling_test.go | 81 ++++++++++++++++++++++++++------------ sdk/trace/trace_test.go | 50 ++++++++++++++--------- 5 files changed, 106 insertions(+), 62 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51bea2a5a..23b3fde9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - The `go.opentelemetry.io/otel/api/trace` `TracerOption` was changed to an interface to conform to project option conventions. (#1109) - Rename Jaeger tags used for instrumentation library information to reflect changes in OpenTelemetry specification. (#1119) +### Changed + +- Rename `ProbabilitySampler` to `TraceIDRatioBased` and change semantics to ignore parent span sampling status. (#1115) + ## [0.11.0] - 2020-08-24 ### Added diff --git a/example/zipkin/main.go b/example/zipkin/main.go index c853be6ae..a331de81c 100644 --- a/example/zipkin/main.go +++ b/example/zipkin/main.go @@ -36,8 +36,8 @@ func initTracer(url string) { // Create Zipkin Exporter and install it as a global tracer. // // For demoing purposes, always sample. In a production application, you should - // configure the sampler to a trace.ProbabilitySampler set at the desired - // probability. + // configure the sampler to a trace.ParentSampler(trace.TraceIDRatioBased) set at the desired + // ratio. err := zipkin.InstallNewPipeline( url, "zipkin-test", diff --git a/sdk/trace/sampling.go b/sdk/trace/sampling.go index bacdf40fd..8ff5b9b0a 100644 --- a/sdk/trace/sampling.go +++ b/sdk/trace/sampling.go @@ -55,32 +55,29 @@ type SamplingResult struct { Attributes []label.KeyValue } -type probabilitySampler struct { +type traceIDRatioSampler struct { traceIDUpperBound uint64 description string } -func (ps probabilitySampler) ShouldSample(p SamplingParameters) SamplingResult { - if p.ParentContext.IsSampled() { - return SamplingResult{Decision: RecordAndSampled} - } - +func (ts traceIDRatioSampler) ShouldSample(p SamplingParameters) SamplingResult { x := binary.BigEndian.Uint64(p.TraceID[0:8]) >> 1 - if x < ps.traceIDUpperBound { + if x < ts.traceIDUpperBound { return SamplingResult{Decision: RecordAndSampled} } return SamplingResult{Decision: NotRecord} } -func (ps probabilitySampler) Description() string { - return ps.description +func (ts traceIDRatioSampler) Description() string { + return ts.description } -// ProbabilitySampler samples a given fraction of traces. Fractions >= 1 will -// always sample. If the parent span is sampled, then it's child spans will -// automatically be sampled. Fractions < 0 are treated as zero, but spans may -// still be sampled if their parent is. -func ProbabilitySampler(fraction float64) Sampler { +// TraceIDRatioBased samples a given fraction of traces. Fractions >= 1 will +// always sample. Fractions < 0 are treated as zero. To respect the +// parent trace's `SampledFlag`, the `TraceIDRatioBased` sampler should be used +// as a delegate of a `Parent` sampler. +//nolint:golint // golint complains about stutter of `trace.TraceIDRatioBased` +func TraceIDRatioBased(fraction float64) Sampler { if fraction >= 1 { return AlwaysSample() } @@ -89,9 +86,9 @@ func ProbabilitySampler(fraction float64) Sampler { fraction = 0 } - return &probabilitySampler{ + return &traceIDRatioSampler{ traceIDUpperBound: uint64(fraction * (1 << 63)), - description: fmt.Sprintf("ProbabilitySampler{%g}", fraction), + description: fmt.Sprintf("TraceIDRatioBased{%g}", fraction), } } diff --git a/sdk/trace/sampling_test.go b/sdk/trace/sampling_test.go index fe9ae0803..c4a36b3b9 100644 --- a/sdk/trace/sampling_test.go +++ b/sdk/trace/sampling_test.go @@ -12,67 +12,98 @@ // See the License for the specific language governing permissions and // limitations under the License. -package trace_test +package trace import ( + "math/rand" "testing" - "go.opentelemetry.io/otel/api/trace" + "github.com/stretchr/testify/require" - sdktrace "go.opentelemetry.io/otel/sdk/trace" + api "go.opentelemetry.io/otel/api/trace" ) func TestAlwaysParentSampleWithParentSampled(t *testing.T) { - sampler := sdktrace.ParentSample(sdktrace.AlwaysSample()) - traceID, _ := trace.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") - spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.SpanContext{ + sampler := ParentSample(AlwaysSample()) + traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") + spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7") + parentCtx := api.SpanContext{ TraceID: traceID, SpanID: spanID, - TraceFlags: trace.FlagsSampled, + TraceFlags: api.FlagsSampled, } - if sampler.ShouldSample(sdktrace.SamplingParameters{ParentContext: parentCtx}).Decision != sdktrace.RecordAndSampled { + if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != RecordAndSampled { t.Error("Sampling decision should be RecordAndSampled") } } func TestNeverParentSampleWithParentSampled(t *testing.T) { - sampler := sdktrace.ParentSample(sdktrace.NeverSample()) - traceID, _ := trace.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") - spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.SpanContext{ + sampler := ParentSample(NeverSample()) + traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") + spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7") + parentCtx := api.SpanContext{ TraceID: traceID, SpanID: spanID, - TraceFlags: trace.FlagsSampled, + TraceFlags: api.FlagsSampled, } - if sampler.ShouldSample(sdktrace.SamplingParameters{ParentContext: parentCtx}).Decision != sdktrace.RecordAndSampled { + if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != RecordAndSampled { t.Error("Sampling decision should be RecordAndSampled") } } func TestAlwaysParentSampleWithParentNotSampled(t *testing.T) { - sampler := sdktrace.ParentSample(sdktrace.AlwaysSample()) - traceID, _ := trace.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") - spanID, _ := trace.SpanIDFromHex("00f067aa0ba902b7") - parentCtx := trace.SpanContext{ + sampler := ParentSample(AlwaysSample()) + traceID, _ := api.IDFromHex("4bf92f3577b34da6a3ce929d0e0e4736") + spanID, _ := api.SpanIDFromHex("00f067aa0ba902b7") + parentCtx := api.SpanContext{ TraceID: traceID, SpanID: spanID, } - if sampler.ShouldSample(sdktrace.SamplingParameters{ParentContext: parentCtx}).Decision != sdktrace.NotRecord { + if sampler.ShouldSample(SamplingParameters{ParentContext: parentCtx}).Decision != NotRecord { t.Error("Sampling decision should be NotRecord") } } func TestParentSampleWithNoParent(t *testing.T) { - params := sdktrace.SamplingParameters{} + params := SamplingParameters{} - sampler := sdktrace.ParentSample(sdktrace.AlwaysSample()) - if sampler.ShouldSample(params).Decision != sdktrace.RecordAndSampled { + sampler := ParentSample(AlwaysSample()) + if sampler.ShouldSample(params).Decision != RecordAndSampled { t.Error("Sampling decision should be RecordAndSampled") } - sampler = sdktrace.ParentSample(sdktrace.NeverSample()) - if sampler.ShouldSample(params).Decision != sdktrace.NotRecord { + sampler = ParentSample(NeverSample()) + if sampler.ShouldSample(params).Decision != NotRecord { t.Error("Sampling decision should be NotRecord") } } + +// TraceIDRatioBased sampler requirements state +// "A TraceIDRatioBased sampler with a given sampling rate MUST also sample +// all traces that any TraceIDRatioBased sampler with a lower sampling rate +// would sample." +func TestTraceIdRatioSamplesInclusively(t *testing.T) { + const ( + numSamplers = 1000 + numTraces = 100 + ) + idg := defIDGenerator() + + for i := 0; i < numSamplers; i++ { + ratioLo, ratioHi := rand.Float64(), rand.Float64() + if ratioHi < ratioLo { + ratioLo, ratioHi = ratioHi, ratioLo + } + samplerHi := TraceIDRatioBased(ratioHi) + samplerLo := TraceIDRatioBased(ratioLo) + for j := 0; j < numTraces; j++ { + traceID := idg.NewTraceID() + + params := SamplingParameters{TraceID: traceID} + if samplerLo.ShouldSample(params).Decision == RecordAndSampled { + require.Equal(t, RecordAndSampled, samplerHi.ShouldSample(params).Decision, + "%s sampled but %s did not", samplerLo.Description(), samplerHi.Description()) + } + } + } +} diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 8a35b2930..c7ac11a52 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -59,7 +59,7 @@ func init() { } func TestTracerFollowsExpectedAPIBehaviour(t *testing.T) { - tp, err := NewProvider(WithConfig(Config{DefaultSampler: ProbabilitySampler(0)})) + tp, err := NewProvider(WithConfig(Config{DefaultSampler: TraceIDRatioBased(0)})) if err != nil { t.Fatalf("failed to create provider, err: %v\n", err) } @@ -174,25 +174,37 @@ func TestSampling(t *testing.T) { sampledParent bool }{ // Span w/o a parent - "NeverSample": {sampler: NeverSample(), expect: 0}, - "AlwaysSample": {sampler: AlwaysSample(), expect: 1.0}, - "ProbabilitySampler_-1": {sampler: ProbabilitySampler(-1.0), expect: 0}, - "ProbabilitySampler_.25": {sampler: ProbabilitySampler(0.25), expect: .25}, - "ProbabilitySampler_.50": {sampler: ProbabilitySampler(0.50), expect: .5}, - "ProbabilitySampler_.75": {sampler: ProbabilitySampler(0.75), expect: .75}, - "ProbabilitySampler_2.0": {sampler: ProbabilitySampler(2.0), expect: 1}, - // Spans with a parent that is *not* sampled act like spans w/o a parent - "UnsampledParentSpanWithProbabilitySampler_-1": {sampler: ProbabilitySampler(-1.0), expect: 0, parent: true}, - "UnsampledParentSpanWithProbabilitySampler_.25": {sampler: ProbabilitySampler(.25), expect: .25, parent: true}, - "UnsampledParentSpanWithProbabilitySampler_.50": {sampler: ProbabilitySampler(0.50), expect: .5, parent: true}, - "UnsampledParentSpanWithProbabilitySampler_.75": {sampler: ProbabilitySampler(0.75), expect: .75, parent: true}, - "UnsampledParentSpanWithProbabilitySampler_2.0": {sampler: ProbabilitySampler(2.0), expect: 1, parent: true}, - // Spans with a parent that is sampled, will always sample, regardless of the probability - "SampledParentSpanWithProbabilitySampler_-1": {sampler: ProbabilitySampler(-1.0), expect: 1, parent: true, sampledParent: true}, - "SampledParentSpanWithProbabilitySampler_.25": {sampler: ProbabilitySampler(.25), expect: 1, parent: true, sampledParent: true}, - "SampledParentSpanWithProbabilitySampler_2.0": {sampler: ProbabilitySampler(2.0), expect: 1, parent: true, sampledParent: true}, - // Spans with a sampled parent, but when using the NeverSample Sampler, aren't sampled + "NeverSample": {sampler: NeverSample(), expect: 0}, + "AlwaysSample": {sampler: AlwaysSample(), expect: 1.0}, + "TraceIdRatioBased_-1": {sampler: TraceIDRatioBased(-1.0), expect: 0}, + "TraceIdRatioBased_.25": {sampler: TraceIDRatioBased(0.25), expect: .25}, + "TraceIdRatioBased_.50": {sampler: TraceIDRatioBased(0.50), expect: .5}, + "TraceIdRatioBased_.75": {sampler: TraceIDRatioBased(0.75), expect: .75}, + "TraceIdRatioBased_2.0": {sampler: TraceIDRatioBased(2.0), expect: 1}, + + // Spans w/o a parent and using ParentSample(DelegateSampler()) Sampler, receive DelegateSampler's sampling decision + "ParentNeverSample": {sampler: ParentSample(NeverSample()), expect: 0}, + "ParentAlwaysSample": {sampler: ParentSample(AlwaysSample()), expect: 1}, + "ParentTraceIdRatioBased_.50": {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: .5}, + + // An unadorned TraceIDRatioBased sampler ignores parent spans + "UnsampledParentSpanWithTraceIdRatioBased_.25": {sampler: TraceIDRatioBased(0.25), expect: .25, parent: true}, + "SampledParentSpanWithTraceIdRatioBased_.25": {sampler: TraceIDRatioBased(0.25), expect: .25, parent: true, sampledParent: true}, + "UnsampledParentSpanWithTraceIdRatioBased_.50": {sampler: TraceIDRatioBased(0.50), expect: .5, parent: true}, + "SampledParentSpanWithTraceIdRatioBased_.50": {sampler: TraceIDRatioBased(0.50), expect: .5, parent: true, sampledParent: true}, + "UnsampledParentSpanWithTraceIdRatioBased_.75": {sampler: TraceIDRatioBased(0.75), expect: .75, parent: true}, + "SampledParentSpanWithTraceIdRatioBased_.75": {sampler: TraceIDRatioBased(0.75), expect: .75, parent: true, sampledParent: true}, + + // Spans with a sampled parent but using NeverSample Sampler, are not sampled "SampledParentSpanWithNeverSample": {sampler: NeverSample(), expect: 0, parent: true, sampledParent: true}, + + // Spans with a sampled parent and using ParentSample(DelegateSampler()) Sampler, inherit the parent span's sampling status + "SampledParentSpanWithParentNeverSample": {sampler: ParentSample(NeverSample()), expect: 1, parent: true, sampledParent: true}, + "UnsampledParentSpanWithParentNeverSampler": {sampler: ParentSample(NeverSample()), expect: 0, parent: true, sampledParent: false}, + "SampledParentSpanWithParentAlwaysSampler": {sampler: ParentSample(AlwaysSample()), expect: 1, parent: true, sampledParent: true}, + "UnsampledParentSpanWithParentAlwaysSampler": {sampler: ParentSample(AlwaysSample()), expect: 0, parent: true, sampledParent: false}, + "SampledParentSpanWithParentTraceIdRatioBased_.50": {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: 1, parent: true, sampledParent: true}, + "UnsampledParentSpanWithParentTraceIdRatioBased_.50": {sampler: ParentSample(TraceIDRatioBased(0.50)), expect: 0, parent: true, sampledParent: false}, } { tc := tc t.Run(name, func(t *testing.T) {