fix(telemetry): opt-in preservation of trace-context headers on subgraph requests - #10014
Draft
OriginLeon wants to merge 8 commits into
Draft
fix(telemetry): opt-in preservation of trace-context headers on subgraph requests#10014OriginLeon wants to merge 8 commits into
OriginLeon wants to merge 8 commits into
Conversation
…ubgraph requests Under OTLP, HttpClientService::call unconditionally re-injects the router's own span context into outgoing traceparent/tracestate (and the custom trace ID header, if configured) on every subgraph fetch, discarding any value already present — whether from a coprocessor rewrite, header propagation, or a Rhai script. This silently breaks RUM trace correlation for customers who deliberately set these headers before the subgraph call (RH-1411, TSH-23612, TSH-24441). Adds telemetry.exporters.tracing.propagation.preserve_subgraph_trace_context (default false): when enabled, the router snapshots whichever trace-context header is already present before injection and restores it afterward, without conditioning or skipping the injection call itself, so no other propagator (baggage, jaeger, zipkin, datadog, x-ray) is affected. Implementation only, to validate the approach locally — tests, changeset, and usage metric are pending sign-off from engineering on ROUTER-2060. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 1206e637f973d2be4c6397b4 ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
This comment has been minimized.
This comment has been minimized.
CI's fmt_check/lint jobs failed on an import ordering issue introduced by the ROUTER-2060 changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… into marcelo/ROUTER-2060
- Renamed preserve_subgraph_trace_context to preserve_trace_context_on_subgraph_requests for clearer directionality. - Fixed router_factory.rs looking up the telemetry plugin by "telemetry" instead of its actual registered key "apollo.telemetry" (group.name, per PluginFactory::new_private) — this silently made the config always read as disabled regardless of what was set in router.yaml. - Added temporary tracing::warn! debug logs around the config resolution and the snapshot/inject/restore steps in HttpClientService::call, to validate the fix end-to-end. Marked TEMP DEBUG ROUTER-2060 for removal once validated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…text preservation - Remove the temporary TEMP DEBUG logging used to validate the fix live. - Add unit tests in services/http/service.rs covering: preserve disabled (baseline overwrite unaffected), preserve enabled restores traceparent + tracestate, custom trace-ID header takes priority, and falls back to traceparent when the custom header is empty. - Add integration tests in tests/integration/coprocessor.rs (+ two new fixtures) exercising the coprocessor-rewrite scenario end-to-end with the config on and off. - Add the required apollo.router.config.telemetry usage-metric attribute for the new preserve_trace_context_on_subgraph_requests option. - Add the changeset for this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
OriginLeon
marked this pull request as draft
August 19, 2026 12:43
OriginLeon
changed the title
otel::layer() defaults to a NoopTracer, which never produces a valid OTel SpanContext. TraceContextPropagator::inject_context silently declines to write anything when the span context is invalid, so the "disabled" and "custom header priority" tests were only exercising the restore side of snapshot/restore, not real injection -- CI caught this (both failed with `left == right`, i.e. no overwrite happened even with preservation off). - Add a local setup_tracing_with_real_span_context() that wires a real SdkTracerProvider via otel::layer().with_tracer(...), following the same pattern already used in crate::tracer::test. - Drop test_preserve_enabled_custom_header_takes_priority_over_traceparent: its assertion specifically needed real injection to be meaningful, and a dedicated regression test for that exact behavior is redundant with the integration test's coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status: draft, pending engineering sign-off on strategy
Implementation is complete and has been validated end-to-end (live, in a real cloud deployment, across multiple scenarios — see below). This is still marked draft because the underlying strategy (introducing a new opt-in config option) has not yet been signed off by the team — see the three questions posted on ROUTER-2060:
Please don't merge this until that sign-off lands, even if CI is green.
What changed
Under the OTLP exporter,
HttpClientService::call(apollo-router/src/services/http/service.rs) unconditionally re-injects the router's own span context into outgoingtraceparent/tracestate(and the custom trace-ID header, ifpropagation.request.header_nameis configured) on every subgraph HTTP call — discarding any value already present, whether it came from a coprocessor rewrite, header propagation, or a Rhai script. Under the Datadog-native exporter this doesn't happen, but only becauseDatadogPropagatornever touches those headers — incidental, not a designed safeguard.This silently breaks trace correlation for customers who deliberately set these headers before the subgraph call (RUM tracing use cases). Confirmed independently across two customers and reproduced in a controlled test environment — see the repro table in ROUTER-2060.
Adds
telemetry.exporters.tracing.propagation.preserve_trace_context_on_subgraph_requests(defaultfalse). When enabled, the router snapshots whichever trace-context header is already present immediately before the propagator injection and restores it afterward — the injection call itself is never conditioned or skipped, so no other propagator (baggage, jaeger, zipkin, datadog, x-ray) is affected. Only one trace-ID-carrying header is preserved per call: the custom header (if configured and non-empty) takes priority, falling back totraceparent, mirroring the same precedence already used for extraction (#9984).tracestatehas no custom-header equivalent and is always preserved independently when present. Scoped to Router→subgraph calls only — Router→coprocessor and Router→connector calls are unaffected.Validation
traceparent's span-id (rewritten value correctly preserved end-to-end), config enabled with no coprocessor rewrite (original client-sent value correctly passed through), and a fan-out scenario across multiple subgraphs.Checklist
Exceptions
Pending items above (compatibility review, docs) are deliberately left for after the ROUTER-2060 sign-off, since the config name/shape could still change based on that discussion. Test execution in my own dev sandbox was constrained by local environment issues unrelated to this change (DNS resolver config, missing test Apollo Studio credentials for GraphOS-gated integration tests) — tests are written and compile cleanly; CI has what it needs to actually run them.