From b6189b05f915a9470ffba06b525c677b6927c41e Mon Sep 17 00:00:00 2001 From: "Joseph T. Lyons" Date: Thu, 16 May 2024 17:18:32 -0400 Subject: [PATCH] Add telemetry for supermaven (#11821) Data migration plan: - [X] Make a duplicate table of `copilot_events` - Name: `inline_completion_events` - Omit `suggestion_id` column - [X-reverted-skipping] In collab, continue to match on copilot_events, but simply stuff their data into inline_completion_events, to forward it to the new table - [skipping] Once collab is deployed, ensure no events are being sent to copilot_events, migrate `copilot_events` to new table via a transaction - [skipping] Delete `copilot_events` table --- - [X] Locally test that copilot events sent from old clients get put into inline_completions_table - [X] Locally test that copilot events and supermaven events sent from new clients get put into inline_completions_table --- - [X] Why are discard events being spammed? - A: https://github.com/zed-industries/zed/blob/8d4315712bc706a9b04d9daf638b264e5789b1f9/crates/editor/src/editor.rs#L2147 ![scr-20240514-pqmg](https://github.com/zed-industries/zed/assets/19867440/e51e7ae4-21b8-47a2-bfaa-f68fb355e409) This will throw off the past results for accepted / dismissed that I was wanting to use to evaluate Supermaven quality, by comparing its rate with copilot's rate. I'm not super thrilled with this fix, but I think it'll do. In the `supermaven_completions_provider`, we check if there's a `completion_id` before sending either an accepted or discard completion event. I don't see a similar construct in the `copilot_completions_provider` to piggyback off of, so I begrudgingly introduced `should_allow_event_to_send` and had it follow the same pattern that `completion_id` does. Maybe there's a better way? --- Adds events to supermaven suggestions. Makes "CopilotEvents" generic -> "InlineCompletionEvents". Release Notes: - N/A --- crates/client/src/telemetry.rs | 14 +++--- crates/collab/src/api/events.rs | 45 +++++++++-------- .../src/copilot_completion_provider.rs | 32 ++++++++++--- .../editor/src/inline_completion_provider.rs | 1 + crates/language_tools/src/lsp_log.rs | 9 ++-- .../src/supermaven_completion_provider.rs | 48 ++++++++++++++++++- .../telemetry_events/src/telemetry_events.rs | 11 ++++- .../zed/src/zed/inline_completion_registry.rs | 4 +- docs/src/telemetry.md | 1 - 9 files changed, 123 insertions(+), 42 deletions(-) diff --git a/crates/client/src/telemetry.rs b/crates/client/src/telemetry.rs index 8d7a30ca6c..feaa11dffb 100644 --- a/crates/client/src/telemetry.rs +++ b/crates/client/src/telemetry.rs @@ -15,9 +15,9 @@ use std::io::Write; use std::{env, mem, path::PathBuf, sync::Arc, time::Duration}; use sysinfo::{CpuRefreshKind, Pid, ProcessRefreshKind, RefreshKind, System}; use telemetry_events::{ - ActionEvent, AppEvent, AssistantEvent, AssistantKind, CallEvent, CopilotEvent, CpuEvent, - EditEvent, EditorEvent, Event, EventRequestBody, EventWrapper, ExtensionEvent, MemoryEvent, - SettingEvent, + ActionEvent, AppEvent, AssistantEvent, AssistantKind, CallEvent, CpuEvent, EditEvent, + EditorEvent, Event, EventRequestBody, EventWrapper, ExtensionEvent, InlineCompletionEvent, + MemoryEvent, SettingEvent, }; use tempfile::NamedTempFile; #[cfg(not(debug_assertions))] @@ -241,14 +241,14 @@ impl Telemetry { self.report_event(event) } - pub fn report_copilot_event( + pub fn report_inline_completion_event( self: &Arc, - suggestion_id: Option, + provider: String, suggestion_accepted: bool, file_extension: Option, ) { - let event = Event::Copilot(CopilotEvent { - suggestion_id, + let event = Event::InlineCompletion(InlineCompletionEvent { + provider, suggestion_accepted, file_extension, }); diff --git a/crates/collab/src/api/events.rs b/crates/collab/src/api/events.rs index 0ab1f63158..4539e13c89 100644 --- a/crates/collab/src/api/events.rs +++ b/crates/collab/src/api/events.rs @@ -15,8 +15,9 @@ use serde::{Serialize, Serializer}; use sha2::{Digest, Sha256}; use std::sync::{Arc, OnceLock}; use telemetry_events::{ - ActionEvent, AppEvent, AssistantEvent, CallEvent, CopilotEvent, CpuEvent, EditEvent, - EditorEvent, Event, EventRequestBody, EventWrapper, ExtensionEvent, MemoryEvent, SettingEvent, + ActionEvent, AppEvent, AssistantEvent, CallEvent, CpuEvent, EditEvent, EditorEvent, Event, + EventRequestBody, EventWrapper, ExtensionEvent, InlineCompletionEvent, MemoryEvent, + SettingEvent, }; use uuid::Uuid; @@ -424,13 +425,19 @@ pub async fn post_events( first_event_at, country_code.clone(), )), - Event::Copilot(event) => to_upload.copilot_events.push(CopilotEventRow::from_event( - event.clone(), - &wrapper, - &request_body, - first_event_at, - country_code.clone(), - )), + // Needed for clients sending old copilot_event types + Event::Copilot(_) => {} + Event::InlineCompletion(event) => { + to_upload + .inline_completion_events + .push(InlineCompletionEventRow::from_event( + event.clone(), + &wrapper, + &request_body, + first_event_at, + country_code.clone(), + )) + } Event::Call(event) => to_upload.call_events.push(CallEventRow::from_event( event.clone(), &wrapper, @@ -512,7 +519,7 @@ pub async fn post_events( #[derive(Default)] struct ToUpload { editor_events: Vec, - copilot_events: Vec, + inline_completion_events: Vec, assistant_events: Vec, call_events: Vec, cpu_events: Vec, @@ -531,14 +538,14 @@ impl ToUpload { .await .with_context(|| format!("failed to upload to table '{EDITOR_EVENTS_TABLE}'"))?; - const COPILOT_EVENTS_TABLE: &str = "copilot_events"; + const INLINE_COMPLETION_EVENTS_TABLE: &str = "inline_completion_events"; Self::upload_to_table( - COPILOT_EVENTS_TABLE, - &self.copilot_events, + INLINE_COMPLETION_EVENTS_TABLE, + &self.inline_completion_events, clickhouse_client, ) .await - .with_context(|| format!("failed to upload to table '{COPILOT_EVENTS_TABLE}'"))?; + .with_context(|| format!("failed to upload to table '{INLINE_COMPLETION_EVENTS_TABLE}'"))?; const ASSISTANT_EVENTS_TABLE: &str = "assistant_events"; Self::upload_to_table( @@ -708,9 +715,9 @@ impl EditorEventRow { } #[derive(Serialize, Debug, clickhouse::Row)] -pub struct CopilotEventRow { +pub struct InlineCompletionEventRow { pub installation_id: String, - pub suggestion_id: String, + pub provider: String, pub suggestion_accepted: bool, pub app_version: String, pub file_extension: String, @@ -730,9 +737,9 @@ pub struct CopilotEventRow { pub patch: Option, } -impl CopilotEventRow { +impl InlineCompletionEventRow { fn from_event( - event: CopilotEvent, + event: InlineCompletionEvent, wrapper: &EventWrapper, body: &EventRequestBody, first_event_at: chrono::DateTime, @@ -759,7 +766,7 @@ impl CopilotEventRow { country_code: country_code.unwrap_or("XX".to_string()), region_code: "".to_string(), city: "".to_string(), - suggestion_id: event.suggestion_id.unwrap_or_default(), + provider: event.provider, suggestion_accepted: event.suggestion_accepted, } } diff --git a/crates/copilot/src/copilot_completion_provider.rs b/crates/copilot/src/copilot_completion_provider.rs index 6264250885..5a88762759 100644 --- a/crates/copilot/src/copilot_completion_provider.rs +++ b/crates/copilot/src/copilot_completion_provider.rs @@ -22,6 +22,7 @@ pub struct CopilotCompletionProvider { pending_cycling_refresh: Task>, copilot: Model, telemetry: Option>, + should_allow_event_to_send: bool, } impl CopilotCompletionProvider { @@ -36,6 +37,7 @@ impl CopilotCompletionProvider { pending_cycling_refresh: Task::ready(Ok(())), copilot, telemetry: None, + should_allow_event_to_send: false, } } @@ -59,6 +61,10 @@ impl CopilotCompletionProvider { } impl InlineCompletionProvider for CopilotCompletionProvider { + fn name() -> &'static str { + "copilot" + } + fn is_enabled( &self, buffer: &Model, @@ -99,6 +105,7 @@ impl InlineCompletionProvider for CopilotCompletionProvider { this.update(&mut cx, |this, cx| { if !completions.is_empty() { + this.should_allow_event_to_send = true; this.cycled = false; this.pending_cycling_refresh = Task::ready(Ok(())); this.completions.clear(); @@ -187,13 +194,17 @@ impl InlineCompletionProvider for CopilotCompletionProvider { .update(cx, |copilot, cx| copilot.accept_completion(completion, cx)) .detach_and_log_err(cx); if let Some(telemetry) = self.telemetry.as_ref() { - telemetry.report_copilot_event( - Some(completion.uuid.clone()), - true, - self.file_extension.clone(), - ); + if self.should_allow_event_to_send { + telemetry.report_inline_completion_event( + Self::name().to_string(), + true, + self.file_extension.clone(), + ); + } } } + + self.should_allow_event_to_send = false; } fn discard(&mut self, cx: &mut ModelContext) { @@ -210,9 +221,18 @@ impl InlineCompletionProvider for CopilotCompletionProvider { copilot.discard_completions(&self.completions, cx) }) .detach_and_log_err(cx); + if let Some(telemetry) = self.telemetry.as_ref() { - telemetry.report_copilot_event(None, false, self.file_extension.clone()); + if self.should_allow_event_to_send { + telemetry.report_inline_completion_event( + Self::name().to_string(), + false, + self.file_extension.clone(), + ); + } } + + self.should_allow_event_to_send = false; } fn active_completion_text<'a>( diff --git a/crates/editor/src/inline_completion_provider.rs b/crates/editor/src/inline_completion_provider.rs index 11658a2d75..0bf73faa01 100644 --- a/crates/editor/src/inline_completion_provider.rs +++ b/crates/editor/src/inline_completion_provider.rs @@ -3,6 +3,7 @@ use gpui::{AppContext, Model, ModelContext}; use language::Buffer; pub trait InlineCompletionProvider: 'static + Sized { + fn name() -> &'static str; fn is_enabled( &self, buffer: &Model, diff --git a/crates/language_tools/src/lsp_log.rs b/crates/language_tools/src/lsp_log.rs index ad08d5529d..44141f89ce 100644 --- a/crates/language_tools/src/lsp_log.rs +++ b/crates/language_tools/src/lsp_log.rs @@ -129,9 +129,8 @@ impl LogStore { let copilot_subscription = Copilot::global(cx).map(|copilot| { let copilot = &copilot; - cx.subscribe( - copilot, - |this, copilot, copilot_event, cx| match copilot_event { + cx.subscribe(copilot, |this, copilot, inline_completion_event, cx| { + match inline_completion_event { copilot::Event::CopilotLanguageServerStarted => { if let Some(server) = copilot.read(cx).language_server() { let server_id = server.server_id(); @@ -159,8 +158,8 @@ impl LogStore { ); } } - }, - ) + } + }) }); let this = Self { diff --git a/crates/supermaven/src/supermaven_completion_provider.rs b/crates/supermaven/src/supermaven_completion_provider.rs index e939a7ef9c..f7dbaeead4 100644 --- a/crates/supermaven/src/supermaven_completion_provider.rs +++ b/crates/supermaven/src/supermaven_completion_provider.rs @@ -1,30 +1,46 @@ use crate::{Supermaven, SupermavenCompletionStateId}; use anyhow::Result; +use client::telemetry::Telemetry; use editor::{Direction, InlineCompletionProvider}; use futures::StreamExt as _; -use gpui::{AppContext, Model, ModelContext, Task}; +use gpui::{AppContext, EntityId, Model, ModelContext, Task}; use language::{language_settings::all_language_settings, Anchor, Buffer}; -use std::time::Duration; +use std::{path::Path, sync::Arc, time::Duration}; pub const DEBOUNCE_TIMEOUT: Duration = Duration::from_millis(75); pub struct SupermavenCompletionProvider { supermaven: Model, + buffer_id: Option, completion_id: Option, + file_extension: Option, pending_refresh: Task>, + telemetry: Option>, } impl SupermavenCompletionProvider { pub fn new(supermaven: Model) -> Self { Self { supermaven, + buffer_id: None, completion_id: None, + file_extension: None, pending_refresh: Task::ready(Ok(())), + telemetry: None, } } + + pub fn with_telemetry(mut self, telemetry: Arc) -> Self { + self.telemetry = Some(telemetry); + self + } } impl InlineCompletionProvider for SupermavenCompletionProvider { + fn name() -> &'static str { + "supermaven" + } + fn is_enabled(&self, buffer: &Model, cursor_position: Anchor, cx: &AppContext) -> bool { if !self.supermaven.read(cx).is_enabled() { return false; @@ -58,6 +74,15 @@ impl InlineCompletionProvider for SupermavenCompletionProvider { while let Some(()) = completion.updates.next().await { this.update(&mut cx, |this, cx| { this.completion_id = Some(completion.id); + this.buffer_id = Some(buffer_handle.entity_id()); + this.file_extension = buffer_handle.read(cx).file().and_then(|file| { + Some( + Path::new(file.file_name(cx)) + .extension()? + .to_str()? + .to_string(), + ) + }); cx.notify(); })?; } @@ -75,11 +100,30 @@ impl InlineCompletionProvider for SupermavenCompletionProvider { } fn accept(&mut self, _cx: &mut ModelContext) { + if let Some(telemetry) = self.telemetry.as_ref() { + if let Some(_) = self.completion_id { + telemetry.report_inline_completion_event( + Self::name().to_string(), + true, + self.file_extension.clone(), + ); + } + } self.pending_refresh = Task::ready(Ok(())); self.completion_id = None; } fn discard(&mut self, _cx: &mut ModelContext) { + if let Some(telemetry) = self.telemetry.as_ref() { + if let Some(_) = self.completion_id { + telemetry.report_inline_completion_event( + Self::name().to_string(), + false, + self.file_extension.clone(), + ); + } + } + self.pending_refresh = Task::ready(Ok(())); self.completion_id = None; } diff --git a/crates/telemetry_events/src/telemetry_events.rs b/crates/telemetry_events/src/telemetry_events.rs index 1f2a0f0245..fd1ee54ba3 100644 --- a/crates/telemetry_events/src/telemetry_events.rs +++ b/crates/telemetry_events/src/telemetry_events.rs @@ -53,7 +53,8 @@ impl Display for AssistantKind { #[serde(tag = "type")] pub enum Event { Editor(EditorEvent), - Copilot(CopilotEvent), + Copilot(CopilotEvent), // Needed for clients sending old copilot_event types + InlineCompletion(InlineCompletionEvent), Call(CallEvent), Assistant(AssistantEvent), Cpu(CpuEvent), @@ -74,6 +75,7 @@ pub struct EditorEvent { pub copilot_enabled_for_language: bool, } +// Needed for clients sending old copilot_event types #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct CopilotEvent { pub suggestion_id: Option, @@ -81,6 +83,13 @@ pub struct CopilotEvent { pub file_extension: Option, } +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +pub struct InlineCompletionEvent { + pub provider: String, + pub suggestion_accepted: bool, + pub file_extension: Option, +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct CallEvent { pub operation: String, diff --git a/crates/zed/src/zed/inline_completion_registry.rs b/crates/zed/src/zed/inline_completion_registry.rs index 7ea50322a3..2cc6b23651 100644 --- a/crates/zed/src/zed/inline_completion_registry.rs +++ b/crates/zed/src/zed/inline_completion_registry.rs @@ -118,7 +118,9 @@ fn assign_inline_completion_provider( } language::language_settings::InlineCompletionProvider::Supermaven => { if let Some(supermaven) = Supermaven::global(cx) { - let provider = cx.new_model(|_| SupermavenCompletionProvider::new(supermaven)); + let provider = cx.new_model(|_| { + SupermavenCompletionProvider::new(supermaven).with_telemetry(telemetry.clone()) + }); editor.set_inline_completion_provider(Some(provider), cx); } } diff --git a/docs/src/telemetry.md b/docs/src/telemetry.md index e798590c73..9f4a6954b0 100644 --- a/docs/src/telemetry.md +++ b/docs/src/telemetry.md @@ -97,7 +97,6 @@ The following data is sent: - `copilot_enabled_for_language`: A boolean that indicates whether the user has copilot enabled for the language of the file that was opened or saved - `milliseconds_since_first_event`: Duration of time between this event's timestamp and the timestamp of the first event in the current batch - `copilot` - - `suggestion_id`: The ID of the suggestion - `suggestion_accepted`: A boolean that indicates whether the suggestion was accepted or not - `file_extension`: The file extension of the file that was opened or saved - `milliseconds_since_first_event`: Same as above