From d4d4c20b2d207355aeda9209b19b835a867b2426 Mon Sep 17 00:00:00 2001 From: Brent Date: Fri, 14 Aug 2026 19:34:07 -0400 Subject: [PATCH 1/2] Inject traceparent on egress so a trace survives the service hop (th-fb312d) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In 3 hours of production traffic: 34,961 single-service traces and 4 multi-service ones. Not because services don't call each other — because NOTHING injects W3C trace context on the way out. Every hop starts a new root, so a request crossing three services looks like three unrelated traces and "show me this request end to end" cannot be answered. The inbound half was already correct: api-prime EXTRACTs traceparent, and the observability SDK ships a reqwest middleware that injects. It had zero consumers. The missing piece is the client Rust services actually use for outbound calls — this one. Injects at `do_single_request`, so retries and redirects each carry a current traceparent rather than reusing a stale one. Behind an optional `otel` feature: default builds keep the dependency-free client, and callers that opt in get propagation with no code change. Context resolution is tracing-first, then `opentelemetry::Context::current()`. A subagent claimed the second read would be dead code under tracing-opentelemetry. I tested it rather than believing it — both are valid (tracing_span_context_valid=true otel_current_valid=true) — so the fallback stays for callers using the OTel API directly without a tracing subscriber. Two guards, both tested at the wire: - an invalid/absent span context injects NOTHING (no `00-000…-00` headers) - a caller-set traceparent is never overwritten 3 wire-level tests; 110 tests pass; clippy clean on both feature sets. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/traceparent-injection.md | 9 + rust/fetch/Cargo.lock | 172 +++++++++++++++++++- rust/fetch/Cargo.toml | 26 +++ rust/fetch/src/client.rs | 100 ++++++++++++ rust/fetch/tests/trace_propagation_tests.rs | 141 ++++++++++++++++ 5 files changed, 444 insertions(+), 4 deletions(-) create mode 100644 .changeset/traceparent-injection.md create mode 100644 rust/fetch/tests/trace_propagation_tests.rs diff --git a/.changeset/traceparent-injection.md b/.changeset/traceparent-injection.md new file mode 100644 index 0000000..e34a165 --- /dev/null +++ b/.changeset/traceparent-injection.md @@ -0,0 +1,9 @@ +--- +'@smooai/fetch': minor +--- + +Rust: optional `otel` feature that injects W3C trace context (`traceparent`) into +every outbound request, so a call made through this client continues the caller's +trace instead of starting a new root. Off by default — the crate does not link +OpenTelemetry unless you ask for it. Guards an invalid span context and never +overwrites a `traceparent` the caller set explicitly. diff --git a/rust/fetch/Cargo.lock b/rust/fetch/Cargo.lock index 189d455..17b2135 100644 --- a/rust/fetch/Cargo.lock +++ b/rust/fetch/Cargo.lock @@ -300,6 +300,18 @@ dependencies = [ "wasi", ] +[[package]] +name = "getrandom" +version = "0.3.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "899def5c37c4fd7b2664648c28120ecec138e4d395b459e5ca34f9cce2dd77fd" +dependencies = [ + "cfg-if", + "libc", + "r-efi", + "wasip2", +] + [[package]] name = "getrandom" version = "0.4.1" @@ -721,6 +733,15 @@ dependencies = [ "tempfile", ] +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "num_cpus" version = "1.17.0" @@ -781,6 +802,36 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "opentelemetry" +version = "0.32.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b0142c63252a9e054e68a4c61a5778f7b14f576274d593f8ce883d191a099682" +dependencies = [ + "futures-core", + "futures-sink", + "js-sys", + "pin-project-lite", + "thiserror", + "tracing", +] + +[[package]] +name = "opentelemetry_sdk" +version = "0.32.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b59f80e1ac4d5ff7a2db8fb6c80badb7f0f3f858211fba08dd9aaec750894f9" +dependencies = [ + "futures-channel", + "futures-executor", + "futures-util", + "opentelemetry", + "percent-encoding", + "portable-atomic", + "rand 0.9.5", + "thiserror", +] + [[package]] name = "parking_lot" version = "0.12.5" @@ -828,6 +879,12 @@ version = "0.3.32" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7edddbd0b52d732b21ad9a5fab5c704c14cd949e5e9a1ec5929a24fded1b904c" +[[package]] +name = "portable-atomic" +version = "1.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" + [[package]] name = "potential_utf" version = "0.1.4" @@ -887,8 +944,18 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404" dependencies = [ "libc", - "rand_chacha", - "rand_core", + "rand_chacha 0.3.1", + "rand_core 0.6.4", +] + +[[package]] +name = "rand" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9ef1d0d795eb7d84685bca4f72f3649f064e6641543d3a8c415898726a57b41" +dependencies = [ + "rand_chacha 0.9.0", + "rand_core 0.9.5", ] [[package]] @@ -898,7 +965,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" dependencies = [ "ppv-lite86", - "rand_core", + "rand_core 0.6.4", +] + +[[package]] +name = "rand_chacha" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3022b5f1df60f26e1ffddd6c66e8aa15de382ae63b3a0c1bfc0e4d3e3f325cb" +dependencies = [ + "ppv-lite86", + "rand_core 0.9.5", ] [[package]] @@ -910,6 +987,15 @@ dependencies = [ "getrandom 0.2.17", ] +[[package]] +name = "rand_core" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "76afc826de14238e6e8c374ddcc1fa19e374fd8dd986b0d2af0d02377261d83c" +dependencies = [ + "getrandom 0.3.4", +] + [[package]] name = "redox_syscall" version = "0.5.18" @@ -1159,6 +1245,15 @@ dependencies = [ "serde", ] +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + [[package]] name = "shlex" version = "1.3.0" @@ -1191,13 +1286,17 @@ checksum = "67b1b7a3b5fe4f1376887184045fcf45c69e92af734b7aaddc05fb777b6fbd03" name = "smooai-fetch" version = "2.1.2" dependencies = [ - "rand", + "opentelemetry", + "opentelemetry_sdk", + "rand 0.8.5", "reqwest", "serde", "serde_json", "thiserror", "tokio", "tracing", + "tracing-opentelemetry", + "tracing-subscriber", "wiremock", ] @@ -1308,6 +1407,15 @@ dependencies = [ "syn", ] +[[package]] +name = "thread_local" +version = "1.1.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1ad99c4c6d32803332c548b1af0540b357b3f5fc0be8f6c6bfe8b2e6ae784070" +dependencies = [ + "cfg-if", +] + [[package]] name = "tinystr" version = "0.8.2" @@ -1453,6 +1561,46 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", +] + +[[package]] +name = "tracing-opentelemetry" +version = "0.33.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adbc64cba7137545b8044cb1fe9814f7aacf3c6b5f9b45be8bb5db538befdb26" +dependencies = [ + "js-sys", + "opentelemetry", + "tracing", + "tracing-core", + "tracing-subscriber", + "web-time", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319" +dependencies = [ + "nu-ansi-term", + "sharded-slab", + "smallvec", + "thread_local", + "tracing-core", + "tracing-log", ] [[package]] @@ -1497,6 +1645,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" +[[package]] +name = "valuable" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" + [[package]] name = "vcpkg" version = "0.2.15" @@ -1639,6 +1793,16 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "web-time" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a6580f308b1fad9207618087a65c04e7a10bc77e02c8e84e9b00dd4b12fa0bb" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + [[package]] name = "windows-link" version = "0.2.1" diff --git a/rust/fetch/Cargo.toml b/rust/fetch/Cargo.toml index b9e5a3b..6301421 100644 --- a/rust/fetch/Cargo.toml +++ b/rust/fetch/Cargo.toml @@ -19,6 +19,32 @@ tokio = { version = "1", features = ["full"] } rand = "0.8" tracing = "0.1" +# W3C trace-context propagation, OPTIONAL by design. +# +# @smooai/fetch is an OSS HTTP client; external consumers must not be forced into +# an OpenTelemetry dependency to use it. With the feature off, `inject_trace_context` +# compiles to a no-op and nothing here links OTel at all. +# +# Turn it on inside the platform (`features = ["otel"]`) and every outbound call +# made through this client continues the caller's trace instead of starting a new +# root — which is the whole gap: 34,961 traces touched one service and 4 touched +# two, because nothing injected the header. +opentelemetry = { version = "0.32", optional = true, default-features = false, features = ["trace"] } +# Needed to read a `tracing` span's OTel context. Every SmooAI Rust service +# carries its span through a tracing-opentelemetry layer, and +# `opentelemetry::Context::current()` does NOT see those — reading only the +# OTel-native context makes this whole feature a no-op in production. +tracing-opentelemetry = { version = "0.33", optional = true, default-features = false } + +[features] +default = [] +otel = ["dep:opentelemetry", "dep:tracing-opentelemetry"] + [dev-dependencies] tokio = { version = "1", features = ["test-util", "macros", "rt-multi-thread"] } wiremock = "0.6" +# The propagation tests drive a real tracer + the real W3C propagator so the +# assertion is on what the SERVER received, not on our own builder. +opentelemetry_sdk = { version = "0.32", features = ["trace"] } +tracing-subscriber = { version = "0.3", features = ["registry"] } +serde_json = "1" diff --git a/rust/fetch/src/client.rs b/rust/fetch/src/client.rs index 1562574..19c502e 100644 --- a/rust/fetch/src/client.rs +++ b/rust/fetch/src/client.rs @@ -71,6 +71,102 @@ async fn acquire_with_retry( }) } +/// Inject W3C trace context (`traceparent`/`tracestate`) into an outbound request. +/// +/// # Why this exists +/// +/// api-prime already EXTRACTS `traceparent` on ingress, but nothing on the +/// platform ever injected it on egress — so every service-to-service call began a +/// brand new root trace. Measured on 2026-08-14 over a three-hour window: +/// 34,961 traces touched exactly one service, and 4 touched two. Distributed +/// tracing did not work, and no amount of extra spans inside a service could fix +/// it. +/// +/// This is the client, which is the correct place for propagation: services are +/// already required to use `@smooai/fetch` over raw HTTP, so wiring it once here +/// covers the fleet. +/// +/// # Three guards, each for a reason +/// +/// 1. **Optional feature.** With `otel` off this is a no-op and the crate does +/// not link OpenTelemetry — an OSS HTTP client must not force that on anyone. +/// 2. **Valid span contexts only.** An unregistered TracerProvider yields +/// `INVALID_SPAN_CONTEXT` (all-zero ids). Injecting that writes a malformed +/// `traceparent` that a downstream service will either reject or, worse, +/// adopt — poisoning its trace. The sibling logger shipped exactly this bug, +/// where all-zero ids overwrote a real correlation id. +/// 3. **Caller wins.** If the caller already set `traceparent` explicitly, theirs +/// is left alone. A client that silently rewrites an intentional header is +/// worse than one that does nothing. +#[cfg(feature = "otel")] +fn inject_trace_context( + builder: reqwest::RequestBuilder, + caller_headers: &std::collections::HashMap, +) -> reqwest::RequestBuilder { + use opentelemetry::trace::TraceContextExt as _; + use tracing_opentelemetry::OpenTelemetrySpanExt as _; + + if caller_headers + .keys() + .any(|k| k.eq_ignore_ascii_case("traceparent")) + { + return builder; + } + + // TWO context homes, and neither falls back to the other. + // + // Every SmooAI Rust service carries its span as a `tracing` span picked up by + // a tracing-opentelemetry layer; that context is reachable ONLY through + // `Span::current().context()`. `opentelemetry::Context::current()` sees just + // OTel-native spans. Reading only the latter — which is what this function + // did first — makes the entire feature a silent no-op in production while + // passing a test that happens to create an OTel-native span. + // Read the `tracing` span's context first, then fall back to the OTel-native + // one. + // + // Measured, not assumed: with tracing-opentelemetry 0.33 + opentelemetry + // 0.32, `Context::current()` DOES see a tracing span, so the fallback alone + // would work today. This ordering is belt-and-braces — it does not depend on + // tracing-opentelemetry continuing to mirror into the OTel thread-local, and + // it costs one extra call. + let cx = tracing::Span::current().context(); + let cx = if cx.span().span_context().is_valid() { + cx + } else { + opentelemetry::Context::current() + }; + + if !cx.span().span_context().is_valid() { + return builder; + } + + // `HashMap` implements `Injector` upstream (opentelemetry + // 0.32 propagation/mod.rs), so there is no carrier type to write and no + // `opentelemetry-http` dependency to add. + let mut carrier: std::collections::HashMap = std::collections::HashMap::new(); + opentelemetry::global::get_text_map_propagator(|propagator| { + propagator.inject_context(&cx, &mut carrier); + }); + + let mut builder = builder; + for (key, value) in carrier { + // reqwest's `.header()` APPENDS rather than replaces, so a duplicate + // would send two traceparents. The caller-wins guard above is what keeps + // that from happening. + builder = builder.header(key, value); + } + builder +} + +/// No-op when the `otel` feature is off — the crate does not link OpenTelemetry. +#[cfg(not(feature = "otel"))] +fn inject_trace_context( + builder: reqwest::RequestBuilder, + _caller_headers: &std::collections::HashMap, +) -> reqwest::RequestBuilder { + builder +} + /// Perform a single HTTP request (no retry, no timeout wrapper). async fn do_single_request( url: &str, @@ -85,6 +181,10 @@ async fn do_single_request( request_builder = request_builder.header(key, value); } + // Continue the caller's trace across the hop. Applied AFTER the caller's own + // headers so an explicitly-set `traceparent` still wins (see the function). + request_builder = inject_trace_context(request_builder, &init.headers); + // Set body if let Some(ref body) = init.body { request_builder = request_builder.body(body.clone()); diff --git a/rust/fetch/tests/trace_propagation_tests.rs b/rust/fetch/tests/trace_propagation_tests.rs new file mode 100644 index 0000000..fe6a6a9 --- /dev/null +++ b/rust/fetch/tests/trace_propagation_tests.rs @@ -0,0 +1,141 @@ +//! Trace-context propagation on egress (feature `otel`). +//! +//! The gap these guard: api-prime EXTRACTS `traceparent` on ingress, but nothing +//! ever INJECTED it, so every service-to-service call began a new root trace. +//! Measured 2026-08-14 over three hours: 34,961 traces touched one service, 4 +//! touched two. +//! +//! Asserted at the WIRE — what the server actually received — rather than by +//! inspecting our own builder. A header we believe we set and the server never +//! sees is the exact failure being fixed. +#![cfg(feature = "otel")] + +use opentelemetry::trace::TraceContextExt; +use opentelemetry::trace::TracerProvider as _; +use opentelemetry_sdk::propagation::TraceContextPropagator; +use opentelemetry_sdk::trace::SdkTracerProvider; +use std::collections::HashMap; +use tracing_opentelemetry::OpenTelemetrySpanExt as _; +use tracing_subscriber::layer::SubscriberExt as _; +use wiremock::matchers::{method, path}; +use wiremock::{Mock, MockServer, Request, ResponseTemplate}; + +use smooai_fetch::client; +use smooai_fetch::types::{Method as FetchMethod, RequestInit}; + +fn init_from(server: &MockServer, headers: HashMap) -> (String, RequestInit) { + ( + format!("{}/x", server.uri()), + RequestInit { + method: FetchMethod::GET, + headers, + ..Default::default() + }, + ) +} + +async fn captured_header(server: &MockServer, name: &str) -> Option { + let requests = server.received_requests().await.expect("recording enabled"); + requests + .first() + .and_then(|r: &Request| r.headers.get(name)) + .and_then(|v| v.to_str().ok()) + .map(str::to_owned) +} + +#[tokio::test] +async fn an_active_span_is_propagated_as_a_traceparent_header() { + opentelemetry::global::set_text_map_propagator(TraceContextPropagator::new()); + let provider = SdkTracerProvider::builder().build(); + let tracer = provider.tracer("fetch-propagation-test"); + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/x")) + .respond_with(ResponseTemplate::new(200).set_body_string("{}")) + .mount(&server) + .await; + + // Production shape: a `tracing` span picked up by a tracing-opentelemetry + // layer — NOT an OTel-native span. An earlier version of this test used the + // native form and passed against an implementation that read only + // `Context::current()`, which sees nothing in any real SmooAI service. The + // feature would have shipped as a silent no-op. + let subscriber = + tracing_subscriber::registry().with(tracing_opentelemetry::layer().with_tracer(tracer)); + let (url, init) = init_from(&server, HashMap::new()); + + let expected_trace_id = { + let _sub = tracing::subscriber::set_default(subscriber); + let span = tracing::info_span!("caller"); + let entered = span.enter(); + let id = span.context().span().span_context().trace_id().to_string(); + let _ = client::fetch::(&url, init, None, None, None, None, None).await; + drop(entered); + id + }; + + let traceparent = captured_header(&server, "traceparent") + .await + .expect("the server received a traceparent header"); + assert!( + traceparent.contains(&expected_trace_id), + "traceparent {traceparent} must carry the caller's trace id {expected_trace_id}" + ); +} + +#[tokio::test] +async fn no_active_span_means_no_header() { + opentelemetry::global::set_text_map_propagator(TraceContextPropagator::new()); + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/x")) + .respond_with(ResponseTemplate::new(200).set_body_string("{}")) + .mount(&server) + .await; + + let (url, init) = init_from(&server, HashMap::new()); + let _ = client::fetch::(&url, init, None, None, None, None, None).await; + + // An unregistered/absent span yields INVALID_SPAN_CONTEXT — all-zero ids. + // Injecting that writes a malformed traceparent the downstream service may + // reject, or worse adopt, poisoning its trace. The sibling logger shipped + // exactly this bug, where all-zero ids overwrote a real correlation id. + assert_eq!(captured_header(&server, "traceparent").await, None); +} + +#[tokio::test] +async fn an_explicit_caller_header_is_never_overwritten() { + opentelemetry::global::set_text_map_propagator(TraceContextPropagator::new()); + let provider = SdkTracerProvider::builder().build(); + let tracer = provider.tracer("fetch-propagation-test"); + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/x")) + .respond_with(ResponseTemplate::new(200).set_body_string("{}")) + .mount(&server) + .await; + + const CALLER: &str = "00-11111111111111111111111111111111-2222222222222222-01"; + let mut headers = HashMap::new(); + headers.insert("traceparent".to_string(), CALLER.to_string()); + + let subscriber = + tracing_subscriber::registry().with(tracing_opentelemetry::layer().with_tracer(tracer)); + let (url, init) = init_from(&server, headers); + { + let _sub = tracing::subscriber::set_default(subscriber); + let span = tracing::info_span!("caller"); + let _entered = span.enter(); + let _ = client::fetch::(&url, init, None, None, None, None, None).await; + } + + // A client that silently rewrites an intentional header is worse than one + // that does nothing. + assert_eq!( + captured_header(&server, "traceparent").await.as_deref(), + Some(CALLER) + ); +} From 35655613c61b9a56ad1231719b7725d7d5d93a86 Mon Sep 17 00:00:00 2001 From: Brent Date: Fri, 14 Aug 2026 19:59:21 -0400 Subject: [PATCH 2/2] =?UTF-8?q?Fan=20traceparent=20injection=20out=20to=20?= =?UTF-8?q?TypeScript,=20Go,=20Python=20=E2=80=94=20and=20prove=20.NET=20a?= =?UTF-8?q?lready=20does=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same semantics in every language, because a trace that survives one hop and breaks at the next is not much better than no trace: - inject at the SINGLE-REQUEST site, inside the retry/timeout/breaker wrappers, so each attempt carries a CURRENT traceparent instead of re-sending a stale one - never overwrite a caller-supplied traceparent - emit NOTHING when there is no valid span context — never `00-000…-00` Per-language notes worth keeping: TypeScript (src/fetch.ts, in doGlobalFetch) — builds a COPY of the headers rather than mutating `init`. Mutating would make attempt 2 see our own header as "caller-supplied" and pin every retry to a stale traceparent. Optional peer dep on @opentelemetry/api via a guarded dynamic import; verified the built bundles keep it external. Go (go/fetch/client.go, in executeHTTPRequest) — injected AFTER the pre-request hook, because that hook may replace the whole *http.Request and would silently discard a header injected before it. Pinned otel v1.38: v1.39+ declare go 1.24 and CI pins 1.23. Python (_client.py, in _do_request) — copies request kwargs per attempt so retries re-inject. Optional `[otel]` extra. .NET — NO production change. HttpClient's DiagnosticsHandler already injects from the current Activity, outside redirects and pooling, and this client keeps that chain intact. Adding our own would double-inject. Tests pin the behaviour so a future custom primary handler can't silently drop it; the negative control (propagation disabled) fails 5 of 7, which is what makes the verdict worth anything. Every language is tested at the WIRE — a real local server, asserting the headers it actually received — not against mocks. A mock that agrees with our own bookkeeping is precisely the failure mode being fixed here. Each suite was mutation-checked: remove a guard, watch the specific test fail. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/traceparent-injection.md | 21 ++ .../TraceContextPropagationTests.cs | 297 ++++++++++++++++++ go/fetch/client.go | 55 ++++ go/fetch/go.mod | 18 +- go/fetch/go.sum | 29 +- go/fetch/trace_propagation_test.go | 190 +++++++++++ package.json | 10 + pnpm-lock.yaml | 91 ++++++ python/pyproject.toml | 7 +- python/src/smooai_fetch/_client.py | 51 ++- python/tests/test_trace_propagation.py | 144 +++++++++ src/fetch.no-otel.spec.ts | 44 +++ src/fetch.traceparent.spec.ts | 102 ++++++ src/fetch.ts | 64 ++++ 14 files changed, 1117 insertions(+), 6 deletions(-) create mode 100644 dotnet/SmooAI.Fetch.Tests/TraceContextPropagationTests.cs create mode 100644 go/fetch/trace_propagation_test.go create mode 100644 python/tests/test_trace_propagation.py create mode 100644 src/fetch.no-otel.spec.ts create mode 100644 src/fetch.traceparent.spec.ts diff --git a/.changeset/traceparent-injection.md b/.changeset/traceparent-injection.md index e34a165..11d44cc 100644 --- a/.changeset/traceparent-injection.md +++ b/.changeset/traceparent-injection.md @@ -7,3 +7,24 @@ every outbound request, so a call made through this client continues the caller' trace instead of starting a new root. Off by default — the crate does not link OpenTelemetry unless you ask for it. Guards an invalid span context and never overwrites a `traceparent` the caller set explicitly. + +TypeScript: the same injection at the same place — the single-request site, so every +retry carries a current traceparent — behind an optional `@opentelemetry/api` peer +dependency. Without it installed (or without a registered SDK) it is a no-op, not a +crash, and no all-zero `traceparent` is ever emitted. + +Python: the same injection at the same place — the single-request site, so retries carry +a current traceparent — behind an optional `smooai-fetch[otel]` extra. Without +`opentelemetry-api` installed it is a no-op, not an import error. + +Go: the same injection at the same place — `executeHTTPRequest`, the single-request +site inside the retry/timeout/breaker wrappers, so every attempt carries a current +traceparent. Uses the OTel global propagator, which defaults to a no-op, so a +service that never configured one sends nothing extra. Never overwrites a +caller-set `traceparent`, and emits nothing at all when there is no valid span +context. + +.NET: no change needed — `HttpClient`'s `DiagnosticsHandler` already injects +`traceparent` from the current `Activity`, ahead of redirects and connection +pooling, and this client keeps that handler chain intact. Tests were added to pin +that behaviour so a future custom primary handler cannot silently drop it. diff --git a/dotnet/SmooAI.Fetch.Tests/TraceContextPropagationTests.cs b/dotnet/SmooAI.Fetch.Tests/TraceContextPropagationTests.cs new file mode 100644 index 0000000..17577fe --- /dev/null +++ b/dotnet/SmooAI.Fetch.Tests/TraceContextPropagationTests.cs @@ -0,0 +1,297 @@ +using System.Diagnostics; +using System.Text.RegularExpressions; +using Microsoft.Extensions.DependencyInjection; +using SmooAI.Fetch; +using WireMock.RequestBuilders; +using WireMock.ResponseBuilders; +using WireMock.Server; + +namespace SmooAI.Fetch.Tests; + +/// +/// Trace-context propagation on egress, asserted at the WIRE — what the server +/// actually received, not what we believe we set. +/// +/// The gap these guard: api-prime EXTRACTS traceparent on ingress, but if +/// nothing INJECTS it on egress every service-to-service call begins a new root +/// trace. Measured 2026-08-14 over three hours: 34,961 traces touched one +/// service, 4 touched two. +/// +/// On .NET this is the framework's job, not ours. SocketsHttpHandler +/// installs DiagnosticsHandler as the OUTERMOST stage of its chain, and +/// that stage injects W3C headers via +/// whenever an is in play. These tests exist to prove that +/// holds through 's per-attempt request cloning, its retry +/// pipeline, and redirects — and to fail loudly if a future handler change +/// (a custom HttpMessageHandler, ActivityHeadersPropagator = null) +/// silently removes it. +/// +public class TraceContextPropagationTests : IAsyncLifetime +{ + // A valid traceparent: version 00, non-zero 16-byte trace id, non-zero 8-byte + // parent id. The negative lookaheads are the point — an all-zero id is the + // failure mode that poisons a downstream trace, so "well-formed" must exclude it. + private static readonly Regex WellFormed = new( + "^00-(?![0]{32})[0-9a-f]{32}-(?![0]{16})[0-9a-f]{16}-[0-9a-f]{2}$", + RegexOptions.Compiled); + + private WireMockServer _server = null!; + + public Task InitializeAsync() + { + _server = WireMockServer.Start(); + return Task.CompletedTask; + } + + public Task DisposeAsync() + { + _server.Stop(); + _server.Dispose(); + return Task.CompletedTask; + } + + private sealed record Thing(string Ok); + + private static void Ok(WireMockServer server, string path) => + server + .Given(Request.Create().WithPath(path).UsingGet()) + .RespondWith(Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody("{\"ok\":\"yes\"}")); + + private IReadOnlyList CapturedTraceparents() => + _server.LogEntries + .Select(e => e.RequestMessage.Headers is { } h && h.TryGetValue("traceparent", out var v) + ? v.FirstOrDefault() + : null) + .ToList(); + + private SmooFetch Fetch(Action? extra = null) => + SmooFetch.Create(opts => + { + opts.BaseUrl = _server.Urls[0]; + opts.RetryPolicy = RetryPolicy.None; + opts.Timeout = TimeSpan.FromSeconds(5); + extra?.Invoke(opts); + }); + + /// + /// Production shape: an OpenTelemetry-style scoped + /// to our own source. Scoping matters — a listener that matched every source + /// would also match "System.Net.Http", changing which code path + /// DiagnosticsHandler takes and leaking into sibling tests running in parallel. + /// + private static (ActivitySource Source, ActivityListener Listener) Tracing(string name) + { + var source = new ActivitySource(name); + var listener = new ActivityListener + { + ShouldListenTo = s => s.Name == name, + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.AllDataAndRecorded, + }; + ActivitySource.AddActivityListener(listener); + return (source, listener); + } + + [Fact] + public async Task Current_activity_is_propagated_as_a_traceparent_header() + { + Ok(_server, "/x"); + var (source, listener) = Tracing(nameof(Current_activity_is_propagated_as_a_traceparent_header)); + using (source) + using (listener) + { + using var caller = source.StartActivity("caller"); + Assert.NotNull(caller); + + await Fetch().GetAsync("/x"); + + var traceparent = Assert.Single(CapturedTraceparents()); + Assert.NotNull(traceparent); + Assert.Matches(WellFormed, traceparent); + // Same trace, and a CHILD span — the outbound call is its own span, so + // the parent-id must not be the caller's own span id. + Assert.StartsWith($"00-{caller.TraceId.ToHexString()}-", traceparent, StringComparison.Ordinal); + Assert.DoesNotContain(caller.SpanId.ToHexString(), traceparent, StringComparison.Ordinal); + } + } + + [Fact] + public async Task No_current_activity_means_no_traceparent_header() + { + Ok(_server, "/x"); + Activity.Current = null; + + await Fetch().GetAsync("/x"); + + // Never an all-zero "00-000…-000…-00": a downstream service either rejects + // that or, worse, adopts it and poisons its own trace. Absent is correct. + Assert.Null(Assert.Single(CapturedTraceparents())); + } + + [Fact] + public async Task Caller_supplied_traceparent_is_never_overwritten() + { + const string caller = "00-11111111111111111111111111111111-2222222222222222-01"; + Ok(_server, "/x"); + var (source, listener) = Tracing(nameof(Caller_supplied_traceparent_is_never_overwritten)); + using (source) + using (listener) + { + using var activity = source.StartActivity("caller"); + Assert.NotNull(activity); + + await Fetch(o => o.DefaultHeaders["traceparent"] = caller).GetAsync("/x"); + + // A client that silently rewrites an intentional header is worse than + // one that does nothing. + Assert.Equal(caller, Assert.Single(CapturedTraceparents())); + } + } + + [Fact] + public async Task Each_retry_attempt_carries_its_own_fresh_traceparent() + { + _server + .Given(Request.Create().WithPath("/flaky").UsingGet()) + .InScenario("flaky") + .WillSetStateTo("failed-once") + .RespondWith(Response.Create().WithStatusCode(500).WithBody("boom")); + _server + .Given(Request.Create().WithPath("/flaky").UsingGet()) + .InScenario("flaky") + .WhenStateIs("failed-once") + .RespondWith(Response.Create() + .WithStatusCode(200) + .WithHeader("Content-Type", "application/json") + .WithBody("{\"ok\":\"yes\"}")); + + var (source, listener) = Tracing(nameof(Each_retry_attempt_carries_its_own_fresh_traceparent)); + using (source) + using (listener) + { + using var caller = source.StartActivity("caller"); + Assert.NotNull(caller); + + await Fetch(o => o.RetryPolicy = new RetryPolicy + { + MaxRetries = 3, + BaseDelay = TimeSpan.FromMilliseconds(1), + MaxDelay = TimeSpan.FromMilliseconds(5), + UseJitter = false, + BackoffFactor = 1.0, + }).GetAsync("/flaky"); + + var captured = CapturedTraceparents(); + Assert.Equal(2, captured.Count); + Assert.All(captured, tp => + { + Assert.NotNull(tp); + Assert.Matches(WellFormed, tp); + Assert.StartsWith($"00-{caller.TraceId.ToHexString()}-", tp, StringComparison.Ordinal); + }); + + // The point of the test: attempt two is a NEW span, not a stale copy of + // attempt one. SmooFetch clones the pristine request per attempt, so the + // retry re-enters DiagnosticsHandler and gets a fresh parent-id. + Assert.NotEqual(captured[0], captured[1]); + } + } + + [Fact] + public async Task Redirect_hops_each_carry_a_traceparent() + { + _server + .Given(Request.Create().WithPath("/from").UsingGet()) + .RespondWith(Response.Create() + .WithStatusCode(302) + .WithHeader("Location", $"{_server.Urls[0]}/to")); + Ok(_server, "/to"); + + var (source, listener) = Tracing(nameof(Redirect_hops_each_carry_a_traceparent)); + using (source) + using (listener) + { + using var caller = source.StartActivity("caller"); + Assert.NotNull(caller); + + await Fetch().GetAsync("/from"); + + var captured = CapturedTraceparents(); + Assert.Equal(2, captured.Count); + Assert.All(captured, tp => + { + Assert.NotNull(tp); + Assert.Matches(WellFormed, tp); + Assert.StartsWith($"00-{caller.TraceId.ToHexString()}-", tp, StringComparison.Ordinal); + }); + } + } + + [Fact] + public async Task Sampled_out_activity_still_propagates_with_the_not_recorded_flag() + { + Ok(_server, "/x"); + var source = new ActivitySource(nameof(Sampled_out_activity_still_propagates_with_the_not_recorded_flag)); + var listener = new ActivityListener + { + ShouldListenTo = s => s.Name == source.Name, + // What a head-based sampler does to a span it drops. It still yields a + // real span context, so the trace stitches across the hop even though + // nothing is recorded — that is the whole point of the W3C flags byte. + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.PropagationData, + }; + ActivitySource.AddActivityListener(listener); + using (source) + using (listener) + { + using var caller = source.StartActivity("caller"); + Assert.NotNull(caller); + Assert.False(caller.Recorded); + + await Fetch().GetAsync("/x"); + + var traceparent = Assert.Single(CapturedTraceparents()); + Assert.NotNull(traceparent); + Assert.Matches(WellFormed, traceparent); + Assert.EndsWith("-00", traceparent, StringComparison.Ordinal); + Assert.StartsWith($"00-{caller.TraceId.ToHexString()}-", traceparent, StringComparison.Ordinal); + } + + // Note the sibling case: a sampler returning ActivitySamplingResult.None + // makes StartActivity return null, leaving Activity.Current null and no + // header on the wire — covered by No_current_activity_means_no_traceparent_header. + } + + [Fact] + public async Task HttpClientFactory_registered_client_also_propagates() + { + Ok(_server, "/x"); + var services = new ServiceCollection(); + services.AddSmooFetch(opts => + { + opts.BaseUrl = _server.Urls[0]; + opts.RetryPolicy = RetryPolicy.None; + }); + using var provider = services.BuildServiceProvider(); + + var (source, listener) = Tracing(nameof(HttpClientFactory_registered_client_also_propagates)); + using (source) + using (listener) + { + using var caller = source.StartActivity("caller"); + Assert.NotNull(caller); + + // The DI path builds its handler chain through IHttpClientFactory, which + // wraps the primary handler in logging handlers. Those sit OUTSIDE the + // primary handler, so DiagnosticsHandler is still in the chain. + await provider.GetRequiredService().GetAsync("/x"); + + var traceparent = Assert.Single(CapturedTraceparents()); + Assert.NotNull(traceparent); + Assert.Matches(WellFormed, traceparent); + Assert.StartsWith($"00-{caller.TraceId.ToHexString()}-", traceparent, StringComparison.Ordinal); + } + } +} diff --git a/go/fetch/client.go b/go/fetch/client.go index ad5084e..72be91c 100644 --- a/go/fetch/client.go +++ b/go/fetch/client.go @@ -9,6 +9,10 @@ import ( "net/http" "strings" "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" + "go.opentelemetry.io/otel/trace" ) // Client is a resilient HTTP client with built-in retry, timeout, @@ -132,6 +136,50 @@ func Fetch[T any](ctx context.Context, client *Client, method, url string, body return doRequest(ctx) } +// injectTraceContext writes W3C trace context (`traceparent`/`tracestate`) onto +// an outbound request so a trace survives the service hop. +// +// # Why this exists +// +// api-prime already EXTRACTS `traceparent` on ingress, but nothing on the +// platform ever injected it on egress — so every service-to-service call began a +// brand new root trace. Measured over a three-hour production window: 34,961 +// traces touched exactly one service, and 4 touched two. Distributed tracing did +// not work, and no amount of extra spans inside a service could fix it. +// +// The client is the correct place for propagation: services are already required +// to use @smooai/fetch over raw net/http, so wiring it once here covers the fleet. +// +// # Two guards, each for a reason +// +// 1. Caller wins. If the caller already set `traceparent` explicitly, theirs is +// left alone. A client that silently rewrites an intentional header is worse +// than one that does nothing. +// 2. Valid span contexts only. With no active span the context carries +// `trace.SpanContext{}` — all-zero ids. Injecting that writes a malformed +// `00-000…-00` traceparent that a downstream service will either reject or, +// worse, adopt — poisoning its trace. The sibling logger shipped exactly this +// bug, where all-zero ids overwrote a real correlation id. The stock W3C +// propagator happens to guard this too, so the check is defence in depth +// against a custom or composite propagator that does not. +// +// With no propagator configured the OTel global default is a no-op composite, so +// a service that has not set one up sends nothing extra either way. +func injectTraceContext(req *http.Request) { + // http.Header.Get canonicalises, so this catches whatever casing the caller + // used — the case-insensitive check the header map cannot do on its own. + if req.Header.Get("traceparent") != "" { + return + } + + ctx := req.Context() + if !trace.SpanContextFromContext(ctx).IsValid() { + return + } + + otel.GetTextMapPropagator().Inject(ctx, propagation.HeaderCarrier(req.Header)) +} + // executeHTTPRequest performs the actual HTTP request and parses the response. func executeHTTPRequest[T any]( ctx context.Context, @@ -188,6 +236,13 @@ func executeHTTPRequest[T any]( } } + // Continue the caller's trace across the hop. Applied AFTER the caller's own + // headers and after the pre-request hook (which may replace the whole + // request) so an explicitly-set `traceparent` still wins. This is the + // single-request site, inside the retry/timeout/breaker wrappers, so every + // attempt carries a CURRENT traceparent rather than a stale one. + injectTraceContext(req) + // Apply auth-token provider (after the pre-request hook so the hook can // adjust the URL/init first; the resulting Authorization header overrides // any value the hook may have set). diff --git a/go/fetch/go.mod b/go/fetch/go.mod index 91de04d..7819ff8 100644 --- a/go/fetch/go.mod +++ b/go/fetch/go.mod @@ -1,5 +1,19 @@ module github.com/SmooAI/fetch/go/fetch -go 1.23 +go 1.23.0 -require github.com/sony/gobreaker/v2 v2.0.0 +require ( + github.com/sony/gobreaker/v2 v2.0.0 + go.opentelemetry.io/otel v1.38.0 + go.opentelemetry.io/otel/sdk v1.38.0 + go.opentelemetry.io/otel/trace v1.38.0 +) + +require ( + github.com/go-logr/logr v1.4.3 // indirect + github.com/go-logr/stdr v1.2.2 // indirect + github.com/google/uuid v1.6.0 // indirect + go.opentelemetry.io/auto/sdk v1.1.0 // indirect + go.opentelemetry.io/otel/metric v1.38.0 // indirect + golang.org/x/sys v0.35.0 // indirect +) diff --git a/go/fetch/go.sum b/go/fetch/go.sum index f3e32e6..4ced8e6 100644 --- a/go/fetch/go.sum +++ b/go/fetch/go.sum @@ -1,10 +1,35 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= +github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI= +github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= +github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= +github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= +github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= +github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= +github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/sony/gobreaker/v2 v2.0.0 h1:23AaR4JQ65y4rz8JWMzgXw2gKOykZ/qfqYunll4OwJ4= github.com/sony/gobreaker/v2 v2.0.0/go.mod h1:8JnRUz80DJ1/ne8M8v7nmTs2713i58nIt4s7XcGe/DI= -github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= -github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +go.opentelemetry.io/auto/sdk v1.1.0 h1:cH53jehLUN6UFLY71z+NDOiNJqDdPRaXzTel0sJySYA= +go.opentelemetry.io/auto/sdk v1.1.0/go.mod h1:3wSPjt5PWp2RhlCcmmOial7AvC4DQqZb7a7wCow3W8A= +go.opentelemetry.io/otel v1.38.0 h1:RkfdswUDRimDg0m2Az18RKOsnI8UDzppJAtj01/Ymk8= +go.opentelemetry.io/otel v1.38.0/go.mod h1:zcmtmQ1+YmQM9wrNsTGV/q/uyusom3P8RxwExxkZhjM= +go.opentelemetry.io/otel/metric v1.38.0 h1:Kl6lzIYGAh5M159u9NgiRkmoMKjvbsKtYRwgfrA6WpA= +go.opentelemetry.io/otel/metric v1.38.0/go.mod h1:kB5n/QoRM8YwmUahxvI3bO34eVtQf2i4utNVLr9gEmI= +go.opentelemetry.io/otel/sdk v1.38.0 h1:l48sr5YbNf2hpCUj/FoGhW9yDkl+Ma+LrVl8qaM5b+E= +go.opentelemetry.io/otel/sdk v1.38.0/go.mod h1:ghmNdGlVemJI3+ZB5iDEuk4bWA3GkTpW+DOoZMYBVVg= +go.opentelemetry.io/otel/sdk/metric v1.38.0 h1:aSH66iL0aZqo//xXzQLYozmWrXxyFkBJ6qT5wthqPoM= +go.opentelemetry.io/otel/sdk/metric v1.38.0/go.mod h1:dg9PBnW9XdQ1Hd6ZnRz689CbtrUp0wMMs9iPcgT9EZA= +go.opentelemetry.io/otel/trace v1.38.0 h1:Fxk5bKrDZJUH+AMyyIXGcFAPah0oRcT+LuNtJrmcNLE= +go.opentelemetry.io/otel/trace v1.38.0/go.mod h1:j1P9ivuFsTceSWe1oY+EeW3sc+Pp42sO++GHkg4wwhs= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= +golang.org/x/sys v0.35.0 h1:vz1N37gP5bs89s7He8XuIYXpyY0+QlsKmzipCbUtyxI= +golang.org/x/sys v0.35.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/go/fetch/trace_propagation_test.go b/go/fetch/trace_propagation_test.go new file mode 100644 index 0000000..7b7cbdc --- /dev/null +++ b/go/fetch/trace_propagation_test.go @@ -0,0 +1,190 @@ +package fetch + +import ( + "context" + "net/http" + "net/http/httptest" + "regexp" + "sync" + "testing" + "time" + + "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/propagation" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" +) + +// Trace-context propagation on egress. +// +// The gap these guard: api-prime EXTRACTS `traceparent` on ingress, but nothing +// ever INJECTED it, so every service-to-service call began a new root trace. +// Measured over three hours of production traffic: 34,961 traces touched one +// service, 4 touched two. +// +// Asserted at the WIRE — what the server actually received — rather than by +// inspecting our own *http.Request. A header we believe we set and the server +// never sees is the exact failure being fixed. + +// w3cTraceparent is the shape a downstream service will accept: +// version-traceid(32 hex)-spanid(16 hex)-flags. +var w3cTraceparent = regexp.MustCompile(`^00-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$`) + +// captureServer records the headers of the first request it receives. +func captureServer(t *testing.T) (*httptest.Server, *http.Header) { + t.Helper() + var mu sync.Mutex + var got http.Header + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + if got == nil { + got = r.Header.Clone() + } + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(server.Close) + return server, &got +} + +// withPropagator installs a global propagator for one test and restores the +// previous one, since the global is process-wide and tests share a process. +func withPropagator(t *testing.T, p propagation.TextMapPropagator) { + t.Helper() + previous := otel.GetTextMapPropagator() + otel.SetTextMapPropagator(p) + t.Cleanup(func() { otel.SetTextMapPropagator(previous) }) +} + +// startSpan starts a real SDK-backed span, the production shape — a context +// hand-built with trace.ContextWithSpanContext would exercise the propagator +// without proving a tracer-created span actually reaches the wire. +func startSpan(t *testing.T, ctx context.Context) (context.Context, trace.Span) { + t.Helper() + provider := sdktrace.NewTracerProvider() + t.Cleanup(func() { _ = provider.Shutdown(context.Background()) }) + ctx, span := provider.Tracer("fetch-propagation-test").Start(ctx, "caller") + t.Cleanup(func() { span.End() }) + return ctx, span +} + +func TestActiveSpanIsInjectedAsTraceparent(t *testing.T) { + withPropagator(t, propagation.TraceContext{}) + server, received := captureServer(t) + + ctx, span := startSpan(t, context.Background()) + wantTraceID := span.SpanContext().TraceID().String() + + if _, err := SimpleGet(ctx, NewClient(), server.URL, nil); err != nil { + t.Fatalf("request failed: %v", err) + } + + traceparent := received.Get("traceparent") + if traceparent == "" { + t.Fatal("the server received no traceparent header") + } + if !w3cTraceparent.MatchString(traceparent) { + t.Errorf("traceparent %q is not a well-formed W3C header", traceparent) + } + if !regexp.MustCompile(wantTraceID).MatchString(traceparent) { + t.Errorf("traceparent %q must carry the caller's trace id %s", traceparent, wantTraceID) + } +} + +func TestNoActiveSpanMeansNoTraceparent(t *testing.T) { + withPropagator(t, propagation.TraceContext{}) + server, received := captureServer(t) + + // No span on the context: the span context is all-zero ids. Injecting that + // writes a malformed `00-000…-00` traceparent the downstream service may + // reject, or worse adopt, poisoning its trace. + if _, err := SimpleGet(context.Background(), NewClient(), server.URL, nil); err != nil { + t.Fatalf("request failed: %v", err) + } + + if traceparent := received.Get("traceparent"); traceparent != "" { + t.Errorf("expected no traceparent header, server received %q", traceparent) + } +} + +func TestCallerSuppliedTraceparentIsNeverOverwritten(t *testing.T) { + withPropagator(t, propagation.TraceContext{}) + server, received := captureServer(t) + + const caller = "00-11111111111111111111111111111111-2222222222222222-01" + headers := http.Header{} + // Non-canonical casing on purpose: a caller who lowercases the key must + // still win. + headers["traceparent"] = []string{caller} + + ctx, _ := startSpan(t, context.Background()) + if _, err := SimpleGet(ctx, NewClient(), server.URL, &RequestOptions{Headers: headers}); err != nil { + t.Fatalf("request failed: %v", err) + } + + // A client that silently rewrites an intentional header is worse than one + // that does nothing. + if got := (*received)["Traceparent"]; len(got) != 1 || got[0] != caller { + t.Errorf("caller traceparent not preserved: got %v, want [%s]", got, caller) + } +} + +func TestDefaultPropagatorInjectsNothing(t *testing.T) { + // The OTel global default is a no-op composite. Verified rather than + // assumed: a service that never configured a propagator must not start + // emitting headers just because it linked this client. + withPropagator(t, otel.GetTextMapPropagator()) + otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator()) + server, received := captureServer(t) + + ctx, _ := startSpan(t, context.Background()) + if _, err := SimpleGet(ctx, NewClient(), server.URL, nil); err != nil { + t.Fatalf("request failed: %v", err) + } + + if traceparent := received.Get("traceparent"); traceparent != "" { + t.Errorf("default propagator should inject nothing, server received %q", traceparent) + } +} + +func TestEveryRetryAttemptCarriesATraceparent(t *testing.T) { + withPropagator(t, propagation.TraceContext{}) + + var mu sync.Mutex + var seen []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + seen = append(seen, r.Header.Get("traceparent")) + attempts := len(seen) + mu.Unlock() + if attempts < 3 { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + ctx, _ := startSpan(t, context.Background()) + // Attempts is the retry count on top of the initial call: 2 => 3 requests. + opts := &RequestOptions{Retry: &RetryOptions{Attempts: 2, InitialInterval: time.Millisecond, Factor: 1.0}} + if _, err := SimpleGet(ctx, NewClient(), server.URL, opts); err != nil { + t.Fatalf("request failed: %v", err) + } + + mu.Lock() + defer mu.Unlock() + if len(seen) != 3 { + t.Fatalf("expected 3 attempts, got %d", len(seen)) + } + // Injection lives at the single-request site, so a retried attempt builds a + // fresh header instead of replaying a stale one. + for i, traceparent := range seen { + if !w3cTraceparent.MatchString(traceparent) { + t.Errorf("attempt %d sent %q, not a well-formed traceparent", i+1, traceparent) + } + } +} diff --git a/package.json b/package.json index 8a3d973..e8de3c2 100644 --- a/package.json +++ b/package.json @@ -131,6 +131,8 @@ }, "devDependencies": { "@changesets/cli": "^2.28.1", + "@opentelemetry/api": "^1.9.1", + "@opentelemetry/sdk-trace-node": "^2.10.0", "@rollup/plugin-alias": "latest", "@smooai/config-typescript": "^1.0.16", "@types/lodash.merge": "^4.6.9", @@ -148,5 +150,13 @@ "vitest": "^3.1.1", "zod": "^4.1.5" }, + "peerDependencies": { + "@opentelemetry/api": "^1.9.0" + }, + "peerDependenciesMeta": { + "@opentelemetry/api": { + "optional": true + } + }, "packageManager": "pnpm@10.6.1+sha512.40ee09af407fa9fbb5fbfb8e1cb40fbb74c0af0c3e10e9224d7b53c7658528615b2c92450e74cfad91e3a2dcafe3ce4050d80bda71d757756d2ce2b66213e9a3" } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c7feb9f..35a54d6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -34,6 +34,12 @@ importers: '@changesets/cli': specifier: ^2.28.1 version: 2.28.1 + '@opentelemetry/api': + specifier: ^1.9.1 + version: 1.9.1 + '@opentelemetry/sdk-trace-node': + specifier: ^2.10.0 + version: 2.10.0(@opentelemetry/api@1.9.1) '@rollup/plugin-alias': specifier: latest version: 6.0.0(rollup@4.38.0) @@ -566,6 +572,50 @@ packages: '@open-draft/until@2.1.0': resolution: {integrity: sha512-U69T3ItWHvLwGg5eJ0n3I62nWuE6ilHlmz7zM0npLBRvPRd7e6NYmg54vvRtP5mZG7kZqZCFVdsTWo7BPtBujg==} + '@opentelemetry/api@1.9.1': + resolution: {integrity: sha512-gLyJlPHPZYdAk1JENA9LeHejZe1Ti77/pTeFm/nMXmQH/HFZlcS/O2XJB+L8fkbrNSqhdtlvjBVjxwUYanNH5Q==} + engines: {node: '>=8.0.0'} + + '@opentelemetry/context-async-hooks@2.10.0': + resolution: {integrity: sha512-bvyMcgLEkozzSzpEEEo1OMoeQ97bxj6Qs2uN3mPrSdDvObMI1myffD/BPqcLlzZO9//d1SqQA/WPw7Cz2AiqhA==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.0.0 <1.10.0' + + '@opentelemetry/core@2.10.0': + resolution: {integrity: sha512-/wNZ8twnEQQA4HoHu22+vcsdru6pWPWxW+7w+FlxT6Id7PE/WIbZmVKkte+PF72e0F2dnImFeHD2syyE1Mw6MQ==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.0.0 <1.10.0' + + '@opentelemetry/resources@2.10.0': + resolution: {integrity: sha512-q6MMm2zhggzsHVNbabYwut+a6nbuQQe3URUoxaojM/8K1IBfwwPzvxIjNi2/lI1TFe+fMHMW9MWhrtDLEXEnkA==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.3.0 <1.10.0' + + '@opentelemetry/sdk-trace-base@2.10.0': + resolution: {integrity: sha512-GuYQQT7QD2EeO8lcZLRQzcbOyhqAzL+6WWTKTU9mSUBYBazkEDl+VrQcXQhbB08OWM9anD1aHleVadzulpOaUQ==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.3.0 <1.10.0' + + '@opentelemetry/sdk-trace-node@2.10.0': + resolution: {integrity: sha512-GZK/G6oZyBLGlH1pUgeDch7D91KoHd2uotUGIkWCPi9GI5T9X0p4L7nNAMDR1BQjkRYoDqo+ddfVx9t5Uhys+Q==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.0.0 <1.10.0' + + '@opentelemetry/sdk-trace@2.10.0': + resolution: {integrity: sha512-MfQGq3GRmTh5fM/y+OjaO0vj6+luCB1XO2gfXCalKCfgKw0eHL++sm75DNweC6ohlp+aFvACqeE0fYayqdRaoQ==} + engines: {node: ^18.19.0 || >=20.6.0} + peerDependencies: + '@opentelemetry/api': '>=1.3.0 <1.10.0' + + '@opentelemetry/semantic-conventions@1.43.0': + resolution: {integrity: sha512-eSYWTm620tTk45EKSedaUL8MFYI8hW164hIXsgIHyxu3VobUB3fFCu5t0hQby6OoWRPsG1KkKUG2M5UadiLiVg==} + engines: {node: '>=14'} + '@oxc-project/types@0.130.0': resolution: {integrity: sha512-ibD2usx9JRu7f5pu2tMKMI4cpA4NgXJQoYRP4pQ7Pxmn1l6k/53qWtQWZayhYy3X4QZkt90Ot+mJEaeXouio6Q==} @@ -3302,6 +3352,47 @@ snapshots: '@open-draft/until@2.1.0': {} + '@opentelemetry/api@1.9.1': {} + + '@opentelemetry/context-async-hooks@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + + '@opentelemetry/core@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + '@opentelemetry/semantic-conventions': 1.43.0 + + '@opentelemetry/resources@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + '@opentelemetry/core': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/semantic-conventions': 1.43.0 + + '@opentelemetry/sdk-trace-base@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + '@opentelemetry/core': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/resources': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/sdk-trace': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/semantic-conventions': 1.43.0 + + '@opentelemetry/sdk-trace-node@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + '@opentelemetry/context-async-hooks': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/core': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/sdk-trace-base': 2.10.0(@opentelemetry/api@1.9.1) + + '@opentelemetry/sdk-trace@2.10.0(@opentelemetry/api@1.9.1)': + dependencies: + '@opentelemetry/api': 1.9.1 + '@opentelemetry/core': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/resources': 2.10.0(@opentelemetry/api@1.9.1) + '@opentelemetry/semantic-conventions': 1.43.0 + + '@opentelemetry/semantic-conventions@1.43.0': {} + '@oxc-project/types@0.130.0': {} '@oxfmt/darwin-arm64@0.28.0': diff --git a/python/pyproject.toml b/python/pyproject.toml index 65c5358..c8f8b6d 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -16,6 +16,11 @@ classifiers = [ ] keywords = ["fetch", "http", "retry", "circuit-breaker", "smooai"] +# Trace-context propagation on egress is opt-in: an HTTP client must not force +# OpenTelemetry on every consumer. Install as `smooai-fetch[otel]`. +[project.optional-dependencies] +otel = ["opentelemetry-api>=1.20.0"] + [project.urls] Homepage = "https://github.com/SmooAI/fetch" Repository = "https://github.com/SmooAI/fetch" @@ -29,7 +34,7 @@ build-backend = "hatchling.build" packages = ["src/smooai_fetch"] [dependency-groups] -dev = ["pytest>=8.0.0", "pytest-asyncio>=0.24.0", "respx>=0.22.0", "ruff>=0.11.0", "basedpyright>=1.0.0", "poethepoet>=0.29.0"] +dev = ["pytest>=8.0.0", "pytest-asyncio>=0.24.0", "respx>=0.22.0", "ruff>=0.11.0", "basedpyright>=1.0.0", "poethepoet>=0.29.0", "opentelemetry-api>=1.20.0"] [tool.poe.tasks] lint = "ruff check src/ tests/" diff --git a/python/src/smooai_fetch/_client.py b/python/src/smooai_fetch/_client.py index 34f2355..ba6622f 100644 --- a/python/src/smooai_fetch/_client.py +++ b/python/src/smooai_fetch/_client.py @@ -140,6 +140,48 @@ def _parse_response(response: httpx.Response, schema: type[BaseModel] | None = N raise HTTPResponseError(response) +def _inject_trace_context(headers: dict[str, str]) -> dict[str, str]: + """Return ``headers`` plus W3C trace context (``traceparent``/``tracestate``). + + Why this exists: api-prime already EXTRACTS ``traceparent`` on ingress, but + nothing on the platform ever injected it on egress, so every service-to-service + call began a brand new root trace. Measured 2026-08-14 over three hours: 34,961 + traces touched exactly one service, 4 touched two. The client is the right place + to fix that once for the fleet. + + Three guards, mirroring the Rust port: + + 1. **Optional dependency.** ``opentelemetry-api`` is an extra (``smooai-fetch[otel]``). + Without it this is a no-op — an OSS HTTP client must not force OTel on anyone. + 2. **Valid span contexts only.** With no active span the current span context is + ``INVALID_SPAN_CONTEXT`` (all-zero ids). Injecting that writes a malformed + ``traceparent`` a downstream service will reject or, worse, adopt — poisoning + its trace. ``inject()`` happens to skip an invalid context today; the explicit + check means we do not depend on that staying true. + 3. **Caller wins.** An explicitly-set ``traceparent`` is left alone. A client that + silently rewrites an intentional header is worse than one that does nothing. + + Returns a NEW dict rather than mutating: the caller's ``request_kwargs`` are shared + across retries, and a latched header would make guard 3 preserve a stale traceparent + on every attempt after the first. + """ + if any(key.lower() == "traceparent" for key in headers): + return headers + + try: + from opentelemetry.propagate import inject + from opentelemetry.trace import get_current_span + except ImportError: + return headers + + if not get_current_span().get_span_context().is_valid: + return headers + + carrier = dict(headers) + inject(carrier) + return carrier + + def _get_retry_after(error: Exception) -> float | None: """Extract Retry-After header value from an HTTPResponseError. @@ -282,9 +324,16 @@ async def _execute() -> FetchResponse[Any]: """Inner execution: rate limit -> circuit breaker -> HTTP call -> parse.""" async def _do_request() -> FetchResponse[Any]: + # Continue the caller's trace across the hop. Injected HERE, at the + # single-request site, not at fetch() entry — so every retry attempt + # carries a CURRENT traceparent, not one baked when fetch() was called. + attempt_kwargs = { + **request_kwargs, + "headers": _inject_trace_context(request_kwargs.get("headers") or {}), + } try: async with httpx.AsyncClient() as client: - response = await client.request(**request_kwargs) + response = await client.request(**attempt_kwargs) return _parse_response(response, schema) except httpx.TimeoutException as e: raise FetchTimeoutError( diff --git a/python/tests/test_trace_propagation.py b/python/tests/test_trace_propagation.py new file mode 100644 index 0000000..e047905 --- /dev/null +++ b/python/tests/test_trace_propagation.py @@ -0,0 +1,144 @@ +"""Trace-context propagation on egress. + +The gap these guard: api-prime EXTRACTS ``traceparent`` on ingress, but nothing ever +INJECTED it, so every service-to-service call began a new root trace. Measured +2026-08-14 over three hours: 34,961 traces touched one service, 4 touched two. + +Asserted at the WIRE — what a real local HTTP server actually received — rather than +against a mock transport. A header we believe we set and the server never sees is the +exact failure being fixed, and respx would happily agree with our own bookkeeping. +""" + +import sys +import threading +from collections.abc import Iterator +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer + +import pytest +from opentelemetry import context as otel_context +from opentelemetry import trace + +from smooai_fetch import FetchOptions, fetch + +TRACE_ID = 0x4BF92F3577B34DA6A3CE929D0E0E4736 +SPAN_ID = 0x00F067AA0BA902B7 + + +@pytest.fixture +def wire() -> Iterator[tuple[str, list[dict[str, str]]]]: + """A real local HTTP server; yields (url, headers-received-per-request).""" + received: list[dict[str, str]] = [] + + class Handler(BaseHTTPRequestHandler): + protocol_version = "HTTP/1.1" + + def do_GET(self) -> None: # noqa: N802 - BaseHTTPRequestHandler's contract + received.append({k.lower(): v for k, v in self.headers.items()}) + body = b"{}" + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + _ = self.wfile.write(body) + + def log_message(self, format: str, *args: object) -> None: + pass # keep pytest output clean + + server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + try: + yield f"http://127.0.0.1:{server.server_port}/x", received + finally: + server.shutdown() + server.server_close() + thread.join(timeout=5) + + +@pytest.fixture +def active_span() -> Iterator[None]: + """Activate a valid (non-recording) span context — no SDK required.""" + span = trace.NonRecordingSpan( + trace.SpanContext( + trace_id=TRACE_ID, + span_id=SPAN_ID, + is_remote=False, + trace_flags=trace.TraceFlags(trace.TraceFlags.SAMPLED), + ) + ) + token = otel_context.attach(trace.set_span_in_context(span)) + try: + yield + finally: + otel_context.detach(token) + + +async def test_active_span_is_propagated_as_traceparent_header( + wire: tuple[str, list[dict[str, str]]], active_span: None +) -> None: + """An active span produces a well-formed traceparent carrying its trace id.""" + url, received = wire + + response = await fetch(url) + + assert response.ok + assert len(received) == 1 + traceparent = received[0].get("traceparent") + assert traceparent is not None, "the server received no traceparent header" + + version, trace_id, span_id, flags = traceparent.split("-") + assert version == "00" + assert trace_id == format(TRACE_ID, "032x") + assert span_id == format(SPAN_ID, "016x") + assert flags == "01" + + +async def test_no_active_span_means_no_traceparent_header(wire: tuple[str, list[dict[str, str]]]) -> None: + """No valid span context injects NOTHING — never an all-zero traceparent. + + An absent span yields INVALID_SPAN_CONTEXT (all-zero ids). Injecting that writes a + malformed traceparent the downstream service may reject, or worse adopt, poisoning + its trace. The sibling logger shipped exactly this bug. + """ + url, received = wire + + response = await fetch(url) + + assert response.ok + assert len(received) == 1 + assert "traceparent" not in received[0] + + +async def test_missing_opentelemetry_is_a_no_op_not_a_crash( + wire: tuple[str, list[dict[str, str]]], active_span: None, monkeypatch: pytest.MonkeyPatch +) -> None: + """``opentelemetry-api`` is an extra: absent, the client still works and injects nothing. + + A ``None`` entry in ``sys.modules`` makes ``import`` raise ImportError, which is what a + consumer who installed plain ``smooai-fetch`` sees. An active span is deliberately in + scope so the only reason nothing is injected is the missing dependency. + """ + url, received = wire + for name in list(sys.modules): + if name == "opentelemetry" or name.startswith("opentelemetry."): + monkeypatch.setitem(sys.modules, name, None) + + response = await fetch(url) + + assert response.ok + assert len(received) == 1 + assert "traceparent" not in received[0] + + +async def test_caller_traceparent_is_never_overwritten( + wire: tuple[str, list[dict[str, str]]], active_span: None +) -> None: + """An explicitly-set traceparent survives untouched, active span or not.""" + url, received = wire + caller = "00-11111111111111111111111111111111-2222222222222222-01" + + response = await fetch(url, FetchOptions(headers={"traceparent": caller})) + + assert response.ok + assert len(received) == 1 + assert received[0].get("traceparent") == caller diff --git a/src/fetch.no-otel.spec.ts b/src/fetch.no-otel.spec.ts new file mode 100644 index 0000000..bd6fba7 --- /dev/null +++ b/src/fetch.no-otel.spec.ts @@ -0,0 +1,44 @@ +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { afterAll, beforeAll, expect, it, vi } from 'vitest'; + +/** + * `@opentelemetry/api` is an OPTIONAL peer dependency. Simulate it being absent — + * the import rejects exactly as it does when the package is not installed — and + * assert the client still works: no injection, no crash. + */ +vi.mock('@opentelemetry/api', () => { + throw new Error("Cannot find module '@opentelemetry/api'"); +}); + +const fetch = (await import('./fetch')).default; + +let server: http.Server; +let baseUrl: string; +const received: http.IncomingHttpHeaders[] = []; + +beforeAll(async () => { + server = http.createServer((req, res) => { + received.push(req.headers); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end('{"ok":true}'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +it('still fetches, without a traceparent, when @opentelemetry/api is not installed', async () => { + // Prove the simulated absence is actually in effect for this module registry — + // otherwise this test would pass for the wrong reason (no active span). + await expect(import('@opentelemetry/api')).rejects.toThrow(); + + const response = await fetch(`${baseUrl}/x`); + + expect(response.ok).toBe(true); + expect(received).toHaveLength(1); + expect(received[0].traceparent).toBeUndefined(); +}); diff --git a/src/fetch.traceparent.spec.ts b/src/fetch.traceparent.spec.ts new file mode 100644 index 0000000..bb5f5e9 --- /dev/null +++ b/src/fetch.traceparent.spec.ts @@ -0,0 +1,102 @@ +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { trace } from '@opentelemetry/api'; +import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import fetch from './fetch'; + +/** + * Trace-context propagation on egress. + * + * The gap these guard: api-prime EXTRACTS `traceparent` on ingress, but nothing + * ever INJECTED it, so every service-to-service call began a new root trace. + * Measured 2026-08-14 over three hours: 34,961 traces touched one service, 4 + * touched two. + * + * Asserted at the WIRE — against a real HTTP server, on the headers it actually + * received — rather than against a mock. A header we believe we set and the + * server never sees is the exact failure being fixed. + */ + +// `register()` installs the W3C propagator and the AsyncLocalStorage context +// manager, i.e. the shape a real service runs in. +new NodeTracerProvider().register(); + +const TRACEPARENT = /^00-[0-9a-f]{32}-[0-9a-f]{16}-0[01]$/; + +let server: http.Server; +let baseUrl: string; +let received: http.IncomingHttpHeaders[] = []; +let failNext = 0; + +beforeAll(async () => { + server = http.createServer((req, res) => { + received.push(req.headers); + const status = failNext-- > 0 ? 503 : 200; + res.writeHead(status, { 'Content-Type': 'application/json' }); + res.end('{"ok":true}'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +beforeEach(() => { + received = []; + failNext = 0; +}); + +describe('traceparent injection on egress', () => { + it('sends a traceparent carrying the active trace id', async () => { + const tracer = trace.getTracer('fetch-propagation-test'); + const traceId = await tracer.startActiveSpan('caller', async (span) => { + await fetch(`${baseUrl}/x`); + span.end(); + return span.spanContext().traceId; + }); + + expect(received).toHaveLength(1); + expect(received[0].traceparent).toMatch(TRACEPARENT); + expect(received[0].traceparent).toContain(traceId); + }); + + it('sends no traceparent when there is no active span', async () => { + await fetch(`${baseUrl}/x`); + + // No active span yields INVALID_SPAN_CONTEXT — all-zero ids. Injecting that + // writes a malformed traceparent the downstream service may reject, or worse + // adopt, poisoning its trace. + expect(received).toHaveLength(1); + expect(received[0].traceparent).toBeUndefined(); + }); + + it('never overwrites a caller-supplied traceparent', async () => { + const caller = '00-11111111111111111111111111111111-2222222222222222-01'; + const tracer = trace.getTracer('fetch-propagation-test'); + await tracer.startActiveSpan('caller', async (span) => { + await fetch(`${baseUrl}/x`, { headers: { traceparent: caller } }); + span.end(); + }); + + expect(received[0].traceparent).toBe(caller); + }); + + it('injects on every attempt, so a retry carries a current traceparent', async () => { + failNext = 1; + const tracer = trace.getTracer('fetch-propagation-test'); + await tracer.startActiveSpan('caller', async (span) => { + await fetch(`${baseUrl}/x`); + span.end(); + }); + + // Injecting at the top-level entry instead of the single-request site, or + // mutating the shared init, would leave the retry without a fresh header. + expect(received.length).toBeGreaterThan(1); + for (const headers of received) { + expect(headers.traceparent).toMatch(TRACEPARENT); + } + }); +}); diff --git a/src/fetch.ts b/src/fetch.ts index a87c0b9..4a7be22 100644 --- a/src/fetch.ts +++ b/src/fetch.ts @@ -494,6 +494,65 @@ function prepareFetchContainerModules(options: RequestOptions, containerOptions? return modules; } +/** + * Cached, best-effort handle on `@opentelemetry/api`. + * + * The package is an OPTIONAL peer dependency: an OSS HTTP client must not force + * OpenTelemetry on anyone. When it is absent the import rejects, this resolves + * `null`, and injection becomes a no-op — no crash, no behaviour change. + */ +let otelApi: Promise | undefined; + +/** + * Inject W3C trace context (`traceparent`/`tracestate`) into an outbound request. + * + * # Why this exists + * + * api-prime already EXTRACTS `traceparent` on ingress, but nothing on the + * platform ever injected it on egress — so every service-to-service call began a + * brand new root trace. Measured on 2026-08-14 over a three-hour window: 34,961 + * traces touched exactly one service, and 4 touched two. + * + * This is the client, which is the correct place for propagation: services are + * already required to use `@smooai/fetch` over raw HTTP, so wiring it once here + * covers the fleet. Mirrors `rust/fetch/src/client.rs::inject_trace_context`. + * + * # Three guards, each for a reason + * + * 1. **Optional dependency.** No `@opentelemetry/api` installed → no-op. + * 2. **Valid span contexts only.** No registered SDK / no active span yields + * INVALID_SPAN_CONTEXT (all-zero ids). Injecting that writes a malformed + * `traceparent` a downstream service will either reject or, worse, adopt — + * poisoning its trace. The sibling logger shipped exactly this bug, where + * all-zero ids overwrote a real correlation id. + * 3. **Caller wins.** An explicitly-set `traceparent` is left alone. A client + * that silently rewrites an intentional header is worse than one that does + * nothing. + */ +async function injectTraceContext(init: RequestInit): Promise { + otelApi ??= import('@opentelemetry/api').catch(() => null); + const otel = await otelApi; + if (!otel) return; + + const activeContext = otel.context.active(); + const spanContext = otel.trace.getSpanContext(activeContext); + if (!spanContext || !otel.isSpanContextValid(spanContext)) return; + + // Copy rather than mutate: the caller's `init` is reused across retries, and a + // header we wrote ourselves must never be mistaken for the caller's on the next + // attempt — that would pin every retry to the traceparent of the first one. + const headers = new Headers(init.headers as HeadersInit | undefined); + if (headers.has('traceparent')) return; + + const carrier: Record = {}; + otel.propagation.inject(activeContext, carrier); + if (Object.keys(carrier).length === 0) return; + for (const [key, value] of Object.entries(carrier)) { + headers.set(key, value); + } + init.headers = headers; +} + async function doGlobalFetch( url: RequestInfo, init?: RequestInit, @@ -506,6 +565,11 @@ async function doGlobalFetch( useInit.body = JSON.stringify(useInit.body); } + // Continue the caller's trace across the hop. This is the single-request site — + // the innermost place that performs exactly one HTTP call — so every retry and + // every re-issued request carries a CURRENT traceparent instead of a stale one. + await injectTraceContext(useInit); + const response = await globalFetch()(url, useInit); let isJson = false; let data: ResponseType | undefined;