diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index ebc7ddd33..4e1758355 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1920,6 +1920,7 @@ dependencies = [ "codex-app-server-protocol", "codex-protocol", "codex-utils-absolute-path", + "codex-utils-string", "eventsource-stream", "http 1.4.0", "opentelemetry", @@ -1927,6 +1928,7 @@ dependencies = [ "opentelemetry-otlp", "opentelemetry-semantic-conventions", "opentelemetry_sdk", + "os_info", "pretty_assertions", "reqwest", "serde", @@ -2244,6 +2246,9 @@ dependencies = [ [[package]] name = "codex-utils-string" version = "0.0.0" +dependencies = [ + "pretty_assertions", +] [[package]] name = "codex-windows-sandbox" diff --git a/codex-rs/otel/Cargo.toml b/codex-rs/otel/Cargo.toml index 0fbd2a805..c0bcb365b 100644 --- a/codex-rs/otel/Cargo.toml +++ b/codex-rs/otel/Cargo.toml @@ -23,6 +23,7 @@ disable-default-metrics-exporter = [] chrono = { workspace = true } codex-app-server-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } +codex-utils-string = { workspace = true } codex-api = { workspace = true } codex-protocol = { workspace = true } eventsource-stream = { workspace = true } @@ -50,6 +51,7 @@ opentelemetry_sdk = { workspace = true, features = [ "trace", ] } http = { workspace = true } +os_info = { workspace = true } reqwest = { workspace = true, features = ["blocking", "rustls-tls"] } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/otel/src/lib.rs b/codex-rs/otel/src/lib.rs index aed9e15b8..ed6e5a1c9 100644 --- a/codex-rs/otel/src/lib.rs +++ b/codex-rs/otel/src/lib.rs @@ -14,6 +14,7 @@ use crate::metrics::validation::validate_tag_key; use crate::metrics::validation::validate_tag_value; use crate::otel_provider::OtelProvider; use codex_protocol::ThreadId; +pub use codex_utils_string::sanitize_metric_tag_value; use opentelemetry_sdk::metrics::data::ResourceMetrics; use serde::Serialize; use std::time::Duration; @@ -188,7 +189,7 @@ impl OtelManager { if !self.metrics_use_metadata_tags { return Ok(Vec::new()); } - let mut tags = Vec::with_capacity(6); + let mut tags = Vec::with_capacity(5); Self::push_metadata_tag(&mut tags, "auth_mode", self.metadata.auth_mode.as_deref())?; Self::push_metadata_tag( &mut tags, diff --git a/codex-rs/otel/src/metrics/client.rs b/codex-rs/otel/src/metrics/client.rs index 386f81142..417c1f4bd 100644 --- a/codex-rs/otel/src/metrics/client.rs +++ b/codex-rs/otel/src/metrics/client.rs @@ -9,6 +9,7 @@ use crate::metrics::validation::validate_metric_name; use crate::metrics::validation::validate_tag_key; use crate::metrics::validation::validate_tag_value; use crate::metrics::validation::validate_tags; +use codex_utils_string::sanitize_metric_tag_value; use opentelemetry::KeyValue; use opentelemetry::metrics::Counter; use opentelemetry::metrics::Histogram; @@ -197,12 +198,17 @@ impl MetricsClient { validate_tags(&default_tags)?; + let mut resource_attributes = Vec::with_capacity(4); + resource_attributes.push(KeyValue::new( + semconv::attribute::SERVICE_VERSION, + service_version, + )); + resource_attributes.push(KeyValue::new(ENV_ATTRIBUTE, environment)); + resource_attributes.extend(os_resource_attributes()); + let resource = Resource::builder() .with_service_name(service_name) - .with_attributes(vec![ - KeyValue::new(semconv::attribute::SERVICE_VERSION, service_version), - KeyValue::new(ENV_ATTRIBUTE, environment), - ]) + .with_attributes(resource_attributes) .build(); let runtime_reader = runtime_reader.then(|| { @@ -284,6 +290,22 @@ impl MetricsClient { } } +fn os_resource_attributes() -> Vec { + let os_info = os_info::get(); + let os_type_raw = os_info.os_type().to_string(); + let os_type = sanitize_metric_tag_value(os_type_raw.as_str()); + let os_version_raw = os_info.version().to_string(); + let os_version = sanitize_metric_tag_value(os_version_raw.as_str()); + let mut attributes = Vec::new(); + if os_type != "unspecified" { + attributes.push(KeyValue::new("os", os_type)); + } + if os_version != "unspecified" { + attributes.push(KeyValue::new("os_version", os_version)); + } + attributes +} + fn build_provider( resource: Resource, exporter: E, diff --git a/codex-rs/utils/string/Cargo.toml b/codex-rs/utils/string/Cargo.toml index 29437eaef..8a6253938 100644 --- a/codex-rs/utils/string/Cargo.toml +++ b/codex-rs/utils/string/Cargo.toml @@ -6,3 +6,6 @@ license.workspace = true [lints] workspace = true + +[dev-dependencies] +pretty_assertions = { workspace = true } diff --git a/codex-rs/utils/string/src/lib.rs b/codex-rs/utils/string/src/lib.rs index f7299d437..b69f18564 100644 --- a/codex-rs/utils/string/src/lib.rs +++ b/codex-rs/utils/string/src/lib.rs @@ -36,3 +36,46 @@ pub fn take_last_bytes_at_char_boundary(s: &str, maxb: usize) -> &str { } &s[start..] } + +/// Sanitize a tag value to comply with metric tag validation rules: +/// only ASCII alphanumeric, '.', '_', '-', and '/' are allowed. +pub fn sanitize_metric_tag_value(value: &str) -> String { + const MAX_LEN: usize = 256; + let sanitized: String = value + .chars() + .map(|ch| { + if ch.is_ascii_alphanumeric() || matches!(ch, '.' | '_' | '-' | '/') { + ch + } else { + '_' + } + }) + .collect(); + let trimmed = sanitized.trim_matches('_'); + if trimmed.is_empty() || trimmed.chars().all(|ch| !ch.is_ascii_alphanumeric()) { + return "unspecified".to_string(); + } + if trimmed.len() <= MAX_LEN { + trimmed.to_string() + } else { + trimmed[..MAX_LEN].to_string() + } +} + +#[cfg(test)] +mod tests { + use super::sanitize_metric_tag_value; + use pretty_assertions::assert_eq; + + #[test] + fn sanitize_metric_tag_value_trims_and_fills_unspecified() { + let msg = "///"; + assert_eq!(sanitize_metric_tag_value(msg), "unspecified"); + } + + #[test] + fn sanitize_metric_tag_value_replaces_invalid_chars() { + let msg = "bad value!"; + assert_eq!(sanitize_metric_tag_value(msg), "bad_value"); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index b938a86e6..4a97efaaf 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -91,7 +91,7 @@ pub use setup::SETUP_VERSION; #[cfg(target_os = "windows")] pub use setup_error::extract_failure as extract_setup_failure; #[cfg(target_os = "windows")] -pub use setup_error::sanitize_tag_value as sanitize_setup_metric_tag_value; +pub use setup_error::sanitize_setup_metric_tag_value; #[cfg(target_os = "windows")] pub use setup_error::setup_error_path; #[cfg(target_os = "windows")] diff --git a/codex-rs/windows-sandbox-rs/src/setup_error.rs b/codex-rs/windows-sandbox-rs/src/setup_error.rs index 9ee9ebf7f..23022450a 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_error.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_error.rs @@ -1,5 +1,6 @@ use anyhow::Context; use anyhow::Result; +use codex_utils_string::sanitize_metric_tag_value; use serde::Deserialize; use serde::Serialize; use std::fs; @@ -126,7 +127,7 @@ impl SetupFailure { } pub fn metric_message(&self) -> String { - sanitize_tag_value(&self.message) + sanitize_setup_metric_tag_value(&self.message) } } @@ -181,30 +182,9 @@ pub fn read_setup_error_report(codex_home: &Path) -> Result String { - const MAX_LEN: usize = 256; - let redacted = redact_home_paths(value); - let sanitized: String = redacted - .chars() - .map(|ch| { - if ch.is_ascii_alphanumeric() || matches!(ch, '.' | '_' | '-' | '/') { - ch - } else { - '_' - } - }) - .collect(); - let trimmed = sanitized.trim_matches('_'); - if trimmed.is_empty() { - return "unspecified".to_string(); - } - if trimmed.len() <= MAX_LEN { - trimmed.to_string() - } else { - trimmed[..MAX_LEN].to_string() - } +/// Sanitize a setup error message for use as a metric tag. +pub fn sanitize_setup_metric_tag_value(value: &str) -> String { + sanitize_metric_tag_value(redact_home_paths(value).as_str()) } fn redact_home_paths(value: &str) -> String { @@ -267,7 +247,7 @@ fn redact_username_segments(value: &str, usernames: &[String]) -> String { #[cfg(test)] mod tests { - use super::*; + use super::redact_username_segments; use pretty_assertions::assert_eq; #[test]