From 2d6be3cdc00b1e90590b3cf92ee5e3ab240fb5a5 Mon Sep 17 00:00:00 2001 From: Chris Moore Date: Wed, 19 Aug 2026 12:46:26 -0400 Subject: [PATCH 1/2] feat(cli): forward TRACEPARENT/TRACESTATE to gateway for CI trace continuity The CLI's gateway gRPC channel now passively forwards the W3C trace context (TRACEPARENT/TRACESTATE) from its environment, so a CI pipeline's trace parents the gateway's spans. This needs no OTel collector or provider on the CLI side - short-lived clients on CI runners and jump hosts just pass through what the CI system already exports. Add EnvTraceContextInterceptor to openshell-otel: it reads the trace context once at construction and injects it as gRPC metadata, and is a no-op when the context is unset or invalid. Compose it with EdgeAuthInterceptor via a new GatewayInterceptor, since tonic allows only one interceptor per channel. Closes issue #2639 Signed-off-by: Chris Moore --- Cargo.lock | 1 + crates/openshell-cli/Cargo.toml | 1 + crates/openshell-cli/src/tls.rs | 40 +++++- crates/openshell-otel/src/lib.rs | 6 +- crates/openshell-otel/src/propagation.rs | 154 +++++++++++++++++++++++ docs/observability/traces.mdx | 2 + 6 files changed, 195 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e152fdc96d..5aaa13f4cd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4266,6 +4266,7 @@ dependencies = [ "oauth2", "openshell-bootstrap", "openshell-core", + "openshell-otel", "openshell-policy", "openshell-providers", "openshell-sdk", diff --git a/crates/openshell-cli/Cargo.toml b/crates/openshell-cli/Cargo.toml index 5c92eefe5b..198304d712 100644 --- a/crates/openshell-cli/Cargo.toml +++ b/crates/openshell-cli/Cargo.toml @@ -17,6 +17,7 @@ path = "src/main.rs" [dependencies] openshell-bootstrap = { path = "../openshell-bootstrap" } openshell-core = { path = "../openshell-core", default-features = false } +openshell-otel = { path = "../openshell-otel" } openshell-policy = { path = "../openshell-policy" } openshell-providers = { path = "../openshell-providers" } openshell-sdk = { path = "../openshell-sdk" } diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index 5b379cbee9..00044d00d9 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -5,6 +5,7 @@ use miette::{IntoDiagnostic, Result, WrapErr}; use openshell_core::auth::EdgeAuthInterceptor; use openshell_core::net::set_tcp_nodelay_best_effort; use openshell_core::proto::open_shell_client::OpenShellClient; +use openshell_otel::EnvTraceContextInterceptor; use rustls::{ RootCertStore, client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}, @@ -25,7 +26,29 @@ use tracing::debug; use url::{Host, Url}; /// Concrete gRPC client type used by all commands. -pub type GrpcClient = OpenShellClient>; +pub type GrpcClient = OpenShellClient>; + +/// Interceptor stack applied to every gateway gRPC request. +/// +/// Runs authentication header injection first, then passive W3C trace-context +/// forwarding so a CI pipeline's `TRACEPARENT` extends into the gateway's +/// spans. tonic allows a single interceptor per channel, so the two concerns +/// are composed here. +#[derive(Clone)] +pub struct GatewayInterceptor { + auth: EdgeAuthInterceptor, + trace: EnvTraceContextInterceptor, +} + +impl tonic::service::Interceptor for GatewayInterceptor { + fn call( + &mut self, + request: tonic::Request<()>, + ) -> std::result::Result, tonic::Status> { + let request = self.auth.call(request)?; + self.trace.call(request) + } +} #[derive(Clone, Debug, Default)] pub struct TlsOptions { @@ -451,17 +474,22 @@ pub async fn build_channel(server: &str, tls: &TlsOptions) -> Result { /// Build a gRPC [`OpenShellClient`]. /// -/// When `tls.edge_token` is set, the returned client is wrapped with an -/// interceptor that injects authentication headers on every request. -/// Otherwise, standard mTLS is used (interceptor is a no-op). +/// The returned client carries a [`GatewayInterceptor`] that injects +/// authentication headers (when a token is set) and passively forwards the +/// `TRACEPARENT`/`TRACESTATE` trace context from the environment on every +/// request. With no token and no trace context, the interceptor is a no-op. pub async fn grpc_client(server: &str, tls: &TlsOptions) -> Result { let channel = build_channel(server, tls).await?; let interceptor = interceptor_from_tls(tls)?; Ok(OpenShellClient::with_interceptor(channel, interceptor)) } -fn interceptor_from_tls(tls: &TlsOptions) -> Result { - EdgeAuthInterceptor::new(tls.oidc_token.as_deref(), tls.edge_token.as_deref()) +fn interceptor_from_tls(tls: &TlsOptions) -> Result { + let auth = EdgeAuthInterceptor::new(tls.oidc_token.as_deref(), tls.edge_token.as_deref())?; + Ok(GatewayInterceptor { + auth, + trace: EnvTraceContextInterceptor::from_env(), + }) } /// Gateway replica that owns the sandbox named in `response`, when it says. diff --git a/crates/openshell-otel/src/lib.rs b/crates/openshell-otel/src/lib.rs index b10c300833..02843b4c93 100644 --- a/crates/openshell-otel/src/lib.rs +++ b/crates/openshell-otel/src/lib.rs @@ -21,9 +21,9 @@ pub use grpc::{ }; pub use ocsf::{OcsfOtlp, OcsfOtlpLayer}; pub use propagation::{ - HeaderMapExtractor, MetadataMapInjector, TRACEPARENT_ENV, TRACESTATE_ENV, - TraceContextInterceptor, current_trace_context_carrier, current_trace_context_environment, - set_parent_from_environment, trace_context_environment, + EnvTraceContextInterceptor, HeaderMapExtractor, MetadataMapInjector, TRACEPARENT_ENV, + TRACESTATE_ENV, TraceContextInterceptor, current_trace_context_carrier, + current_trace_context_environment, set_parent_from_environment, trace_context_environment, }; use opentelemetry::KeyValue; diff --git a/crates/openshell-otel/src/propagation.rs b/crates/openshell-otel/src/propagation.rs index 764e343146..64ab5d80c9 100644 --- a/crates/openshell-otel/src/propagation.rs +++ b/crates/openshell-otel/src/propagation.rs @@ -6,6 +6,7 @@ use std::collections::BTreeMap; use http::HeaderMap; +use opentelemetry::Context; use opentelemetry::propagation::{Extractor, Injector, TextMapPropagator}; use opentelemetry_sdk::propagation::TraceContextPropagator; use tracing_opentelemetry::OpenTelemetrySpanExt as _; @@ -167,9 +168,108 @@ impl tonic::service::Interceptor for TraceContextInterceptor { } } +/// Reads W3C trace-context propagation fields from process environment +/// variables (`TRACEPARENT` and `TRACESTATE`). +/// +/// CI systems (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) +/// export these variables to propagate the pipeline trace to child processes. +/// Both upper-case and lower-case spellings are accepted; blank values are +/// treated as unset. +#[derive(Debug, Clone, Default)] +struct EnvTraceContext { + traceparent: Option, + tracestate: Option, +} + +impl EnvTraceContext { + fn from_env() -> Self { + Self { + traceparent: read_env("TRACEPARENT"), + tracestate: read_env("TRACESTATE"), + } + } +} + +/// Read an environment variable by its upper-case name, falling back to the +/// lower-case spelling, trimming whitespace and discarding blank values. +fn read_env(upper: &str) -> Option { + std::env::var(upper) + .ok() + .or_else(|| std::env::var(upper.to_ascii_lowercase()).ok()) + .map(|value| value.trim().to_string()) + .filter(|value| !value.is_empty()) +} + +impl Extractor for EnvTraceContext { + fn get(&self, key: &str) -> Option<&str> { + match key { + "traceparent" => self.traceparent.as_deref(), + "tracestate" => self.tracestate.as_deref(), + _ => None, + } + } + + fn keys(&self) -> Vec<&str> { + let mut keys = Vec::new(); + if self.traceparent.is_some() { + keys.push("traceparent"); + } + if self.tracestate.is_some() { + keys.push("tracestate"); + } + keys + } +} + +/// Passively forwards a W3C trace context captured from the process +/// environment onto outbound tonic requests. +/// +/// Unlike [`TraceContextInterceptor`], this does not require an active +/// OpenTelemetry span or an `SdkTracerProvider`; it reads `TRACEPARENT` +/// (and optionally `TRACESTATE`) once at construction and injects them as +/// gRPC metadata. This lets short-lived clients (e.g. the CLI on a CI runner) +/// extend a pipeline trace to the gateway without any collector configuration. +/// +/// When `TRACEPARENT` is unset or invalid the captured context carries no +/// valid span, so injection is a no-op. +#[derive(Debug, Clone)] +pub struct EnvTraceContextInterceptor { + context: Context, +} + +impl EnvTraceContextInterceptor { + /// Capture the W3C trace context from the process environment. + #[must_use] + pub fn from_env() -> Self { + Self { + context: context_from(&EnvTraceContext::from_env()), + } + } +} + +/// Extract a [`Context`] from an [`Extractor`] using a fresh base context, so +/// the result depends only on the extractor and not on any ambient context. +fn context_from(extractor: &impl Extractor) -> Context { + TraceContextPropagator::new().extract_with_context(&Context::new(), extractor) +} + +impl tonic::service::Interceptor for EnvTraceContextInterceptor { + fn call( + &mut self, + mut request: tonic::Request<()>, + ) -> Result, tonic::Status> { + TraceContextPropagator::new().inject_context( + &self.context, + &mut MetadataMapInjector::new(request.metadata_mut()), + ); + Ok(request) + } +} + #[cfg(test)] mod tests { use super::*; + use tonic::service::Interceptor as _; fn env_test_exporter() -> ( opentelemetry_sdk::trace::SdkTracerProvider, @@ -320,4 +420,58 @@ mod tests { Some("value") ); } + + fn interceptor_with(extractor: EnvTraceContext) -> EnvTraceContextInterceptor { + EnvTraceContextInterceptor { + context: context_from(&extractor), + } + } + + #[test] + fn env_interceptor_forwards_valid_traceparent() { + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + let mut interceptor = interceptor_with(EnvTraceContext { + traceparent: Some(traceparent.to_string()), + tracestate: Some("vendor=value".to_string()), + }); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert_eq!( + request + .metadata() + .get("traceparent") + .and_then(|value| value.to_str().ok()), + Some(traceparent) + ); + assert_eq!( + request + .metadata() + .get("tracestate") + .and_then(|value| value.to_str().ok()), + Some("vendor=value") + ); + } + + #[test] + fn env_interceptor_is_noop_without_traceparent() { + let mut interceptor = interceptor_with(EnvTraceContext::default()); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert!(request.metadata().get("traceparent").is_none()); + assert!(request.metadata().get("tracestate").is_none()); + } + + #[test] + fn env_interceptor_is_noop_for_invalid_traceparent() { + let mut interceptor = interceptor_with(EnvTraceContext { + traceparent: Some("not-a-valid-traceparent".to_string()), + tracestate: None, + }); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert!(request.metadata().get("traceparent").is_none()); + } } diff --git a/docs/observability/traces.mdx b/docs/observability/traces.mdx index ef8bd6f5f8..b87454c436 100644 --- a/docs/observability/traces.mdx +++ b/docs/observability/traces.mdx @@ -69,6 +69,8 @@ Give each installation that reports to the same collector its own name, includin The gateway continues a valid W3C `traceparent` sent by the caller, so a client's trace can include the gateway's spans for its requests. Without one, the gateway starts a new trace. +The CLI forwards `TRACEPARENT` and `TRACESTATE` from its environment on every gateway request. A CI system that exports these variables (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) therefore parents the gateway's spans for the commands it runs, with no collector or provider configuration on the CI runner. + The gateway also passes its trace context to compute drivers, so driver spans appear in the same trace as the gateway request that caused them. Built-in drivers export their spans through the gateway's process. Managed external drivers receive the trace endpoint, gateway name, and compute driver from the gateway. Drivers that you run yourself own their telemetry configuration. ## Next Steps From 369bd0c00d24268160fa5fdda63655269d9173ee Mon Sep 17 00:00:00 2001 From: Chris Moore Date: Mon, 19 Oct 2026 11:40:19 -0400 Subject: [PATCH 2/2] fix(cli): normalize trace-context env reading and cover interceptor composition Review follow-ups on the trace-context forwarding feature: - Normalize each TRACEPARENT/TRACESTATE spelling (trim, drop blanks) before the lower-case fallback so a blank upper-case value no longer shadows a valid lower-case one - Share one env-reading helper between EnvTraceContextInterceptor and set_parent_from_environment so both paths agree on what counts as set - Pass the TRACEPARENT_ENV/TRACESTATE_ENV constants instead of duplicated string literals - Log at debug level when a present TRACEPARENT is invalid, keeping the passive no-op behavior but leaving a grep-able signal - Cover EnvTraceContext::from_env (both spellings, padding, blanks) and the GatewayInterceptor auth+trace composition with tests - Drop tracestate when traceparent is invalid, and assert it in tests - Narrow the traces.mdx claim to standard CLI commands, since term and completions build clients without the trace interceptor Signed-off-by: Chris Moore --- crates/openshell-cli/src/tls.rs | 55 ++++++++++- crates/openshell-otel/src/propagation.rs | 116 +++++++++++++++++++---- docs/observability/traces.mdx | 2 +- 3 files changed, 153 insertions(+), 20 deletions(-) diff --git a/crates/openshell-cli/src/tls.rs b/crates/openshell-cli/src/tls.rs index 00044d00d9..bd8a369384 100644 --- a/crates/openshell-cli/src/tls.rs +++ b/crates/openshell-cli/src/tls.rs @@ -28,7 +28,8 @@ use url::{Host, Url}; /// Concrete gRPC client type used by all commands. pub type GrpcClient = OpenShellClient>; -/// Interceptor stack applied to every gateway gRPC request. +/// Interceptor stack applied to gateway requests from clients built by +/// [`grpc_client`]. /// /// Runs authentication header injection first, then passive W3C trace-context /// forwarding so a CI pipeline's `TRACEPARENT` extends into the gateway's @@ -538,10 +539,60 @@ pub fn retry_unrouted(replica: Option<&AsciiMetadataValue>, status: &tonic::Stat #[cfg(test)] mod tests { - use super::{owner_replica, retry_unrouted, routed_to, tls_server_name, with_owner_hint}; + use super::{ + TlsOptions, interceptor_from_tls, owner_replica, retry_unrouted, routed_to, + tls_server_name, with_owner_hint, + }; + use crate::TEST_ENV_LOCK; use openshell_core::replica_routing::{ OWNER_REPLICA_HEADER, OWNER_REQUEST_HEADER, ROUTE_REPLICA_HEADER, }; + use tonic::service::Interceptor as _; + + #[test] + fn gateway_interceptor_composes_auth_and_trace_forwarding() { + let _guard = TEST_ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + temp_env::with_vars( + [ + ("TRACEPARENT", Some(traceparent)), + ("TRACESTATE", Some("vendor=value")), + ], + || { + let tls = TlsOptions { + oidc_token: Some("test-token".to_string()), + ..TlsOptions::new(None, None, None) + }; + let mut interceptor = interceptor_from_tls(&tls).unwrap(); + + let request = interceptor.call(tonic::Request::new(())).unwrap(); + + assert_eq!( + request + .metadata() + .get("authorization") + .and_then(|value| value.to_str().ok()), + Some("Bearer test-token") + ); + assert_eq!( + request + .metadata() + .get("traceparent") + .and_then(|value| value.to_str().ok()), + Some(traceparent) + ); + assert_eq!( + request + .metadata() + .get("tracestate") + .and_then(|value| value.to_str().ok()), + Some("vendor=value") + ); + }, + ); + } #[test] fn owner_hint_is_requested_explicitly() { diff --git a/crates/openshell-otel/src/propagation.rs b/crates/openshell-otel/src/propagation.rs index 64ab5d80c9..895bc04113 100644 --- a/crates/openshell-otel/src/propagation.rs +++ b/crates/openshell-otel/src/propagation.rs @@ -113,18 +113,18 @@ pub fn current_trace_context_environment() -> Vec<(&'static str, String)> { /// Parent `span` under the trace context in `TRACEPARENT` and `TRACESTATE`. /// +/// Reads the environment the same way [`EnvTraceContextInterceptor`] does: +/// both spellings are accepted, whitespace is trimmed, and blank values are +/// treated as unset. +/// /// Leaves `span` unchanged when the environment carries no valid context. pub fn set_parent_from_environment(span: &tracing::Span) { let carrier = [ - ("traceparent", TRACEPARENT_ENV), - ("tracestate", TRACESTATE_ENV), + ("traceparent", read_env(TRACEPARENT_ENV)), + ("tracestate", read_env(TRACESTATE_ENV)), ] .into_iter() - .filter_map(|(field, name)| { - std::env::var(name) - .ok() - .map(|value| (field.to_string(), value)) - }) + .filter_map(|(field, value)| value.map(|value| (field.to_string(), value))) .collect::>(); set_parent_from_carrier(span, &carrier); } @@ -184,22 +184,29 @@ struct EnvTraceContext { impl EnvTraceContext { fn from_env() -> Self { Self { - traceparent: read_env("TRACEPARENT"), - tracestate: read_env("TRACESTATE"), + traceparent: read_env(TRACEPARENT_ENV), + tracestate: read_env(TRACESTATE_ENV), } } } -/// Read an environment variable by its upper-case name, falling back to the -/// lower-case spelling, trimming whitespace and discarding blank values. -fn read_env(upper: &str) -> Option { - std::env::var(upper) +/// Read one environment variable, trimming whitespace and discarding blanks. +fn read_one(name: &str) -> Option { + std::env::var(name) .ok() - .or_else(|| std::env::var(upper.to_ascii_lowercase()).ok()) .map(|value| value.trim().to_string()) .filter(|value| !value.is_empty()) } +/// Read an environment variable by its upper-case name, falling back to the +/// lower-case spelling, trimming whitespace and discarding blank values. +/// +/// Each spelling is normalized before the fallback, so a blank upper-case +/// value never shadows a valid lower-case one. +fn read_env(upper: &str) -> Option { + read_one(upper).or_else(|| read_one(&upper.to_ascii_lowercase())) +} + impl Extractor for EnvTraceContext { fn get(&self, key: &str) -> Option<&str> { match key { @@ -241,9 +248,12 @@ impl EnvTraceContextInterceptor { /// Capture the W3C trace context from the process environment. #[must_use] pub fn from_env() -> Self { - Self { - context: context_from(&EnvTraceContext::from_env()), + let env = EnvTraceContext::from_env(); + let context = context_from(&env); + if env.traceparent.is_some() && !span_context_is_valid(&context) { + tracing::debug!("TRACEPARENT is set but invalid; not forwarding trace context"); } + Self { context } } } @@ -253,6 +263,12 @@ fn context_from(extractor: &impl Extractor) -> Context { TraceContextPropagator::new().extract_with_context(&Context::new(), extractor) } +fn span_context_is_valid(context: &Context) -> bool { + use opentelemetry::trace::TraceContextExt as _; + + context.span().span_context().is_valid() +} + impl tonic::service::Interceptor for EnvTraceContextInterceptor { fn call( &mut self, @@ -427,6 +443,71 @@ mod tests { } } + fn snapshot_env(names: &[&'static str]) -> Vec<(&'static str, Option)> { + names + .iter() + .map(|name| (*name, std::env::var(name).ok())) + .collect() + } + + #[allow(unsafe_code)] + fn restore_env(saved: &[(&'static str, Option)]) { + for (name, value) in saved { + unsafe { + match value { + Some(value) => std::env::set_var(name, value), + None => std::env::remove_var(name), + } + } + } + } + + #[test] + #[allow(unsafe_code)] + fn env_trace_context_from_env_reads_both_spellings_and_skips_blanks() { + let _tracing_lock = crate::test_lock(); + let saved = snapshot_env(&[TRACEPARENT_ENV, "traceparent", TRACESTATE_ENV, "tracestate"]); + let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; + + // Upper-case spelling with surrounding whitespace. + unsafe { + std::env::set_var(TRACEPARENT_ENV, format!(" {traceparent} ")); + std::env::set_var(TRACESTATE_ENV, " vendor=value "); + } + let padded = EnvTraceContext::from_env(); + assert_eq!(padded.traceparent.as_deref(), Some(traceparent)); + assert_eq!(padded.tracestate.as_deref(), Some("vendor=value")); + + // Lower-case-only spelling. + unsafe { + std::env::remove_var(TRACEPARENT_ENV); + std::env::remove_var(TRACESTATE_ENV); + std::env::set_var("traceparent", traceparent); + std::env::set_var("tracestate", "vendor=value"); + } + let lower = EnvTraceContext::from_env(); + assert_eq!(lower.traceparent.as_deref(), Some(traceparent)); + assert_eq!(lower.tracestate.as_deref(), Some("vendor=value")); + + // A blank upper-case value must not shadow a valid lower-case one. + unsafe { + std::env::set_var(TRACEPARENT_ENV, " "); + std::env::remove_var("tracestate"); + } + let shadowed = EnvTraceContext::from_env(); + assert_eq!(shadowed.traceparent.as_deref(), Some(traceparent)); + assert_eq!(shadowed.tracestate, None); + + // Blank values in both spellings are treated as unset. + unsafe { + std::env::set_var("traceparent", ""); + } + let blank = EnvTraceContext::from_env(); + assert_eq!(blank.traceparent, None); + + restore_env(&saved); + } + #[test] fn env_interceptor_forwards_valid_traceparent() { let traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"; @@ -467,11 +548,12 @@ mod tests { fn env_interceptor_is_noop_for_invalid_traceparent() { let mut interceptor = interceptor_with(EnvTraceContext { traceparent: Some("not-a-valid-traceparent".to_string()), - tracestate: None, + tracestate: Some("vendor=value".to_string()), }); let request = interceptor.call(tonic::Request::new(())).unwrap(); assert!(request.metadata().get("traceparent").is_none()); + assert!(request.metadata().get("tracestate").is_none()); } } diff --git a/docs/observability/traces.mdx b/docs/observability/traces.mdx index b87454c436..16f8af0347 100644 --- a/docs/observability/traces.mdx +++ b/docs/observability/traces.mdx @@ -69,7 +69,7 @@ Give each installation that reports to the same collector its own name, includin The gateway continues a valid W3C `traceparent` sent by the caller, so a client's trace can include the gateway's spans for its requests. Without one, the gateway starts a new trace. -The CLI forwards `TRACEPARENT` and `TRACESTATE` from its environment on every gateway request. A CI system that exports these variables (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) therefore parents the gateway's spans for the commands it runs, with no collector or provider configuration on the CI runner. +The CLI forwards `TRACEPARENT` and `TRACESTATE` from its environment on gateway requests from its standard commands. A CI system that exports these variables (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) therefore parents the gateway's spans for the commands it runs, with no collector or provider configuration on the CI runner. The gateway also passes its trace context to compute drivers, so driver spans appear in the same trace as the gateway request that caused them. Built-in drivers export their spans through the gateway's process. Managed external drivers receive the trace endpoint, gateway name, and compute driver from the gateway. Drivers that you run yourself own their telemetry configuration.