Safe Self-Tracing via OTel Collector Telemetry Factory#8208
Conversation
yurishkuro
changed the title
There was a problem hiding this comment.
Pull request overview
This PR centralizes Jaeger’s internal “self-tracing” by wiring tracing through the OpenTelemetry Collector TelemetryFactory hook (instead of per-extension manual jtracer.NewProvider setup), and adds support for the jaeger_remote sampler lifecycle.
Changes:
- Add a Collector telemetry-factory wrapper (
WrapFactory) that returns a filteringTracerProviderallowlisting specific component IDs for real tracing. - Update
jtracerto supportOTEL_TRACES_SAMPLER=jaeger_remoteand to return a sampler-closer that is invoked during shutdown. - Remove manual tracer initialization/shutdown from
jaegerqueryandremotestorage, and add an ADR + tests covering framework attribute injection and filtering behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/jtracer/jtracer.go | Adds jaeger_remote sampler support and ensures sampler lifecycle is closed during provider shutdown. |
| internal/jtracer/jtracer_test.go | Updates tests for new helper signatures returning an additional closer. |
| cmd/jaeger/internal/telemetry.go | Introduces filtering tracer provider + telemetry factory wrapper used by the collector service. |
| cmd/jaeger/internal/telemetry_test.go | Adds unit/integration-style tests validating allowlist behavior and collector attribute injection. |
| cmd/jaeger/internal/components.go | Switches telemetry factory wiring to use WrapFactory(...). |
| cmd/jaeger/internal/extension/jaegerquery/server.go | Removes manual tracer init and preserves enable_tracing behavior by overriding to noop when disabled. |
| cmd/jaeger/internal/extension/remotestorage/server.go | Removes manual tracer init/shutdown and simplifies shutdown handling. |
| docs/adr/README.md | Adds ADR-006 to the ADR index. |
| docs/adr/006-internal-tracing-via-otelcol-telemetry-factory.md | Documents the new centralized self-tracing design and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return &filteringTracerProvider{ | ||
| real: realTP, | ||
| noop: nooptrace.NewTracerProvider(), | ||
| closer: closer, | ||
| }, nil |
There was a problem hiding this comment.
The blocking path returns a standard nooptrace.NewTracerProvider(). That noop tracer still propagates the parent span context into the returned context.Context on Start(), which undermines the goal of preventing incoming trace context from being carried through receiver/processor/exporter code paths (and it also contradicts the ADR text in this PR that calls out the need for a noopNoContextTracerProvider-style implementation). Consider replacing this with a local “no-context noop” TracerProvider/Tracer that returns the original context unchanged from Start(), so blocked components cannot propagate inbound context further downstream.
There was a problem hiding this comment.
there is no such thing as noopNoContextTracerProvider. Context is passed explicitly, not by means of a tracer.
| func (f *filteringTracerProvider) Shutdown(ctx context.Context) error { | ||
| if f.closer != nil { | ||
| return f.closer(ctx) | ||
| } |
There was a problem hiding this comment.
Shutdown is documented as flushing/shutting down the underlying real TracerProvider, but if closer is nil it currently returns nil without shutting down real. This can leak exporter/span-processor resources if the provider is ever constructed without a closer (including in tests). Consider falling back to calling Shutdown(ctx) on f.real when it implements interface{ Shutdown(context.Context) error }, or adjust the comment to match the actual behavior.
| } | |
| } | |
| if s, ok := f.real.(interface{ Shutdown(context.Context) error }); ok { | |
| return s.Shutdown(ctx) | |
| } |
There was a problem hiding this comment.
the closer will not be nil if real tracer was returned from creator/factory
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8208 +/- ##
==========================================
- Coverage 95.63% 95.61% -0.03%
==========================================
Files 318 319 +1
Lines 16785 16793 +8
==========================================
+ Hits 16052 16056 +4
- Misses 578 582 +4
Partials 155 155
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Problem
Jaeger traces its own internal operations (query API, MCP server) by sending spans to an OTLP exporter. When the OTLP receiver in the same Jaeger process is the export destination, every outgoing span triggers another incoming span, causing a recursive self-tracing loop that can flood storage and degrade performance.
Previously, each extension (
jaeger_query,remotestorage) independently calledjtracer.NewProviderto initialize its own tracer, leading to multiple provider instances and the risk of runaway self-tracing.External Changes
New behavior:
Jaeger no longer creates a recursive self-tracing loop when its OTLP receiver is the
export destination for internal telemetry.
Per-operation sampling rates are now configurable for internal spans via the Jaeger remote sampler:
The standard
OTEL_TRACES_SAMPLER/OTEL_TRACES_SAMPLER_ARGenv vars are honoured forall built-in sampler types (
always_on,always_off,traceidratio,parentbased_*).The default sampler is
parentbased_always_on.Unchanged:
enable_tracing: falsein thejaeger_queryconfig continues to disable query tracing.OTEL_EXPORTER_OTLP_*andOTEL_TRACES_*environment variables work as before.Internal Changes
TracerProviderinjected through the OTelCollector's
TelemetryFactoryhook.jaeger_query,jaeger_mcp) receive a realrecording tracer. All other components — receivers, exporters, processors, and unlisted
extensions — receive a no-op tracer, preventing recursive self-tracing loops.
jaeger_remotesampler lifecycle is now managed correctly: its background pollinggoroutine is stopped on shutdown.
central factory.
How was this change tested?
AI Usage in this PR (choose one)
See AI Usage Policy.