diff --git a/codex-rs/otel/src/metrics/client.rs b/codex-rs/otel/src/metrics/client.rs index a36157aae..73fc3c661 100644 --- a/codex-rs/otel/src/metrics/client.rs +++ b/codex-rs/otel/src/metrics/client.rs @@ -43,12 +43,21 @@ use tracing::debug; const ENV_ATTRIBUTE: &str = "env"; const METER_NAME: &str = "codex"; -const DURATION_UNIT: &str = "ms"; -const DURATION_DESCRIPTION: &str = "Duration in milliseconds."; +const MILLISECOND_DURATION_UNIT: &str = "ms"; +const MILLISECOND_DURATION_DESCRIPTION: &str = "Duration in milliseconds."; +const MILLISECOND_DURATION_BOUNDARIES: &[f64] = &[ + 0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, + 10000.0, +]; +const SECOND_DURATION_UNIT: &str = "s"; +const SECOND_DURATION_BOUNDARIES: &[f64] = &[ + 0.0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1.0, 2.5, 5.0, 7.5, 10.0, +]; #[derive(Debug, Eq, Hash, PartialEq)] struct InstrumentKey { name: String, + unit: Option<&'static str>, description: Option, } @@ -92,7 +101,7 @@ struct MetricsClientInner { counters: Mutex>>, gauges: Mutex>>, histograms: Mutex>>, - duration_histograms: Mutex>>, + duration_histograms: Mutex>>, runtime_reader: Option>, default_tags: BTreeMap, } @@ -120,6 +129,7 @@ impl MetricsClientInner { .unwrap_or_else(std::sync::PoisonError::into_inner); let key = InstrumentKey { name: name.to_string(), + unit: None, description: description.map(str::to_string), }; let counter = counters.entry(key).or_insert_with(|| { @@ -164,6 +174,7 @@ impl MetricsClientInner { .unwrap_or_else(std::sync::PoisonError::into_inner); let key = InstrumentKey { name: name.to_string(), + unit: None, description: description.map(str::to_string), }; let gauge = gauges.entry(key).or_insert_with(|| { @@ -177,7 +188,15 @@ impl MetricsClientInner { Ok(()) } - fn duration_histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) -> Result<()> { + fn duration_histogram( + &self, + name: &str, + value: f64, + unit: &'static str, + description: &str, + boundaries: &'static [f64], + tags: &[(&str, &str)], + ) -> Result<()> { validate_metric_name(name)?; let attributes = self.attributes(tags)?; @@ -185,14 +204,20 @@ impl MetricsClientInner { .duration_histograms .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - let histogram = histograms.entry(name.to_string()).or_insert_with(|| { + let key = InstrumentKey { + name: name.to_string(), + unit: Some(unit), + description: Some(description.to_string()), + }; + let histogram = histograms.entry(key).or_insert_with(|| { self.meter .f64_histogram(name.to_string()) - .with_unit(DURATION_UNIT) - .with_description(DURATION_DESCRIPTION) + .with_unit(unit) + .with_description(description.to_string()) + .with_boundaries(boundaries.to_vec()) .build() }); - histogram.record(value as f64, &attributes); + histogram.record(value, &attributes); Ok(()) } @@ -338,7 +363,28 @@ impl MetricsClient { ) -> Result<()> { self.0.duration_histogram( name, - duration.as_millis().min(i64::MAX as u128) as i64, + duration.as_millis().min(i64::MAX as u128) as f64, + MILLISECOND_DURATION_UNIT, + MILLISECOND_DURATION_DESCRIPTION, + MILLISECOND_DURATION_BOUNDARIES, + tags, + ) + } + + /// Record a duration in seconds using a histogram with an instrument description. + pub fn record_duration_seconds_with_description( + &self, + name: &str, + description: &str, + duration: Duration, + tags: &[(&str, &str)], + ) -> Result<()> { + self.0.duration_histogram( + name, + duration.as_secs_f64(), + SECOND_DURATION_UNIT, + description, + SECOND_DURATION_BOUNDARIES, tags, ) } diff --git a/codex-rs/otel/tests/suite/timing.rs b/codex-rs/otel/tests/suite/timing.rs index 0cf73b9a5..5955979f8 100644 --- a/codex-rs/otel/tests/suite/timing.rs +++ b/codex-rs/otel/tests/suite/timing.rs @@ -33,6 +33,50 @@ fn record_duration_records_histogram() -> Result<()> { Ok(()) } +#[test] +fn record_duration_seconds_uses_fractional_seconds_and_scaled_buckets() -> Result<()> { + let (metrics, exporter) = build_metrics_with_defaults(&[])?; + + for duration in [ + Duration::from_millis(200), + Duration::from_secs(1), + Duration::from_millis(4900), + ] { + metrics.record_duration_seconds_with_description( + "codex.request_duration_seconds", + "Duration of Codex requests in seconds.", + duration, + &[("method", "initialize")], + )?; + } + metrics.shutdown()?; + + let resource_metrics = latest_metrics(&exporter); + let (bounds, bucket_counts, sum, count) = + histogram_data(&resource_metrics, "codex.request_duration_seconds"); + assert_eq!( + bounds, + vec![ + 0.0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1.0, 2.5, 5.0, 7.5, 10.0, + ] + ); + assert_eq!( + bucket_counts, + vec![0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 0, 1, 0, 0, 0] + ); + assert!((sum - 6.1).abs() < f64::EPSILON * 8.0); + assert_eq!(count, 3); + let metric = crate::harness::find_metric(&resource_metrics, "codex.request_duration_seconds") + .unwrap_or_else(|| panic!("metric codex.request_duration_seconds missing")); + assert_eq!(metric.unit(), "s"); + assert_eq!( + metric.description(), + "Duration of Codex requests in seconds." + ); + + Ok(()) +} + // Ensures time_result returns the closure output and records timing. #[test] fn timer_result_records_success() -> Result<()> {