Add support for all otel tracing configs for self tracing#7577
Conversation
Adjust dns.NewProvider call sites for the new maxIdleConnections param.
Delegate tracer setup to dskit's NewOTelOrJaegerFromEnv like Mimir. Adds jaeger_remote and parentbased_jaeger_remote sampler support, and restores enabling self-tracing via JAEGER_* env vars. Fixes grafana#7556.
- rename InstallOpenTelemetryTracer to InstallOTelOrJaegerFromEnv - JAEGER_SERVICE_NAME takes precedence over OTEL_SERVICE_NAME - refine self-tracing docs, changelog wording, test env isolation
e9dbf90 to
d171d6f
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR updates Tempo’s self-tracing initialization to delegate OpenTelemetry/Jaeger environment-variable parsing (including sampler configuration like OTEL_TRACES_SAMPLER*) to dskit, aligning behavior with Grafana Mimir and addressing the missing sampler-env-var support described in #7556.
Changes:
- Replace Tempo’s custom OpenTelemetry tracer setup with
dskit/tracing.NewOTelOrJaegerFromEnv()and add service-name override handling viaOTEL_SERVICE_NAME/JAEGER_SERVICE_NAME. - Add unit tests validating sampler env var behavior (including
jaeger_remote/parentbased_jaeger_remote) and the no-op behavior when tracing isn’t configured. - Add/adjust documentation for self-tracing configuration and update config reference outputs for new memberlist settings; bump
dskit(and related deps) accordingly.
Reviewed changes
Copilot reviewed 11 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/tracing/tracing.go | Switches self-tracing initialization to dskit and adds env-based service name override. |
| pkg/tracing/tracing_test.go | Adds tests covering sampler env vars, invalid args handling, and no-op behavior. |
| cmd/tempo/main.go | Always calls the new self-tracing init (expected to be a no-op unless env-configured). |
| pkg/cache/memcached_client.go | Updates DNS provider construction to match updated dskit/dns API. |
| cmd/tempo/app/modules.go | Updates DNS provider construction to match updated dskit/dns API. |
| docs/sources/tempo/operations/monitor/self-tracing.md | New doc page describing supported OTel/Jaeger env vars for self-tracing. |
| docs/sources/tempo/operations/monitor/_index.md | Links monitoring docs to the new self-tracing documentation. |
| modules/frontend/docs/config-reference.md | Updates rendered config reference with new memberlist options. |
| docs/sources/tempo/configuration/manifest.md | Updates manifest with new memberlist options. |
| go.mod | Bumps github.com/grafana/dskit and updates dependency direct/indirect annotations. |
| go.sum | Updates dependency checksums for the version bumps. |
| .chloggen/selftracing-otel-sampler-env.yaml | Adds changelog entry for the self-tracing env-var support change. |
Files excluded by content exclusion policy (25)
- vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.go
- vendor/github.com/grafana/dskit/dns/miekgdns/resolver.go
- vendor/github.com/grafana/dskit/dns/provider.go
- vendor/github.com/grafana/dskit/flagext/url.go
- vendor/github.com/grafana/dskit/kv/memberlist/memberlist_client.go
- vendor/github.com/grafana/dskit/kv/memberlist/propagation_tracker.go
- vendor/github.com/grafana/dskit/limiter/rate_limiter.go
- vendor/github.com/grafana/dskit/log/doc.go
- vendor/github.com/grafana/dskit/log/sanitize.go
- vendor/github.com/grafana/dskit/middleware/http_tracing.go
- vendor/github.com/grafana/dskit/middleware/logging.go
- vendor/github.com/grafana/dskit/ring/client/ring_service_discovery.go
- vendor/github.com/grafana/dskit/ring/partition_instance_rings.go
- vendor/github.com/grafana/dskit/ring/partition_ring.go
- vendor/github.com/grafana/dskit/ring/partition_ring_watchers.go
- vendor/github.com/grafana/dskit/ring/ring_status.gohtml
- vendor/github.com/grafana/dskit/runtimeconfig/manager.go
- vendor/github.com/grafana/dskit/runtimeconfig/provider.go
- vendor/github.com/grafana/dskit/server/fake_server.pb.go
- vendor/github.com/grafana/dskit/services/README.md
- vendor/github.com/grafana/dskit/tracing/otel.go
- vendor/github.com/grafana/otel-profiling-go/README.md
- vendor/github.com/grafana/otel-profiling-go/otelpyroscope.go
- vendor/github.com/grafana/otel-profiling-go/renovate.json
- vendor/modules.txt
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 11 out of 37 changed files in this pull request and generated 1 comment.
Files excluded by content exclusion policy (25)
- vendor/github.com/golang/protobuf/ptypes/empty/empty.pb.go
- vendor/github.com/grafana/dskit/dns/miekgdns/resolver.go
- vendor/github.com/grafana/dskit/dns/provider.go
- vendor/github.com/grafana/dskit/flagext/url.go
- vendor/github.com/grafana/dskit/kv/memberlist/memberlist_client.go
- vendor/github.com/grafana/dskit/kv/memberlist/propagation_tracker.go
- vendor/github.com/grafana/dskit/limiter/rate_limiter.go
- vendor/github.com/grafana/dskit/log/doc.go
- vendor/github.com/grafana/dskit/log/sanitize.go
- vendor/github.com/grafana/dskit/middleware/http_tracing.go
- vendor/github.com/grafana/dskit/middleware/logging.go
- vendor/github.com/grafana/dskit/ring/client/ring_service_discovery.go
- vendor/github.com/grafana/dskit/ring/partition_instance_rings.go
- vendor/github.com/grafana/dskit/ring/partition_ring.go
- vendor/github.com/grafana/dskit/ring/partition_ring_watchers.go
- vendor/github.com/grafana/dskit/ring/ring_status.gohtml
- vendor/github.com/grafana/dskit/runtimeconfig/manager.go
- vendor/github.com/grafana/dskit/runtimeconfig/provider.go
- vendor/github.com/grafana/dskit/server/fake_server.pb.go
- vendor/github.com/grafana/dskit/services/README.md
- vendor/github.com/grafana/dskit/tracing/otel.go
- vendor/github.com/grafana/otel-profiling-go/README.md
- vendor/github.com/grafana/otel-profiling-go/otelpyroscope.go
- vendor/github.com/grafana/otel-profiling-go/renovate.json
- vendor/modules.txt
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| if err := tpsdk.Shutdown(ctx); err != nil { | ||
| if err := closer.Close(); err != nil { |
There was a problem hiding this comment.
Does the closer inherently have a timeout?
There was a problem hiding this comment.
yes, it has a 5 second timeout.
see here https://github.com/grafana/dskit/blob/de1ec7541c44b599f92d97590ed508f301446269/tracing/otel.go#L87 and here https://github.com/grafana/dskit/blob/de1ec7541c44b599f92d97590ed508f301446269/tracing/otel_jaeger.go#L310
What this PR does:
Follow Mimir and use dskit for self tracing and add support for all via dskit/tracing.NewOTelOrJaegerFromEnv()
needed to bump dskit for it as well
Which issue(s) this PR fixes:
Fixes #7556
Checklist
.chloggen/(runmake chlog-new, ormake chlog-new FILENAME=<name>to override the default branch-name file; see.chloggen/README.md)