mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Add second-based OTEL duration histograms (#27058)
## Why Exec-server request and connection latencies need fractional-second histograms. The existing duration API records integer milliseconds and uses millisecond-scale buckets. ## What changed - Adds a described duration API that records `Duration` values as fractional seconds. - Uses second-scale explicit histogram boundaries. - Caches duration histograms by name, unit, and description, matching the existing instrument caching model. - Covers exact boundaries, representative bucket placement, fractional sums, and exported metadata. This PR only adds the duration primitive. It does not add exec-server adoption. ## Stack 1. #26091: counter descriptions 2. #27057: gauge instruments 3. **#27058: second-based duration histograms** 4. #25019: initialize exec-server OpenTelemetry at startup Related independent coverage: #27059 tests OTLP HTTP log and trace event export. ## Validation - `just test -p codex-otel`
This commit is contained in:
committed by
GitHub
Unverified
parent
515f705426
commit
8aac63f477
@@ -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<String>,
|
||||
}
|
||||
|
||||
@@ -92,7 +101,7 @@ struct MetricsClientInner {
|
||||
counters: Mutex<HashMap<InstrumentKey, Counter<u64>>>,
|
||||
gauges: Mutex<HashMap<InstrumentKey, Gauge<i64>>>,
|
||||
histograms: Mutex<HashMap<String, Histogram<f64>>>,
|
||||
duration_histograms: Mutex<HashMap<String, Histogram<f64>>>,
|
||||
duration_histograms: Mutex<HashMap<InstrumentKey, Histogram<f64>>>,
|
||||
runtime_reader: Option<Arc<ManualReader>>,
|
||||
default_tags: BTreeMap<String, String>,
|
||||
}
|
||||
@@ -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,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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<()> {
|
||||
|
||||
Reference in New Issue
Block a user