mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
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
This commit is contained in:
@@ -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<BTreeMap<(String, String), Option<f64>>>,
|
||||
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
|
||||
|
||||
@@ -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()?;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)],
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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)],
|
||||
);
|
||||
}
|
||||
|
||||
@@ -239,6 +239,16 @@ type RecordedMetric = (String, i64, Vec<(String, String)>);
|
||||
struct RecordingMetrics(Mutex<Vec<RecordedMetric>>);
|
||||
|
||||
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)]) {
|
||||
|
||||
@@ -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}");
|
||||
}
|
||||
|
||||
@@ -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}");
|
||||
}
|
||||
|
||||
@@ -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}");
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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() {
|
||||
|
||||
Reference in New Issue
Block a user