diff --git a/.changesets/fix_router_2060_preserve_subgraph_trace_context.md b/.changesets/fix_router_2060_preserve_subgraph_trace_context.md new file mode 100644 index 0000000000..b61ab333d4 --- /dev/null +++ b/.changesets/fix_router_2060_preserve_subgraph_trace_context.md @@ -0,0 +1,17 @@ +### Add an opt-in setting to preserve an existing trace-context header on outgoing subgraph requests ([PR #10014](https://github.com/apollographql/router/pull/10014)) + +Under the OTLP exporter, the router's outbound HTTP client unconditionally re-injected its own span context into `traceparent`/`tracestate` (and the custom trace ID header, if `propagation.request.header_name` is configured) on every subgraph request, discarding any value already present on that header — whether set by a coprocessor, a Rhai script, or header propagation from the original client request. This silently broke trace correlation for setups that deliberately rewrite these headers before the subgraph call, for example a coprocessor inserting itself as a hop in the trace (standard, spec-compliant W3C Trace Context practice) or forwarding a client-generated trace ID for RUM correlation. Under the Datadog-native exporter this didn't happen, but only because Datadog's own propagator never touches those headers — an incidental side effect of that exporter, not a documented guarantee. + +A new setting, `telemetry.exporters.tracing.propagation.preserve_trace_context_on_subgraph_requests`, lets you opt into keeping whatever trace-context header is already present on a subgraph request instead of overwriting it: + +```yaml +telemetry: + exporters: + tracing: + propagation: + preserve_trace_context_on_subgraph_requests: true +``` + +It defaults to `false`, so existing deployments are unaffected. When enabled, the router still performs its own trace-context injection exactly as before; it then restores whichever header was present beforehand, so every other propagator (baggage, Jaeger, Zipkin, Datadog, X-Ray) behaves identically to today regardless of this setting. Only one trace-ID-carrying header is preserved per call: the custom trace ID header takes priority when configured and non-empty, falling back to `traceparent` otherwise — the same precedence already used when extracting trace context from inbound requests. This setting only affects the router's calls to subgraphs; calls to coprocessors and connectors are unaffected. + +By [@OriginLeon](https://github.com/OriginLeon) in https://github.com/apollographql/router/pull/10014 diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index 68199305fc..c29c485f7e 100644 --- a/apollo-router/src/configuration/metrics.rs +++ b/apollo-router/src/configuration/metrics.rs @@ -393,7 +393,12 @@ impl InstrumentData { opt.spans.supergraph, "$..spans.supergraph", opt.tracing.common.sampler, - "$..tracing.common.sampler" + "$..tracing.common.sampler", + // BEGIN/END ROUTER-2060 + opt.tracing + .propagation + .preserve_trace_context_on_subgraph_requests, + "$..tracing.propagation[?(@.preserve_trace_context_on_subgraph_requests==true)]" ); populate_config_instrument!( diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap index 4358de1af8..885611b785 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__metrics__test__metrics@telemetry.router.yaml.snap @@ -1,5 +1,6 @@ --- source: apollo-router/src/configuration/metrics.rs +assertion_line: 737 expression: "& metrics.non_zero()" --- - name: apollo.router.config.telemetry @@ -30,4 +31,5 @@ expression: "& metrics.non_zero()" opt.tracing.common.sampler: false opt.tracing.datadog: true opt.tracing.otlp: true + opt.tracing.propagation.preserve_trace_context_on_subgraph_requests: false opt.tracing.zipkin: true diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 85a9ecaeaa..34c6f1e433 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -1,5 +1,6 @@ --- source: apollo-router/src/configuration/tests.rs +assertion_line: 28 expression: "&schema" --- { @@ -8142,6 +8143,11 @@ expression: "&schema" "description": "Propagate Jaeger", "type": "boolean" }, + "preserve_trace_context_on_subgraph_requests": { + "default": false, + "description": "If a trace-context header (traceparent/tracestate, or the custom trace ID header if\nconfigured) is already present on an outgoing subgraph request, keep it instead of\noverwriting it with the router's own span context.", + "type": "boolean" + }, "request": { "allOf": [ { diff --git a/apollo-router/src/plugins/telemetry/config.rs b/apollo-router/src/plugins/telemetry/config.rs index 0b82321236..89fa6302ff 100644 --- a/apollo-router/src/plugins/telemetry/config.rs +++ b/apollo-router/src/plugins/telemetry/config.rs @@ -414,6 +414,12 @@ pub(crate) struct Propagation { pub(crate) zipkin: bool, /// Propagate AWS X-Ray pub(crate) aws_xray: bool, + // BEGIN ROUTER-2060 + /// If a trace-context header (traceparent/tracestate, or the custom trace ID header if + /// configured) is already present on an outgoing subgraph request, keep it instead of + /// overwriting it with the router's own span context. + pub(crate) preserve_trace_context_on_subgraph_requests: bool, + // END ROUTER-2060 } #[derive(Clone, Debug, Deserialize, JsonSchema, Default, PartialEq)] diff --git a/apollo-router/src/router_factory.rs b/apollo-router/src/router_factory.rs index 96fb58b0ea..949ef6a8ec 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -29,6 +29,7 @@ use crate::plugin::Handler; use crate::plugin::PluginFactory; use crate::plugin::PluginInit; use crate::plugins::subscription::notification::Notify; +use crate::plugins::telemetry::Telemetry; // BEGIN/END ROUTER-2060 use crate::plugins::telemetry::reload::otel::apollo_opentelemetry_initialized; use crate::plugins::traffic_shaping::APOLLO_TRAFFIC_SHAPING; use crate::plugins::traffic_shaping::TrafficShaping; @@ -41,6 +42,7 @@ use crate::services::SupergraphCreator; use crate::services::apollo_graph_reference; use crate::services::apollo_key; use crate::services::http::HttpClientServiceFactory; +use crate::services::http::TraceContextPreservation; // BEGIN/END ROUTER-2060 use crate::services::layers::persisted_queries::PersistedQueryLayer; use crate::services::layers::query_analysis::QueryAnalysisLayer; use crate::services::new_service::ServiceFactory; @@ -446,6 +448,28 @@ pub(crate) async fn create_http_services( .and_then(|plugin| (*plugin.1).as_any().downcast_ref::()) .expect("traffic shaping should always be part of the plugin list"); + // BEGIN ROUTER-2060 + let telemetry_config = plugins + .iter() + .find(|i| i.0.as_str() == "apollo.telemetry") // BEGIN/END ROUTER-2060: registered as "{group}.{name}", see PluginFactory::new_private + .and_then(|plugin| (*plugin.1).as_any().downcast_ref::()) + .map(|t| t.config.clone()); + let subgraph_trace_context_preservation = TraceContextPreservation { + enabled: telemetry_config + .as_ref() + .map(|c| { + c.exporters + .tracing + .propagation + .preserve_trace_context_on_subgraph_requests + }) + .unwrap_or(false), + custom_header_name: telemetry_config + .as_ref() + .and_then(|c| c.exporters.tracing.propagation.request.header_name.clone()), + }; + // END ROUTER-2060 + let connector_subgraphs: HashSet = schema .connectors .as_ref() @@ -467,6 +491,7 @@ pub(crate) async fn create_http_services( configuration, &subgraph_tls_root_store, shaping.subgraph_client_config(name), + subgraph_trace_context_preservation.clone(), // BEGIN/END ROUTER-2060 )?; let http_service_factory = HttpClientServiceFactory::new(http_service, plugins.clone()); diff --git a/apollo-router/src/services/http.rs b/apollo-router/src/services/http.rs index 1e5676f7ea..7ea597c19d 100644 --- a/apollo-router/src/services/http.rs +++ b/apollo-router/src/services/http.rs @@ -15,6 +15,7 @@ pub(crate) mod service; mod tests; pub(crate) use service::HttpClientService; +pub(crate) use service::TraceContextPreservation; // BEGIN/END ROUTER-2060 pub(crate) type BoxService = tower::util::BoxService; pub(crate) type BoxCloneService = tower::util::BoxCloneService; @@ -56,6 +57,7 @@ impl HttpClientServiceFactory { configuration, &rustls::RootCertStore::empty(), client_config, + TraceContextPreservation::default(), // BEGIN/END ROUTER-2060 ) .unwrap(); diff --git a/apollo-router/src/services/http/service.rs b/apollo-router/src/services/http/service.rs index d8951cb30d..17fc98aad8 100644 --- a/apollo-router/src/services/http/service.rs +++ b/apollo-router/src/services/http/service.rs @@ -9,6 +9,7 @@ use std::task::Poll; use ::serde::Deserialize; use bytes::Buf; use futures::future::BoxFuture; +use http::HeaderName; // BEGIN/END ROUTER-2060 use http::HeaderValue; use http::Request; use http::header::ACCEPT_ENCODING; @@ -204,6 +205,18 @@ impl Display for Compression { } } +// BEGIN ROUTER-2060 +/// Whether an already-present trace-context header on an outgoing subgraph request should be +/// preserved instead of being overwritten by the router's own span context, and which header +/// name carries the trace ID when a custom one is configured +/// (`telemetry.exporters.tracing.propagation.request.header_name`). +#[derive(Clone, Default)] +pub(crate) struct TraceContextPreservation { + pub(crate) enabled: bool, + pub(crate) custom_header_name: Option, +} +// END ROUTER-2060 + /// A set of clients (http, unix) for talking with external services like coprocessors, subgraphs, /// connectors-connected subgraphs, and so on, implemented as a tower service #[derive(Clone)] @@ -215,6 +228,7 @@ pub(crate) struct HttpClientService { #[cfg(unix)] unix_client: UnixHTTPClient, service: Arc, + trace_context_preservation: TraceContextPreservation, // BEGIN/END ROUTER-2060 } impl HttpClientService { @@ -231,7 +245,14 @@ impl HttpClientService { let service_target = ServiceTarget::Subgraph { name: Arc::from(service.into().as_str()), }; - Self::new(service_target, tls_config, client_config) + // BEGIN ROUTER-2060 + Self::new( + service_target, + tls_config, + client_config, + TraceContextPreservation::default(), + ) + // END ROUTER-2060 } /// Create a new HttpClientService using: @@ -246,6 +267,7 @@ impl HttpClientService { service_target: ServiceTarget, tls_config: ClientConfig, client_config: crate::configuration::shared::Client, + trace_context_preservation: TraceContextPreservation, // BEGIN/END ROUTER-2060 ) -> Result { let service_name: Arc = match &service_target { ServiceTarget::Coprocessor => Arc::from("coprocessor"), @@ -330,6 +352,7 @@ impl HttpClientService { #[cfg(unix)] unix_client, service: service_name, + trace_context_preservation, // BEGIN/END ROUTER-2060 }) } @@ -339,6 +362,7 @@ impl HttpClientService { configuration: &Configuration, tls_root_store: &RootCertStore, client_config: crate::configuration::shared::Client, + trace_context_preservation: TraceContextPreservation, // BEGIN/END ROUTER-2060 ) -> Result { let name: String = service.into(); let default_client_cert_config = configuration @@ -372,7 +396,14 @@ impl HttpClientService { name: Arc::from(name.as_str()), }; - Self::new(service_target, tls_client_config, client_config) + // BEGIN ROUTER-2060 + Self::new( + service_target, + tls_client_config, + client_config, + trace_context_preservation, + ) + // END ROUTER-2060 } /// Creates a client for talking to connectors-connected subgraphs @@ -414,7 +445,14 @@ impl HttpClientService { name: Arc::from(name.as_str()), }; - Self::new(service_target, tls_client_config, client_config) + // BEGIN ROUTER-2060: out of scope, this feature only applies to subgraph calls + Self::new( + service_target, + tls_client_config, + client_config, + TraceContextPreservation::default(), + ) + // END ROUTER-2060 } /// Creates a client for talking to coprocessors @@ -427,7 +465,14 @@ impl HttpClientService { // Coprocessors don't use client certificates, so use no client auth let tls_client_config = generate_tls_client_config(tls_root_store.clone(), None)?; - Self::new(ServiceTarget::Coprocessor, tls_client_config, client_config) + // BEGIN ROUTER-2060: out of scope, this feature only applies to subgraph calls + Self::new( + ServiceTarget::Coprocessor, + tls_client_config, + client_config, + TraceContextPreservation::default(), + ) + // END ROUTER-2060 } /// Creates a root certificate store with native certificates. These are used for root-of-trust @@ -477,7 +522,14 @@ impl HttpClientService { let service_target = ServiceTarget::Subgraph { name: Arc::from("test"), }; - HttpClientService::new(service_target, tls_client_config, client_config) + // BEGIN ROUTER-2060 + HttpClientService::new( + service_target, + tls_client_config, + client_config, + TraceContextPreservation::default(), + ) + // END ROUTER-2060 } } @@ -529,6 +581,44 @@ impl tower::Service for HttpClientService { let service_name = self.service.clone(); let http_req_span = Span::current(); + // BEGIN ROUTER-2060 + // + // If the router is configured to preserve an already-present trace-context header on + // this subgraph request, snapshot it before letting the propagator injection below run + // exactly as it always has. Afterward, restore the snapshotted value over whatever the + // injection just wrote. This never skips or conditions the injection call itself, so + // every other propagator (baggage, jaeger, zipkin, datadog, x-ray) behaves identically + // to today, regardless of this feature. + // + // Only one trace-ID-carrying header is preserved per call: if a custom trace ID header + // is configured and present and non-empty, it takes priority (mirroring the same + // precedence used when extracting trace context from inbound requests); otherwise we + // fall back to `traceparent`. `tracestate` has no custom-header equivalent, so it is + // always preserved independently when present. + let saved_trace_header = self + .trace_context_preservation + .enabled + .then(|| { + if let Some(custom_header) = &self.trace_context_preservation.custom_header_name + && let Some(value) = http_request.headers().get(custom_header) + && !value.is_empty() + { + return Some((custom_header.clone(), value.clone())); + } + http_request + .headers() + .get("traceparent") + .map(|v| (HeaderName::from_static("traceparent"), v.clone())) + }) + .flatten(); + + let saved_tracestate = self + .trace_context_preservation + .enabled + .then(|| http_request.headers().get("tracestate").cloned()) + .flatten(); + // END ROUTER-2060 + get_text_map_propagator(|propagator| { propagator.inject_context( &prepare_context(http_req_span.context()), @@ -536,6 +626,15 @@ impl tower::Service for HttpClientService { ); }); + // BEGIN ROUTER-2060 + if let Some((name, value)) = saved_trace_header { + http_request.headers_mut().insert(name, value); + } + if let Some(value) = saved_tracestate { + http_request.headers_mut().insert("tracestate", value); + } + // END ROUTER-2060 + let (parts, body) = http_request.into_parts(); let content_encoding = parts.headers.get(&CONTENT_ENCODING); @@ -651,11 +750,16 @@ mod tests { use std::sync::Arc; use std::sync::Mutex; + // BEGIN ROUTER-2060 + use http::HeaderName; + use http::HeaderValue; use http::StatusCode; use http::Uri; use http::header::CONTENT_TYPE; use hyper_rustls::ConfigBuilderExt; use mime::APPLICATION_JSON; + use opentelemetry::trace::TracerProvider; + use opentelemetry_sdk::propagation::TraceContextPropagator; use tokio::net::TcpListener; use tower::ServiceExt; use tracing::Subscriber; @@ -672,8 +776,10 @@ mod tests { use crate::services::http::BoxService; use crate::services::http::HttpClientService; use crate::services::http::HttpRequest; + use crate::services::http::TraceContextPreservation; // BEGIN/END ROUTER-2060 use crate::services::http::service::WireByteCount; use crate::services::router; + // END ROUTER-2060 async fn emulate_subgraph_with_status_code(listener: TcpListener, status_code: StatusCode) { crate::services::http::tests::serve(listener, move |_| async move { @@ -777,6 +883,7 @@ mod tests { .expect("Able to load native roots") .with_no_client_auth(), crate::configuration::shared::Client::builder().build(), + TraceContextPreservation::default(), // BEGIN/END ROUTER-2060 ) .expect("can create a HttpClientService"); @@ -929,6 +1036,7 @@ mod tests { .expect("Able to load native roots") .with_no_client_auth(), crate::configuration::shared::Client::builder().build(), + TraceContextPreservation::default(), // BEGIN/END ROUTER-2060 ) .expect("can create a HttpClientService"); @@ -1022,6 +1130,7 @@ mod tests { .expect("read native TLS root certificates") .with_no_client_auth(), crate::configuration::shared::Client::builder().build(), + TraceContextPreservation::default(), // BEGIN/END ROUTER-2060 ) .expect("can create a HttpClientService"); @@ -1060,4 +1169,219 @@ mod tests { "WireByteCount should equal the compressed (wire) size, not the decompressed size" ); } + + // BEGIN ROUTER-2060 + // + // These tests exercise the snapshot/restore logic in `call()` end-to-end, which requires a + // genuinely valid, sampled span context (via `otel::layer().force_sampling()` + the + // telemetry plugin's `http_client_service` wrapping, which is what actually creates the + // "http_request" span `Span::current()` sees inside `call()`) and a real globally-registered + // `TraceContextPropagator` (otherwise `get_text_map_propagator` is a no-op and every + // assertion here would pass for the wrong reason). `set_text_map_propagator` mutates + // process-wide OTel state, so these tests are serialized by the mutex below, following the + // same pattern already used for `Telemetry::activate()` in + // `plugins::telemetry::mod::tests` (see `.config/nextest.toml`'s + // `serial-router-2060-trace-context-preservation-unit` group for the nextest-side guard). + static TRACE_CONTEXT_PRESERVATION_TEST: once_cell::sync::Lazy>> = + once_cell::sync::Lazy::new(Default::default); + + async fn make_telemetry_http_client_with_preservation( + service_name: &str, + trace_context_preservation: TraceContextPreservation, + ) -> BoxService { + let full_config = serde_json::json!({ + "telemetry": {} + }); + let telemetry_config = full_config + .as_object() + .expect("must be an object") + .get("telemetry") + .expect("telemetry must be a root key"); + let init = crate::plugin::PluginInit::fake_builder() + .config(telemetry_config.clone()) + .full_config(full_config) + .build() + .with_deserialized_config() + .expect("unable to deserialize telemetry config"); + let plugin = crate::plugin::plugins() + .find(|factory| factory.name == "apollo.telemetry") + .expect("Plugin not found") + .create_instance(init) + .await + .expect("unable to create telemetry plugin"); + + let service_target = ServiceTarget::Subgraph { + name: Arc::from(service_name), + }; + let http_client_service = HttpClientService::new( + service_target, + rustls::ClientConfig::builder() + .with_native_roots() + .expect("Able to load native roots") + .with_no_client_auth(), + crate::configuration::shared::Client::builder().build(), + trace_context_preservation, + ) + .expect("can create a HttpClientService"); + + plugin.http_client_service(service_name, BoxService::new(http_client_service)) + } + + /// Emulates a subgraph that records the headers of the first request it receives. + async fn emulate_subgraph_capturing_headers( + listener: TcpListener, + captured: Arc>>, + ) { + crate::services::http::tests::serve(listener, move |req| { + let captured = captured.clone(); + async move { + *captured.lock().unwrap() = Some(req.headers().clone()); + Ok(http::Response::builder() + .status(StatusCode::OK) + .body(r#"{}"#.into()) + .unwrap()) + } + }) + .await + .unwrap(); + } + + /// `otel::layer()` defaults to a `NoopTracer`, which never produces a *valid* OTel + /// `SpanContext` (see `crate::tracer::test::it_returns_valid_trace_id` for the same + /// `.with_tracer(...)` pattern used to get a real one). Without a valid span context, + /// `TraceContextPropagator::inject_context` silently declines to write anything at all -- + /// so these tests need a genuinely valid one to meaningfully exercise the router's own + /// injection, not just the restore side of the snapshot/restore logic. + fn setup_tracing_with_real_span_context() -> DefaultGuard { + let provider = opentelemetry_sdk::trace::SdkTracerProvider::builder() + .with_simple_exporter(opentelemetry_stdout::SpanExporter::default()) + .build(); + let tracer = provider.tracer_with_scope( + opentelemetry::InstrumentationScope::builder("router-2060-test").build(), + ); + let subscriber = tracing_subscriber::Registry::default() + .with(otel::layer().force_sampling().with_tracer(tracer)); + tracing::subscriber::set_default(subscriber) + } + + /// Sends one request through a telemetry-wrapped `HttpClientService` configured with + /// `preservation`, with `initial_headers` already present on the outgoing request before it + /// reaches the service (emulating a coprocessor/Rhai/header-propagation rewrite that already + /// happened upstream), and returns the headers the mock subgraph actually received. + async fn run_preservation_scenario( + preservation: TraceContextPreservation, + initial_headers: Vec<(HeaderName, &'static str)>, + ) -> http::HeaderMap { + let _propagator_guard = TRACE_CONTEXT_PRESERVATION_TEST.lock().await; + opentelemetry::global::set_text_map_propagator(TraceContextPropagator::new()); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let socket_addr = listener.local_addr().unwrap(); + let captured: Arc>> = Arc::new(Mutex::new(None)); + tokio::task::spawn(emulate_subgraph_capturing_headers( + listener, + captured.clone(), + )); + + let service = make_telemetry_http_client_with_preservation("test", preservation).await; + let _tracing_guard = setup_tracing_with_real_span_context(); + + let mut builder = http::Request::builder() + .uri(Uri::from_str(&format!("http://{socket_addr}")).unwrap()) + .header(CONTENT_TYPE, APPLICATION_JSON.essence_str()); + for (name, value) in initial_headers { + builder = builder.header(name, HeaderValue::from_static(value)); + } + + let response = service + .oneshot(HttpRequest { + http_request: builder + .body(router::body::from_bytes(r#"{"query":"{ me { name } }"#)) + .unwrap(), + context: Context::new(), + }) + .await + .unwrap(); + + let (parts, _) = response.http_response.into_parts(); + assert_eq!(parts.status, StatusCode::OK); + + captured + .lock() + .unwrap() + .take() + .expect("subgraph should have received a request") + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_preserve_disabled_overwrites_existing_traceparent() { + let fake_traceparent = "00-11111111111111111111111111111111-1111111111111111-01"; + let headers = run_preservation_scenario( + TraceContextPreservation::default(), + vec![(HeaderName::from_static("traceparent"), fake_traceparent)], + ) + .await; + + let received = headers + .get("traceparent") + .expect("traceparent should still be set by the router's own injection"); + assert_ne!( + received.to_str().unwrap(), + fake_traceparent, + "with preservation disabled, the router must overwrite a pre-existing traceparent \ + with its own span context (this is the pre-existing, correct default behavior)" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_preserve_enabled_restores_traceparent_and_tracestate() { + let fake_traceparent = "00-22222222222222222222222222222222-2222222222222222-01"; + let fake_tracestate = "vendor=abc123"; + let headers = run_preservation_scenario( + TraceContextPreservation { + enabled: true, + custom_header_name: None, + }, + vec![ + (HeaderName::from_static("traceparent"), fake_traceparent), + (HeaderName::from_static("tracestate"), fake_tracestate), + ], + ) + .await; + + assert_eq!( + headers.get("traceparent").and_then(|v| v.to_str().ok()), + Some(fake_traceparent), + "with preservation enabled, a pre-existing traceparent must survive verbatim" + ); + assert_eq!( + headers.get("tracestate").and_then(|v| v.to_str().ok()), + Some(fake_tracestate), + "tracestate must be preserved independently of traceparent" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn test_preserve_enabled_falls_back_to_traceparent_when_custom_header_empty() { + let fake_traceparent = "00-44444444444444444444444444444444-4444444444444444-01"; + let headers = run_preservation_scenario( + TraceContextPreservation { + enabled: true, + custom_header_name: Some(HeaderName::from_static("x-trace-id")), + }, + vec![ + (HeaderName::from_static("traceparent"), fake_traceparent), + (HeaderName::from_static("x-trace-id"), ""), + ], + ) + .await; + + assert_eq!( + headers.get("traceparent").and_then(|v| v.to_str().ok()), + Some(fake_traceparent), + "an empty custom header must not be treated as present; traceparent should survive \ + via the fallback path (mirrors the extraction-side precedent fixed in PR #9984)" + ); + } + // END ROUTER-2060 } diff --git a/apollo-router/src/services/http/tests.rs b/apollo-router/src/services/http/tests.rs index dd8523d4cb..f17dfa9aca 100644 --- a/apollo-router/src/services/http/tests.rs +++ b/apollo-router/src/services/http/tests.rs @@ -50,6 +50,7 @@ use crate::plugins::traffic_shaping::Http2Config; use crate::services::http::HttpClientService; use crate::services::http::HttpRequest; use crate::services::http::HttpResponse; +use crate::services::http::TraceContextPreservation; // BEGIN/END ROUTER-2060 use crate::services::router; use crate::services::supergraph; @@ -81,7 +82,15 @@ fn make_service( let root_store = &rustls::RootCertStore::empty(); match kind { ServiceKind::Subgraph => { - HttpClientService::from_config_for_subgraph("test", config, root_store, client) + // BEGIN ROUTER-2060 + HttpClientService::from_config_for_subgraph( + "test", + config, + root_store, + client, + TraceContextPreservation::default(), + ) + // END ROUTER-2060 } ServiceKind::Connector => { HttpClientService::from_config_for_connector("test", config, root_store, client) @@ -1473,6 +1482,7 @@ mod http_version_negotiation { crate::configuration::shared::Client::builder() .experimental_http2(http2_config) .build(), + TraceContextPreservation::default(), // BEGIN/END ROUTER-2060 ) .expect("created http client"); diff --git a/apollo-router/tests/integration/coprocessor.rs b/apollo-router/tests/integration/coprocessor.rs index 7e3b7df6c2..0df4dcabab 100644 --- a/apollo-router/tests/integration/coprocessor.rs +++ b/apollo-router/tests/integration/coprocessor.rs @@ -233,6 +233,132 @@ async fn exported_span_name_by_id(otlp: &wiremock::MockServer, span_id: &str) -> .and_then(|v| v.as_string()) } +// BEGIN ROUTER-2060 + +const SUBGRAPH_TRACE_CONTEXT_FABRICATED_TRACEPARENT: &str = + "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-bbbbbbbbbbbbbbbb-01"; + +/// Shared scenario for the `preserve_trace_context_on_subgraph_requests` tests: mocks a +/// coprocessor that rewrites `traceparent` on the `SubgraphRequest` stage to a fabricated hop +/// value, mocks the "products" subgraph (the one `execute_default_query` hits) to capture the +/// `traceparent` it actually receives, and returns that captured value. +async fn run_subgraph_trace_context_preservation_scenario( + preserve_enabled: bool, +) -> Option { + let coprocessor = wiremock::MockServer::start().await; + Mock::given(method("POST")) + .respond_with(move |req: &wiremock::Request| { + let mut body: serde_json::Value = serde_json::from_slice(&req.body).unwrap(); + if body["stage"] == "SubgraphRequest" { + body["control"] = json!("continue"); + body["headers"]["traceparent"] = + json!([SUBGRAPH_TRACE_CONTEXT_FABRICATED_TRACEPARENT]); + } + ResponseTemplate::new(200).set_body_json(body) + }) + .mount(&coprocessor) + .await; + + let captured_traceparent: Arc>> = Arc::new(Mutex::new(None)); + let captured = captured_traceparent.clone(); + let mock_products = wiremock::MockServer::start().await; + Mock::given(method("POST")) + .respond_with(move |req: &wiremock::Request| { + if let Some(tp) = req.headers.get("traceparent") { + *captured.lock().unwrap() = tp.to_str().ok().map(str::to_string); + } + ResponseTemplate::new(200).set_body_json(json!({ + "data": { "topProducts": [{ "name": "Table" }] } + })) + }) + .mount(&mock_products) + .await; + + let otlp = wiremock::MockServer::start().await; + Mock::given(method("POST")) + .and(path("/v1/traces")) + .respond_with(ResponseTemplate::new(200).set_body_raw( + ExportTraceServiceResponse::default().encode_to_vec(), + "application/x-protobuf", + )) + .mount(&otlp) + .await; + + let fixture = if preserve_enabled { + include_str!("fixtures/coprocessor_subgraph_trace_context_preservation_enabled.router.yaml") + } else { + include_str!( + "fixtures/coprocessor_subgraph_trace_context_preservation_disabled.router.yaml" + ) + }; + let config = fixture + .replace("", &coprocessor.uri()) + .replace("", &otlp.uri()); + + let mut router = IntegrationTest::builder() + .config(config) + .telemetry(Telemetry::Otlp { + endpoint: Some(format!("{}/v1/traces", otlp.uri())), + }) + .subgraph_overrides([("products".to_string(), mock_products.uri())].into()) + .reqwest_client(no_keepalive_reqwest_client()) + .build() + .await; + + router.start().await; + router.assert_started().await; + + let (_trace_id, response) = router.execute_default_query().await; + assert_eq!(response.status(), 200); + + router.graceful_shutdown().await; + + captured_traceparent.lock().unwrap().clone() +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_coprocessor_subgraph_trace_context_preserved_when_enabled() -> Result<(), BoxError> { + if !graph_os_enabled() { + return Ok(()); + } + + let received = run_subgraph_trace_context_preservation_scenario(true).await; + + assert_eq!( + received.as_deref(), + Some(SUBGRAPH_TRACE_CONTEXT_FABRICATED_TRACEPARENT), + "with preserve_trace_context_on_subgraph_requests enabled, the coprocessor's explicit \ + traceparent rewrite must survive HttpClientService::call under OTLP, not be silently \ + overwritten by the router's own span context" + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_coprocessor_subgraph_trace_context_overwritten_when_disabled() -> Result<(), BoxError> +{ + if !graph_os_enabled() { + return Ok(()); + } + + let received = run_subgraph_trace_context_preservation_scenario(false).await; + + assert_ne!( + received.as_deref(), + Some(SUBGRAPH_TRACE_CONTEXT_FABRICATED_TRACEPARENT), + "with the config disabled (default), the coprocessor's rewrite must still be discarded \ + and replaced with the router's own span context -- this is the pre-existing, correct \ + default behavior and must not regress" + ); + assert!( + received.is_some(), + "the subgraph must still receive *some* traceparent from the router's normal injection" + ); + Ok(()) +} + +// END ROUTER-2060 + #[tokio::test(flavor = "multi_thread")] async fn test_coprocessor_response_handling() -> Result<(), BoxError> { if !graph_os_enabled() { diff --git a/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_disabled.router.yaml b/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_disabled.router.yaml new file mode 100644 index 0000000000..04c2b719ee --- /dev/null +++ b/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_disabled.router.yaml @@ -0,0 +1,21 @@ +coprocessor: + url: "" + subgraph: + all: + request: + headers: true +telemetry: + exporters: + tracing: + propagation: + preserve_trace_context_on_subgraph_requests: false + common: + service_name: router + sampler: 1.0 + parent_based_sampler: false + otlp: + enabled: true + protocol: http + endpoint: + batch_processor: + scheduled_delay: 10ms diff --git a/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_enabled.router.yaml b/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_enabled.router.yaml new file mode 100644 index 0000000000..4c643f48ad --- /dev/null +++ b/apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_enabled.router.yaml @@ -0,0 +1,21 @@ +coprocessor: + url: "" + subgraph: + all: + request: + headers: true +telemetry: + exporters: + tracing: + propagation: + preserve_trace_context_on_subgraph_requests: true + common: + service_name: router + sampler: 1.0 + parent_based_sampler: false + otlp: + enabled: true + protocol: http + endpoint: + batch_processor: + scheduled_delay: 10ms