mirror of
https://github.com/openai/codex.git
synced 2026-10-24 18:18:41 +00:00
Record per-tool incremental update and description size metrics (#54133)
## What changed Tag `codex.tools.incremental_updates` counters with the qualified tool name alongside the action. Continue excluding initial catalogs and namespace headers, while counting each removed namespace member. Add `codex.tools.spec_description_bytes` to measure UTF-8 description bytes when an existing tool definition is replaced. Skip definitions without descriptions and use logarithmic histogram boundaries up to 1 MiB. ## Testing Extend namespace transition tests to check per-tool counts and description sizes. Add scenario assertions for replacement metrics with incremental tool updates and their absence on the non-incremental request path. GitOrigin-RevId: e20e6d81f21a867b285df1f0da005c2806dd03c6
This commit is contained in:
@@ -2,7 +2,7 @@
|
||||
//! Missing state starts a fresh catalog; this section does not migrate legacy history.
|
||||
//! One developer notice precedes each incremental namespace batch; catalog hashes are unchanged.
|
||||
//! Whole namespace removals subsume their members in the removal notice.
|
||||
//! Metrics count rendered tool redefinitions, excluding namespace headers and initial catalogs.
|
||||
//! Metrics count rendered tool deltas and measure replacement description bytes.
|
||||
|
||||
use super::PreviousSectionState;
|
||||
use super::SectionTransition;
|
||||
@@ -13,6 +13,8 @@ use crate::context::ContextualUserFragment;
|
||||
use crate::context::DeveloperInstructions;
|
||||
use codex_extension_api::ExtensionMetrics;
|
||||
use codex_otel::TOOL_INCREMENTAL_UPDATES_METRIC;
|
||||
use codex_otel::TOOL_SPEC_DESCRIPTION_BYTES_METRIC;
|
||||
use codex_otel::TOOL_SPEC_DESCRIPTION_BYTES_METRIC_BUCKETS;
|
||||
use codex_prompts::ResolvedIncrementalToolMessages;
|
||||
use codex_protocol::error::CodexErrorDetails;
|
||||
use codex_protocol::error::Result;
|
||||
@@ -89,47 +91,37 @@ fn definition_name(definition: &Value) -> &str {
|
||||
.unwrap_or_else(|| definition["type"].as_str().expect("serialized tool type"))
|
||||
}
|
||||
|
||||
/// Accumulates callable changes during rendering without retaining tool definitions.
|
||||
#[derive(Default)]
|
||||
struct ToolUpdateMetrics {
|
||||
count_added: usize,
|
||||
count_removed: usize,
|
||||
count_schema_changed: usize,
|
||||
}
|
||||
fn record_changed_tool_metrics(
|
||||
name: &str,
|
||||
definition: &Value,
|
||||
previous: Option<&BTreeMap<String, WorldStateHash>>,
|
||||
metrics: Option<&dyn ExtensionMetrics>,
|
||||
) {
|
||||
const ACTION_TAG: &str = "action";
|
||||
const TOOL_TAG: &str = "tool";
|
||||
|
||||
impl ToolUpdateMetrics {
|
||||
fn count_changed_tool(
|
||||
&mut self,
|
||||
name: &str,
|
||||
previous: Option<&BTreeMap<String, WorldStateHash>>,
|
||||
) {
|
||||
if let Some(previous) = previous {
|
||||
if previous.contains_key(name) {
|
||||
// The existing hash covers the entire definition, including descriptions.
|
||||
self.count_schema_changed += 1;
|
||||
} else {
|
||||
self.count_added += 1;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn record(self, metrics: Option<&dyn ExtensionMetrics>) {
|
||||
let Some(metrics) = metrics else {
|
||||
return;
|
||||
};
|
||||
for (action, count) in [
|
||||
(ACTION_ADDED, self.count_added),
|
||||
(ACTION_REMOVED, self.count_removed),
|
||||
(ACTION_SCHEMA_CHANGED, self.count_schema_changed),
|
||||
] {
|
||||
if count > 0 {
|
||||
metrics.counter(
|
||||
TOOL_INCREMENTAL_UPDATES_METRIC,
|
||||
i64::try_from(count).unwrap_or(i64::MAX),
|
||||
&[("action", action)],
|
||||
);
|
||||
}
|
||||
}
|
||||
let (Some(previous), Some(metrics)) = (previous, metrics) else {
|
||||
return;
|
||||
};
|
||||
let action = if previous.contains_key(name) {
|
||||
ACTION_SCHEMA_CHANGED
|
||||
} else {
|
||||
ACTION_ADDED
|
||||
};
|
||||
metrics.counter(
|
||||
TOOL_INCREMENTAL_UPDATES_METRIC,
|
||||
/*inc*/ 1,
|
||||
&[(ACTION_TAG, action), (TOOL_TAG, name)],
|
||||
);
|
||||
if action == ACTION_SCHEMA_CHANGED
|
||||
&& let Some(description) = definition["description"].as_str()
|
||||
{
|
||||
metrics.histogram_with_boundaries(
|
||||
TOOL_SPEC_DESCRIPTION_BYTES_METRIC,
|
||||
i64::try_from(description.len()).unwrap_or(i64::MAX),
|
||||
TOOL_SPEC_DESCRIPTION_BYTES_METRIC_BUCKETS.as_slice(),
|
||||
&[(TOOL_TAG, name)],
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -147,14 +139,18 @@ impl WorldStateSection for TopLevelToolsState {
|
||||
};
|
||||
let changed =
|
||||
|name: &str| previous.and_then(|previous| previous.get(name)) != self.hashes.get(name);
|
||||
let mut counts = ToolUpdateMetrics::default();
|
||||
let mut namespace_updates = Vec::new();
|
||||
let mut tools = Vec::new();
|
||||
for definition in &self.definitions {
|
||||
let name = definition_name(definition);
|
||||
let Some(members) = definition["tools"].as_array() else {
|
||||
if changed(name) {
|
||||
counts.count_changed_tool(name, previous);
|
||||
record_changed_tool_metrics(
|
||||
name,
|
||||
definition,
|
||||
previous,
|
||||
self.metrics.as_deref(),
|
||||
);
|
||||
tools.push(definition.clone());
|
||||
}
|
||||
continue;
|
||||
@@ -163,7 +159,12 @@ impl WorldStateSection for TopLevelToolsState {
|
||||
for tool in members {
|
||||
let tool_name = format!("{name}.{}", definition_name(tool));
|
||||
if changed(&tool_name) {
|
||||
counts.count_changed_tool(&tool_name, previous);
|
||||
record_changed_tool_metrics(
|
||||
&tool_name,
|
||||
tool,
|
||||
previous,
|
||||
self.metrics.as_deref(),
|
||||
);
|
||||
changed_members.push(tool.clone());
|
||||
}
|
||||
}
|
||||
@@ -229,9 +230,15 @@ impl WorldStateSection for TopLevelToolsState {
|
||||
.keys()
|
||||
.filter(|key| !self.hashes.contains_key(*key))
|
||||
.filter(|key| !namespaces.contains(key.as_str()))
|
||||
.inspect(|_| {
|
||||
.inspect(|name| {
|
||||
// Count members even when the notice collapses their removed namespace.
|
||||
counts.count_removed += 1;
|
||||
if let Some(metrics) = self.metrics.as_deref() {
|
||||
metrics.counter(
|
||||
TOOL_INCREMENTAL_UPDATES_METRIC,
|
||||
/*inc*/ 1,
|
||||
&[("action", ACTION_REMOVED), ("tool", name.as_str())],
|
||||
);
|
||||
}
|
||||
})
|
||||
.filter(|key| {
|
||||
!key.split_once('.')
|
||||
@@ -250,7 +257,6 @@ impl WorldStateSection for TopLevelToolsState {
|
||||
);
|
||||
}
|
||||
}
|
||||
counts.record(self.metrics.as_deref());
|
||||
// Persist the empty map too: it is a known empty catalog, not missing state.
|
||||
(Some(self.hashes.clone()), updates)
|
||||
}
|
||||
|
||||
@@ -198,19 +198,59 @@ async fn snapshots_namespace_transitions() {
|
||||
.attributes()
|
||||
.map(|attribute| (attribute.key.as_str(), attribute.value.as_str().to_string()))
|
||||
.collect::<BTreeMap<_, _>>();
|
||||
assert_eq!(tags.len(), 1);
|
||||
(tags["action"].clone(), point.value())
|
||||
assert_eq!(tags.len(), 2);
|
||||
(
|
||||
(tags["action"].clone(), tags["tool"].clone()),
|
||||
point.value(),
|
||||
)
|
||||
})
|
||||
.collect::<BTreeMap<_, _>>();
|
||||
// Initial/full snapshots and unchanged catalogs contribute nothing; namespace
|
||||
// headers do not count, and removing a whole namespace counts each member.
|
||||
assert_eq!(
|
||||
counts,
|
||||
BTreeMap::from([
|
||||
("added".to_string(), 6),
|
||||
("removed".to_string(), 11),
|
||||
("schema_changed".to_string(), 1),
|
||||
])
|
||||
[
|
||||
("added", "functions.added", 1),
|
||||
("added", "functions.changed", 1),
|
||||
("added", "functions.removed", 1),
|
||||
("added", "functions.restored", 1),
|
||||
("added", "functions.unchanged", 2),
|
||||
("removed", "functions.added", 3),
|
||||
("removed", "functions.changed", 3),
|
||||
("removed", "functions.removed", 1),
|
||||
("removed", "functions.unchanged", 2),
|
||||
("removed", "other.lookup", 2),
|
||||
("schema_changed", "functions.changed", 1),
|
||||
]
|
||||
.into_iter()
|
||||
.map(|(action, tool, count)| ((action.to_string(), tool.to_string()), count))
|
||||
.collect()
|
||||
);
|
||||
let histogram = snapshot
|
||||
.scope_metrics()
|
||||
.flat_map(ScopeMetrics::metrics)
|
||||
.find(|metric| metric.name() == TOOL_SPEC_DESCRIPTION_BYTES_METRIC)
|
||||
.expect("tool description byte histogram");
|
||||
let AggregatedMetrics::F64(MetricData::Histogram(histogram)) = histogram.data() else {
|
||||
panic!("tool description bytes must be a histogram");
|
||||
};
|
||||
assert_eq!(
|
||||
histogram
|
||||
.data_points()
|
||||
.map(|point| (
|
||||
point
|
||||
.attributes()
|
||||
.map(|tag| (tag.key.as_str(), tag.value.to_string()))
|
||||
.collect::<Vec<_>>(),
|
||||
point.count(),
|
||||
point.sum(),
|
||||
))
|
||||
.collect::<Vec<_>>(),
|
||||
vec![(
|
||||
vec![("tool", "functions.changed".to_string())],
|
||||
1,
|
||||
"Look up a value.".len() as f64
|
||||
)],
|
||||
);
|
||||
metrics.shutdown().unwrap();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user