[codex] trace MCP startup latency - #28630
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f01296ebe1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| impl AsyncManagedClient { | ||
| // Keep this constructor flat so the startup inputs remain readable at the | ||
| // single call site instead of introducing a one-off params wrapper. | ||
| #[instrument(level = "trace", skip_all, fields(server_name = %server_name))] |
There was a problem hiding this comment.
Because new is synchronous, this span ends after constructing the shared startup future below; the actual client construction/initialize/tool-list work is later polled from McpConnectionManager::new's background JoinSet task. For non-required MCP servers, or whenever that background task polls the shared future first, the new startup spans are detached from the session_init trace, so they still cannot explain which server made startup slow. Capture and instrument the startup future or spawned task with the current span so these definition-level spans remain under the session init trace.
Useful? React with 👍 / 👎.
f01296e to
6bf6afe
Compare
6bf6afe to
5dde64d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
server_nameto server-scoped spans so slow startup work can be attributed to a specific MCP serverWhy
session_init.mcp_manager_initcan occasionally be slow, but its existing coarse span does not identify whether time is spent loading the Codex Apps cache, constructing a client, initializing a transport, or listing tools. These definition-level spans provide that breakdown without changing startup behavior.Validation
just test -p codex-mcp(87 passed)just test -p codex-rmcp-client(86 passed, 2 skipped)