mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex] Record exec-server lifecycle metrics (#27467)
## Summary - Record bounded connection, request, and process lifecycle metrics. - Report active gauges from callbacks on every collection, including delta exports. - Serialize active-count updates so concurrent starts and finishes cannot publish stale values. - Serialize process exit, explicit termination, and shutdown through the process registry so exactly one completion result wins. - Keep the implementation small with single-owner RAII guards and one real OTLP/HTTP integration test using the existing `wiremock` dependency. ## Root cause Process exit and session shutdown previously used cloned completion state. That avoided duplicate emission, but it duplicated lifecycle ownership and made the ordering harder to reason about. The process registry mutex already defines the lifecycle ordering, so the final implementation stores the metric guard and termination flag directly on the process entry. Whichever path claims the entry first owns the completion result. Production metric export uses delta temporality. Event-only synchronous gauge recordings disappear after the next collection when no count changes, so active counts now use observable callbacks that report current state on every collection. The cleanup also removes the constant `result="accepted"` connection tag, redundant route and response assertions, a custom HTTP collector, and fallback initialization machinery that did not add behavior. ## Stack Review and land this stack in order: 1. #27466 — trace exec-server JSON-RPC requests 2. #27467 — record bounded connection, request, and process lifecycle metrics **(this PR)** 3. #27470 — observe remote registration and Noise rendezvous lifecycle ## Validation - `just test -p codex-exec-server --lib` (158 passed) - `just test -p codex-cli --test exec_server` (3 passed) - `just test -p codex-otel observable_gauge_is_collected_on_every_delta_snapshot` (1 passed) - `CARGO_BUILD_JOBS=1 just fix -p codex-otel -p codex-exec-server` - `just fmt` - `git diff --check`
This commit is contained in:
committed by
GitHub
Unverified
parent
8f02973d25
commit
2dec46e30a
@@ -82,7 +82,7 @@ fn test_runtime_paths() -> ExecServerRuntimePaths {
|
||||
|
||||
async fn initialized_handler() -> Arc<ExecServerHandler> {
|
||||
let (outgoing_tx, _outgoing_rx) = mpsc::channel(16);
|
||||
let registry = SessionRegistry::new();
|
||||
let registry = SessionRegistry::new(crate::ExecServerTelemetry::default());
|
||||
let handler = Arc::new(ExecServerHandler::new(
|
||||
registry,
|
||||
RpcNotificationSender::new(outgoing_tx),
|
||||
@@ -160,7 +160,7 @@ async fn terminate_reports_false_after_process_exit() {
|
||||
#[tokio::test]
|
||||
async fn long_poll_read_fails_after_session_resume() {
|
||||
let (first_tx, _first_rx) = mpsc::channel(16);
|
||||
let registry = SessionRegistry::new();
|
||||
let registry = SessionRegistry::new(crate::ExecServerTelemetry::default());
|
||||
let first_handler = Arc::new(ExecServerHandler::new(
|
||||
Arc::clone(®istry),
|
||||
RpcNotificationSender::new(first_tx),
|
||||
@@ -233,7 +233,7 @@ async fn long_poll_read_fails_after_session_resume() {
|
||||
#[tokio::test]
|
||||
async fn active_session_resume_is_rejected() {
|
||||
let (first_tx, _first_rx) = mpsc::channel(16);
|
||||
let registry = SessionRegistry::new();
|
||||
let registry = SessionRegistry::new(crate::ExecServerTelemetry::default());
|
||||
let first_handler = Arc::new(ExecServerHandler::new(
|
||||
Arc::clone(®istry),
|
||||
RpcNotificationSender::new(first_tx),
|
||||
@@ -277,7 +277,7 @@ async fn active_session_resume_is_rejected() {
|
||||
async fn output_and_exit_are_retained_after_notification_receiver_closes() {
|
||||
let (outgoing_tx, outgoing_rx) = mpsc::channel(16);
|
||||
let handler = Arc::new(ExecServerHandler::new(
|
||||
SessionRegistry::new(),
|
||||
SessionRegistry::new(crate::ExecServerTelemetry::default()),
|
||||
RpcNotificationSender::new(outgoing_tx),
|
||||
test_runtime_paths(),
|
||||
));
|
||||
|
||||
@@ -13,6 +13,7 @@ use crate::protocol::TerminateResponse;
|
||||
use crate::protocol::WriteParams;
|
||||
use crate::protocol::WriteResponse;
|
||||
use crate::rpc::RpcNotificationSender;
|
||||
use crate::telemetry::ExecServerTelemetry;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct ProcessHandler {
|
||||
@@ -22,10 +23,11 @@ pub(crate) struct ProcessHandler {
|
||||
impl ProcessHandler {
|
||||
pub(crate) fn new(
|
||||
notifications: RpcNotificationSender,
|
||||
telemetry: ExecServerTelemetry,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
) -> Self {
|
||||
Self {
|
||||
process: LocalProcess::new(notifications, runtime_paths),
|
||||
process: LocalProcess::new(notifications, telemetry, runtime_paths),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use std::sync::Arc;
|
||||
use std::time::Instant;
|
||||
|
||||
use tokio::sync::mpsc;
|
||||
use tracing::Instrument;
|
||||
@@ -17,36 +18,61 @@ use crate::rpc::method_not_found;
|
||||
use crate::server::ExecServerHandler;
|
||||
use crate::server::registry::build_router;
|
||||
use crate::server::session_registry::SessionRegistry;
|
||||
use crate::telemetry::ConnectionTransport;
|
||||
use crate::telemetry::ExecServerTelemetry;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct ConnectionProcessor {
|
||||
session_registry: Arc<SessionRegistry>,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
}
|
||||
|
||||
impl ConnectionProcessor {
|
||||
#[cfg(test)]
|
||||
pub(crate) fn new(runtime_paths: ExecServerRuntimePaths) -> Self {
|
||||
Self::new_with_telemetry(runtime_paths, ExecServerTelemetry::default())
|
||||
}
|
||||
|
||||
pub(crate) fn new_with_telemetry(
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
) -> Self {
|
||||
Self {
|
||||
session_registry: SessionRegistry::new(),
|
||||
session_registry: SessionRegistry::new(telemetry.clone()),
|
||||
runtime_paths,
|
||||
telemetry,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) async fn run_connection(&self, connection: JsonRpcConnection) {
|
||||
pub(crate) async fn run_connection(
|
||||
&self,
|
||||
connection: JsonRpcConnection,
|
||||
transport: ConnectionTransport,
|
||||
) {
|
||||
run_connection(
|
||||
connection,
|
||||
Arc::clone(&self.session_registry),
|
||||
self.runtime_paths.clone(),
|
||||
self.telemetry.clone(),
|
||||
transport,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(crate) async fn shutdown(&self) {
|
||||
self.session_registry.shutdown().await;
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_connection(
|
||||
connection: JsonRpcConnection,
|
||||
session_registry: Arc<SessionRegistry>,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
transport: ConnectionTransport,
|
||||
) {
|
||||
let _connection_metrics = telemetry.connection_started(transport);
|
||||
let router = Arc::new(build_router());
|
||||
let JsonRpcConnection {
|
||||
outgoing_tx: json_outgoing_tx,
|
||||
@@ -101,12 +127,18 @@ async fn run_connection(
|
||||
}
|
||||
JsonRpcConnectionEvent::Message(message) => match message {
|
||||
codex_exec_server_protocol::JSONRPCMessage::Request(request) => {
|
||||
if let Some(route) = router.request_route(request.method.as_str()) {
|
||||
let request_span = request_span(request.method.as_str(), &request);
|
||||
let request_started_at = Instant::now();
|
||||
if let Some((method, route)) = router.request_route(request.method.as_str()) {
|
||||
let request_span = request_span(method, &request);
|
||||
let message = tokio::select! {
|
||||
message = route(Arc::clone(&handler), request).instrument(request_span.clone()) => message,
|
||||
_ = disconnected_rx.changed() => {
|
||||
request_span.record("result", "disconnected");
|
||||
telemetry.request_completed(
|
||||
method,
|
||||
"disconnected",
|
||||
request_started_at.elapsed(),
|
||||
);
|
||||
debug!("exec-server transport disconnected while handling request");
|
||||
break;
|
||||
}
|
||||
@@ -116,11 +148,19 @@ async fn run_connection(
|
||||
&& outgoing_tx.send(message).await.is_err()
|
||||
{
|
||||
request_span.record("result", "disconnected");
|
||||
telemetry.request_completed(
|
||||
method,
|
||||
"disconnected",
|
||||
request_started_at.elapsed(),
|
||||
);
|
||||
break;
|
||||
}
|
||||
request_span.record("result", result);
|
||||
telemetry.request_completed(method, result, request_started_at.elapsed());
|
||||
drop(request_span);
|
||||
} else {
|
||||
let request_span = request_span("unknown", &request);
|
||||
let method = "unknown";
|
||||
let request_span = request_span(method, &request);
|
||||
if outgoing_tx
|
||||
.send(RpcServerOutboundMessage::Error {
|
||||
request_id: request.id,
|
||||
@@ -133,9 +173,15 @@ async fn run_connection(
|
||||
.is_err()
|
||||
{
|
||||
request_span.record("result", "disconnected");
|
||||
telemetry.request_completed(
|
||||
method,
|
||||
"disconnected",
|
||||
request_started_at.elapsed(),
|
||||
);
|
||||
break;
|
||||
}
|
||||
request_span.record("result", "error");
|
||||
telemetry.request_completed(method, "error", request_started_at.elapsed());
|
||||
}
|
||||
}
|
||||
codex_exec_server_protocol::JSONRPCMessage::Notification(notification) => {
|
||||
@@ -330,7 +376,7 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn connection_accepts_pipelined_scalar_requests() {
|
||||
let registry = SessionRegistry::new();
|
||||
let registry = SessionRegistry::new(crate::ExecServerTelemetry::default());
|
||||
let (mut writer, mut lines, task) = spawn_test_connection(registry, "pipelined-scalar");
|
||||
|
||||
send_request(
|
||||
@@ -362,7 +408,7 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn transport_disconnect_detaches_session_during_in_flight_read() {
|
||||
let registry = SessionRegistry::new();
|
||||
let registry = SessionRegistry::new(crate::ExecServerTelemetry::default());
|
||||
let (mut first_writer, mut first_lines, first_task) =
|
||||
spawn_test_connection(Arc::clone(®istry), "first");
|
||||
|
||||
@@ -458,7 +504,13 @@ mod tests {
|
||||
let (server_writer, client_reader) = duplex(1 << 20);
|
||||
let connection =
|
||||
JsonRpcConnection::from_stdio(server_reader, server_writer, label.to_string());
|
||||
let task = tokio::spawn(run_connection(connection, registry, test_runtime_paths()));
|
||||
let task = tokio::spawn(run_connection(
|
||||
connection,
|
||||
registry,
|
||||
test_runtime_paths(),
|
||||
crate::ExecServerTelemetry::default(),
|
||||
crate::telemetry::ConnectionTransport::Stdio,
|
||||
));
|
||||
(client_writer, BufReader::new(client_reader).lines(), task)
|
||||
}
|
||||
|
||||
|
||||
@@ -12,6 +12,7 @@ use crate::rpc::RpcNotificationSender;
|
||||
use crate::rpc::invalid_request;
|
||||
use crate::rpc::session_already_attached;
|
||||
use crate::server::process_handler::ProcessHandler;
|
||||
use crate::telemetry::ExecServerTelemetry;
|
||||
|
||||
#[cfg(test)]
|
||||
const DETACHED_SESSION_TTL: Duration = Duration::from_millis(200);
|
||||
@@ -20,6 +21,7 @@ const DETACHED_SESSION_TTL: Duration = Duration::from_secs(30);
|
||||
|
||||
pub(crate) struct SessionRegistry {
|
||||
sessions: Mutex<HashMap<String, Arc<SessionEntry>>>,
|
||||
telemetry: ExecServerTelemetry,
|
||||
}
|
||||
|
||||
struct SessionEntry {
|
||||
@@ -51,9 +53,10 @@ pub(crate) struct SessionHandle {
|
||||
}
|
||||
|
||||
impl SessionRegistry {
|
||||
pub(crate) fn new() -> Arc<Self> {
|
||||
pub(crate) fn new(telemetry: ExecServerTelemetry) -> Arc<Self> {
|
||||
Arc::new(Self {
|
||||
sessions: Mutex::new(HashMap::new()),
|
||||
telemetry,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -97,7 +100,7 @@ impl SessionRegistry {
|
||||
let session_id = Uuid::new_v4().to_string();
|
||||
let entry = Arc::new(SessionEntry::new(
|
||||
session_id.clone(),
|
||||
ProcessHandler::new(notifications, runtime_paths),
|
||||
ProcessHandler::new(notifications, self.telemetry.clone(), runtime_paths),
|
||||
connection_id,
|
||||
));
|
||||
sessions.insert(session_id, Arc::clone(&entry));
|
||||
@@ -119,6 +122,13 @@ impl SessionRegistry {
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) async fn shutdown(&self) {
|
||||
let sessions = std::mem::take(&mut *self.sessions.lock().await);
|
||||
for entry in sessions.into_values() {
|
||||
entry.process.shutdown().await;
|
||||
}
|
||||
}
|
||||
|
||||
async fn expire_if_detached(&self, session_id: String, connection_id: ConnectionId) {
|
||||
tokio::time::sleep(DETACHED_SESSION_TTL).await;
|
||||
|
||||
@@ -143,6 +153,7 @@ impl Default for SessionRegistry {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
sessions: Mutex::new(HashMap::new()),
|
||||
telemetry: ExecServerTelemetry::default(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -22,8 +22,10 @@ use tracing::info;
|
||||
use tracing::warn;
|
||||
|
||||
use crate::ExecServerRuntimePaths;
|
||||
use crate::ExecServerTelemetry;
|
||||
use crate::connection::JsonRpcConnection;
|
||||
use crate::server::processor::ConnectionProcessor;
|
||||
use crate::telemetry::ConnectionTransport;
|
||||
|
||||
pub const DEFAULT_LISTEN_URL: &str = "ws://127.0.0.1:0";
|
||||
|
||||
@@ -80,49 +82,54 @@ pub(crate) fn parse_listen_url(
|
||||
pub(crate) async fn run_transport(
|
||||
listen_url: &str,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
|
||||
match parse_listen_url(listen_url)? {
|
||||
ExecServerListenTransport::WebSocket(bind_address) => {
|
||||
run_websocket_listener(bind_address, runtime_paths).await
|
||||
run_websocket_listener(bind_address, runtime_paths, telemetry).await
|
||||
}
|
||||
ExecServerListenTransport::Stdio => run_stdio_connection(runtime_paths).await,
|
||||
ExecServerListenTransport::Stdio => run_stdio_connection(runtime_paths, telemetry).await,
|
||||
}
|
||||
}
|
||||
|
||||
async fn run_stdio_connection(
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
|
||||
run_stdio_connection_with_io(io::stdin(), io::stdout(), runtime_paths).await
|
||||
run_stdio_connection_with_io(io::stdin(), io::stdout(), runtime_paths, telemetry).await
|
||||
}
|
||||
|
||||
async fn run_stdio_connection_with_io<R, W>(
|
||||
reader: R,
|
||||
writer: W,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>>
|
||||
where
|
||||
R: AsyncRead + Unpin + Send + 'static,
|
||||
W: AsyncWrite + Unpin + Send + 'static,
|
||||
{
|
||||
let processor = ConnectionProcessor::new(runtime_paths);
|
||||
let processor = ConnectionProcessor::new_with_telemetry(runtime_paths, telemetry);
|
||||
tracing::info!("codex-exec-server listening on stdio");
|
||||
processor
|
||||
.run_connection(JsonRpcConnection::from_stdio(
|
||||
reader,
|
||||
writer,
|
||||
"exec-server stdio".to_string(),
|
||||
))
|
||||
.run_connection(
|
||||
JsonRpcConnection::from_stdio(reader, writer, "exec-server stdio".to_string()),
|
||||
ConnectionTransport::Stdio,
|
||||
)
|
||||
.await;
|
||||
// Stdio serves exactly one connection, so detached sessions cannot be resumed.
|
||||
processor.shutdown().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn run_websocket_listener(
|
||||
bind_address: SocketAddr,
|
||||
runtime_paths: ExecServerRuntimePaths,
|
||||
telemetry: ExecServerTelemetry,
|
||||
) -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
|
||||
let listener = TcpListener::bind(bind_address).await?;
|
||||
let local_addr = listener.local_addr()?;
|
||||
let processor = ConnectionProcessor::new(runtime_paths);
|
||||
let processor = ConnectionProcessor::new_with_telemetry(runtime_paths, telemetry);
|
||||
info!("codex-exec-server listening on ws://{local_addr}");
|
||||
println!("ws://{local_addr}");
|
||||
std::io::stdout().flush()?;
|
||||
@@ -174,10 +181,13 @@ async fn websocket_upgrade_handler(
|
||||
websocket.on_upgrade(move |stream| async move {
|
||||
state
|
||||
.processor
|
||||
.run_connection(JsonRpcConnection::from_axum_websocket(
|
||||
stream,
|
||||
format!("exec-server websocket {peer_addr}"),
|
||||
))
|
||||
.run_connection(
|
||||
JsonRpcConnection::from_axum_websocket(
|
||||
stream,
|
||||
format!("exec-server websocket {peer_addr}"),
|
||||
),
|
||||
ConnectionTransport::WebSocket,
|
||||
)
|
||||
.await;
|
||||
})
|
||||
}
|
||||
|
||||
@@ -61,6 +61,7 @@ async fn stdio_listen_transport_serves_initialize() {
|
||||
server_reader,
|
||||
server_writer,
|
||||
test_runtime_paths(),
|
||||
crate::ExecServerTelemetry::default(),
|
||||
));
|
||||
let mut client_lines = BufReader::new(client_reader).lines();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user