feat(epp): complete W3C trace propagation for OpenTelemetry export - #2702
feat(epp): complete W3C trace propagation for OpenTelemetry export#2702dayakar349 wants to merge 1 commit into
Conversation
|
🚨 Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
github-actions
Bot
added
size/L
| tracing.InitTextMapPropagator() | ||
|
|
||
| // Initialize tracing conditionally using config | ||
| if opts.Tracing { | ||
| shutdown, err := tracing.InitTracing(ctx, logger, "llm-d-disagg-sidecar") |
There was a problem hiding this comment.
Question: should this be behind the same if opts.Tracing gate as InitTracing or is the current unconditional InitTextMapPropegator what we want?
There was a problem hiding this comment.
Unconditional is intentional. Propagation needs to work when --tracing=false so EPP can still read and forward upstream traceparent headers. Gating it behind opts.Tracing would only enable that when OTLP export is on.
| tracing.InitTextMapPropagator() | ||
|
|
||
| if opts.Tracing { | ||
| shutdown, err := tracing.InitTracing(ctx, setupLog, "llm-d-epp") |
There was a problem hiding this comment.
Question: should this be behind the same if opts.Tracing gate as InitTracing or is the current unconditional InitTextMapPropegator what we want?
There was a problem hiding this comment.
Unconditional is intentional. Propagation needs to work when --tracing=false so EPP can still read and forward upstream traceparent headers. Gating it behind opts.Tracing would only enable that when OTLP export is on.
| tracerProvider := sdktrace.NewTracerProvider(opt...) | ||
| otel.SetTracerProvider(tracerProvider) | ||
| otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{})) | ||
| InitTextMapPropagator() |
There was a problem hiding this comment.
Question: in pd-sidecar/main.go and epp/runner/runner.go there is a direct call to InitTextPropagator then have a conditional that calls tracing.InitTracing which now also includes InitTextMapPropagator. Whats the reason for the duplication and is it necessary?
There was a problem hiding this comment.
The startup call covers the tracing-off path. InitTracing() calls it again when tracing is on, so there is duplication on that path. It is idempotent. I can remove the call from InitTracing() and keep only the startup init if you prefer.
f8043bb to
0f9de8a
Compare
|
/assign @gyliu513 |
0f9de8a to
970fb27
Compare
US-02 (RHOAIENG-83714): finish W3C trace propagation on the Endpoint Picker. - Treat traceparent, tracestate, and baggage as system-owned headers so client values are not re-forwarded after generateHeaders injects the active span. - Add InitTextMapPropagator and call it at EPP and P/D sidecar startup so extract/inject work when OTLP export is disabled. - Add unit and hermetic integration coverage for propagation and injection. Signed-off-by: Dayakar Maruboena <dmaruboe@dmaruboe-thinkpadp1gen7.rht.csb> Signed-off-by: Dayakar Maruboena <dmaruboe@redhat.com>
970fb27 to
7a2136c
Compare
4 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Completes W3C trace propagation for RHOAIENG-83714 (US-02) on the Endpoint Picker and P/D sidecar.
traceparent,tracestate, andbaggageas system-owned headers so client values are not re-forwarded aftergenerateHeadersinjects the active span context.InitTextMapPropagator()and call it at EPP and P/D sidecar startup so extract/inject work when OTLP export is disabled.With
--tracing=trueand standardOTEL_*env vars, EPP joins upstream traces, records routing spans, injects a single updatedtraceparentfor downstream workloads, and exports spans to the platform collector.Test plan
Which issue(s) this PR fixes:
Related to RHOAIENG-83714
Fixes #
Release note (write
NONEif no user-facing change):