fix NewRandomRatioBased panic

Author: alanprot

pkg/tracing/sampler/sampling.go

@@ -1,39 +1,37 @@
 package sampler
 
 import (
+	"encoding/binary"
 	"fmt"
+	"math"
 
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/trace"
 )
 
-type randGenerator interface {
-	Float64() float64
-}
-
 type RandomRatioBased struct {
 	sdktrace.Sampler
-	rnd      randGenerator
-	fraction float64
+	max uint64
 }
 
 // NewRandomRatioBased creates a sampler based on random number.
 // fraction parameter should be between 0 and 1 where:
 // fraction >= 1 it will always sample
 // fraction <= 0 it will never sample
-func NewRandomRatioBased(fraction float64, rnd randGenerator) sdktrace.Sampler {
+func NewRandomRatioBased(fraction float64) sdktrace.Sampler {
 	if fraction >= 1 {
 		return sdktrace.AlwaysSample()
 	} else if fraction <= 0 {
 		return sdktrace.NeverSample()
 	}
 
-	return &RandomRatioBased{rnd: rnd, fraction: fraction}
+	return &RandomRatioBased{max: uint64(fraction * math.MaxUint64)}
 }
 
 func (s *RandomRatioBased) ShouldSample(p sdktrace.SamplingParameters) sdktrace.SamplingResult {
+	val := binary.BigEndian.Uint64(p.TraceID[8:16])
 	psc := trace.SpanContextFromContext(p.ParentContext)
-	shouldSample := s.rnd.Float64() < s.fraction
+	shouldSample := val < s.max
 	if shouldSample {
 		return sdktrace.SamplingResult{
 			Decision:   sdktrace.RecordAndSample,
@@ -47,5 +45,5 @@ func (s *RandomRatioBased) ShouldSample(p sdktrace.SamplingParameters) sdktrace.
 }
 
 func (s *RandomRatioBased) Description() string {
-	return fmt.Sprintf("RandomRatioBased{%g}", s.fraction)
+	return fmt.Sprintf("RandomRatioBased{%v}", s.max)
 }

pkg/tracing/sampler/sampling_test.go

@@ -2,78 +2,72 @@ package sampler
 
 import (
 	"context"
-	"math/rand"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/contrib/propagators/aws/xray"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/trace"
 )
 
-type mockGenerator struct {
-	mockedValue float64
-}
-
-func (g *mockGenerator) Float64() float64 {
-	return g.mockedValue
-}
-
 func Test_ShouldSample(t *testing.T) {
-	traceID, _ := trace.TraceIDFromHex("4bf92f3577b34da6a3ce929d0e0e4736")
 	parentCtx := trace.ContextWithSpanContext(
 		context.Background(),
 		trace.NewSpanContext(trace.SpanContextConfig{
 			TraceState: trace.TraceState{},
 		}),
 	)
 
+	generator := xray.NewIDGenerator()
+
 	tests := []struct {
-		name             string
-		samplingDecision sdktrace.SamplingDecision
-		fraction         float64
-		generator        randGenerator
+		name     string
+		fraction float64
 	}{
 		{
-			name:             "should always sample",
-			samplingDecision: sdktrace.RecordAndSample,
-			fraction:         1,
-			generator:        rand.New(rand.NewSource(rand.Int63())),
+			name:     "should always sample",
+			fraction: 1,
 		},
 		{
-			name:             "should nerver sample",
-			samplingDecision: sdktrace.Drop,
-			fraction:         0,
-			generator:        rand.New(rand.NewSource(rand.Int63())),
+			name:     "should nerver sample",
+			fraction: 0,
 		},
 		{
-			name:             "should sample when fraction is above generated",
-			samplingDecision: sdktrace.RecordAndSample,
-			fraction:         0.5,
-			generator:        &mockGenerator{0.2},
+			name:     "should sample 50%",
+			fraction: 0.5,
 		},
 		{
-			name:             "should not sample when fraction is not above generated",
-			samplingDecision: sdktrace.Drop,
-			fraction:         0.5,
-			generator:        &mockGenerator{0.8},
+			name:     "should sample 10%",
+			fraction: 0.1,
+		},
+		{
+			name:     "should sample 1%",
+			fraction: 0.01,
 		},
 	}
 
+	totalIterations := 10000
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			s := NewRandomRatioBased(tt.fraction, tt.generator)
-			for i := 0; i < 100; i++ {
-
+			totalSampled := 0
+			s := NewRandomRatioBased(tt.fraction)
+			for i := 0; i < totalIterations; i++ {
+				traceId, _ := generator.NewIDs(context.Background())
 				r := s.ShouldSample(
 					sdktrace.SamplingParameters{
 						ParentContext: parentCtx,
-						TraceID:       traceID,
+						TraceID:       traceId,
 						Name:          "test",
 						Kind:          trace.SpanKindServer,
 					})
-
-				require.Equal(t, tt.samplingDecision, r.Decision)
+				if r.Decision == sdktrace.RecordAndSample {
+					totalSampled += 1
+				}
 			}
+
+			tolerance := 0.1
+			expected := tt.fraction * float64(totalIterations)
+			require.InDelta(t, expected, totalSampled, expected*tolerance)
 		})
 	}
 }

pkg/tracing/tracing.go

@@ -4,9 +4,7 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"math/rand"
 	"strings"
-	"time"
 
 	"github.com/go-kit/log/level"
 	"github.com/pkg/errors"
@@ -134,7 +132,7 @@ func newTraceProvider(r *resource.Resource, c Config, exporter *otlptrace.Export
 	switch strings.ToLower(c.Otel.ExporterType) {
 	case "awsxray":
 		options = append(options, sdktrace.WithIDGenerator(xray.NewIDGenerator()))
-		options = append(options, sdktrace.WithSampler(sdktrace.ParentBased(sampler.NewRandomRatioBased(c.Otel.SampleRatio, rand.New(rand.NewSource(time.Now().Unix()))))))
+		options = append(options, sdktrace.WithSampler(sdktrace.ParentBased(sampler.NewRandomRatioBased(c.Otel.SampleRatio))))
 	default:
 		options = append(options, sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(c.Otel.SampleRatio))))
 	}