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>
thanos-io · Nov 15, 2022 · df047ef · df047ef
1 parent dda0c25
commit df047ef
Show file tree

Hide file tree

Showing 4 changed files with 88 additions and 21 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -3,6 +3,7 @@ version: 2.1
 
 orbs:
   go: circleci/go@1.7.1
+  git-shallow-clone: guitarrapc/git-shallow-clone@2.4.0
 
 executors:
   golang:
@@ -19,7 +20,7 @@ jobs:
     environment:
       GO111MODULE: 'on'
     steps:
-      - checkout
+      - git-shallow-clone/checkout
       - go/mod-download-cached
       - setup_remote_docker:
           version: 20.10.12
@@ -58,7 +59,7 @@ jobs:
       GOBIN: "/home/circleci/.go_workspace/go/bin"
       PROMU_VERSION: "0.5.0"
     steps:
-      - checkout
+      - git-shallow-clone/checkout
       - run: mkdir -p ${GOBIN}
       - run: curl -L "https://github.com/prometheus/promu/releases/download/v${PROMU_VERSION}/promu-${PROMU_VERSION}.$(go env GOOS)-$(go env GOARCH).tar.gz" | tar --strip-components=1 -xzf - -C ${GOBIN}
       - run: mv -f ${GOBIN}/promu "${GOBIN}/promu-v${PROMU_VERSION}"
@@ -71,7 +72,7 @@ jobs:
   publish_main:
     executor: golang
     steps:
-      - checkout
+      - git-shallow-clone/checkout
       - go/mod-download-cached
       - setup_remote_docker:
           version: 20.10.12
@@ -93,7 +94,7 @@ jobs:
   publish_release:
     executor: golang
     steps:
-      - checkout
+      - git-shallow-clone/checkout
       - go/mod-download-cached
       - setup_remote_docker:
           version: 20.10.12

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,18 @@ NOTE: As semantic versioning states all 0.y.z releases can contain breaking chan
 
 We use *breaking :warning:* to mark changes that are not backward compatible (relates only to v0.y.z releases.)
 
+## Unreleased
+
+## [v0.29.1](https://github.com/thanos-io/thanos/tree/release-0.29) - In progress
+
+### Fixed
+
+- [#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.
+
+### Added
+
+### Changed
+
 ## [v0.29.0](https://github.com/thanos-io/thanos/tree/release-0.29) - 2022.11.03
 
 ### Fixed

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) {