diff --git a/.circleci/config.yml b/.circleci/config.yml index 60fb3d6ec5..d2c464ec8a 100644 --- 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 index 8ed9cce32a..6fb4e17eb8 100644 --- 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 index 71009070b3..fae3f8c21c 100644 --- 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,22 +121,27 @@ 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...) @@ -137,17 +149,20 @@ func getSampler(config Config) tracesdk.Sampler { 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 index bc220bed0e..5e48b3e88a 100644 --- 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) {