From 464150d64371b0816982ec399ba00470292c7c7e Mon Sep 17 00:00:00 2001 From: Jason Anderson Date: Wed, 5 Mar 2025 17:38:50 -0800 Subject: [PATCH 1/6] align jaeger probabilistic sampler with traceidratiobased currently the jaegerremotesampler has a custom probabilistic sampler that does not align with how the traceid_ratiobased sampler works. it only looks at the first 64 bits of the trace; the opentelemetry-go sampler looks at the last 64 bits, because there are cases when trace IDs having 64 bits are converted to otel trace IDs, which have 128 bits, and when that happens the first 64 bits are typically ignored/zero-d out. rather than re-implement this logic, this just pulls in the traceid_ratiobased sampler so we re-use it, ensuring the decisions will be made similarly and predictably. --- samplers/jaegerremote/sampler.go | 31 +++++--------------- samplers/jaegerremote/sampler_remote_test.go | 2 +- samplers/jaegerremote/sampler_test.go | 12 ++++---- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/samplers/jaegerremote/sampler.go b/samplers/jaegerremote/sampler.go index 8be522f0f90..44c24618f30 100644 --- a/samplers/jaegerremote/sampler.go +++ b/samplers/jaegerremote/sampler.go @@ -19,7 +19,6 @@ package jaegerremote // import "go.opentelemetry.io/contrib/samplers/jaegerremote" import ( - "encoding/binary" "fmt" "math" "sync" @@ -39,25 +38,20 @@ const ( // probabilisticSampler is a sampler that randomly samples a certain percentage // of traces. type probabilisticSampler struct { - samplingRate float64 - samplingBoundary uint64 + samplingRate float64 + sampler trace.Sampler } -const maxRandomNumber = ^(uint64(1) << 63) // i.e. 0x7fffffffffffffff - // newProbabilisticSampler creates a sampler that randomly samples a certain percentage of traces specified by the -// samplingRate, in the range between 0.0 and 1.0. -// -// It relies on the fact that new trace IDs are 63bit random numbers themselves, thus making the sampling decision -// without generating a new random number, but simply calculating if traceID < (samplingRate * 2^63). +// samplingRate, in the range between 0.0 and 1.0. it utilizes the SDK `trace.TraceIDRatioBased` sampler. func newProbabilisticSampler(samplingRate float64) *probabilisticSampler { s := new(probabilisticSampler) return s.init(samplingRate) } func (s *probabilisticSampler) init(samplingRate float64) *probabilisticSampler { + s.sampler = trace.TraceIDRatioBased(samplingRate) s.samplingRate = math.Max(0.0, math.Min(samplingRate, 1.0)) - s.samplingBoundary = uint64(float64(maxRandomNumber) * s.samplingRate) return s } @@ -67,24 +61,13 @@ func (s *probabilisticSampler) SamplingRate() float64 { } func (s *probabilisticSampler) ShouldSample(p trace.SamplingParameters) trace.SamplingResult { - psc := oteltrace.SpanContextFromContext(p.ParentContext) - traceID := binary.BigEndian.Uint64(p.TraceID[0:8]) - if s.samplingBoundary >= traceID&maxRandomNumber { - return trace.SamplingResult{ - Decision: trace.RecordAndSample, - Tracestate: psc.TraceState(), - } - } - return trace.SamplingResult{ - Decision: trace.Drop, - Tracestate: psc.TraceState(), - } + return s.sampler.ShouldSample(p) } // Equal compares with another sampler. func (s *probabilisticSampler) Equal(other trace.Sampler) bool { if o, ok := other.(*probabilisticSampler); ok { - return s.samplingBoundary == o.samplingBoundary + return s.samplingRate == o.samplingRate } return false } @@ -99,7 +82,7 @@ func (s *probabilisticSampler) Update(samplingRate float64) error { } func (s *probabilisticSampler) Description() string { - return "probabilisticSampler{}" + return s.sampler.Description() } // ----------------------- diff --git a/samplers/jaegerremote/sampler_remote_test.go b/samplers/jaegerremote/sampler_remote_test.go index 19ecb0c9b1d..2a9768815db 100644 --- a/samplers/jaegerremote/sampler_remote_test.go +++ b/samplers/jaegerremote/sampler_remote_test.go @@ -165,7 +165,7 @@ func initAgent(t *testing.T) (*testutils.MockAgent, *Sampler) { func makeSamplingParameters(id uint64, operationName string) trace.SamplingParameters { var traceID oteltrace.TraceID - binary.BigEndian.PutUint64(traceID[:], id) + binary.BigEndian.PutUint64(traceID[8:], id) return trace.SamplingParameters{ TraceID: traceID, diff --git a/samplers/jaegerremote/sampler_test.go b/samplers/jaegerremote/sampler_test.go index 243536a5a4e..4c3bfd85a43 100644 --- a/samplers/jaegerremote/sampler_test.go +++ b/samplers/jaegerremote/sampler_test.go @@ -20,6 +20,7 @@ package jaegerremote import ( "encoding/binary" + "math" "testing" "github.com/stretchr/testify/assert" @@ -34,7 +35,7 @@ const ( testFirstTimeOperationName = "firstTimeOp" testDefaultSamplingProbability = 0.5 - testMaxID = uint64(1) << 62 + testMaxID = uint64(1) << 63 testDefaultMaxOperations = 10 ) @@ -42,18 +43,19 @@ func TestProbabilisticSampler(t *testing.T) { var traceID oteltrace.TraceID sampler := newProbabilisticSampler(0.5) - binary.BigEndian.PutUint64(traceID[:], testMaxID+10) + binary.BigEndian.PutUint64(traceID[8:], testMaxID+10) result := sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) assert.Equal(t, trace.Drop, result.Decision) - binary.BigEndian.PutUint64(traceID[:], testMaxID-20) + binary.BigEndian.PutUint64(traceID[8:], testMaxID-20) result = sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) assert.Equal(t, trace.RecordAndSample, result.Decision) t.Run("test_64bit_id", func(t *testing.T) { - binary.BigEndian.PutUint64(traceID[:], (testMaxID+10)|1<<63) + binary.BigEndian.PutUint64(traceID[:8], math.MaxUint64) + binary.BigEndian.PutUint64(traceID[8:], testMaxID+10) result = sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) assert.Equal(t, trace.Drop, result.Decision) - binary.BigEndian.PutUint64(traceID[:], (testMaxID-20)|1<<63) + binary.BigEndian.PutUint64(traceID[8:], testMaxID-20) result = sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) assert.Equal(t, trace.RecordAndSample, result.Decision) }) From bdd49a981925d701c1f22d39fc59b90caaebd3e1 Mon Sep 17 00:00:00 2001 From: Jason Anderson Date: Wed, 5 Mar 2025 21:53:39 -0800 Subject: [PATCH 2/6] add parity test --- samplers/jaegerremote/sampler_test.go | 42 +++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/samplers/jaegerremote/sampler_test.go b/samplers/jaegerremote/sampler_test.go index 4c3bfd85a43..4f81a9a14bb 100644 --- a/samplers/jaegerremote/sampler_test.go +++ b/samplers/jaegerremote/sampler_test.go @@ -19,8 +19,10 @@ package jaegerremote import ( + crand "crypto/rand" "encoding/binary" "math" + "math/rand" "testing" "github.com/stretchr/testify/assert" @@ -39,6 +41,30 @@ const ( testDefaultMaxOperations = 10 ) +type randomIDGenerator struct { + randSource *rand.Rand +} + +// NewTraceID returns a non-zero trace ID from a randomly-chosen sequence. +func (gen *randomIDGenerator) NewTraceID() oteltrace.TraceID { + tid := oteltrace.TraceID{} + for { + _, _ = gen.randSource.Read(tid[:]) + if tid.IsValid() { + break + } + } + return tid +} + +func defaultIDGenerator() *randomIDGenerator { + gen := &randomIDGenerator{} + var rngSeed int64 + _ = binary.Read(crand.Reader, binary.LittleEndian, &rngSeed) + gen.randSource = rand.New(rand.NewSource(rngSeed)) + return gen +} + func TestProbabilisticSampler(t *testing.T) { var traceID oteltrace.TraceID @@ -59,6 +85,22 @@ func TestProbabilisticSampler(t *testing.T) { result = sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) assert.Equal(t, trace.RecordAndSample, result.Decision) }) + + t.Run("test_parity", func(t *testing.T) { + numTests := 1000 + + sampler := newProbabilisticSampler(0.5) + oracle := trace.TraceIDRatioBased(0.5) + idGenerator := defaultIDGenerator() + + for range numTests { + traceID := idGenerator.NewTraceID() + assert.Equal(t, + oracle.ShouldSample(trace.SamplingParameters{TraceID: traceID}), + sampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}), + ) + } + }) } func TestRateLimitingSampler(t *testing.T) { From 9f9cadebc1f0380f0f37856fa7c07b1d51194416 Mon Sep 17 00:00:00 2001 From: Jason Anderson Date: Thu, 6 Mar 2025 13:42:14 -0800 Subject: [PATCH 3/6] address comments --- samplers/jaegerremote/sampler.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samplers/jaegerremote/sampler.go b/samplers/jaegerremote/sampler.go index 44c24618f30..9eb59319374 100644 --- a/samplers/jaegerremote/sampler.go +++ b/samplers/jaegerremote/sampler.go @@ -50,8 +50,8 @@ func newProbabilisticSampler(samplingRate float64) *probabilisticSampler { } func (s *probabilisticSampler) init(samplingRate float64) *probabilisticSampler { - s.sampler = trace.TraceIDRatioBased(samplingRate) s.samplingRate = math.Max(0.0, math.Min(samplingRate, 1.0)) + s.sampler = trace.TraceIDRatioBased(s.samplingRate) return s } @@ -67,7 +67,7 @@ func (s *probabilisticSampler) ShouldSample(p trace.SamplingParameters) trace.Sa // Equal compares with another sampler. func (s *probabilisticSampler) Equal(other trace.Sampler) bool { if o, ok := other.(*probabilisticSampler); ok { - return s.samplingRate == o.samplingRate + return math.Abs(s.samplingRate-o.samplingRate) < 1e-9 // consider equal if within 0.000001% } return false } From b8aa0bb3b37a386ce1a0bd423e43ad827f7e3457 Mon Sep 17 00:00:00 2001 From: Jason Anderson Date: Thu, 6 Mar 2025 14:13:56 -0800 Subject: [PATCH 4/6] add test coverage for Equals --- samplers/jaegerremote/sampler_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/samplers/jaegerremote/sampler_test.go b/samplers/jaegerremote/sampler_test.go index 4f81a9a14bb..323d1c7ebde 100644 --- a/samplers/jaegerremote/sampler_test.go +++ b/samplers/jaegerremote/sampler_test.go @@ -101,6 +101,14 @@ func TestProbabilisticSampler(t *testing.T) { ) } }) + + t.Run("Equals", func(t *testing.T) { + sampler := newProbabilisticSampler(0.5) + assert.True(t, sampler.Equal(newProbabilisticSampler(0.5))) + assert.False(t, sampler.Equal(newProbabilisticSampler(0.0))) + assert.False(t, sampler.Equal(newProbabilisticSampler(0.75))) + assert.False(t, sampler.Equal(newProbabilisticSampler(1.0))) + }) } func TestRateLimitingSampler(t *testing.T) { From 072b17b657a75c6a054caf3e868197ad5364cdd2 Mon Sep 17 00:00:00 2001 From: Jason Anderson Date: Thu, 6 Mar 2025 14:18:13 -0800 Subject: [PATCH 5/6] add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 818e63f9ec2..52443b49899 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Changed + +- Jaeger remote sampler's probabilistic strategy now uses the same sampling algorithm as TraceIDRatioBased. (#6892) + ### Removed - Drop support for [Go 1.22]. (#6853) From fa76e44911f1befb79deeb3d35abf87afd702180 Mon Sep 17 00:00:00 2001 From: Jason Anderson <796413+diurnalist@users.noreply.github.com> Date: Fri, 7 Mar 2025 10:04:01 -0800 Subject: [PATCH 6/6] Update CHANGELOG.md Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52443b49899..825db98005f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed -- Jaeger remote sampler's probabilistic strategy now uses the same sampling algorithm as TraceIDRatioBased. (#6892) +- Jaeger remote sampler's probabilistic strategy now uses the same sampling algorithm as `trace.TraceIDRatioBased` in `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#6892) ### Removed