From f76ee794f44ec6af472c1363c09fee635f3f328b Mon Sep 17 00:00:00 2001 From: OriginLeon Date: Mon, 17 Aug 2026 15:35:27 -0300 Subject: [PATCH 1/5] fix(telemetry): add opt-in preservation of trace-context headers on subgraph requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ...nfiguration__tests__schema_generation.snap | 6 + apollo-router/src/plugins/telemetry/config.rs | 6 + apollo-router/src/router_factory.rs | 20 ++++ apollo-router/src/services/http.rs | 2 + apollo-router/src/services/http/service.rs | 113 +++++++++++++++++- apollo-router/src/services/http/tests.rs | 12 +- 6 files changed, 153 insertions(+), 6 deletions(-) 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 23bdf6f1b6..0dc40a26c8 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_subgraph_trace_context": { + "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..4a88160ac2 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_subgraph_trace_context: 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..fb20034aa7 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -29,11 +29,13 @@ 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; use crate::query_planner::QueryPlannerService; use crate::services::HasSchema; +use crate::services::http::TraceContextPreservation; // BEGIN/END ROUTER-2060 use crate::services::PluggableSupergraphServiceBuilder; use crate::services::Plugins; use crate::services::SubgraphService; @@ -446,6 +448,23 @@ 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() == "telemetry") + .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_subgraph_trace_context) + .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 +486,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..e63178c561 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); @@ -672,6 +771,7 @@ 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; @@ -777,6 +877,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 +1030,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 +1124,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"); 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"); From 5bb9954eb6b1fbda0998d5460440e8e04d07c6c7 Mon Sep 17 00:00:00 2001 From: OriginLeon Date: Mon, 17 Aug 2026 18:36:32 -0300 Subject: [PATCH 2/5] style: apply cargo fmt to router_factory.rs CI's fmt_check/lint jobs failed on an import ordering issue introduced by the ROUTER-2060 changes. Co-Authored-By: Claude Sonnet 5 --- apollo-router/src/router_factory.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/router_factory.rs b/apollo-router/src/router_factory.rs index fb20034aa7..d0be872fea 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -35,7 +35,6 @@ use crate::plugins::traffic_shaping::APOLLO_TRAFFIC_SHAPING; use crate::plugins::traffic_shaping::TrafficShaping; use crate::query_planner::QueryPlannerService; use crate::services::HasSchema; -use crate::services::http::TraceContextPreservation; // BEGIN/END ROUTER-2060 use crate::services::PluggableSupergraphServiceBuilder; use crate::services::Plugins; use crate::services::SubgraphService; @@ -43,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; @@ -457,7 +457,12 @@ pub(crate) async fn create_http_services( let subgraph_trace_context_preservation = TraceContextPreservation { enabled: telemetry_config .as_ref() - .map(|c| c.exporters.tracing.propagation.preserve_subgraph_trace_context) + .map(|c| { + c.exporters + .tracing + .propagation + .preserve_subgraph_trace_context + }) .unwrap_or(false), custom_header_name: telemetry_config .as_ref() From 127464a777f0db5656c022d8a7fb96fe5a64f84e Mon Sep 17 00:00:00 2001 From: OriginLeon Date: Tue, 18 Aug 2026 13:05:20 -0300 Subject: [PATCH 3/5] fix(telemetry): correct plugin lookup key and rename config field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- ...nfiguration__tests__schema_generation.snap | 2 +- apollo-router/src/plugins/telemetry/config.rs | 2 +- apollo-router/src/router_factory.rs | 12 +++++-- apollo-router/src/services/http/service.rs | 34 +++++++++++++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) 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 519f11a1bf..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 @@ -8143,7 +8143,7 @@ expression: "&schema" "description": "Propagate Jaeger", "type": "boolean" }, - "preserve_subgraph_trace_context": { + "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" diff --git a/apollo-router/src/plugins/telemetry/config.rs b/apollo-router/src/plugins/telemetry/config.rs index 4a88160ac2..89fa6302ff 100644 --- a/apollo-router/src/plugins/telemetry/config.rs +++ b/apollo-router/src/plugins/telemetry/config.rs @@ -418,7 +418,7 @@ pub(crate) struct Propagation { /// 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_subgraph_trace_context: bool, + pub(crate) preserve_trace_context_on_subgraph_requests: bool, // END ROUTER-2060 } diff --git a/apollo-router/src/router_factory.rs b/apollo-router/src/router_factory.rs index d0be872fea..9c5f39845a 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -451,7 +451,7 @@ pub(crate) async fn create_http_services( // BEGIN ROUTER-2060 let telemetry_config = plugins .iter() - .find(|i| i.0.as_str() == "telemetry") + .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 { @@ -461,13 +461,21 @@ pub(crate) async fn create_http_services( c.exporters .tracing .propagation - .preserve_subgraph_trace_context + .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()), }; + // TEMP DEBUG ROUTER-2060 - remove after validation + tracing::warn!( + telemetry_plugin_found = telemetry_config.is_some(), + enabled = subgraph_trace_context_preservation.enabled, + custom_header_name = ?subgraph_trace_context_preservation.custom_header_name, + "ROUTER-2060 DEBUG: resolved trace_context_preservation for subgraph HttpClientServices" + ); + // END TEMP DEBUG ROUTER-2060 // END ROUTER-2060 let connector_subgraphs: HashSet = schema diff --git a/apollo-router/src/services/http/service.rs b/apollo-router/src/services/http/service.rs index e63178c561..c4a025cc50 100644 --- a/apollo-router/src/services/http/service.rs +++ b/apollo-router/src/services/http/service.rs @@ -581,6 +581,16 @@ impl tower::Service for HttpClientService { let service_name = self.service.clone(); let http_req_span = Span::current(); + // TEMP DEBUG ROUTER-2060 - remove after validation + tracing::warn!( + preserve_enabled = self.trace_context_preservation.enabled, + custom_header_name = ?self.trace_context_preservation.custom_header_name, + traceparent_before = ?http_request.headers().get("traceparent"), + tracestate_before = ?http_request.headers().get("tracestate"), + "ROUTER-2060 DEBUG: entering call(), state before snapshot" + ); + // END TEMP DEBUG ROUTER-2060 + // BEGIN ROUTER-2060 // // If the router is configured to preserve an already-present trace-context header on @@ -619,6 +629,14 @@ impl tower::Service for HttpClientService { .flatten(); // END ROUTER-2060 + // TEMP DEBUG ROUTER-2060 - remove after validation + tracing::warn!( + ?saved_trace_header, + ?saved_tracestate, + "ROUTER-2060 DEBUG: snapshot captured (this is what will be restored, if Some)" + ); + // END TEMP DEBUG ROUTER-2060 + get_text_map_propagator(|propagator| { propagator.inject_context( &prepare_context(http_req_span.context()), @@ -626,6 +644,14 @@ impl tower::Service for HttpClientService { ); }); + // TEMP DEBUG ROUTER-2060 - remove after validation + tracing::warn!( + traceparent_after_injection = ?http_request.headers().get("traceparent"), + tracestate_after_injection = ?http_request.headers().get("tracestate"), + "ROUTER-2060 DEBUG: state right after router's own propagator injection, before restore" + ); + // END TEMP DEBUG ROUTER-2060 + // BEGIN ROUTER-2060 if let Some((name, value)) = saved_trace_header { http_request.headers_mut().insert(name, value); @@ -635,6 +661,14 @@ impl tower::Service for HttpClientService { } // END ROUTER-2060 + // TEMP DEBUG ROUTER-2060 - remove after validation + tracing::warn!( + traceparent_final = ?http_request.headers().get("traceparent"), + tracestate_final = ?http_request.headers().get("tracestate"), + "ROUTER-2060 DEBUG: final state about to be sent to the subgraph" + ); + // END TEMP DEBUG ROUTER-2060 + let (parts, body) = http_request.into_parts(); let content_encoding = parts.headers.get(&CONTENT_ENCODING); From 83c0a6d55412b2e13d7c603b3221ccbd31d53340 Mon Sep 17 00:00:00 2001 From: OriginLeon Date: Wed, 19 Aug 2026 09:42:58 -0300 Subject: [PATCH 4/5] test(telemetry): add tests, usage metric, and changeset for trace-context 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 --- ...er_2060_preserve_subgraph_trace_context.md | 17 ++ apollo-router/src/configuration/metrics.rs | 7 +- ...__test__metrics@telemetry.router.yaml.snap | 2 + apollo-router/src/router_factory.rs | 8 - apollo-router/src/services/http/service.rs | 265 +++++++++++++++--- .../tests/integration/coprocessor.rs | 126 +++++++++ ..._context_preservation_disabled.router.yaml | 21 ++ ...e_context_preservation_enabled.router.yaml | 21 ++ 8 files changed, 424 insertions(+), 43 deletions(-) create mode 100644 .changesets/fix_router_2060_preserve_subgraph_trace_context.md create mode 100644 apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_disabled.router.yaml create mode 100644 apollo-router/tests/integration/fixtures/coprocessor_subgraph_trace_context_preservation_enabled.router.yaml 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/router_factory.rs b/apollo-router/src/router_factory.rs index 9c5f39845a..949ef6a8ec 100644 --- a/apollo-router/src/router_factory.rs +++ b/apollo-router/src/router_factory.rs @@ -468,14 +468,6 @@ pub(crate) async fn create_http_services( .as_ref() .and_then(|c| c.exporters.tracing.propagation.request.header_name.clone()), }; - // TEMP DEBUG ROUTER-2060 - remove after validation - tracing::warn!( - telemetry_plugin_found = telemetry_config.is_some(), - enabled = subgraph_trace_context_preservation.enabled, - custom_header_name = ?subgraph_trace_context_preservation.custom_header_name, - "ROUTER-2060 DEBUG: resolved trace_context_preservation for subgraph HttpClientServices" - ); - // END TEMP DEBUG ROUTER-2060 // END ROUTER-2060 let connector_subgraphs: HashSet = schema diff --git a/apollo-router/src/services/http/service.rs b/apollo-router/src/services/http/service.rs index c4a025cc50..6dc4bc11bc 100644 --- a/apollo-router/src/services/http/service.rs +++ b/apollo-router/src/services/http/service.rs @@ -581,16 +581,6 @@ impl tower::Service for HttpClientService { let service_name = self.service.clone(); let http_req_span = Span::current(); - // TEMP DEBUG ROUTER-2060 - remove after validation - tracing::warn!( - preserve_enabled = self.trace_context_preservation.enabled, - custom_header_name = ?self.trace_context_preservation.custom_header_name, - traceparent_before = ?http_request.headers().get("traceparent"), - tracestate_before = ?http_request.headers().get("tracestate"), - "ROUTER-2060 DEBUG: entering call(), state before snapshot" - ); - // END TEMP DEBUG ROUTER-2060 - // BEGIN ROUTER-2060 // // If the router is configured to preserve an already-present trace-context header on @@ -629,14 +619,6 @@ impl tower::Service for HttpClientService { .flatten(); // END ROUTER-2060 - // TEMP DEBUG ROUTER-2060 - remove after validation - tracing::warn!( - ?saved_trace_header, - ?saved_tracestate, - "ROUTER-2060 DEBUG: snapshot captured (this is what will be restored, if Some)" - ); - // END TEMP DEBUG ROUTER-2060 - get_text_map_propagator(|propagator| { propagator.inject_context( &prepare_context(http_req_span.context()), @@ -644,14 +626,6 @@ impl tower::Service for HttpClientService { ); }); - // TEMP DEBUG ROUTER-2060 - remove after validation - tracing::warn!( - traceparent_after_injection = ?http_request.headers().get("traceparent"), - tracestate_after_injection = ?http_request.headers().get("tracestate"), - "ROUTER-2060 DEBUG: state right after router's own propagator injection, before restore" - ); - // END TEMP DEBUG ROUTER-2060 - // BEGIN ROUTER-2060 if let Some((name, value)) = saved_trace_header { http_request.headers_mut().insert(name, value); @@ -661,14 +635,6 @@ impl tower::Service for HttpClientService { } // END ROUTER-2060 - // TEMP DEBUG ROUTER-2060 - remove after validation - tracing::warn!( - traceparent_final = ?http_request.headers().get("traceparent"), - tracestate_final = ?http_request.headers().get("tracestate"), - "ROUTER-2060 DEBUG: final state about to be sent to the subgraph" - ); - // END TEMP DEBUG ROUTER-2060 - let (parts, body) = http_request.into_parts(); let content_encoding = parts.headers.get(&CONTENT_ENCODING); @@ -784,11 +750,15 @@ 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_sdk::propagation::TraceContextPropagator; use tokio::net::TcpListener; use tower::ServiceExt; use tracing::Subscriber; @@ -808,6 +778,7 @@ mod tests { 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 { @@ -1197,4 +1168,230 @@ 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(); + } + + /// 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, _recording_layer) = setup_tracing(); + + 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_custom_header_takes_priority_over_traceparent() { + let fake_traceparent = "00-33333333333333333333333333333333-3333333333333333-01"; + let fake_custom_trace_id = "custom-trace-id-value"; + 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"), fake_custom_trace_id), + ], + ) + .await; + + assert_eq!( + headers.get("x-trace-id").and_then(|v| v.to_str().ok()), + Some(fake_custom_trace_id), + "the configured custom trace ID header takes priority and must be preserved" + ); + assert_ne!( + headers.get("traceparent").and_then(|v| v.to_str().ok()), + Some(fake_traceparent), + "when the custom header wins, traceparent itself is not separately protected and \ + is overwritten by the router's own injection" + ); + } + + #[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/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 From 45cbf6d6ac7449f9c5966d5cb8c54cfc23eaedaa Mon Sep 17 00:00:00 2001 From: OriginLeon Date: Wed, 19 Aug 2026 11:01:59 -0300 Subject: [PATCH 5/5] fix(telemetry): use a real tracer in trace-context preservation tests 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 --- apollo-router/src/services/http/service.rs | 50 +++++++++------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/apollo-router/src/services/http/service.rs b/apollo-router/src/services/http/service.rs index 6dc4bc11bc..17fc98aad8 100644 --- a/apollo-router/src/services/http/service.rs +++ b/apollo-router/src/services/http/service.rs @@ -758,6 +758,7 @@ mod tests { 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; @@ -1245,6 +1246,24 @@ mod tests { .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 @@ -1265,7 +1284,7 @@ mod tests { )); let service = make_telemetry_http_client_with_preservation("test", preservation).await; - let (_tracing_guard, _recording_layer) = setup_tracing(); + let _tracing_guard = setup_tracing_with_real_span_context(); let mut builder = http::Request::builder() .uri(Uri::from_str(&format!("http://{socket_addr}")).unwrap()) @@ -1342,35 +1361,6 @@ mod tests { ); } - #[tokio::test(flavor = "multi_thread")] - async fn test_preserve_enabled_custom_header_takes_priority_over_traceparent() { - let fake_traceparent = "00-33333333333333333333333333333333-3333333333333333-01"; - let fake_custom_trace_id = "custom-trace-id-value"; - 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"), fake_custom_trace_id), - ], - ) - .await; - - assert_eq!( - headers.get("x-trace-id").and_then(|v| v.to_str().ok()), - Some(fake_custom_trace_id), - "the configured custom trace ID header takes priority and must be preserved" - ); - assert_ne!( - headers.get("traceparent").and_then(|v| v.to_str().ok()), - Some(fake_traceparent), - "when the custom header wins, traceparent itself is not separately protected and \ - is overwritten by the router's own injection" - ); - } - #[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";