From 2bba3a29a0537a1869d76ec2982b8d5e8d02ee9f Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 9 Sep 2026 15:30:53 +0000 Subject: [PATCH] Use explicit histogram buckets for Guardian context metrics (#44181) ## Why Guardian request and section cost distributions need shared bucket boundaries across synchronous and asynchronous reviewers so their measurements align. ## What changed - Add `histogram_with_boundaries` to session telemetry and extension metrics while preserving session attribution. - Use shared request-token buckets up to 2,000,000 tokens and section-cost buckets up to 16,777,216 for both review paths. ## Testing Extend telemetry tests to verify explicit bucket boundaries and sample counts, and Guardian integration coverage to check exported request and section metric bounds for both synchronous and asynchronous reviews. GitOrigin-RevId: e34f6972b3418bac3b061939b62eeeccaec5a299 --- .../app-server/tests/suite/v2/guardian_v2.rs | 39 ++++++++++++++++--- codex-rs/core/src/guardian/prompt.rs | 22 ++++++----- codex-rs/core/src/guardian/request_budget.rs | 4 +- .../core/src/session/extension_metrics.rs | 11 ++++++ .../extension-api/src/capabilities/metrics.rs | 10 +++++ .../guardian-v2/src/async_scorer/extension.rs | 3 +- .../src/async_scorer/extension_tests.rs | 10 +++++ .../guardian-v2/src/async_scorer/sampler.rs | 3 +- .../src/async_scorer/sampler_tests.rs | 10 +++++ codex-rs/ext/skills/src/extension_tests.rs | 10 +++++ .../skills/src/render_observability_tests.rs | 10 +++++ codex-rs/ext/skills/tests/skills_extension.rs | 10 +++++ codex-rs/guardian-context/src/budget.rs | 35 +++++++++++++++++ codex-rs/guardian-context/src/lib.rs | 2 + codex-rs/otel/src/events/session_telemetry.rs | 23 +++++++++++ codex-rs/otel/tests/suite/manager_metrics.rs | 18 +++++++++ 16 files changed, 202 insertions(+), 18 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index 75374ccb9e..0bfbe618df 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::io::Write; use std::path::Path; use std::sync::Arc; @@ -179,7 +180,8 @@ struct MockResponsesState { allow_guardian_review: Notify, classification_completed: Notify, truncation_recorded: Notify, - sync_cost_recorded: Notify, + context_metric_bounds: Mutex>>, + context_metrics_recorded: Notify, luna_score: f64, invalid_classification: bool, review_outcome: ReviewOutcome, @@ -723,12 +725,22 @@ async fn guardian_v2_routes_scoped_tool_approvals( for metric in payload["resourceMetrics"].as_array().into_iter().flatten() .flat_map(|resource| resource["scopeMetrics"].as_array().into_iter().flatten()) .flat_map(|scope| scope["metrics"].as_array().into_iter().flatten()) - .filter(|metric| metric["name"] == "codex.guardian.context.request_tokens") + .filter(|metric| matches!(metric["name"].as_str(), Some("codex.guardian.context.request_tokens" | "codex.guardian.context.section_cost"))) { + let name = metric["name"].as_str().expect("context metric name"); for point in metric["histogram"]["dataPoints"].as_array().into_iter().flatten() { let attributes = point["attributes"].as_array().expect("metric attributes"); - if attributes.iter().any(|attr| attr["key"] == "target" && attr["value"]["stringValue"] == "sync") { - state.sync_cost_recorded.notify_one(); + for target in ["sync", "async"] { + if attributes.iter().any(|attr| attr["key"] == "target" && attr["value"]["stringValue"] == target) { + let mut bounds = state.context_metric_bounds.lock().expect("context metric bounds"); + bounds.insert( + (name.to_owned(), target.to_owned()), + point["explicitBounds"].as_array().and_then(|bounds| bounds.last()).and_then(Value::as_f64), + ); + if bounds.len() == 4 { + state.context_metrics_recorded.notify_one(); + } + } } } } @@ -1342,9 +1354,24 @@ async fn guardian_v2_routes_scoped_tool_approvals( } } if mixed_evidence { - timeout(TIMEOUT, responses_state.sync_cost_recorded.notified()) + timeout(TIMEOUT, responses_state.context_metrics_recorded.notified()) .await - .expect("sync request cost should be exported"); + .expect("sync and async request/section metrics should all be exported"); + let bounds = responses_state + .context_metric_bounds + .lock() + .expect("context metric bounds"); + for ((metric, target), bound) in bounds.iter() { + assert_eq!( + *bound, + Some(if metric == "codex.guardian.context.request_tokens" { + 2_000_000.0 + } else { + 16_777_216.0 + }), + "{metric} ({target}) must export its context metric buckets" + ); + } } if lifecycle.has_user_answer() { let reviews = responses_state diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 43f5b83214..52ab173d3e 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -216,15 +216,19 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn( let context = sections.compose(presentation, transcript)?; for (section, cost) in context.section_costs() { for (measurement, value) in cost.measurements() { - session.services.session_telemetry.histogram( - codex_guardian_context::SECTION_COST_METRIC, - i64::try_from(value).unwrap_or(i64::MAX), - &[ - ("target", "sync"), - ("section", section), - ("measurement", measurement), - ], - ); + session + .services + .session_telemetry + .histogram_with_boundaries( + codex_guardian_context::SECTION_COST_METRIC, + i64::try_from(value).unwrap_or(i64::MAX), + codex_guardian_context::SECTION_COST_BOUNDARIES, + &[ + ("target", "sync"), + ("section", section), + ("measurement", measurement), + ], + ); } } let items = context.into_user_inputs()?; diff --git a/codex-rs/core/src/guardian/request_budget.rs b/codex-rs/core/src/guardian/request_budget.rs index 50b356a435..ad53549845 100644 --- a/codex-rs/core/src/guardian/request_budget.rs +++ b/codex-rs/core/src/guardian/request_budget.rs @@ -1,6 +1,7 @@ //! Measures the complete synchronous request after image admission and wire //! prefix assembly. Includes reused history, tool definitions and output format. +use codex_guardian_context::REQUEST_TOKENS_BOUNDARIES; use codex_guardian_context::REQUEST_TOKENS_METRIC; use codex_protocol::protocol::TruncationPolicy; @@ -24,9 +25,10 @@ pub(crate) fn observe(telemetry: &SessionTelemetry, request: &ResponsesApiReques .saturating_add(metadata); // The assembled input already includes inherited history and the current // review. Do not report a guessed old/new split after context injection. - telemetry.histogram( + telemetry.histogram_with_boundaries( REQUEST_TOKENS_METRIC, i64::try_from(total).unwrap_or(i64::MAX), + REQUEST_TOKENS_BOUNDARIES, &[("target", "sync"), ("component", "total")], ); total diff --git a/codex-rs/core/src/session/extension_metrics.rs b/codex-rs/core/src/session/extension_metrics.rs index 13547c6bc2..bb4b271080 100644 --- a/codex-rs/core/src/session/extension_metrics.rs +++ b/codex-rs/core/src/session/extension_metrics.rs @@ -15,6 +15,17 @@ impl ExtensionMetrics for SessionTelemetryExtensionMetrics { fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) { self.session_telemetry.histogram(name, value, tags); } + + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.session_telemetry + .histogram_with_boundaries(name, value, boundaries, tags); + } } pub(crate) fn from_session_telemetry( diff --git a/codex-rs/ext/extension-api/src/capabilities/metrics.rs b/codex-rs/ext/extension-api/src/capabilities/metrics.rs index b61b3d9616..67f395b480 100644 --- a/codex-rs/ext/extension-api/src/capabilities/metrics.rs +++ b/codex-rs/ext/extension-api/src/capabilities/metrics.rs @@ -8,4 +8,14 @@ pub trait ExtensionMetrics: Send + Sync { /// Records one histogram sample with optional extension-provided tags. fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]); + + /// Records a histogram with explicit buckets, preserving host attribution. + /// All callers of the same metric name must use the same boundaries. + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + boundaries: &[f64], + tags: &[(&str, &str)], + ); } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index 54bdf39a79..6860cf52cb 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -603,9 +603,10 @@ impl GuardianV2Extension { if let Some(metrics) = metrics.as_deref() { for (section, cost) in transcript.section_costs() { for (measurement, value) in cost.measurements() { - metrics.histogram( + metrics.histogram_with_boundaries( codex_guardian_context::SECTION_COST_METRIC, i64::try_from(value).unwrap_or(i64::MAX), + codex_guardian_context::SECTION_COST_BOUNDARIES, &[ ("target", "async"), ("section", section), diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index c41025a8f6..85c130050c 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -358,6 +358,16 @@ impl RecordingMetrics { } impl ExtensionMetrics for RecordingMetrics { + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + _boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.histogram(name, value, tags); + } + fn counter(&self, name: &str, inc: i64, tags: &[(&str, &str)]) { self.0.lock().unwrap().push(RecordedMetric::Counter( name.to_owned(), diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/sampler.rs b/codex-rs/ext/guardian-v2/src/async_scorer/sampler.rs index 3474c2471b..2f68b94e49 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/sampler.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/sampler.rs @@ -302,9 +302,10 @@ impl LunaSampler { ("new_input", total_tokens), ("total", total_tokens), ] { - metrics.histogram( + metrics.histogram_with_boundaries( codex_guardian_context::REQUEST_TOKENS_METRIC, i64::try_from(tokens).unwrap_or(i64::MAX), + codex_guardian_context::REQUEST_TOKENS_BOUNDARIES, &[("target", "async"), ("component", component)], ); } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs index c822c51944..6dc562c92c 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs @@ -239,6 +239,16 @@ type RecordedMetric = (String, i64, Vec<(String, String)>); struct RecordingMetrics(Mutex>); impl ExtensionMetrics for RecordingMetrics { + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + _boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.histogram(name, value, tags); + } + fn counter(&self, _name: &str, _inc: i64, _tags: &[(&str, &str)]) {} fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) { diff --git a/codex-rs/ext/skills/src/extension_tests.rs b/codex-rs/ext/skills/src/extension_tests.rs index 604ceaebf4..118812bc97 100644 --- a/codex-rs/ext/skills/src/extension_tests.rs +++ b/codex-rs/ext/skills/src/extension_tests.rs @@ -15,6 +15,16 @@ struct RecordingMetrics { } impl ExtensionMetrics for RecordingMetrics { + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + _boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.histogram(name, value, tags); + } + fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) { panic!("unexpected counter: {name}"); } diff --git a/codex-rs/ext/skills/src/render_observability_tests.rs b/codex-rs/ext/skills/src/render_observability_tests.rs index a3715f5891..f18ee7d08d 100644 --- a/codex-rs/ext/skills/src/render_observability_tests.rs +++ b/codex-rs/ext/skills/src/render_observability_tests.rs @@ -18,6 +18,16 @@ struct RecordingMetrics { } impl ExtensionMetrics for RecordingMetrics { + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + _boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.histogram(name, value, tags); + } + fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) { panic!("unexpected counter: {name}"); } diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index cf6857bb7d..ac9cc37781 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -2503,6 +2503,16 @@ impl RecordingMetrics { } impl ExtensionMetrics for RecordingMetrics { + fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + _boundaries: &[f64], + tags: &[(&str, &str)], + ) { + self.histogram(name, value, tags); + } + fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) { panic!("unexpected counter: {name}"); } diff --git a/codex-rs/guardian-context/src/budget.rs b/codex-rs/guardian-context/src/budget.rs index a400c1967b..9a9d0d79a9 100644 --- a/codex-rs/guardian-context/src/budget.rs +++ b/codex-rs/guardian-context/src/budget.rs @@ -14,6 +14,41 @@ use crate::composition::SectionOutput; pub const SECTION_COST_METRIC: &str = "codex.guardian.context.section_cost"; pub const REQUEST_TOKENS_METRIC: &str = "codex.guardian.context.request_tokens"; +/// Fixed across reviewers and models so complete-request distributions align. +pub const REQUEST_TOKENS_BOUNDARIES: &[f64] = &[ + 1_000.0, + 4_000.0, + 16_000.0, + 32_000.0, + 64_000.0, + 128_000.0, + 256_000.0, + 400_000.0, + 512_000.0, + 1_000_000.0, + 2_000_000.0, +]; +/// One shared scale for the section metric's count, token and byte measurements. +/// Buckets belong to the metric name, not its measurement tag. +pub const SECTION_COST_BOUNDARIES: &[f64] = &[ + 0.0, + 1.0, + 2.0, + 4.0, + 8.0, + 16.0, + 64.0, + 256.0, + 1_024.0, + 4_096.0, + 16_384.0, + 65_536.0, + 262_144.0, + 1_048_576.0, + 4_194_304.0, + 8_388_608.0, + 16_777_216.0, +]; // A conservative reservation matching the existing original-image patch ceiling. const IMAGE_TOKEN_RESERVATION: usize = 10_000; /// Conservative input ceiling when the model catalog has no authoritative window. diff --git a/codex-rs/guardian-context/src/lib.rs b/codex-rs/guardian-context/src/lib.rs index df76d79600..0ba280b870 100644 --- a/codex-rs/guardian-context/src/lib.rs +++ b/codex-rs/guardian-context/src/lib.rs @@ -55,8 +55,10 @@ pub(crate) use enforcement::Retention; mod budget; mod composition; pub use budget::DEFAULT_MAX_INPUT_TOKENS; +pub use budget::REQUEST_TOKENS_BOUNDARIES; pub use budget::REQUEST_TOKENS_METRIC; pub use budget::RequestBudget; +pub use budget::SECTION_COST_BOUNDARIES; pub use budget::SECTION_COST_METRIC; pub use budget::SectionCost; pub use budget::effective_input_token_limit; diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index ddcd7fb2bb..5e8f7cebee 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -205,6 +205,29 @@ impl SessionTelemetry { } } + /// Records a histogram with explicit buckets and the usual session attribution. + /// All callers of the same metric name must use the same boundaries. + pub fn histogram_with_boundaries( + &self, + name: &str, + value: i64, + boundaries: &[f64], + tags: &[(&str, &str)], + ) { + let res: MetricsResult<()> = (|| { + let Some(metrics) = &self.metrics else { + return Ok(()); + }; + + let tags = self.tags_with_metadata(tags)?; + metrics.histogram_with_boundaries(name, value, boundaries, &tags) + })(); + + if let Err(e) = res { + tracing::warn!("metrics histogram [{name}] failed: {e}"); + } + } + pub fn record_duration(&self, name: &str, duration: Duration, tags: &[(&str, &str)]) { let res: MetricsResult<()> = (|| { let Some(metrics) = &self.metrics else { diff --git a/codex-rs/otel/tests/suite/manager_metrics.rs b/codex-rs/otel/tests/suite/manager_metrics.rs index e13de3f432..1a7a719d50 100644 --- a/codex-rs/otel/tests/suite/manager_metrics.rs +++ b/codex-rs/otel/tests/suite/manager_metrics.rs @@ -1,6 +1,7 @@ use crate::harness::attributes_to_map; use crate::harness::build_metrics_with_defaults; use crate::harness::find_metric; +use crate::harness::histogram_data; use crate::harness::latest_metrics; use codex_otel::PLUGIN_INSTALL_ELICITATION_SENT_METRIC; use codex_otel::PLUGIN_INSTALL_SUGGESTION_METRIC; @@ -37,9 +38,26 @@ fn manager_attaches_metadata_tags_to_metrics() -> Result<()> { /*inc*/ 1, &[("source", "tui")], ); + for tokens in [32_000, 256_000] { + manager.histogram_with_boundaries( + "codex.request_tokens", + tokens, + &[16_000.0, 128_000.0, 512_000.0], + &[("source", "tui")], + ); + } manager.shutdown_metrics()?; let resource_metrics = latest_metrics(&exporter); + assert_eq!( + histogram_data(&resource_metrics, "codex.request_tokens"), + ( + vec![16_000.0, 128_000.0, 512_000.0], + vec![0, 1, 1, 0], + 288_000.0, + 2, + ) + ); let metric = find_metric(&resource_metrics, "codex.session_started").expect("counter metric missing"); let attrs = match metric.data() {