From cef5444a80ac5a94d435ab780fba5d6f433c504f Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 24 Jun 2026 21:08:39 -0700 Subject: [PATCH] Report MCP error codes with server attribution (#29969) ## Why MCP error-code telemetry special-cased Codex Apps: its reported error codes were retained, while codes from every other MCP server were replaced with `unknown`. Error reporting should behave consistently for every MCP server. The server name already identifies where an error came from, so telemetry does not need a separate Codex Apps classification. This follows up on [#28976](https://github.com/openai/codex/pull/28976), which introduced MCP error-code telemetry. ## What changed - Add the MCP server name to call, duration, and error metrics. - Retain bounded, sanitized tool error codes from every MCP server. - Remove `McpErrorCodeSource` and the Codex Apps ownership lookup from telemetry collection. - Use the same metric-tagging path for blocked, rejected, and executed MCP calls. ## Test plan - Verify the complete metric tag set includes the sanitized MCP server name. - Verify error codes from ordinary MCP servers are retained, bounded, and sanitized. - Preserve coverage for request failures, tool-result failures, nested auth failures, and span attributes. --- codex-rs/core/src/mcp_tool_call.rs | 55 ++++++------- codex-rs/core/src/mcp_tool_call/telemetry.rs | 79 +++++++++---------- .../core/src/mcp_tool_call/telemetry_tests.rs | 59 ++++++++------ codex-rs/core/src/mcp_tool_call_tests.rs | 14 +--- 4 files changed, 101 insertions(+), 106 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index fea6e45e9..df2a375e3 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -92,9 +92,7 @@ use url::Url; mod telemetry; -use telemetry::MCP_CALL_COUNT_METRIC; use telemetry::McpCallMetricOutcome; -use telemetry::McpErrorCodeSource; use telemetry::emit_mcp_call_metrics; use telemetry::mcp_call_metric_outcome; use telemetry::record_mcp_call_outcome_span_telemetry; @@ -182,6 +180,13 @@ pub(crate) async fn handle_mcp_tool_call( .await }; + let connector_id = metadata + .as_ref() + .and_then(|metadata| metadata.connector_id.clone()); + let connector_name = metadata + .as_ref() + .and_then(|metadata| metadata.connector_name.clone()); + if server == CODEX_APPS_MCP_SERVER_NAME && !app_tool_policy.enabled { let result = notify_mcp_tool_call_skip( sess.as_ref(), @@ -194,10 +199,15 @@ pub(crate) async fn handle_mcp_tool_call( ) .await; let status = if result.is_ok() { "ok" } else { "error" }; - turn_context.session_telemetry.counter( - MCP_CALL_COUNT_METRIC, - /*inc*/ 1, - &[("status", status)], + let outcome = McpCallMetricOutcome::from_status(status); + emit_mcp_call_metrics( + turn_context.as_ref(), + &outcome, + &server, + &tool_name, + connector_id.as_deref(), + connector_name.as_deref(), + /*duration*/ None, ); return HandledMcpToolCall { result: CallToolResult::from_result(result), @@ -205,13 +215,6 @@ pub(crate) async fn handle_mcp_tool_call( .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), }; } - let connector_id = metadata - .as_ref() - .and_then(|metadata| metadata.connector_id.clone()); - let connector_name = metadata - .as_ref() - .and_then(|metadata| metadata.connector_name.clone()); - notify_mcp_tool_call_started( sess.as_ref(), turn_context.as_ref(), @@ -279,6 +282,7 @@ pub(crate) async fn handle_mcp_tool_call( emit_mcp_call_metrics( turn_context.as_ref(), &outcome, + &server, &tool_name, connector_id.as_deref(), connector_name.as_deref(), @@ -348,17 +352,11 @@ async fn handle_approved_mcp_tool_call( let arguments_value = invocation.arguments.clone(); let connector_id = metadata.and_then(|metadata| metadata.connector_id.as_deref()); let connector_name = metadata.and_then(|metadata| metadata.connector_name.as_deref()); - let (server_origin, error_code_source) = { + let server_origin = { let mcp_connection_manager = sess.services.mcp_connection_manager.load_full(); - let server_origin = mcp_connection_manager + mcp_connection_manager .server_origin(&server) - .map(str::to_string); - let error_code_source = if mcp_connection_manager.is_host_owned_codex_apps_server(&server) { - McpErrorCodeSource::HostedPluginService - } else { - McpErrorCodeSource::Untrusted - }; - (server_origin, error_code_source) + .map(str::to_string) }; let start = Instant::now(); @@ -392,7 +390,7 @@ async fn handle_approved_mcp_tool_call( .await } .await; - record_mcp_result_span_telemetry(&Span::current(), &result, error_code_source); + record_mcp_result_span_telemetry(&Span::current(), &result); result } .instrument(mcp_tool_call_span( @@ -424,10 +422,11 @@ async fn handle_approved_mcp_tool_call( .await; maybe_track_codex_app_used(sess, turn_context, &server, &tool_name).await; - let outcome = mcp_call_metric_outcome(&result, error_code_source); + let outcome = mcp_call_metric_outcome(&result); emit_mcp_call_metrics( turn_context, &outcome, + &server, &tool_name, connector_id, connector_name, @@ -501,12 +500,8 @@ fn record_server_fields(span: &Span, url: Option<&str>) { } } -fn record_mcp_result_span_telemetry( - span: &Span, - result: &Result, - error_code_source: McpErrorCodeSource, -) { - record_mcp_call_outcome_span_telemetry(span, result, error_code_source); +fn record_mcp_result_span_telemetry(span: &Span, result: &Result) { + record_mcp_call_outcome_span_telemetry(span, result); let Some(span_telemetry) = result .as_ref() diff --git a/codex-rs/core/src/mcp_tool_call/telemetry.rs b/codex-rs/core/src/mcp_tool_call/telemetry.rs index 9d9f4ec46..5f3600264 100644 --- a/codex-rs/core/src/mcp_tool_call/telemetry.rs +++ b/codex-rs/core/src/mcp_tool_call/telemetry.rs @@ -7,7 +7,7 @@ use codex_protocol::mcp::CallToolResult; use serde_json::Value as JsonValue; use tracing::Span; -pub(super) const MCP_CALL_COUNT_METRIC: &str = "codex.mcp.call"; +const MCP_CALL_COUNT_METRIC: &str = "codex.mcp.call"; const MCP_CALL_DURATION_METRIC: &str = "codex.mcp.call.duration_ms"; const MCP_CALL_ERROR_COUNT_METRIC: &str = "codex.mcp.call.error"; // No CallToolResult was received. This includes request setup, transport, timeout, protocol, and @@ -37,21 +37,22 @@ impl McpCallMetricOutcome { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(super) enum McpErrorCodeSource { - HostedPluginService, - Untrusted, -} - pub(super) fn emit_mcp_call_metrics( turn_context: &TurnContext, outcome: &McpCallMetricOutcome, + server_name: &str, tool_name: &str, connector_id: Option<&str>, connector_name: Option<&str>, duration: Option, ) { - let tags = mcp_call_metric_tags(outcome.status, tool_name, connector_id, connector_name); + let tags = mcp_call_metric_tags( + outcome.status, + server_name, + tool_name, + connector_id, + connector_name, + ); let tag_refs: Vec<(&str, &str)> = tags .iter() .map(|(key, value)| (*key, value.as_str())) @@ -87,12 +88,14 @@ pub(super) fn emit_mcp_call_metrics( fn mcp_call_metric_tags( status: &str, + server_name: &str, tool_name: &str, connector_id: Option<&str>, connector_name: Option<&str>, ) -> Vec<(&'static str, String)> { let mut tags = vec![ ("status", sanitize_metric_tag_value(status)), + ("server", sanitize_metric_tag_value(server_name)), ("tool", sanitize_metric_tag_value(tool_name)), ]; if let Some(connector_id) = connector_id.filter(|connector_id| !connector_id.is_empty()) { @@ -107,40 +110,35 @@ fn mcp_call_metric_tags( pub(super) fn mcp_call_metric_outcome( result: &Result, - error_code_source: McpErrorCodeSource, ) -> McpCallMetricOutcome { match result { Ok(result) if result.is_error.unwrap_or(false) => { - let error_code = if error_code_source == McpErrorCodeSource::HostedPluginService { - result - .structured_content - .as_ref() - .and_then(JsonValue::as_object) - .and_then(|structured_content| structured_content.get("error_code")) - .and_then(JsonValue::as_str) - .filter(|error_code| !error_code.is_empty()) - .or_else(|| { - result - .meta - .as_ref() - .and_then(JsonValue::as_object) - .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) - .and_then(JsonValue::as_object) - .and_then(|codex_apps| codex_apps.get("connector_auth_failure")) - .and_then(JsonValue::as_object) - .filter(|auth_failure| { - auth_failure - .get("is_auth_failure") - .and_then(JsonValue::as_bool) - == Some(true) - }) - .and_then(|auth_failure| auth_failure.get("error_code")) - .and_then(JsonValue::as_str) - .filter(|error_code| !error_code.is_empty()) - }) - } else { - None - }; + let error_code = result + .structured_content + .as_ref() + .and_then(JsonValue::as_object) + .and_then(|structured_content| structured_content.get("error_code")) + .and_then(JsonValue::as_str) + .filter(|error_code| !error_code.is_empty()) + .or_else(|| { + result + .meta + .as_ref() + .and_then(JsonValue::as_object) + .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) + .and_then(JsonValue::as_object) + .and_then(|codex_apps| codex_apps.get("connector_auth_failure")) + .and_then(JsonValue::as_object) + .filter(|auth_failure| { + auth_failure + .get("is_auth_failure") + .and_then(JsonValue::as_bool) + == Some(true) + }) + .and_then(|auth_failure| auth_failure.get("error_code")) + .and_then(JsonValue::as_str) + .filter(|error_code| !error_code.is_empty()) + }); let error_code: String = error_code .unwrap_or(MCP_CALL_ERROR_CODE_UNKNOWN) .chars() @@ -164,9 +162,8 @@ pub(super) fn mcp_call_metric_outcome( pub(super) fn record_mcp_call_outcome_span_telemetry( span: &Span, result: &Result, - error_code_source: McpErrorCodeSource, ) { - let outcome = mcp_call_metric_outcome(result, error_code_source); + let outcome = mcp_call_metric_outcome(result); let (Some(error_type), Some(error_code)) = (outcome.error_type, outcome.error_code) else { return; }; diff --git a/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs b/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs index b11df28d1..8b17d5f6b 100644 --- a/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs +++ b/codex-rs/core/src/mcp_tool_call/telemetry_tests.rs @@ -13,15 +13,32 @@ fn metric_call_tool_result( } } +#[test] +fn mcp_call_metric_tags_include_server_name() { + assert_eq!( + mcp_call_metric_tags( + "error", + "docs server", + "search docs", + Some("connector/docs"), + Some("Docs connector"), + ), + vec![ + ("status", "error".to_string()), + ("server", "docs_server".to_string()), + ("tool", "search_docs".to_string()), + ("connector_id", "connector/docs".to_string()), + ("connector_name", "Docs_connector".to_string()), + ], + ); +} + #[test] fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { assert_eq!( - mcp_call_metric_outcome( - &Ok(metric_call_tool_result( - /*is_error*/ false, /*structured_content*/ None, - )), - McpErrorCodeSource::HostedPluginService, - ), + mcp_call_metric_outcome(&Ok(metric_call_tool_result( + /*is_error*/ false, /*structured_content*/ None, + )),), McpCallMetricOutcome { status: "ok", error_type: None, @@ -29,13 +46,10 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { } ); assert_eq!( - mcp_call_metric_outcome( - &Ok(metric_call_tool_result( - /*is_error*/ true, - Some(serde_json::json!({"error_code": "RATE_LIMITED"})), - )), - McpErrorCodeSource::HostedPluginService, - ), + mcp_call_metric_outcome(&Ok(metric_call_tool_result( + /*is_error*/ true, + Some(serde_json::json!({"error_code": "RATE_LIMITED"})), + )),), McpCallMetricOutcome { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), @@ -43,10 +57,7 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { } ); assert_eq!( - mcp_call_metric_outcome( - &Err("connection closed".to_string()), - McpErrorCodeSource::HostedPluginService, - ), + mcp_call_metric_outcome(&Err("connection closed".to_string())), McpCallMetricOutcome { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_MCP_REQUEST), @@ -56,24 +67,24 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { } #[test] -fn mcp_call_metric_outcome_ignores_untrusted_tool_error_codes() { +fn mcp_call_metric_outcome_reports_server_tool_error_codes() { let result = Ok(metric_call_tool_result( /*is_error*/ true, Some(serde_json::json!({"error_code": "arbitrary-user-value"})), )); assert_eq!( - mcp_call_metric_outcome(&result, McpErrorCodeSource::Untrusted), + mcp_call_metric_outcome(&result), McpCallMetricOutcome { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), - error_code: Some(MCP_CALL_ERROR_CODE_UNKNOWN.to_string()), + error_code: Some("arbitrary-user-value".to_string()), } ); } #[test] -fn mcp_call_metric_outcome_reads_hosted_auth_error_code_from_meta() { +fn mcp_call_metric_outcome_reads_auth_error_code_from_meta() { let result = CallToolResult { content: Vec::new(), structured_content: None, @@ -89,7 +100,7 @@ fn mcp_call_metric_outcome_reads_hosted_auth_error_code_from_meta() { }; assert_eq!( - mcp_call_metric_outcome(&Ok(result), McpErrorCodeSource::HostedPluginService), + mcp_call_metric_outcome(&Ok(result)), McpCallMetricOutcome { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), @@ -99,7 +110,7 @@ fn mcp_call_metric_outcome_reads_hosted_auth_error_code_from_meta() { } #[test] -fn mcp_call_metric_outcome_bounds_and_sanitizes_hosted_error_code() { +fn mcp_call_metric_outcome_bounds_and_sanitizes_error_code() { let raw_error_code = format!("BAD CODE {}", "x".repeat(300)); let result = Ok(metric_call_tool_result( /*is_error*/ true, @@ -107,7 +118,7 @@ fn mcp_call_metric_outcome_bounds_and_sanitizes_hosted_error_code() { )); assert_eq!( - mcp_call_metric_outcome(&result, McpErrorCodeSource::HostedPluginService), + mcp_call_metric_outcome(&result), McpCallMetricOutcome { status: "error", error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 1be7f53b6..387987c9f 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -463,7 +463,7 @@ async fn mcp_tool_call_span_records_expected_fields() { } #[tokio::test] -async fn mcp_tool_call_span_records_error_type_and_hosted_error_code() { +async fn mcp_tool_call_span_records_error_type_and_error_code() { let buffer: &'static std::sync::Mutex> = Box::leak(Box::new(std::sync::Mutex::new(Vec::new()))); let subscriber = tracing_subscriber::fmt() @@ -496,11 +496,7 @@ async fn mcp_tool_call_span_records_error_type_and_hosted_error_code() { ); async { - record_mcp_result_span_telemetry( - &Span::current(), - &result, - McpErrorCodeSource::HostedPluginService, - ); + record_mcp_result_span_telemetry(&Span::current(), &result); } .instrument(span) .await; @@ -548,11 +544,7 @@ async fn mcp_result_telemetry_span_logs(meta: Option) -> Stri ); async { - record_mcp_result_span_telemetry( - &Span::current(), - &result, - McpErrorCodeSource::Untrusted, - ); + record_mcp_result_span_telemetry(&Span::current(), &result); } .instrument(span) .await;