mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[codex-analytics] ingest server requests and responses (#17088)
## Why Codex analytics needs a typed seam for app-server-originated request/response traffic so future tool-approval analytics can consume those facts without adding bespoke callsite tracking each time. Server responses arrive as JSON-RPC `id + result` payloads, so analytics has to reconstruct the matching typed response from the original typed request while that request context still exists in app-server. This also puts analytics on the app-server outbound path, which needs to avoid keeping the runtime alive during shutdown. The final ownership fix keeps the normal strong auth-manager retention in analytics and makes the external-auth refresh bridge hold a weak back-reference to `OutgoingMessageSender`, breaking the runtime cycle at the bridge boundary instead of exposing retention policy through the analytics client API. ## What changed - Adds typed `ServerRequest` and `ServerResponse` analytics facts, plus `AnalyticsEventsClient::track_server_request` and `track_server_response`. - Renames the existing client-side facts to `ClientRequest` and `ClientResponse` so reducers can distinguish client-to-server traffic from server-to-client traffic. - Adds `ServerRequest::response_from_result`, allowing a stored typed request to decode the matching typed server response from a raw JSON-RPC result payload. - Threads `AnalyticsEventsClient` through `OutgoingMessageSender` and records targeted server requests, replayed targeted requests, and matching targeted responses with the responding connection id needed for correlation. - Intentionally leaves broadcast server requests/responses out of analytics for now because the current model is per connection, while broadcasts fan one logical request out across multiple connections. - Breaks the app-server shutdown cycle by storing `Weak<OutgoingMessageSender>` in `ExternalAuthRefreshBridge` and upgrading it only when an external-auth refresh is actually requested. - Keeps reducer ingestion of the new server-side facts as no-ops for now; this PR is plumbing for later tool-approval analytics work. ## Verification - `cargo test -p codex-analytics` - `cargo test -p codex-app-server outgoing_message::tests::` - Covers typed-response reconstruction plus the targeted, replayed, broadcast-exclusion, and response-attribution analytics paths. ## Follow-up This PR intentionally stops at ingestion plumbing, so `ServerRequest` and `ServerResponse` facts are still reducer no-ops. Once a follow-up PR adds real downstream analytics output for those facts: - replace the temporary pre-reducer observation seam with reducer tests for the emitted event shape; - add end-to-end coverage in `app-server/tests/suite/v2/analytics.rs` for the real app-server workflow and captured analytics payload; - remove the temporary sender-level observer tests added here in favor of the real-output coverage above. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/17088). * #18748 * #18747 * #17090 * #17089 * #20241 * #20239 * __->__ #17088
This commit is contained in:
@@ -426,7 +426,7 @@ async fn ingest_rejected_turn_steer(
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(4),
|
||||
request: Box::new(sample_turn_steer_request(
|
||||
@@ -486,7 +486,7 @@ async fn ingest_turn_prerequisites(
|
||||
ingest_initialize(reducer, out).await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_thread_start_response(
|
||||
"thread-2", /*ephemeral*/ false, "gpt-5",
|
||||
@@ -500,7 +500,7 @@ async fn ingest_turn_prerequisites(
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(3),
|
||||
request: Box::new(sample_turn_start_request("thread-2", /*request_id*/ 3)),
|
||||
@@ -510,7 +510,7 @@ async fn ingest_turn_prerequisites(
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_turn_start_response("turn-2", /*request_id*/ 3)),
|
||||
},
|
||||
@@ -862,7 +862,7 @@ async fn initialize_caches_client_and_thread_lifecycle_publishes_once_initialize
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_thread_start_response(
|
||||
"thread-no-client",
|
||||
@@ -906,7 +906,7 @@ async fn initialize_caches_client_and_thread_lifecycle_publishes_once_initialize
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_thread_resume_response(
|
||||
"thread-1", /*ephemeral*/ true, "gpt-5",
|
||||
@@ -986,7 +986,7 @@ async fn compaction_event_ingests_custom_fact() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_thread_resume_response_with_source(
|
||||
"thread-1",
|
||||
@@ -1097,7 +1097,7 @@ async fn guardian_review_event_ingests_custom_fact_with_optional_target_item() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_thread_start_response(
|
||||
"thread-guardian",
|
||||
@@ -1867,7 +1867,7 @@ async fn accepted_turn_steer_emits_expected_event() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(4),
|
||||
request: Box::new(sample_turn_steer_request(
|
||||
@@ -1879,7 +1879,7 @@ async fn accepted_turn_steer_emits_expected_event() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_turn_steer_response("turn-2", /*request_id*/ 4)),
|
||||
},
|
||||
@@ -2021,7 +2021,7 @@ async fn turn_start_error_response_discards_pending_start_request() {
|
||||
ingest_initialize(&mut reducer, &mut out).await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(3),
|
||||
request: Box::new(sample_turn_start_request("thread-2", /*request_id*/ 3)),
|
||||
@@ -2045,7 +2045,7 @@ async fn turn_start_error_response_discards_pending_start_request() {
|
||||
// failed turn/start request and attach request-scoped connection metadata.
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_turn_start_response("turn-2", /*request_id*/ 3)),
|
||||
},
|
||||
@@ -2162,7 +2162,7 @@ async fn accepted_steers_increment_turn_steer_count() {
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(4),
|
||||
request: Box::new(sample_turn_steer_request(
|
||||
@@ -2174,7 +2174,7 @@ async fn accepted_steers_increment_turn_steer_count() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_turn_steer_response("turn-2", /*request_id*/ 4)),
|
||||
},
|
||||
@@ -2184,7 +2184,7 @@ async fn accepted_steers_increment_turn_steer_count() {
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(5),
|
||||
request: Box::new(sample_turn_steer_request(
|
||||
@@ -2208,7 +2208,7 @@ async fn accepted_steers_increment_turn_steer_count() {
|
||||
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id: 7,
|
||||
request_id: RequestId::Integer(6),
|
||||
request: Box::new(sample_turn_steer_request(
|
||||
@@ -2220,7 +2220,7 @@ async fn accepted_steers_increment_turn_steer_count() {
|
||||
.await;
|
||||
reducer
|
||||
.ingest(
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id: 7,
|
||||
response: Box::new(sample_turn_steer_response("turn-2", /*request_id*/ 6)),
|
||||
},
|
||||
|
||||
@@ -27,6 +27,8 @@ use codex_app_server_protocol::InitializeParams;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
use codex_app_server_protocol::ServerResponse;
|
||||
use codex_login::AuthManager;
|
||||
use codex_login::default_client::create_client;
|
||||
use codex_plugin::PluginTelemetryMetadata;
|
||||
@@ -49,8 +51,7 @@ pub(crate) struct AnalyticsEventsQueue {
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct AnalyticsEventsClient {
|
||||
queue: AnalyticsEventsQueue,
|
||||
analytics_enabled: Option<bool>,
|
||||
queue: Option<AnalyticsEventsQueue>,
|
||||
}
|
||||
|
||||
impl AnalyticsEventsQueue {
|
||||
@@ -119,11 +120,15 @@ impl AnalyticsEventsClient {
|
||||
analytics_enabled: Option<bool>,
|
||||
) -> Self {
|
||||
Self {
|
||||
queue: AnalyticsEventsQueue::new(Arc::clone(&auth_manager), base_url),
|
||||
analytics_enabled,
|
||||
queue: (analytics_enabled != Some(false))
|
||||
.then(|| AnalyticsEventsQueue::new(Arc::clone(&auth_manager), base_url)),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn disabled() -> Self {
|
||||
Self { queue: None }
|
||||
}
|
||||
|
||||
pub fn track_skill_invocations(
|
||||
&self,
|
||||
tracking: TrackEventsContext,
|
||||
@@ -182,7 +187,7 @@ impl AnalyticsEventsClient {
|
||||
}
|
||||
|
||||
pub fn track_request(&self, connection_id: u64, request_id: RequestId, request: ClientRequest) {
|
||||
self.record_fact(AnalyticsFact::Request {
|
||||
self.record_fact(AnalyticsFact::ClientRequest {
|
||||
connection_id,
|
||||
request_id,
|
||||
request: Box::new(request),
|
||||
@@ -190,7 +195,10 @@ impl AnalyticsEventsClient {
|
||||
}
|
||||
|
||||
pub fn track_app_used(&self, tracking: TrackEventsContext, app: AppInvocation) {
|
||||
if !self.queue.should_enqueue_app_used(&tracking, &app) {
|
||||
let Some(queue) = self.queue.as_ref() else {
|
||||
return;
|
||||
};
|
||||
if !queue.should_enqueue_app_used(&tracking, &app) {
|
||||
return;
|
||||
}
|
||||
self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::AppUsed(
|
||||
@@ -205,7 +213,10 @@ impl AnalyticsEventsClient {
|
||||
}
|
||||
|
||||
pub fn track_plugin_used(&self, tracking: TrackEventsContext, plugin: PluginTelemetryMetadata) {
|
||||
if !self.queue.should_enqueue_plugin_used(&tracking, &plugin) {
|
||||
let Some(queue) = self.queue.as_ref() else {
|
||||
return;
|
||||
};
|
||||
if !queue.should_enqueue_plugin_used(&tracking, &plugin) {
|
||||
return;
|
||||
}
|
||||
self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::PluginUsed(
|
||||
@@ -268,14 +279,13 @@ impl AnalyticsEventsClient {
|
||||
}
|
||||
|
||||
pub(crate) fn record_fact(&self, input: AnalyticsFact) {
|
||||
if self.analytics_enabled == Some(false) {
|
||||
return;
|
||||
if let Some(queue) = self.queue.as_ref() {
|
||||
queue.try_send(input);
|
||||
}
|
||||
self.queue.try_send(input);
|
||||
}
|
||||
|
||||
pub fn track_response(&self, connection_id: u64, response: ClientResponse) {
|
||||
self.record_fact(AnalyticsFact::Response {
|
||||
self.record_fact(AnalyticsFact::ClientResponse {
|
||||
connection_id,
|
||||
response: Box::new(response),
|
||||
});
|
||||
@@ -299,6 +309,19 @@ impl AnalyticsEventsClient {
|
||||
pub fn track_notification(&self, notification: ServerNotification) {
|
||||
self.record_fact(AnalyticsFact::Notification(Box::new(notification)));
|
||||
}
|
||||
|
||||
pub fn track_server_request(&self, connection_id: u64, request: ServerRequest) {
|
||||
self.record_fact(AnalyticsFact::ServerRequest {
|
||||
connection_id,
|
||||
request: Box::new(request),
|
||||
});
|
||||
}
|
||||
|
||||
pub fn track_server_response(&self, response: ServerResponse) {
|
||||
self.record_fact(AnalyticsFact::ServerResponse {
|
||||
response: Box::new(response),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
async fn send_track_events(
|
||||
|
||||
@@ -7,6 +7,8 @@ use codex_app_server_protocol::InitializeParams;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
use codex_app_server_protocol::ServerResponse;
|
||||
use codex_plugin::PluginTelemetryMetadata;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
@@ -272,12 +274,12 @@ pub(crate) enum AnalyticsFact {
|
||||
runtime: CodexRuntimeMetadata,
|
||||
rpc_transport: AppServerRpcTransport,
|
||||
},
|
||||
Request {
|
||||
ClientRequest {
|
||||
connection_id: u64,
|
||||
request_id: RequestId,
|
||||
request: Box<ClientRequest>,
|
||||
},
|
||||
Response {
|
||||
ClientResponse {
|
||||
connection_id: u64,
|
||||
response: Box<ClientResponse>,
|
||||
},
|
||||
@@ -287,6 +289,13 @@ pub(crate) enum AnalyticsFact {
|
||||
error: JSONRPCErrorError,
|
||||
error_type: Option<AnalyticsJsonRpcError>,
|
||||
},
|
||||
ServerRequest {
|
||||
connection_id: u64,
|
||||
request: Box<ServerRequest>,
|
||||
},
|
||||
ServerResponse {
|
||||
response: Box<ServerResponse>,
|
||||
},
|
||||
Notification(Box<ServerNotification>),
|
||||
// Facts that do not naturally exist on the app-server protocol surface, or
|
||||
// would require non-trivial protocol reshaping on this branch.
|
||||
|
||||
@@ -172,14 +172,14 @@ impl AnalyticsReducer {
|
||||
rpc_transport,
|
||||
);
|
||||
}
|
||||
AnalyticsFact::Request {
|
||||
AnalyticsFact::ClientRequest {
|
||||
connection_id,
|
||||
request_id,
|
||||
request,
|
||||
} => {
|
||||
self.ingest_request(connection_id, request_id, *request);
|
||||
}
|
||||
AnalyticsFact::Response {
|
||||
AnalyticsFact::ClientResponse {
|
||||
connection_id,
|
||||
response,
|
||||
} => {
|
||||
@@ -196,6 +196,13 @@ impl AnalyticsReducer {
|
||||
AnalyticsFact::Notification(notification) => {
|
||||
self.ingest_notification(*notification, out);
|
||||
}
|
||||
AnalyticsFact::ServerRequest {
|
||||
connection_id: _connection_id,
|
||||
request: _request,
|
||||
} => {}
|
||||
AnalyticsFact::ServerResponse {
|
||||
response: _response,
|
||||
} => {}
|
||||
AnalyticsFact::Custom(input) => match input {
|
||||
CustomAnalyticsFact::SubAgentThreadStarted(input) => {
|
||||
self.ingest_subagent_thread_started(input, out);
|
||||
|
||||
Reference in New Issue
Block a user