OTEL_TRACES_SAMPLER env var does not seem to be respected by OTLP tracing

[gburek-fastly]

Thanos, Prometheus and Golang version used:

thanos, version 0.29.0 (branch: HEAD, revision: 194e1fadf4302341a0c70c28c0e3f3c658616c19)
  build user:       root@a0c7bba6c208
  build date:       20221103-14:28:25
  go version:       go1.19.3
  platform:         linux/amd64

Object Storage Provider: GCP

What happened:
When tracing was configured with:

---
config:
  client_type: grpc
  compression: gzip
  endpoint: localhost:4317
  insecure: true
type: OTLP

and the process was running with env var OTEL_TRACES_SAMPLER="alwaysoff" traces continued to be emitted.

What you expected to happen:

The OTLP SDK should respond to env vars like OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG as detailed in
https://opentelemetry.io/docs/reference/specification/sdk-environment-variables/#general-sdk-configuration

How to reproduce it (as minimally and precisely as possible):

Run thanos 0.29.0 with --tracing.config-file=/etc/thanos/tracing.yml and tracing.yaml of

---
config:
  client_type: grpc
  compression: gzip
  endpoint: localhost:4317
  insecure: true
type: OTLP

Anything else we need to know:
It appears that the sampler is hard coded as tracesdk.ParentBased(tracesdk.TraceIDRatioBased(1.0)) and tracesdk.WithSampler

As documented in https://pkg.go.dev/go.opentelemetry.io/otel/sdk/trace#WithSampler:

This option overrides the Sampler configured through the OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG environment variables. If this option is not used and the sampler is not configured through environment variables or the environment contains invalid/unsupported configuration, the TracerProvider will use a ParentBased(AlwaysSample) Sampler by default.

Since the effective behavior of the sampler where ParentBased(tracesdk.TraceIDRatioBased(1.0)) is equivalent to ParentBased(AlwaysSample), removing the sampler in the code will have no effective change in the default behavior, but would allow env var configuration to occur.

[darcydai]

https://github.com/thanos-io/thanos/blob/main/pkg/tracing/otlp/otlp.go

[jatinagwal]

Do we just have to remove the sampler?? @gburek-fastly

[vishal-chdhry]

May I pick this up?

[gburek-fastly]

@jatinagwal that appears to be the case, but I think it loses the migration.ForceTracingAttributeKey which I don't completely understand.

It seems the original author was trying to do something like using a custom attr as a context propagation scheme, but it is always ignored due to the sampler here being hardcoded to always sample?

Functionally, removing the sampler will preserve existing default behavior and allow for env vars to configure the sampler, but it will not restore or implement the attr based propagation behavior.

[johurul000]

@vishal-chdhry are you working on this

[vishal-chdhry]

@johurul000 I believe @jatinagwal is

[matej-g]

@jatinagwal let us know if you managed to put some work into this issue 🙃, otherwise we could ask someone else, would be good to have this fixed ASAP.

[jatinagwal]

Hey, @matej-g , I am little confused about migration.ForceTracingAttributeKey . @vishal-chdhry you can pick this up. If I found something useful, will share here.

[johurul000]

@jatinagwal have you found anything more about the migration.ForceTracingAttributeKey.
const ForceTracingAttributeKey = "thanos.force_tracing"
https://github.com/thanos-io/thanos/blob/v0.30.1/pkg/tracing/migration/sampler.go#L14 .

[matej-g]

We should definitely not have the sampler hardcoded to always, but I think we should add this to the config, I'm not sure if should be mixing env vars and config, since I think other parameters must also be configured via the YAML config?

That being said, yes, removing the hardcoded sampler would allow for setting it via env but we would need to account somehow for the override sampler. As for the 'ForceTracingAttributeKey' and override sampler, this is for backwards compatibility reasons, since this feature existed before moved to OpenTelemetry.

[shayyxi]

I want to work on this.

[WillSewell]

Can this be closed following the merge of #6306?

[matej-g]

Looks like there are couple more missing pieces as per my comment in #6306 (review), but the issue at hand should be resolved, so we can close this.