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.
This commit is contained in:
Ahmed Ibrahim authored and GitHub committed 2026-06-24 21:08:39 -07:00
1 parent a74771340d
commit cef5444a80
4 files changed
+101 -106

No files matched your search

+25 -30
View File
@@ -92,9 +92,7 @@ use url::Url;
mod telemetry; mod telemetry;
use telemetry::MCP_CALL_COUNT_METRIC;
use telemetry::McpCallMetricOutcome; use telemetry::McpCallMetricOutcome;
use telemetry::McpErrorCodeSource;
use telemetry::emit_mcp_call_metrics; use telemetry::emit_mcp_call_metrics;
use telemetry::mcp_call_metric_outcome; use telemetry::mcp_call_metric_outcome;
use telemetry::record_mcp_call_outcome_span_telemetry; use telemetry::record_mcp_call_outcome_span_telemetry;
@@ -182,6 +180,13 @@ pub(crate) async fn handle_mcp_tool_call(
.await .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 { if server == CODEX_APPS_MCP_SERVER_NAME && !app_tool_policy.enabled {
let result = notify_mcp_tool_call_skip( let result = notify_mcp_tool_call_skip(
sess.as_ref(), sess.as_ref(),
@@ -194,10 +199,15 @@ pub(crate) async fn handle_mcp_tool_call(
) )
.await; .await;
let status = if result.is_ok() { "ok" } else { "error" }; let status = if result.is_ok() { "ok" } else { "error" };
turn_context.session_telemetry.counter( let outcome = McpCallMetricOutcome::from_status(status);
MCP_CALL_COUNT_METRIC, emit_mcp_call_metrics(
/*inc*/ 1, turn_context.as_ref(),
&[("status", status)], &outcome,
&server,
&tool_name,
connector_id.as_deref(),
connector_name.as_deref(),
/*duration*/ None,
); );
return HandledMcpToolCall { return HandledMcpToolCall {
result: CallToolResult::from_result(result), 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())), .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( notify_mcp_tool_call_started(
sess.as_ref(), sess.as_ref(),
turn_context.as_ref(), turn_context.as_ref(),
@@ -279,6 +282,7 @@ pub(crate) async fn handle_mcp_tool_call(
emit_mcp_call_metrics( emit_mcp_call_metrics(
turn_context.as_ref(), turn_context.as_ref(),
&outcome, &outcome,
&server,
&tool_name, &tool_name,
connector_id.as_deref(), connector_id.as_deref(),
connector_name.as_deref(), connector_name.as_deref(),
@@ -348,17 +352,11 @@ async fn handle_approved_mcp_tool_call(
let arguments_value = invocation.arguments.clone(); let arguments_value = invocation.arguments.clone();
let connector_id = metadata.and_then(|metadata| metadata.connector_id.as_deref()); 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 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 mcp_connection_manager = sess.services.mcp_connection_manager.load_full();
let server_origin = mcp_connection_manager mcp_connection_manager
.server_origin(&server) .server_origin(&server)
.map(str::to_string); .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)
}; };
let start = Instant::now(); let start = Instant::now();
@@ -392,7 +390,7 @@ async fn handle_approved_mcp_tool_call(
.await .await
} }
.await; .await;
record_mcp_result_span_telemetry(&Span::current(), &result, error_code_source); record_mcp_result_span_telemetry(&Span::current(), &result);
result result
} }
.instrument(mcp_tool_call_span( .instrument(mcp_tool_call_span(
@@ -424,10 +422,11 @@ async fn handle_approved_mcp_tool_call(
.await; .await;
maybe_track_codex_app_used(sess, turn_context, &server, &tool_name).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( emit_mcp_call_metrics(
turn_context, turn_context,
&outcome, &outcome,
&server,
&tool_name, &tool_name,
connector_id, connector_id,
connector_name, connector_name,
@@ -501,12 +500,8 @@ fn record_server_fields(span: &Span, url: Option<&str>) {
} }
} }
fn record_mcp_result_span_telemetry( fn record_mcp_result_span_telemetry(span: &Span, result: &Result<CallToolResult, String>) {
span: &Span, record_mcp_call_outcome_span_telemetry(span, result);
result: &Result<CallToolResult, String>,
error_code_source: McpErrorCodeSource,
) {
record_mcp_call_outcome_span_telemetry(span, result, error_code_source);
let Some(span_telemetry) = result let Some(span_telemetry) = result
.as_ref() .as_ref()
+38 -41
View File
@@ -7,7 +7,7 @@ use codex_protocol::mcp::CallToolResult;
use serde_json::Value as JsonValue; use serde_json::Value as JsonValue;
use tracing::Span; 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_DURATION_METRIC: &str = "codex.mcp.call.duration_ms";
const MCP_CALL_ERROR_COUNT_METRIC: &str = "codex.mcp.call.error"; const MCP_CALL_ERROR_COUNT_METRIC: &str = "codex.mcp.call.error";
// No CallToolResult was received. This includes request setup, transport, timeout, protocol, and // 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( pub(super) fn emit_mcp_call_metrics(
turn_context: &TurnContext, turn_context: &TurnContext,
outcome: &McpCallMetricOutcome, outcome: &McpCallMetricOutcome,
server_name: &str,
tool_name: &str, tool_name: &str,
connector_id: Option<&str>, connector_id: Option<&str>,
connector_name: Option<&str>, connector_name: Option<&str>,
duration: Option<Duration>, duration: Option<Duration>,
) { ) {
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 let tag_refs: Vec<(&str, &str)> = tags
.iter() .iter()
.map(|(key, value)| (*key, value.as_str())) .map(|(key, value)| (*key, value.as_str()))
@@ -87,12 +88,14 @@ pub(super) fn emit_mcp_call_metrics(
fn mcp_call_metric_tags( fn mcp_call_metric_tags(
status: &str, status: &str,
server_name: &str,
tool_name: &str, tool_name: &str,
connector_id: Option<&str>, connector_id: Option<&str>,
connector_name: Option<&str>, connector_name: Option<&str>,
) -> Vec<(&'static str, String)> { ) -> Vec<(&'static str, String)> {
let mut tags = vec![ let mut tags = vec![
("status", sanitize_metric_tag_value(status)), ("status", sanitize_metric_tag_value(status)),
("server", sanitize_metric_tag_value(server_name)),
("tool", sanitize_metric_tag_value(tool_name)), ("tool", sanitize_metric_tag_value(tool_name)),
]; ];
if let Some(connector_id) = connector_id.filter(|connector_id| !connector_id.is_empty()) { 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( pub(super) fn mcp_call_metric_outcome(
result: &Result<CallToolResult, String>, result: &Result<CallToolResult, String>,
error_code_source: McpErrorCodeSource,
) -> McpCallMetricOutcome { ) -> McpCallMetricOutcome {
match result { match result {
Ok(result) if result.is_error.unwrap_or(false) => { Ok(result) if result.is_error.unwrap_or(false) => {
let error_code = if error_code_source == McpErrorCodeSource::HostedPluginService { let error_code = result
result .structured_content
.structured_content .as_ref()
.as_ref() .and_then(JsonValue::as_object)
.and_then(JsonValue::as_object) .and_then(|structured_content| structured_content.get("error_code"))
.and_then(|structured_content| structured_content.get("error_code")) .and_then(JsonValue::as_str)
.and_then(JsonValue::as_str) .filter(|error_code| !error_code.is_empty())
.filter(|error_code| !error_code.is_empty()) .or_else(|| {
.or_else(|| { result
result .meta
.meta .as_ref()
.as_ref() .and_then(JsonValue::as_object)
.and_then(JsonValue::as_object) .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY))
.and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) .and_then(JsonValue::as_object)
.and_then(JsonValue::as_object) .and_then(|codex_apps| codex_apps.get("connector_auth_failure"))
.and_then(|codex_apps| codex_apps.get("connector_auth_failure")) .and_then(JsonValue::as_object)
.and_then(JsonValue::as_object) .filter(|auth_failure| {
.filter(|auth_failure| { auth_failure
auth_failure .get("is_auth_failure")
.get("is_auth_failure") .and_then(JsonValue::as_bool)
.and_then(JsonValue::as_bool) == Some(true)
== Some(true) })
}) .and_then(|auth_failure| auth_failure.get("error_code"))
.and_then(|auth_failure| auth_failure.get("error_code")) .and_then(JsonValue::as_str)
.and_then(JsonValue::as_str) .filter(|error_code| !error_code.is_empty())
.filter(|error_code| !error_code.is_empty()) });
})
} else {
None
};
let error_code: String = error_code let error_code: String = error_code
.unwrap_or(MCP_CALL_ERROR_CODE_UNKNOWN) .unwrap_or(MCP_CALL_ERROR_CODE_UNKNOWN)
.chars() .chars()
@@ -164,9 +162,8 @@ pub(super) fn mcp_call_metric_outcome(
pub(super) fn record_mcp_call_outcome_span_telemetry( pub(super) fn record_mcp_call_outcome_span_telemetry(
span: &Span, span: &Span,
result: &Result<CallToolResult, String>, result: &Result<CallToolResult, String>,
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 { let (Some(error_type), Some(error_code)) = (outcome.error_type, outcome.error_code) else {
return; return;
}; };
@@ -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] #[test]
fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() { fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() {
assert_eq!( assert_eq!(
mcp_call_metric_outcome( mcp_call_metric_outcome(&Ok(metric_call_tool_result(
&Ok(metric_call_tool_result( /*is_error*/ false, /*structured_content*/ None,
/*is_error*/ false, /*structured_content*/ None, )),),
)),
McpErrorCodeSource::HostedPluginService,
),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "ok", status: "ok",
error_type: None, error_type: None,
@@ -29,13 +46,10 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() {
} }
); );
assert_eq!( assert_eq!(
mcp_call_metric_outcome( mcp_call_metric_outcome(&Ok(metric_call_tool_result(
&Ok(metric_call_tool_result( /*is_error*/ true,
/*is_error*/ true, Some(serde_json::json!({"error_code": "RATE_LIMITED"})),
Some(serde_json::json!({"error_code": "RATE_LIMITED"})), )),),
)),
McpErrorCodeSource::HostedPluginService,
),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "error", status: "error",
error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), 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!( assert_eq!(
mcp_call_metric_outcome( mcp_call_metric_outcome(&Err("connection closed".to_string())),
&Err("connection closed".to_string()),
McpErrorCodeSource::HostedPluginService,
),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "error", status: "error",
error_type: Some(MCP_CALL_ERROR_TYPE_MCP_REQUEST), error_type: Some(MCP_CALL_ERROR_TYPE_MCP_REQUEST),
@@ -56,24 +67,24 @@ fn mcp_call_metric_outcome_distinguishes_request_and_tool_errors() {
} }
#[test] #[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( let result = Ok(metric_call_tool_result(
/*is_error*/ true, /*is_error*/ true,
Some(serde_json::json!({"error_code": "arbitrary-user-value"})), Some(serde_json::json!({"error_code": "arbitrary-user-value"})),
)); ));
assert_eq!( assert_eq!(
mcp_call_metric_outcome(&result, McpErrorCodeSource::Untrusted), mcp_call_metric_outcome(&result),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "error", status: "error",
error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), 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] #[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 { let result = CallToolResult {
content: Vec::new(), content: Vec::new(),
structured_content: None, structured_content: None,
@@ -89,7 +100,7 @@ fn mcp_call_metric_outcome_reads_hosted_auth_error_code_from_meta() {
}; };
assert_eq!( assert_eq!(
mcp_call_metric_outcome(&Ok(result), McpErrorCodeSource::HostedPluginService), mcp_call_metric_outcome(&Ok(result)),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "error", status: "error",
error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), 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] #[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 raw_error_code = format!("BAD CODE {}", "x".repeat(300));
let result = Ok(metric_call_tool_result( let result = Ok(metric_call_tool_result(
/*is_error*/ true, /*is_error*/ true,
@@ -107,7 +118,7 @@ fn mcp_call_metric_outcome_bounds_and_sanitizes_hosted_error_code() {
)); ));
assert_eq!( assert_eq!(
mcp_call_metric_outcome(&result, McpErrorCodeSource::HostedPluginService), mcp_call_metric_outcome(&result),
McpCallMetricOutcome { McpCallMetricOutcome {
status: "error", status: "error",
error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT), error_type: Some(MCP_CALL_ERROR_TYPE_TOOL_RESULT),
+3 -11
View File
@@ -463,7 +463,7 @@ async fn mcp_tool_call_span_records_expected_fields() {
} }
#[tokio::test] #[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<Vec<u8>> = let buffer: &'static std::sync::Mutex<Vec<u8>> =
Box::leak(Box::new(std::sync::Mutex::new(Vec::new()))); Box::leak(Box::new(std::sync::Mutex::new(Vec::new())));
let subscriber = tracing_subscriber::fmt() let subscriber = tracing_subscriber::fmt()
@@ -496,11 +496,7 @@ async fn mcp_tool_call_span_records_error_type_and_hosted_error_code() {
); );
async { async {
record_mcp_result_span_telemetry( record_mcp_result_span_telemetry(&Span::current(), &result);
&Span::current(),
&result,
McpErrorCodeSource::HostedPluginService,
);
} }
.instrument(span) .instrument(span)
.await; .await;
@@ -548,11 +544,7 @@ async fn mcp_result_telemetry_span_logs(meta: Option<serde_json::Value>) -> Stri
); );
async { async {
record_mcp_result_span_telemetry( record_mcp_result_span_telemetry(&Span::current(), &result);
&Span::current(),
&result,
McpErrorCodeSource::Untrusted,
);
} }
.instrument(span) .instrument(span)
.await; .await;