mirror of
https://github.com/openai/codex.git
synced 2026-11-07 16:35:32 +00:00
Avoid cloning ignored analytics notifications (#36020)
## What changed - Pass server notifications to analytics tracking by reference. - Clone only the notification variants that are recorded as analytics facts. ## Testing - Add unit coverage that verifies a turn diff notification is enqueued and a command output delta notification is ignored. GitOrigin-RevId: 2fa0711dbdcca33dce82c3ef78ee53a3f5a65bd9
This commit is contained in:
1 parent
a5082373f1
commit
88d6c2b2b4
3 files changed
+43
-3
No files matched your search
@@ -582,7 +582,8 @@ impl AnalyticsEventsClient {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn track_notification(&self, notification: ServerNotification) {
|
/// Records analytics-relevant notifications without cloning ignored variants.
|
||||||
|
pub fn track_notification(&self, notification: &ServerNotification) {
|
||||||
if !matches!(
|
if !matches!(
|
||||||
notification,
|
notification,
|
||||||
ServerNotification::TurnStarted(_)
|
ServerNotification::TurnStarted(_)
|
||||||
@@ -595,7 +596,7 @@ impl AnalyticsEventsClient {
|
|||||||
) {
|
) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
self.record_fact(AnalyticsFact::Notification(Box::new(notification)));
|
self.record_fact(AnalyticsFact::Notification(Box::new(notification.clone())));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -42,8 +42,10 @@ use codex_app_server_protocol::ApprovalsReviewer as AppServerApprovalsReviewer;
|
|||||||
use codex_app_server_protocol::AskForApproval as AppServerAskForApproval;
|
use codex_app_server_protocol::AskForApproval as AppServerAskForApproval;
|
||||||
use codex_app_server_protocol::ClientRequest;
|
use codex_app_server_protocol::ClientRequest;
|
||||||
use codex_app_server_protocol::ClientResponsePayload;
|
use codex_app_server_protocol::ClientResponsePayload;
|
||||||
|
use codex_app_server_protocol::CommandExecutionOutputDeltaNotification;
|
||||||
use codex_app_server_protocol::RequestId;
|
use codex_app_server_protocol::RequestId;
|
||||||
use codex_app_server_protocol::SandboxPolicy as AppServerSandboxPolicy;
|
use codex_app_server_protocol::SandboxPolicy as AppServerSandboxPolicy;
|
||||||
|
use codex_app_server_protocol::ServerNotification;
|
||||||
use codex_app_server_protocol::SessionSource as AppServerSessionSource;
|
use codex_app_server_protocol::SessionSource as AppServerSessionSource;
|
||||||
use codex_app_server_protocol::Thread;
|
use codex_app_server_protocol::Thread;
|
||||||
use codex_app_server_protocol::ThreadArchiveParams;
|
use codex_app_server_protocol::ThreadArchiveParams;
|
||||||
@@ -53,6 +55,7 @@ use codex_app_server_protocol::ThreadResumeResponse;
|
|||||||
use codex_app_server_protocol::ThreadStartResponse;
|
use codex_app_server_protocol::ThreadStartResponse;
|
||||||
use codex_app_server_protocol::ThreadStatus as AppServerThreadStatus;
|
use codex_app_server_protocol::ThreadStatus as AppServerThreadStatus;
|
||||||
use codex_app_server_protocol::Turn;
|
use codex_app_server_protocol::Turn;
|
||||||
|
use codex_app_server_protocol::TurnDiffUpdatedNotification;
|
||||||
use codex_app_server_protocol::TurnStartParams;
|
use codex_app_server_protocol::TurnStartParams;
|
||||||
use codex_app_server_protocol::TurnStartResponse;
|
use codex_app_server_protocol::TurnStartResponse;
|
||||||
use codex_app_server_protocol::TurnStatus as AppServerTurnStatus;
|
use codex_app_server_protocol::TurnStatus as AppServerTurnStatus;
|
||||||
@@ -62,6 +65,7 @@ use codex_app_server_protocol::TurnSteerResponse;
|
|||||||
use codex_login::AuthManager;
|
use codex_login::AuthManager;
|
||||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||||
use codex_utils_absolute_path::test_support::test_path_buf;
|
use codex_utils_absolute_path::test_support::test_path_buf;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
use std::collections::HashSet;
|
use std::collections::HashSet;
|
||||||
#[cfg(debug_assertions)]
|
#[cfg(debug_assertions)]
|
||||||
use std::fs;
|
use std::fs;
|
||||||
@@ -682,6 +686,41 @@ async fn flush_is_noop_when_analytics_is_disabled() {
|
|||||||
AnalyticsEventsClient::disabled().flush().await;
|
AnalyticsEventsClient::disabled().flush().await;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn track_notification_only_enqueues_analytics_relevant_notifications() {
|
||||||
|
let (client, mut receiver) = client_with_receiver();
|
||||||
|
let tracked_payload = TurnDiffUpdatedNotification {
|
||||||
|
thread_id: "thread-1".to_string(),
|
||||||
|
turn_id: "turn-1".to_string(),
|
||||||
|
diff: "diff".to_string(),
|
||||||
|
};
|
||||||
|
let tracked_notification = ServerNotification::TurnDiffUpdated(tracked_payload.clone());
|
||||||
|
|
||||||
|
client.track_notification(&tracked_notification);
|
||||||
|
|
||||||
|
let Ok(AnalyticsEventsQueueMessage::Fact(input)) = receiver.try_recv() else {
|
||||||
|
panic!("expected analytics notification");
|
||||||
|
};
|
||||||
|
let AnalyticsFact::Notification(notification) = *input else {
|
||||||
|
panic!("expected analytics notification fact");
|
||||||
|
};
|
||||||
|
let ServerNotification::TurnDiffUpdated(notification) = *notification else {
|
||||||
|
panic!("expected turn diff notification");
|
||||||
|
};
|
||||||
|
assert_eq!(notification, tracked_payload);
|
||||||
|
|
||||||
|
let ignored_notification =
|
||||||
|
ServerNotification::CommandExecutionOutputDelta(CommandExecutionOutputDeltaNotification {
|
||||||
|
thread_id: "thread-1".to_string(),
|
||||||
|
turn_id: "turn-1".to_string(),
|
||||||
|
item_id: "item-1".to_string(),
|
||||||
|
delta: "output".to_string(),
|
||||||
|
});
|
||||||
|
|
||||||
|
client.track_notification(&ignored_notification);
|
||||||
|
assert!(matches!(receiver.try_recv(), Err(TryRecvError::Empty)));
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn track_event_request_batches_only_isolates_accepted_line_fingerprint_events() {
|
fn track_event_request_batches_only_isolates_accepted_line_fingerprint_events() {
|
||||||
let batches = track_event_request_batches(vec![
|
let batches = track_event_request_batches(vec![
|
||||||
|
|||||||
@@ -161,7 +161,7 @@ impl ThreadScopedOutgoingMessageSender {
|
|||||||
pub(crate) async fn send_server_notification(&self, notification: ServerNotification) {
|
pub(crate) async fn send_server_notification(&self, notification: ServerNotification) {
|
||||||
self.outgoing
|
self.outgoing
|
||||||
.analytics_events_client
|
.analytics_events_client
|
||||||
.track_notification(notification.clone());
|
.track_notification(¬ification);
|
||||||
if self.connection_ids.is_empty() {
|
if self.connection_ids.is_empty() {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user