From 5b309ef986c8efd2a53abfe4099bda901f8e263d Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Wed, 10 Dec 2025 17:48:08 +0100 Subject: [PATCH] acp: Better telemetry IDs for ACP agents (#44544) We were defining these in multiple places and also weren't leveraging the ids the agents were already providing. This should make sure we use them consistently and avoid issues in the future. Release Notes: - N/A --- crates/acp_thread/src/acp_thread.rs | 13 +- crates/acp_thread/src/connection.rs | 6 +- crates/action_log/src/action_log.rs | 2 +- crates/agent/src/agent.rs | 4 +- crates/agent/src/native_agent_server.rs | 4 - crates/agent_servers/src/acp.rs | 16 +- crates/agent_servers/src/agent_servers.rs | 1 - crates/agent_servers/src/claude.rs | 6 - crates/agent_servers/src/codex.rs | 6 - crates/agent_servers/src/custom.rs | 10 +- crates/agent_servers/src/gemini.rs | 6 - crates/agent_ui/src/acp/thread_view.rs | 194 +++++++++++----------- crates/agent_ui/src/agent_panel.rs | 6 +- crates/agent_ui/src/agent_ui.rs | 10 -- 14 files changed, 123 insertions(+), 161 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index b96ef1d8980..53294a963d9 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -1372,7 +1372,7 @@ impl AcpThread { let path_style = self.project.read(cx).path_style(cx); let id = update.tool_call_id.clone(); - let agent = self.connection().telemetry_id(); + let agent_telemetry_id = self.connection().telemetry_id(); let session = self.session_id(); if let ToolCallStatus::Completed | ToolCallStatus::Failed = status { let status = if matches!(status, ToolCallStatus::Completed) { @@ -1380,7 +1380,12 @@ impl AcpThread { } else { "failed" }; - telemetry::event!("Agent Tool Call Completed", agent, session, status); + telemetry::event!( + "Agent Tool Call Completed", + agent_telemetry_id, + session, + status + ); } if let Some(ix) = self.index_for_tool_call(&id) { @@ -3556,8 +3561,8 @@ mod tests { } impl AgentConnection for FakeAgentConnection { - fn telemetry_id(&self) -> &'static str { - "fake" + fn telemetry_id(&self) -> SharedString { + "fake".into() } fn auth_methods(&self) -> &[acp::AuthMethod] { diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 8213786a182..3c8c56b2c02 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -20,7 +20,7 @@ impl UserMessageId { } pub trait AgentConnection { - fn telemetry_id(&self) -> &'static str; + fn telemetry_id(&self) -> SharedString; fn new_thread( self: Rc, @@ -322,8 +322,8 @@ mod test_support { } impl AgentConnection for StubAgentConnection { - fn telemetry_id(&self) -> &'static str { - "stub" + fn telemetry_id(&self) -> SharedString { + "stub".into() } fn auth_methods(&self) -> &[acp::AuthMethod] { diff --git a/crates/action_log/src/action_log.rs b/crates/action_log/src/action_log.rs index 80c9438bc9f..6eb18a4f123 100644 --- a/crates/action_log/src/action_log.rs +++ b/crates/action_log/src/action_log.rs @@ -777,7 +777,7 @@ impl ActionLog { #[derive(Clone)] pub struct ActionLogTelemetry { - pub agent_telemetry_id: &'static str, + pub agent_telemetry_id: SharedString, pub session_id: Arc, } diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index aec0767c254..cf98a24ac52 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -947,8 +947,8 @@ impl acp_thread::AgentModelSelector for NativeAgentModelSelector { } impl acp_thread::AgentConnection for NativeAgentConnection { - fn telemetry_id(&self) -> &'static str { - "zed" + fn telemetry_id(&self) -> SharedString { + "zed".into() } fn new_thread( diff --git a/crates/agent/src/native_agent_server.rs b/crates/agent/src/native_agent_server.rs index 4c78c5a3f85..a9ade8141a6 100644 --- a/crates/agent/src/native_agent_server.rs +++ b/crates/agent/src/native_agent_server.rs @@ -21,10 +21,6 @@ impl NativeAgentServer { } impl AgentServer for NativeAgentServer { - fn telemetry_id(&self) -> &'static str { - "zed" - } - fn name(&self) -> SharedString { "Zed Agent".into() } diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 153357a79af..138353592e8 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -29,7 +29,7 @@ pub struct UnsupportedVersion; pub struct AcpConnection { server_name: SharedString, - telemetry_id: &'static str, + telemetry_id: SharedString, connection: Rc, sessions: Rc>>, auth_methods: Vec, @@ -54,7 +54,6 @@ pub struct AcpSession { pub async fn connect( server_name: SharedString, - telemetry_id: &'static str, command: AgentServerCommand, root_dir: &Path, default_mode: Option, @@ -64,7 +63,6 @@ pub async fn connect( ) -> Result> { let conn = AcpConnection::stdio( server_name, - telemetry_id, command.clone(), root_dir, default_mode, @@ -81,7 +79,6 @@ const MINIMUM_SUPPORTED_VERSION: acp::ProtocolVersion = acp::ProtocolVersion::V1 impl AcpConnection { pub async fn stdio( server_name: SharedString, - telemetry_id: &'static str, command: AgentServerCommand, root_dir: &Path, default_mode: Option, @@ -199,6 +196,13 @@ impl AcpConnection { return Err(UnsupportedVersion.into()); } + let telemetry_id = response + .agent_info + // Use the one the agent provides if we have one + .map(|info| info.name.into()) + // Otherwise, just use the name + .unwrap_or_else(|| server_name.clone()); + Ok(Self { auth_methods: response.auth_methods, root_dir: root_dir.to_owned(), @@ -233,8 +237,8 @@ impl Drop for AcpConnection { } impl AgentConnection for AcpConnection { - fn telemetry_id(&self) -> &'static str { - self.telemetry_id + fn telemetry_id(&self) -> SharedString { + self.telemetry_id.clone() } fn new_thread( diff --git a/crates/agent_servers/src/agent_servers.rs b/crates/agent_servers/src/agent_servers.rs index cf03b71a78b..46e8508e44f 100644 --- a/crates/agent_servers/src/agent_servers.rs +++ b/crates/agent_servers/src/agent_servers.rs @@ -56,7 +56,6 @@ impl AgentServerDelegate { pub trait AgentServer: Send { fn logo(&self) -> ui::IconName; fn name(&self) -> SharedString; - fn telemetry_id(&self) -> &'static str; fn default_mode(&self, _cx: &mut App) -> Option { None } diff --git a/crates/agent_servers/src/claude.rs b/crates/agent_servers/src/claude.rs index f49dce59c42..e67ddd5c069 100644 --- a/crates/agent_servers/src/claude.rs +++ b/crates/agent_servers/src/claude.rs @@ -22,10 +22,6 @@ pub struct AgentServerLoginCommand { } impl AgentServer for ClaudeCode { - fn telemetry_id(&self) -> &'static str { - "claude-code" - } - fn name(&self) -> SharedString { "Claude Code".into() } @@ -83,7 +79,6 @@ impl AgentServer for ClaudeCode { cx: &mut App, ) -> Task, Option)>> { let name = self.name(); - let telemetry_id = self.telemetry_id(); let root_dir = root_dir.map(|root_dir| root_dir.to_string_lossy().into_owned()); let is_remote = delegate.project.read(cx).is_via_remote_server(); let store = delegate.store.downgrade(); @@ -108,7 +103,6 @@ impl AgentServer for ClaudeCode { .await?; let connection = crate::acp::connect( name, - telemetry_id, command, root_dir.as_ref(), default_mode, diff --git a/crates/agent_servers/src/codex.rs b/crates/agent_servers/src/codex.rs index d14d2f0c9ae..c2b308e48b7 100644 --- a/crates/agent_servers/src/codex.rs +++ b/crates/agent_servers/src/codex.rs @@ -23,10 +23,6 @@ pub(crate) mod tests { } impl AgentServer for Codex { - fn telemetry_id(&self) -> &'static str { - "codex" - } - fn name(&self) -> SharedString { "Codex".into() } @@ -84,7 +80,6 @@ impl AgentServer for Codex { cx: &mut App, ) -> Task, Option)>> { let name = self.name(); - let telemetry_id = self.telemetry_id(); let root_dir = root_dir.map(|root_dir| root_dir.to_string_lossy().into_owned()); let is_remote = delegate.project.read(cx).is_via_remote_server(); let store = delegate.store.downgrade(); @@ -110,7 +105,6 @@ impl AgentServer for Codex { let connection = crate::acp::connect( name, - telemetry_id, command, root_dir.as_ref(), default_mode, diff --git a/crates/agent_servers/src/custom.rs b/crates/agent_servers/src/custom.rs index 634b31e9026..4cde4d4b9f5 100644 --- a/crates/agent_servers/src/custom.rs +++ b/crates/agent_servers/src/custom.rs @@ -1,4 +1,4 @@ -use crate::{AgentServerDelegate, load_proxy_env}; +use crate::{AgentServer, AgentServerDelegate, load_proxy_env}; use acp_thread::AgentConnection; use agent_client_protocol as acp; use anyhow::{Context as _, Result}; @@ -20,11 +20,7 @@ impl CustomAgentServer { } } -impl crate::AgentServer for CustomAgentServer { - fn telemetry_id(&self) -> &'static str { - "custom" - } - +impl AgentServer for CustomAgentServer { fn name(&self) -> SharedString { self.name.clone() } @@ -112,7 +108,6 @@ impl crate::AgentServer for CustomAgentServer { cx: &mut App, ) -> Task, Option)>> { let name = self.name(); - let telemetry_id = self.telemetry_id(); let root_dir = root_dir.map(|root_dir| root_dir.to_string_lossy().into_owned()); let is_remote = delegate.project.read(cx).is_via_remote_server(); let default_mode = self.default_mode(cx); @@ -139,7 +134,6 @@ impl crate::AgentServer for CustomAgentServer { .await?; let connection = crate::acp::connect( name, - telemetry_id, command, root_dir.as_ref(), default_mode, diff --git a/crates/agent_servers/src/gemini.rs b/crates/agent_servers/src/gemini.rs index c1b2efb0815..5fea74746ae 100644 --- a/crates/agent_servers/src/gemini.rs +++ b/crates/agent_servers/src/gemini.rs @@ -12,10 +12,6 @@ use project::agent_server_store::GEMINI_NAME; pub struct Gemini; impl AgentServer for Gemini { - fn telemetry_id(&self) -> &'static str { - "gemini-cli" - } - fn name(&self) -> SharedString { "Gemini CLI".into() } @@ -31,7 +27,6 @@ impl AgentServer for Gemini { cx: &mut App, ) -> Task, Option)>> { let name = self.name(); - let telemetry_id = self.telemetry_id(); let root_dir = root_dir.map(|root_dir| root_dir.to_string_lossy().into_owned()); let is_remote = delegate.project.read(cx).is_via_remote_server(); let store = delegate.store.downgrade(); @@ -66,7 +61,6 @@ impl AgentServer for Gemini { let connection = crate::acp::connect( name, - telemetry_id, command, root_dir.as_ref(), default_mode, diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index 36fb7e90974..63ea9eb279d 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -170,7 +170,7 @@ impl ThreadFeedbackState { } } let session_id = thread.read(cx).session_id().clone(); - let agent = thread.read(cx).connection().telemetry_id(); + let agent_telemetry_id = thread.read(cx).connection().telemetry_id(); let task = telemetry.thread_data(&session_id, cx); let rating = match feedback { ThreadFeedback::Positive => "positive", @@ -180,7 +180,7 @@ impl ThreadFeedbackState { let thread = task.await?; telemetry::event!( "Agent Thread Rated", - agent = agent, + agent = agent_telemetry_id, session_id = session_id, rating = rating, thread = thread @@ -207,13 +207,13 @@ impl ThreadFeedbackState { self.comments_editor.take(); let session_id = thread.read(cx).session_id().clone(); - let agent = thread.read(cx).connection().telemetry_id(); + let agent_telemetry_id = thread.read(cx).connection().telemetry_id(); let task = telemetry.thread_data(&session_id, cx); cx.background_spawn(async move { let thread = task.await?; telemetry::event!( "Agent Thread Feedback Comments", - agent = agent, + agent = agent_telemetry_id, session_id = session_id, comments = comments, thread = thread @@ -333,6 +333,7 @@ impl AcpThreadView { project: Entity, history_store: Entity, prompt_store: Option>, + track_load_event: bool, window: &mut Window, cx: &mut Context, ) -> Self { @@ -391,8 +392,9 @@ impl AcpThreadView { ), ]; - let show_codex_windows_warning = crate::ExternalAgent::parse_built_in(agent.as_ref()) - == Some(crate::ExternalAgent::Codex); + let show_codex_windows_warning = cfg!(windows) + && project.read(cx).is_local() + && agent.clone().downcast::().is_some(); Self { agent: agent.clone(), @@ -404,6 +406,7 @@ impl AcpThreadView { resume_thread.clone(), workspace.clone(), project.clone(), + track_load_event, window, cx, ), @@ -448,6 +451,7 @@ impl AcpThreadView { self.resume_thread_metadata.clone(), self.workspace.clone(), self.project.clone(), + true, window, cx, ); @@ -461,6 +465,7 @@ impl AcpThreadView { resume_thread: Option, workspace: WeakEntity, project: Entity, + track_load_event: bool, window: &mut Window, cx: &mut Context, ) -> ThreadState { @@ -519,6 +524,10 @@ impl AcpThreadView { } }; + if track_load_event { + telemetry::event!("Agent Thread Started", agent = connection.telemetry_id()); + } + let result = if let Some(native_agent) = connection .clone() .downcast::() @@ -1133,8 +1142,8 @@ impl AcpThreadView { let Some(thread) = self.thread() else { return; }; - let agent_telemetry_id = self.agent.telemetry_id(); let session_id = thread.read(cx).session_id().clone(); + let agent_telemetry_id = thread.read(cx).connection().telemetry_id(); let thread = thread.downgrade(); if self.should_be_following { self.workspace @@ -1512,6 +1521,7 @@ impl AcpThreadView { else { return; }; + let agent_telemetry_id = connection.telemetry_id(); // Check for the experimental "terminal-auth" _meta field let auth_method = connection.auth_methods().iter().find(|m| m.id == method); @@ -1579,19 +1589,18 @@ impl AcpThreadView { ); cx.notify(); self.auth_task = Some(cx.spawn_in(window, { - let agent = self.agent.clone(); async move |this, cx| { let result = authenticate.await; match &result { Ok(_) => telemetry::event!( "Authenticate Agent Succeeded", - agent = agent.telemetry_id() + agent = agent_telemetry_id ), Err(_) => { telemetry::event!( "Authenticate Agent Failed", - agent = agent.telemetry_id(), + agent = agent_telemetry_id, ) } } @@ -1675,6 +1684,7 @@ impl AcpThreadView { None, this.workspace.clone(), this.project.clone(), + true, window, cx, ) @@ -1730,43 +1740,38 @@ impl AcpThreadView { connection.authenticate(method, cx) }; cx.notify(); - self.auth_task = - Some(cx.spawn_in(window, { - let agent = self.agent.clone(); - async move |this, cx| { - let result = authenticate.await; + self.auth_task = Some(cx.spawn_in(window, { + async move |this, cx| { + let result = authenticate.await; - match &result { - Ok(_) => telemetry::event!( - "Authenticate Agent Succeeded", - agent = agent.telemetry_id() - ), - Err(_) => { - telemetry::event!( - "Authenticate Agent Failed", - agent = agent.telemetry_id(), - ) - } + match &result { + Ok(_) => telemetry::event!( + "Authenticate Agent Succeeded", + agent = agent_telemetry_id + ), + Err(_) => { + telemetry::event!("Authenticate Agent Failed", agent = agent_telemetry_id,) } - - this.update_in(cx, |this, window, cx| { - if let Err(err) = result { - if let ThreadState::Unauthenticated { - pending_auth_method, - .. - } = &mut this.thread_state - { - pending_auth_method.take(); - } - this.handle_thread_error(err, cx); - } else { - this.reset(window, cx); - } - this.auth_task.take() - }) - .ok(); } - })); + + this.update_in(cx, |this, window, cx| { + if let Err(err) = result { + if let ThreadState::Unauthenticated { + pending_auth_method, + .. + } = &mut this.thread_state + { + pending_auth_method.take(); + } + this.handle_thread_error(err, cx); + } else { + this.reset(window, cx); + } + this.auth_task.take() + }) + .ok(); + } + })); } fn spawn_external_agent_login( @@ -1896,10 +1901,11 @@ impl AcpThreadView { let Some(thread) = self.thread() else { return; }; + let agent_telemetry_id = thread.read(cx).connection().telemetry_id(); telemetry::event!( "Agent Tool Call Authorized", - agent = self.agent.telemetry_id(), + agent = agent_telemetry_id, session = thread.read(cx).session_id(), option = option_kind ); @@ -3509,6 +3515,8 @@ impl AcpThreadView { (method.id.0.clone(), method.name.clone()) }; + let agent_telemetry_id = connection.telemetry_id(); + Button::new(method_id.clone(), name) .label_size(LabelSize::Small) .map(|this| { @@ -3528,7 +3536,7 @@ impl AcpThreadView { cx.listener(move |this, _, window, cx| { telemetry::event!( "Authenticate Agent Started", - agent = this.agent.telemetry_id(), + agent = agent_telemetry_id, method = method_id ); @@ -5376,47 +5384,39 @@ impl AcpThreadView { ) } - fn render_codex_windows_warning(&self, cx: &mut Context) -> Option { - if self.show_codex_windows_warning { - Some( - Callout::new() - .icon(IconName::Warning) - .severity(Severity::Warning) - .title("Codex on Windows") - .description( - "For best performance, run Codex in Windows Subsystem for Linux (WSL2)", - ) - .actions_slot( - Button::new("open-wsl-modal", "Open in WSL") - .icon_size(IconSize::Small) - .icon_color(Color::Muted) - .on_click(cx.listener({ - move |_, _, _window, cx| { - #[cfg(windows)] - _window.dispatch_action( - zed_actions::wsl_actions::OpenWsl::default().boxed_clone(), - cx, - ); - cx.notify(); - } - })), - ) - .dismiss_action( - IconButton::new("dismiss", IconName::Close) - .icon_size(IconSize::Small) - .icon_color(Color::Muted) - .tooltip(Tooltip::text("Dismiss Warning")) - .on_click(cx.listener({ - move |this, _, _, cx| { - this.show_codex_windows_warning = false; - cx.notify(); - } - })), - ), + fn render_codex_windows_warning(&self, cx: &mut Context) -> Callout { + Callout::new() + .icon(IconName::Warning) + .severity(Severity::Warning) + .title("Codex on Windows") + .description("For best performance, run Codex in Windows Subsystem for Linux (WSL2)") + .actions_slot( + Button::new("open-wsl-modal", "Open in WSL") + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .on_click(cx.listener({ + move |_, _, _window, cx| { + #[cfg(windows)] + _window.dispatch_action( + zed_actions::wsl_actions::OpenWsl::default().boxed_clone(), + cx, + ); + cx.notify(); + } + })), + ) + .dismiss_action( + IconButton::new("dismiss", IconName::Close) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .tooltip(Tooltip::text("Dismiss Warning")) + .on_click(cx.listener({ + move |this, _, _, cx| { + this.show_codex_windows_warning = false; + cx.notify(); + } + })), ) - } else { - None - } } fn render_thread_error(&mut self, window: &mut Window, cx: &mut Context) -> Option
{ @@ -5936,12 +5936,8 @@ impl Render for AcpThreadView { _ => this, }) .children(self.render_thread_retry_status_callout(window, cx)) - .children({ - if cfg!(windows) && self.project.read(cx).is_local() { - self.render_codex_windows_warning(cx) - } else { - None - } + .when(self.show_codex_windows_warning, |this| { + this.child(self.render_codex_windows_warning(cx)) }) .children(self.render_thread_error(window, cx)) .when_some( @@ -6398,6 +6394,7 @@ pub(crate) mod tests { project, history_store, None, + false, window, cx, ) @@ -6475,10 +6472,6 @@ pub(crate) mod tests { where C: 'static + AgentConnection + Send + Clone, { - fn telemetry_id(&self) -> &'static str { - "test" - } - fn logo(&self) -> ui::IconName { ui::IconName::Ai } @@ -6505,8 +6498,8 @@ pub(crate) mod tests { struct SaboteurAgentConnection; impl AgentConnection for SaboteurAgentConnection { - fn telemetry_id(&self) -> &'static str { - "saboteur" + fn telemetry_id(&self) -> SharedString { + "saboteur".into() } fn new_thread( @@ -6569,8 +6562,8 @@ pub(crate) mod tests { struct RefusalAgentConnection; impl AgentConnection for RefusalAgentConnection { - fn telemetry_id(&self) -> &'static str { - "refusal" + fn telemetry_id(&self) -> SharedString { + "refusal".into() } fn new_thread( @@ -6671,6 +6664,7 @@ pub(crate) mod tests { project.clone(), history_store.clone(), None, + false, window, cx, ) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index d5dd0a25c81..2f6a722b471 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -305,6 +305,7 @@ impl ActiveView { project, history_store, prompt_store, + false, window, cx, ) @@ -885,10 +886,6 @@ impl AgentPanel { let server = ext_agent.server(fs, history); - if !loading { - telemetry::event!("Agent Thread Started", agent = server.telemetry_id()); - } - this.update_in(cx, |this, window, cx| { let selected_agent = ext_agent.into(); if this.selected_agent != selected_agent { @@ -905,6 +902,7 @@ impl AgentPanel { project, this.history_store.clone(), this.prompt_store.clone(), + !loading, window, cx, ) diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index f7b07b7bd39..b6f7517ed93 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -160,16 +160,6 @@ pub enum ExternalAgent { } impl ExternalAgent { - pub fn parse_built_in(server: &dyn agent_servers::AgentServer) -> Option { - match server.telemetry_id() { - "gemini-cli" => Some(Self::Gemini), - "claude-code" => Some(Self::ClaudeCode), - "codex" => Some(Self::Codex), - "zed" => Some(Self::NativeAgent), - _ => None, - } - } - pub fn server( &self, fs: Arc,