[codex] Initialize exec-server OpenTelemetry at startup#25019
[codex] Initialize exec-server OpenTelemetry at startup#25019starr-openai wants to merge 4 commits into
Conversation
c8ab9d0 to
4282a6e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4282a6e28b
ℹ️ 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".
| crate::protocol::FS_READ_DIRECTORY_METHOD => crate::protocol::FS_READ_DIRECTORY_METHOD, | ||
| crate::protocol::FS_REMOVE_METHOD => crate::protocol::FS_REMOVE_METHOD, | ||
| crate::protocol::FS_COPY_METHOD => crate::protocol::FS_COPY_METHOD, | ||
| crate::protocol::HTTP_REQUEST_METHOD => crate::protocol::HTTP_REQUEST_METHOD, |
There was a problem hiding this comment.
there is no better way to do this?
| .telemetry | ||
| .remote_registration_completed("success", registration_started_at.elapsed()); | ||
| drop(registration_span); | ||
| eprintln!("codex exec-server remote environment registered"); |
There was a problem hiding this comment.
This regresses the existing raw stderr diagnostic while the PR only needs OTEL export to be sanitized. The base code printed the registered environment_id to local stderr; removing it here makes multi-environment remote exec-server debugging materially worse. Please keep the raw eprintln! detail and sanitize only the OTEL/log-only event payloads.
| JsonRpcConnectionEvent::Message(message) => match message { | ||
| codex_app_server_protocol::JSONRPCMessage::Request(request) => { | ||
| let method = request_method(request.method.as_str()); | ||
| let request_span = request_span(method); |
There was a problem hiding this comment.
This new request span should attach the inbound JSON-RPC trace context before exporting. JSONRPCRequest already carries trace, and the adjacent app-server path calls set_parent_from_w3c_trace_context before emitting server spans. As written, exec-server request spans are orphaned from caller traces, so the new OTEL request spans do not actually join the distributed trace. Please parent request_span from request.trace here; ideally the exec-server client should also stop sending trace: None for outbound requests.
| .open(&instructions_path) | ||
| .await | ||
| { | ||
| Ok(mut file) => { |
There was a problem hiding this comment.
This PR still carries unrelated behavioral fixes outside the exec-server OTEL surface: this ad-hoc memory seed flush and the RMCP ETXTBSY retry in codex-rs/rmcp-client/src/program_resolver.rs. Please split those into separate PRs so review and rollback scope stay tied to the stated feature.
| emit_remote_otel_event!( | ||
| INFO, | ||
| "codex.exec_server.remote_environment_registered", | ||
| "codex exec-server remote environment registered" |
There was a problem hiding this comment.
can we move this code out to a function
| ); | ||
| config | ||
| .telemetry | ||
| .remote_websocket_reconnect("connect_failed"); |
There was a problem hiding this comment.
the event logging is definitely cluttering up the main logic of this function - is there any way to simplify this?
d0265f4 to
db44d86
Compare
ff23345 to
4b17850
Compare
4b17850 to
a107f7b
Compare
db44d86 to
bf4be25
Compare
Codex Cloud Agents (CCA) couldn't complete this review. The original Codex Review is unaffected. |
a848c75 to
7530ed6
Compare
Summary
codex exec-serverstartup, with a root runtime span.Stack
Validation
just test -p codex-exec-server --lib(107 passed)just test -p codex-cli --test exec_server(2 passed)