Skip to content

Commit

Permalink
Change name of ProbabilitySampler to TraceIdRatioBased (#1115)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

Co-authored-by: Tyler Yahn <[email protected]>
  • Loading branch information
@evantorrie @MrAlias
evantorrie and MrAlias authored Sep 3, 2020
1 parent f38e190 commit 4d83d5b
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 62 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions example/zipkin/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
29 changes: 13 additions & 16 deletions sdk/trace/sampling.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand All @@ -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),
}
}

Expand Down
81 changes: 56 additions & 25 deletions sdk/trace/sampling_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
}
}
}
50 changes: 31 additions & 19 deletions sdk/trace/trace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 4d83d5b

Please sign in to comment.