From 1011bcd73e6750e9374fcf31fb513f652e4aa4e3 Mon Sep 17 00:00:00 2001 From: Kris Hicks Date: Thu, 30 Jul 2026 14:53:46 -0700 Subject: [PATCH] refactor(otel): share OTLP trace provider setup Extract common OpenTelemetry provider construction into openshell-otel so OpenShell services share one OTLP/gRPC export implementation. The shared crate owns: - typed exporter setup errors and non-fatal provider enablement; - endpoint trimming and URI validation before lazy exporter connection; - fixed and environment-or-default service-name policies; - service version and caller-supplied resource attributes; - batch tracer-provider construction; and - span-only tracing layers that exclude OpenTelemetry exporter callsites. Migrate the gateway to the shared provider while retaining its configurable service name, error marking, and tracing test collector. Add the shared crate to the architecture inventory and document the tracing boundary. Refs #2507 Signed-off-by: Kris Hicks --- AGENTS.md | 1 + Cargo.lock | 17 +- architecture/gateway.md | 3 +- crates/openshell-otel/Cargo.toml | 27 +++ crates/openshell-otel/src/lib.rs | 198 ++++++++++++++++++++ crates/openshell-server/Cargo.toml | 2 +- crates/openshell-server/src/otel_tracing.rs | 137 +++----------- 7 files changed, 272 insertions(+), 113 deletions(-) create mode 100644 crates/openshell-otel/Cargo.toml create mode 100644 crates/openshell-otel/src/lib.rs diff --git a/AGENTS.md b/AGENTS.md index ce7fc7cd6..540a8d920 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,6 +37,7 @@ These pipelines connect skills into end-to-end workflows. Individual skill files | `crates/openshell-bootstrap/` | Gateway metadata | Gateway registration metadata, auth token storage, mTLS bundle storage | | `crates/openshell-gateway-interceptors/` | Gateway interceptors | Intercepts and transforms configured gRPC requests at the gateway routing boundary | | `crates/openshell-ocsf/` | OCSF logging | OCSF v1.7.0 event types, builders, shorthand/JSONL formatters, tracing layers | +| `crates/openshell-otel/` | OpenTelemetry support | Shared OTLP trace provider, resource, and tracing-layer construction | | `crates/openshell-core/` | Shared core | Common types, configuration, error handling | | `crates/openshell-sdk/` | Shared client SDK | Async Rust gateway client (gRPC transport, TLS, OIDC refresh, edge tunnel); consumed by CLI, TUI, and `@openshell/sdk` | | `crates/openshell-providers/` | Provider management | Credential provider backends | diff --git a/Cargo.lock b/Cargo.lock index cde49c693..717f3260a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4045,6 +4045,21 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "openshell-otel" +version = "0.0.0" +dependencies = [ + "http 1.4.0", + "opentelemetry", + "opentelemetry-otlp", + "opentelemetry_sdk", + "thiserror 2.0.18", + "tokio", + "tracing", + "tracing-opentelemetry", + "tracing-subscriber", +] + [[package]] name = "openshell-policy" version = "0.0.0" @@ -4196,6 +4211,7 @@ dependencies = [ "openshell-driver-podman", "openshell-gateway-interceptors", "openshell-ocsf", + "openshell-otel", "openshell-policy", "openshell-prover", "openshell-providers", @@ -4203,7 +4219,6 @@ dependencies = [ "openshell-supervisor-middleware", "openshell-supervisor-middleware-builtins", "opentelemetry", - "opentelemetry-otlp", "opentelemetry-proto", "opentelemetry_sdk", "petname", diff --git a/architecture/gateway.md b/architecture/gateway.md index 946916147..56899983a 100644 --- a/architecture/gateway.md +++ b/architecture/gateway.md @@ -615,7 +615,8 @@ replace the existing logging paths. export: the table's presence is the on-switch, and `OTEL_EXPORTER_OTLP_ENDPOINT` is ignored so enablement has a single source. TOML decides whether and where to export; the SDK's `OTEL_*` variables tune how. Transport is OTLP over gRPC -only. +only. Shared provider, resource, and tracing-layer construction lives in +`openshell-otel`. Span emission requires no per-handler instrumentation. The `tower_http` `TraceLayer` in `multiplex.rs` opens a span per inbound request, and that span diff --git a/crates/openshell-otel/Cargo.toml b/crates/openshell-otel/Cargo.toml new file mode 100644 index 000000000..bdf42630a --- /dev/null +++ b/crates/openshell-otel/Cargo.toml @@ -0,0 +1,27 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +[package] +name = "openshell-otel" +description = "Shared OpenTelemetry trace export support for OpenShell services" +version.workspace = true +edition.workspace = true +rust-version.workspace = true +license.workspace = true +repository.workspace = true + +[dependencies] +http = { workspace = true } +opentelemetry = { workspace = true } +opentelemetry_sdk = { workspace = true } +opentelemetry-otlp = { workspace = true } +thiserror = { workspace = true } +tracing = { workspace = true } +tracing-opentelemetry = { workspace = true } +tracing-subscriber = { workspace = true } + +[dev-dependencies] +tokio = { workspace = true } + +[lints] +workspace = true diff --git a/crates/openshell-otel/src/lib.rs b/crates/openshell-otel/src/lib.rs new file mode 100644 index 000000000..ae6f503ad --- /dev/null +++ b/crates/openshell-otel/src/lib.rs @@ -0,0 +1,198 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Shared OpenTelemetry trace export support for `OpenShell` services. + +use opentelemetry::KeyValue; +use opentelemetry::trace::TracerProvider as _; +use opentelemetry_otlp::{SpanExporter, WithExportConfig}; +use opentelemetry_sdk::Resource; +use opentelemetry_sdk::trace::SdkTracer; +pub use opentelemetry_sdk::trace::SdkTracerProvider; +use tracing::Subscriber; +use tracing_opentelemetry::OpenTelemetryLayer; +use tracing_subscriber::Layer as _; +use tracing_subscriber::registry::LookupSpan; + +const SDK_UNKNOWN_SERVICE_PREFIX: &str = "unknown_service"; + +/// How a process chooses its OpenTelemetry `service.name`. +#[derive(Debug, Clone, Copy)] +pub enum ServiceName<'a> { + /// Always use this name, overriding `OTEL_SERVICE_NAME`. + Fixed(&'a str), + /// Use `OTEL_SERVICE_NAME` when set, otherwise use this default. + EnvironmentOr(&'a str), +} + +/// Inputs for an OTLP/gRPC trace provider. +#[derive(Debug, Clone)] +pub struct OtlpTraceConfig<'a> { + pub endpoint: &'a str, + pub service_name: ServiceName<'a>, + pub service_version: Option<&'a str>, + pub resource_attributes: Vec, +} + +/// Failure to construct an OTLP trace provider. +#[derive(Debug, thiserror::Error)] +pub enum SetupError { + #[error("OTLP endpoint is empty")] + EmptyEndpoint, + + #[error("invalid OTLP endpoint {endpoint:?}: {source}")] + InvalidEndpoint { + endpoint: String, + source: http::uri::InvalidUri, + }, + + #[error("failed to build the OTLP span exporter: {0}")] + Exporter(#[from] opentelemetry_otlp::ExporterBuildError), +} + +fn resource_attributes(config: &OtlpTraceConfig<'_>) -> Vec { + let mut attributes = config.resource_attributes.clone(); + if let Some(version) = config + .service_version + .map(str::trim) + .filter(|version| !version.is_empty()) + { + attributes.push(KeyValue::new("service.version", version.to_string())); + } + attributes +} + +/// Build the OpenTelemetry resource for a trace provider configuration. +#[must_use] +pub fn resource_for(config: &OtlpTraceConfig<'_>) -> Resource { + let attributes = resource_attributes(config); + match config.service_name { + ServiceName::Fixed(name) => Resource::builder() + .with_service_name(name.trim().to_string()) + .with_attributes(attributes) + .build(), + ServiceName::EnvironmentOr(default) => { + let detected = Resource::builder() + .with_attributes(attributes.clone()) + .build(); + if detected + .get(&opentelemetry::Key::from_static_str("service.name")) + .is_some_and(|value| !value.to_string().starts_with(SDK_UNKNOWN_SERVICE_PREFIX)) + { + detected + } else { + Resource::builder() + .with_service_name(default.trim().to_string()) + .with_attributes(attributes) + .build() + } + } + } +} + +/// Build an OTLP/gRPC trace provider. +pub fn build_provider(config: &OtlpTraceConfig<'_>) -> Result { + let endpoint = config.endpoint.trim(); + if endpoint.is_empty() { + return Err(SetupError::EmptyEndpoint); + } + endpoint + .parse::() + .map_err(|source| SetupError::InvalidEndpoint { + endpoint: endpoint.to_string(), + source, + })?; + + let exporter = SpanExporter::builder() + .with_tonic() + .with_endpoint(endpoint) + .build()?; + + Ok(SdkTracerProvider::builder() + .with_batch_exporter(exporter) + .with_resource(resource_for(config)) + .build()) +} + +/// Build the provider for an optional OTLP configuration. +/// +/// Telemetry setup failures disable export and remain available for the caller +/// to report after its tracing subscriber is installed. +#[must_use] +pub fn provider_for( + config: Option>, +) -> (Option, Option) { + match config.as_ref().map(build_provider) { + None => (None, None), + Some(Ok(provider)) => (Some(provider), None), + Some(Err(error)) => (None, Some(error)), + } +} + +/// Filtered OpenTelemetry layer returned by [`layer`]. +pub type OtlpLayer = tracing_subscriber::filter::Filtered< + OpenTelemetryLayer, + tracing_subscriber::filter::FilterFn, + S, +>; + +/// Build a tracing layer that exports spans and excludes exporter callsites. +pub fn layer(provider: &SdkTracerProvider, instrumentation_scope: &'static str) -> OtlpLayer +where + S: Subscriber + for<'span> LookupSpan<'span>, +{ + tracing_opentelemetry::layer() + .with_tracer(provider.tracer(instrumentation_scope)) + .with_filter(tracing_subscriber::filter::filter_fn(|metadata| { + metadata.is_span() && !metadata.target().starts_with("opentelemetry") + })) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resource_uses_fixed_service_identity_and_custom_attributes() { + let resource = resource_for(&OtlpTraceConfig { + endpoint: "http://127.0.0.1:4317", + service_name: ServiceName::Fixed("openshell-driver-vm"), + service_version: Some("1.2.3"), + resource_attributes: vec![KeyValue::new("openshell.gateway.name", "vm-dev")], + }); + + assert_eq!( + resource + .get(&opentelemetry::Key::from_static_str("service.name")) + .map(|value| value.to_string()), + Some("openshell-driver-vm".to_string()) + ); + assert_eq!( + resource + .get(&opentelemetry::Key::from_static_str("service.version")) + .map(|value| value.to_string()), + Some("1.2.3".to_string()) + ); + assert_eq!( + resource + .get(&opentelemetry::Key::from_static_str( + "openshell.gateway.name", + )) + .map(|value| value.to_string()), + Some("vm-dev".to_string()) + ); + } + + #[tokio::test] + async fn malformed_endpoint_disables_export_with_a_reportable_error() { + let (provider, error) = provider_for(Some(OtlpTraceConfig { + endpoint: "definitely not a url", + service_name: ServiceName::Fixed("test-service"), + service_version: None, + resource_attributes: Vec::new(), + })); + + assert!(provider.is_none()); + assert!(matches!(error, Some(SetupError::InvalidEndpoint { .. }))); + } +} diff --git a/crates/openshell-server/Cargo.toml b/crates/openshell-server/Cargo.toml index 06970735a..e22db21e3 100644 --- a/crates/openshell-server/Cargo.toml +++ b/crates/openshell-server/Cargo.toml @@ -22,6 +22,7 @@ openshell-driver-kubernetes = { path = "../openshell-driver-kubernetes" } openshell-driver-podman = { path = "../openshell-driver-podman" } openshell-gateway-interceptors = { path = "../openshell-gateway-interceptors" } openshell-ocsf = { path = "../openshell-ocsf" } +openshell-otel = { path = "../openshell-otel" } openshell-policy = { path = "../openshell-policy" } openshell-prover = { path = "../openshell-prover" } openshell-providers = { path = "../openshell-providers" } @@ -72,7 +73,6 @@ tracing-subscriber = { workspace = true } # OpenTelemetry (OTLP trace export, opt-in via [openshell.gateway.otlp]) opentelemetry = { workspace = true } opentelemetry_sdk = { workspace = true } -opentelemetry-otlp = { workspace = true } tracing-opentelemetry = { workspace = true } # Metrics diff --git a/crates/openshell-server/src/otel_tracing.rs b/crates/openshell-server/src/otel_tracing.rs index b22f7a8dc..dd143f22b 100644 --- a/crates/openshell-server/src/otel_tracing.rs +++ b/crates/openshell-server/src/otel_tracing.rs @@ -21,14 +21,12 @@ //! Only traces are exported. Logs and metrics have their own surfaces (OCSF //! JSONL and the Prometheus `/metrics` endpoint). -use opentelemetry::KeyValue; -use opentelemetry::trace::TracerProvider as _; -use opentelemetry_otlp::{SpanExporter, WithExportConfig}; +pub use openshell_otel::SetupError; +use openshell_otel::{OtlpTraceConfig, ServiceName}; +#[cfg(test)] use opentelemetry_sdk::Resource; -use opentelemetry_sdk::trace::{SdkTracer, SdkTracerProvider}; +use opentelemetry_sdk::trace::SdkTracerProvider; use tracing::Subscriber; -use tracing_opentelemetry::OpenTelemetryLayer; -use tracing_subscriber::Layer as _; use tracing_subscriber::registry::LookupSpan; use crate::config_file::OtlpConfig; @@ -39,67 +37,28 @@ const DEFAULT_SERVICE_NAME: &str = "openshell-gateway"; /// Instrumentation scope recorded on spans this gateway emits. const INSTRUMENTATION_SCOPE: &str = "openshell-gateway"; -/// Prefix of the placeholder `service.name` the SDK's own detector supplies -/// when nothing else set one. The full value is `unknown_service:`. -/// Used to tell "operator configured nothing" apart from "operator set -/// `OTEL_SERVICE_NAME`". -const SDK_UNKNOWN_SERVICE_PREFIX: &str = "unknown_service"; - -/// Failure to construct the OTLP export pipeline from gateway config. -/// -/// Every variant is a configuration error, surfaced at startup. Collector -/// *reachability* is deliberately not an error here — the batch exporter -/// connects lazily so a down collector never blocks the gateway from serving. -#[derive(Debug, thiserror::Error)] -pub enum SetupError { - #[error("otlp endpoint is empty; set [openshell.gateway.otlp].endpoint")] - EmptyEndpoint, - - #[error("invalid otlp endpoint {endpoint:?}: {source}")] - InvalidEndpoint { - endpoint: String, - source: http::uri::InvalidUri, - }, - - #[error("failed to build the otlp span exporter: {0}")] - Exporter(#[from] opentelemetry_otlp::ExporterBuildError), -} - -/// Build the `OTel` resource describing this gateway process. -/// -/// `Resource::builder` also runs the SDK's env detector, so -/// `OTEL_RESOURCE_ATTRIBUTES` and `OTEL_SERVICE_NAME` are merged in on top of -/// these defaults. -fn build_resource(cfg: &OtlpConfig) -> Resource { - let version = KeyValue::new("service.version", openshell_core::VERSION); - - if let Some(name) = cfg +fn trace_config(cfg: &OtlpConfig) -> OtlpTraceConfig<'_> { + let service_name = cfg .service_name .as_deref() .map(str::trim) .filter(|s| !s.is_empty()) - { - return Resource::builder() - .with_service_name(name.to_string()) - .with_attribute(version) - .build(); - } - - // No name configured: prefer whatever the env detector found, and fall - // back to our own default only if it found nothing. Setting the default - // unconditionally would mask `OTEL_SERVICE_NAME`. - let detected = Resource::builder().with_attribute(version.clone()).build(); - if detected - .get(&opentelemetry::Key::from_static_str("service.name")) - .is_some_and(|v| !v.to_string().starts_with(SDK_UNKNOWN_SERVICE_PREFIX)) - { - return detected; - } - - Resource::builder() - .with_service_name(DEFAULT_SERVICE_NAME) - .with_attribute(version) - .build() + .map_or( + ServiceName::EnvironmentOr(DEFAULT_SERVICE_NAME), + ServiceName::Fixed, + ); + + OtlpTraceConfig { + endpoint: &cfg.endpoint, + service_name, + service_version: Some(openshell_core::VERSION), + resource_attributes: Vec::new(), + } +} + +#[cfg(test)] +fn build_resource(cfg: &OtlpConfig) -> Resource { + openshell_otel::resource_for(&trace_config(cfg)) } /// Build a tracer provider exporting over OTLP/gRPC to the configured endpoint. @@ -110,29 +69,9 @@ fn build_resource(cfg: &OtlpConfig) -> Resource { /// /// The sampler and span limits are left at the SDK's defaults, which are /// themselves resolved from `OTEL_*` env vars (see the module docs). +#[cfg(test)] fn build_provider(cfg: &OtlpConfig) -> Result { - let endpoint = cfg.endpoint.trim(); - if endpoint.is_empty() { - return Err(SetupError::EmptyEndpoint); - } - // Validate up front: the exporter defers connection, so without this a - // typo'd endpoint would surface only as export failures at runtime. - endpoint - .parse::() - .map_err(|source| SetupError::InvalidEndpoint { - endpoint: endpoint.to_string(), - source, - })?; - - let exporter = SpanExporter::builder() - .with_tonic() - .with_endpoint(endpoint) - .build()?; - - Ok(SdkTracerProvider::builder() - .with_batch_exporter(exporter) - .with_resource(build_resource(cfg)) - .build()) + openshell_otel::build_provider(&trace_config(cfg)) } /// Resolve the tracer provider for a gateway config file's optional @@ -144,32 +83,18 @@ fn build_provider(cfg: &OtlpConfig) -> Result { /// The error is returned rather than logged because the provider is built /// before the subscriber it attaches to, so logging here would go nowhere. pub fn provider_for(cfg: Option<&OtlpConfig>) -> (Option, Option) { - match cfg.map(build_provider) { - None => (None, None), - Some(Ok(provider)) => (Some(provider), None), - Some(Err(err)) => (None, Some(err)), - } + openshell_otel::provider_for(cfg.map(trace_config)) } /// Build the `tracing` layer that forwards spans to `provider`. /// /// Events stay on the gateway's logging layers. Spans emitted by the /// OpenTelemetry crates are excluded to prevent recursive export traffic. -pub fn layer( - provider: &SdkTracerProvider, -) -> tracing_subscriber::filter::Filtered< - OpenTelemetryLayer, - tracing_subscriber::filter::FilterFn, - S, -> +pub fn layer(provider: &SdkTracerProvider) -> openshell_otel::OtlpLayer where S: Subscriber + for<'span> LookupSpan<'span>, { - tracing_opentelemetry::layer() - .with_tracer(provider.tracer(INSTRUMENTATION_SCOPE)) - .with_filter(tracing_subscriber::filter::filter_fn(|meta| { - meta.is_span() && !is_opentelemetry_target(meta.target()) - })) + openshell_otel::layer(provider, INSTRUMENTATION_SCOPE) } /// Mark `span` as failed. @@ -180,14 +105,6 @@ pub fn mark_error(span: &tracing::Span) { span.record("otel.status_code", "ERROR"); } -/// Whether `target` belongs to the OpenTelemetry crates themselves. -/// -/// OpenTelemetry crates use their crate name as the target (`opentelemetry`, -/// `opentelemetry_sdk`, `opentelemetry-otlp`), so a prefix match covers them. -fn is_opentelemetry_target(target: &str) -> bool { - target.starts_with("opentelemetry") -} - /// In-process OTLP/gRPC trace collector, for tests that need to assert what /// the gateway actually put on the wire. #[cfg(test)]