diff --git a/codex-rs/core/src/context/world_state/top_level_tools.rs b/codex-rs/core/src/context/world_state/top_level_tools.rs index 89881fca9f..7c5a1580e5 100644 --- a/codex-rs/core/src/context/world_state/top_level_tools.rs +++ b/codex-rs/core/src/context/world_state/top_level_tools.rs @@ -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>, + 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>, - ) { - 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) } diff --git a/codex-rs/core/src/context/world_state/top_level_tools_tests.rs b/codex-rs/core/src/context/world_state/top_level_tools_tests.rs index 210abce3c9..e3e7fb8c4f 100644 --- a/codex-rs/core/src/context/world_state/top_level_tools_tests.rs +++ b/codex-rs/core/src/context/world_state/top_level_tools_tests.rs @@ -198,19 +198,59 @@ async fn snapshots_namespace_transitions() { .attributes() .map(|attribute| (attribute.key.as_str(), attribute.value.as_str().to_string())) .collect::>(); - assert_eq!(tags.len(), 1); - (tags["action"].clone(), point.value()) + assert_eq!(tags.len(), 2); + ( + (tags["action"].clone(), tags["tool"].clone()), + point.value(), + ) }) .collect::>(); // 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::>(), + point.count(), + point.sum(), + )) + .collect::>(), + vec![( + vec![("tool", "functions.changed".to_string())], + 1, + "Look up a value.".len() as f64 + )], ); metrics.shutdown().unwrap(); } diff --git a/codex-rs/core/tests/suite/scenarios_incremental_tools.rs b/codex-rs/core/tests/suite/scenarios_incremental_tools.rs index b434a16845..347f2d9ac2 100644 --- a/codex-rs/core/tests/suite/scenarios_incremental_tools.rs +++ b/codex-rs/core/tests/suite/scenarios_incremental_tools.rs @@ -2,6 +2,10 @@ use anyhow::Result; use codex_features::Feature; +use codex_otel::MetricsClient; +use codex_otel::MetricsConfig; +use codex_otel::TOOL_INCREMENTAL_UPDATES_METRIC; +use codex_otel::TOOL_SPEC_DESCRIPTION_BYTES_METRIC; use codex_protocol::openai_models::CodeModeToolMessages; use codex_protocol::openai_models::IncrementalToolMessages; use codex_protocol::openai_models::ToolMessage; @@ -17,8 +21,13 @@ use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_mcp_server; +use opentelemetry_sdk::metrics::InMemoryMetricExporter; +use opentelemetry_sdk::metrics::data::AggregatedMetrics; +use opentelemetry_sdk::metrics::data::MetricData; +use opentelemetry_sdk::metrics::data::ScopeMetrics; use pretty_assertions::assert_eq; use serde_json::json; +use std::collections::BTreeMap; use test_case::test_case; use super::super::rmcp_client::remote_aware_environment_id; @@ -52,7 +61,16 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( .collect(), ) .await; - let test = test_codex() + let metrics = MetricsClient::new( + MetricsConfig::in_memory( + "test", + "codex-core", + env!("CARGO_PKG_VERSION"), + InMemoryMetricExporter::default(), + ) + .with_runtime_reader(), + )?; + let mut test = test_codex() .with_model_info_override("gpt-5.4", move |model| { model.use_responses_lite = use_responses_lite; model.tool_mode = Some(ToolMode::CodeMode); @@ -106,6 +124,12 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( }) .build_with_auto_env(&server) .await?; + let mut options = test.start_thread_options().await; + options.thread_extension_init.insert(metrics.clone()); + let thread = test.thread_manager.start_thread(options).await?; + test.codex.shutdown_and_wait().await?; + test.codex = thread.thread; + test.session_configured = thread.session_configured; test.submit_turn("Begin work.").await?; test.submit_turn("Continue with the same tools.").await?; core_test_support::submit_thread_settings( @@ -130,6 +154,74 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( } let requests = mock.requests(); + assert_eq!(requests.len(), if use_responses_lite { 6 } else { 4 }); + let snapshot = metrics.snapshot()?; + let mut counts = BTreeMap::<_, u64>::new(); + let mut bytes = BTreeMap::new(); + for metric in snapshot.scope_metrics().flat_map(ScopeMetrics::metrics) { + match metric.name() { + TOOL_INCREMENTAL_UPDATES_METRIC => { + let AggregatedMetrics::U64(MetricData::Sum(sum)) = metric.data() else { + panic!("tool updates must be a counter"); + }; + for point in sum.data_points() { + let tags = point + .attributes() + .map(|tag| (tag.key.as_str(), tag.value.to_string())) + .collect::>(); + *counts + .entry((tags["action"].clone(), tags["tool"].clone())) + .or_default() += point.value(); + } + } + TOOL_SPEC_DESCRIPTION_BYTES_METRIC => { + let AggregatedMetrics::F64(MetricData::Histogram(histogram)) = metric.data() else { + panic!("tool description bytes must be a histogram"); + }; + for point in histogram.data_points() { + let tool = point + .attributes() + .find(|tag| tag.key.as_str() == "tool") + .expect("tool tag") + .value + .to_string(); + let sample = bytes.entry(tool).or_insert((0, 0.0)); + sample.0 += point.count(); + sample.1 += point.sum(); + } + } + _ => {} + } + } + if use_responses_lite { + let body = requests[2].body_json(); + let exec = body["input"] + .as_array() + .expect("request input") + .iter() + .rev() + .filter(|item| item["type"] == "additional_tools") + .find_map(|item| responses::namespace_child_tool(item, "functions", "exec")) + .expect("changed exec definition"); + let description_bytes = exec["description"] + .as_str() + .expect("exec description") + .len() as f64; + assert_eq!( + counts, + BTreeMap::from([( + ("schema_changed".to_string(), "functions.exec".to_string()), + 1 + )]) + ); + assert_eq!( + bytes, + BTreeMap::from([("functions.exec".to_string(), (1, description_bytes))]) + ); + } else { + assert_eq!(counts, BTreeMap::new()); + assert_eq!(bytes, BTreeMap::new()); + } if use_responses_lite { for request in &requests[..5] { assert_eq!( @@ -161,6 +253,7 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( .include_request_settings(), ) ); + metrics.shutdown()?; Ok(()) } diff --git a/codex-rs/otel/src/metrics/names.rs b/codex-rs/otel/src/metrics/names.rs index f7c6242ee9..ba35b516f1 100644 --- a/codex-rs/otel/src/metrics/names.rs +++ b/codex-rs/otel/src/metrics/names.rs @@ -90,10 +90,16 @@ pub const TOOL_REGISTRATIONS_METRIC_BUCKETS: &[f64] = &[ pub static THREAD_TOOLS_METRIC_BUCKETS: LazyLock<[f64; 511]> = LazyLock::new(|| context_log_buckets(/*max_exponent*/ 15.0)); -/// Rendered incremental tool deltas, tagged by action; excludes initial catalogs. +/// Rendered incremental tool deltas by action and qualified tool; excludes initial catalogs. /// schema_changed includes any definition change, such as description edits. pub const TOOL_INCREMENTAL_UPDATES_METRIC: &str = "codex.tools.incremental_updates"; +/// UTF-8 description bytes on replacement by qualified tool; skips missing descriptions. +pub const TOOL_SPEC_DESCRIPTION_BYTES_METRIC: &str = "codex.tools.spec_description_bytes"; +/// Logarithmic boundaries up to 1 MiB. +pub static TOOL_SPEC_DESCRIPTION_BYTES_METRIC_BUCKETS: LazyLock<[f64; 511]> = + LazyLock::new(|| context_log_buckets(/*max_exponent*/ 20.0)); + pub static THREAD_SKILLS_COUNT_METRIC_BUCKETS: LazyLock<[f64; 513]> = LazyLock::new(|| std::array::from_fn(|index| index as f64));