Tracing: Fix sampler defaults (#5887)

* Fix sampler defaults Signed-off-by: Matej Gera <matejgera@gmail.com> * Add CHANGELOG Signed-off-by: Matej Gera <matejgera@gmail.com> * Replace checkout with git-shallow-clone (#5829) Signed-off-by: Matej Gera <matejgera@gmail.com> Signed-off-by: Matej Gera <matejgera@gmail.com> Signed-off-by: Matej Gera <matejgera@gmail.com> Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
thanos-io · Jan 12, 2023 · 95256a7 · 95256a7
1 parent b6970c0
commit 95256a7
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
 ### Fixed
 
 - [#5995] (https://github.com/thanos-io/thanos/pull/5993) Sidecar: Loads the TLS certificate during startup.
+- [#5887](https://github.com/thanos-io/thanos/pull/5887) Tracing: Make sure rate limiting sampler is the default, as was the case in version pre-0.29.0.
 
 ## [v0.30.1](https://github.com/thanos-io/thanos/tree/release-0.30) - 4.01.2023
 

diff --git a/pkg/tracing/jaeger/config_yaml.go b/pkg/tracing/jaeger/config_yaml.go
@@ -19,6 +19,13 @@ import (
 	tracesdk "go.opentelemetry.io/otel/sdk/trace"
 )
 
+const (
+	SamplerTypeRemote        = "remote"
+	SamplerTypeProbabilistic = "probabilistic"
+	SamplerTypeConstant      = "const"
+	SamplerTypeRateLimiting  = "ratelimiting"
+)
+
 type ParentBasedSamplerConfig struct {
 	LocalParentSampled  bool `yaml:"local_parent_sampled"`
 	RemoteParentSampled bool `yaml:"remote_parent_sampled"`
@@ -114,40 +121,48 @@ func getSamplingFraction(samplerType string, samplingFactor float64) float64 {
 
 func getSampler(config Config) tracesdk.Sampler {
 	samplerType := config.SamplerType
+	if samplerType == "" {
+		samplerType = SamplerTypeRateLimiting
+	}
 	samplingFraction := getSamplingFraction(samplerType, config.SamplerParam)
 
 	var sampler tracesdk.Sampler
 	switch samplerType {
-	case "probabilistic":
-		sampler = tracesdk.ParentBased(tracesdk.TraceIDRatioBased(samplingFraction))
-	case "const":
+	case SamplerTypeProbabilistic:
+		sampler = tracesdk.TraceIDRatioBased(samplingFraction)
+	case SamplerTypeConstant:
 		if samplingFraction == 1.0 {
 			sampler = tracesdk.AlwaysSample()
 		} else {
 			sampler = tracesdk.NeverSample()
 		}
-	case "remote":
+	case SamplerTypeRemote:
 		remoteOptions := getRemoteOptions(config)
 		sampler = jaegerremote.New(config.ServiceName, remoteOptions...)
-	case "ratelimiting":
+	// Fallback always to default (rate limiting).
+	case SamplerTypeRateLimiting:
+	default:
 		// The same config options are applicable to both remote and rate-limiting samplers.
 		remoteOptions := getRemoteOptions(config)
 		sampler = jaegerremote.New(config.ServiceName, remoteOptions...)
 		sampler, ok := sampler.(*rateLimitingSampler)
 		if ok {
 			sampler.Update(config.SamplerParam)
 		}
-	default:
-		var root tracesdk.Sampler
-		var parentOptions []tracesdk.ParentBasedSamplerOption
-		if config.SamplerParentConfig.LocalParentSampled {
-			parentOptions = append(parentOptions, tracesdk.WithLocalParentSampled(root))
-		}
-		if config.SamplerParentConfig.RemoteParentSampled {
-			parentOptions = append(parentOptions, tracesdk.WithRemoteParentSampled(root))
-		}
-		sampler = tracesdk.ParentBased(root, parentOptions...)
 	}
+
+	// Use parent-based to make sure we respect the span parent, if
+	// it is sampled. Optionally, allow user to specify the
+	// parent-based options.
+	var parentOptions []tracesdk.ParentBasedSamplerOption
+	if config.SamplerParentConfig.LocalParentSampled {
+		parentOptions = append(parentOptions, tracesdk.WithLocalParentSampled(sampler))
+	}
+	if config.SamplerParentConfig.RemoteParentSampled {
+		parentOptions = append(parentOptions, tracesdk.WithRemoteParentSampled(sampler))
+	}
+	sampler = tracesdk.ParentBased(sampler, parentOptions...)
+
 	return sampler
 }
 

diff --git a/pkg/tracing/jaeger/jaeger_test.go b/pkg/tracing/jaeger/jaeger_test.go
@@ -24,7 +24,7 @@ var parentConfig = ParentBasedSamplerConfig{LocalParentSampled: true}
 
 // This test shows that if sample factor will enable tracing on client process, even when it would be disabled on server
 // it will be still enabled for all spans within this span.
-func TestContextTracing_ClientEnablesTracing(t *testing.T) {
+func TestContextTracing_ClientEnablesProbabilisticTracing(t *testing.T) {
 	exp := tracetest.NewInMemoryExporter()
 	config := Config{
 		SamplerType:         "probabilistic",
@@ -65,7 +65,7 @@ func TestContextTracing_ClientEnablesTracing(t *testing.T) {
 
 // This test shows that if sample factor will disable tracing on client process,  when it would be enabled on server
 // it will be still disabled for all spans within this span.
-func TestContextTracing_ClientDisablesTracing(t *testing.T) {
+func TestContextTracing_ClientDisablesProbabilisticTracing(t *testing.T) {
 	exp := tracetest.NewInMemoryExporter()
 
 	config := Config{
@@ -105,6 +105,45 @@ func TestContextTracing_ClientDisablesTracing(t *testing.T) {
 	tracing.ContextTracing_ClientDisablesTracing(t, exp, clientRoot, srvRoot, srvChild)
 }
 
+func TestContextTracing_ClientDisablesAlwaysOnSampling(t *testing.T) {
+	exp := tracetest.NewInMemoryExporter()
+
+	config := Config{
+		SamplerType:  SamplerTypeConstant,
+		SamplerParam: 0,
+	}
+	sampler := getSampler(config)
+	tracerOtel := newTraceProvider(
+		context.Background(),
+		"tracerOtel",
+		log.NewNopLogger(),
+		tracesdk.NewSimpleSpanProcessor(exp),
+		sampler, // never sample
+		[]attribute.KeyValue{},
+	)
+	tracer, _ := migration.Bridge(tracerOtel, log.NewNopLogger())
+
+	clientRoot, clientCtx := tracing.StartSpan(tracing.ContextWithTracer(context.Background(), tracer), "a")
+
+	config.SamplerParam = 1
+	sampler2 := getSampler(config)
+	// Simulate Server process with different tracer, but with client span in context.
+	srvTracerOtel := newTraceProvider(
+		context.Background(),
+		"srvTracerOtel",
+		log.NewNopLogger(),
+		tracesdk.NewSimpleSpanProcessor(exp),
+		sampler2, // never sample
+		[]attribute.KeyValue{},
+	)
+	srvTracer, _ := migration.Bridge(srvTracerOtel, log.NewNopLogger())
+
+	srvRoot, srvCtx := tracing.StartSpan(tracing.ContextWithTracer(clientCtx, srvTracer), "b")
+	srvChild, _ := tracing.StartSpan(srvCtx, "bb")
+
+	tracing.ContextTracing_ClientDisablesTracing(t, exp, clientRoot, srvRoot, srvChild)
+}
+
 // This test shows that if span will contain special baggage (for example from special HTTP header), even when sample
 // factor will disable client & server tracing, it will be still enabled for all spans within this span.
 func TestContextTracing_ForceTracing(t *testing.T) {