mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[rollout-trace] Include x-request-id in rollout trace. (#20066)
## Why Rollout traces need an identifier that can be used to correlate a Codex inference with upstream Responses API, proxy, and engine logs. The reduced trace model already exposed `upstream_request_id`, but it was being populated from the Responses API `response.id`. That value is useful for `previous_response_id` chaining, but it is not the transport request id that upstream systems key on. This PR separates those concepts so trace consumers can reliably answer both questions: - which Responses API response did this inference produce? - which upstream request handled it? ## Structure The change keeps the upstream request id at the same lifecycle level as the provider stream: - `codex-api` captures the `x-request-id` HTTP response header when the SSE stream is created and exposes it on `ResponseStream`. Fixture and websocket streams set the field to `None` because they do not have that HTTP response header. - `codex-core` carries that stream-level id into `InferenceTraceAttempt` when recording terminal stream outcomes. Completed, failed, cancelled, dropped-stream, and pre-response error paths all record the id when it is available. - `rollout-trace` now records both identifiers in raw terminal inference events and response payloads: `response_id` for the Responses API `response.id`, and `upstream_request_id` for `x-request-id`. - The reducer stores both fields on `InferenceCall`. It also uses `response_id` for `previous_response_id` conversation linking, which removes the old accidental dependency on the misnamed `upstream_request_id` field. - Terminal inference reduction now consumes the full terminal payload (`InferenceCompleted`, `InferenceFailed`, or `InferenceCancelled`) in one place. That keeps status, partial payloads, response ids, and upstream request ids consistent across success, failure, cancellation, and late stream-mapper events. ## Why This Shape `x-request-id` is a property of the HTTP/provider response envelope, not an SSE event. Capturing it once in `codex-api` and plumbing it through terminal trace recording avoids trying to infer the value from stream contents, and it preserves the id even when the stream fails or is cancelled after only partial output. Keeping `response_id` separate from `upstream_request_id` also makes the reduced trace model less surprising: `response_id` remains the conversation-continuation id, while `upstream_request_id` is the operational correlation id for upstream debugging. ## Validation The PR updates trace and reducer coverage for: - reading `x-request-id` from SSE response headers; - storing the true upstream request id on completed inference calls; - preserving upstream request ids for cancelled and late-cancelled inference streams; - keeping `previous_response_id` reconstruction tied to `response_id` rather than transport request ids.
This commit is contained in:
committed by
GitHub
Unverified
parent
10e2a73b3c
commit
89698ad1c3
@@ -287,6 +287,8 @@ pub fn create_text_param_for_request(
|
||||
|
||||
pub struct ResponseStream {
|
||||
pub rx_event: mpsc::Receiver<Result<ResponseEvent, ApiError>>,
|
||||
/// Server-assigned `x-request-id` response header, when present.
|
||||
pub upstream_request_id: Option<String>,
|
||||
}
|
||||
|
||||
impl Stream for ResponseStream {
|
||||
|
||||
@@ -279,7 +279,10 @@ impl ResponsesWebsocketConnection {
|
||||
.instrument(current_span),
|
||||
);
|
||||
|
||||
Ok(ResponseStream { rx_event })
|
||||
Ok(ResponseStream {
|
||||
rx_event,
|
||||
upstream_request_id: None,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -28,6 +28,7 @@ use tracing::trace;
|
||||
|
||||
const X_REASONING_INCLUDED_HEADER: &str = "x-reasoning-included";
|
||||
const OPENAI_MODEL_HEADER: &str = "openai-model";
|
||||
const REQUEST_ID_HEADER: &str = "x-request-id";
|
||||
const TRUSTED_ACCESS_FOR_CYBER_VERIFICATION: &str = "trusted_access_for_cyber";
|
||||
|
||||
/// Streams SSE events from an on-disk fixture for tests.
|
||||
@@ -53,7 +54,10 @@ pub fn stream_from_fixture(
|
||||
idle_timeout,
|
||||
/*telemetry*/ None,
|
||||
));
|
||||
Ok(ResponseStream { rx_event })
|
||||
Ok(ResponseStream {
|
||||
rx_event,
|
||||
upstream_request_id: None,
|
||||
})
|
||||
}
|
||||
|
||||
pub fn spawn_response_stream(
|
||||
@@ -77,6 +81,11 @@ pub fn spawn_response_stream(
|
||||
.headers
|
||||
.get(X_REASONING_INCLUDED_HEADER)
|
||||
.is_some();
|
||||
let upstream_request_id = stream_response
|
||||
.headers
|
||||
.get(REQUEST_ID_HEADER)
|
||||
.and_then(|value| value.to_str().ok())
|
||||
.map(str::to_string);
|
||||
if let Some(turn_state) = turn_state.as_ref()
|
||||
&& let Some(header_value) = stream_response
|
||||
.headers
|
||||
@@ -104,7 +113,10 @@ pub fn spawn_response_stream(
|
||||
process_sse(stream_response.bytes, tx_event, idle_timeout, telemetry).await;
|
||||
});
|
||||
|
||||
ResponseStream { rx_event }
|
||||
ResponseStream {
|
||||
rx_event,
|
||||
upstream_request_id,
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
@@ -1058,8 +1070,9 @@ mod tests {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn spawn_response_stream_emits_server_model_header() {
|
||||
async fn spawn_response_stream_emits_header_events() {
|
||||
let mut headers = HeaderMap::new();
|
||||
headers.insert(REQUEST_ID_HEADER, HeaderValue::from_static("req-1"));
|
||||
headers.insert(
|
||||
OPENAI_MODEL_HEADER,
|
||||
HeaderValue::from_static(CYBER_RESTRICTED_MODEL_FOR_TESTS),
|
||||
@@ -1077,13 +1090,13 @@ mod tests {
|
||||
/*telemetry*/ None,
|
||||
/*turn_state*/ None,
|
||||
);
|
||||
assert_eq!(stream.upstream_request_id.as_deref(), Some("req-1"));
|
||||
let event = stream
|
||||
.rx_event
|
||||
.recv()
|
||||
.await
|
||||
.expect("expected server model event")
|
||||
.expect("expected ok event");
|
||||
|
||||
match event {
|
||||
ResponseEvent::ServerModel(model) => {
|
||||
assert_eq!(model, CYBER_RESTRICTED_MODEL_FOR_TESTS);
|
||||
|
||||
Reference in New Issue
Block a user