Rework call events api

Joseph T. Lyons and Julia created 2 years ago

There were time when events with bad data were being emitted. What we found was that places where certain collaboration-related code could fail, like sending an, would still send events, and those events be in a bad state, as certain elements weren't constructed as expected, thus missing in the event. The new API guarantees that we have data in the correct configuration. In the future, we will add events for certain types of failures within Zed.

Co-Authored-By: Julia <[e-mail]>

Change summary

crates/call/src/call.rs              | 80 +++++++++++++++++------------
crates/collab_ui/src/channel_view.rs | 11 +---
crates/collab_ui/src/collab_ui.rs    | 32 ++++-------
3 files changed, 61 insertions(+), 62 deletions(-)

Detailed changes

crates/call/src/call.rs

@@ -206,9 +206,14 @@ impl ActiveCall {
 
         cx.spawn(|this, mut cx| async move {
             let result = invite.await;
+            if result.is_ok() {
+                this.update(&mut cx, |this, cx| this.report_call_event("invite", cx));
+            } else {
+                // TODO: Resport collaboration error
+            }
+
             this.update(&mut cx, |this, cx| {
                 this.pending_invites.remove(&called_user_id);
-                this.report_call_event("invite", cx);
                 cx.notify();
             });
             result
@@ -273,13 +278,7 @@ impl ActiveCall {
             .borrow_mut()
             .take()
             .ok_or_else(|| anyhow!("no incoming call"))?;
-        Self::report_call_event_for_room(
-            "decline incoming",
-            Some(call.room_id),
-            None,
-            &self.client,
-            cx,
-        );
+        report_call_event_for_room("decline incoming", call.room_id, None, &self.client, cx);
         self.client.send(proto::DeclineCall {
             room_id: call.room_id,
         })?;
@@ -409,31 +408,46 @@ impl ActiveCall {
         &self.pending_invites
     }
 
-    fn report_call_event(&self, operation: &'static str, cx: &AppContext) {
-        let (room_id, channel_id) = match self.room() {
-            Some(room) => {
-                let room = room.read(cx);
-                (Some(room.id()), room.channel_id())
-            }
-            None => (None, None),
-        };
-        Self::report_call_event_for_room(operation, room_id, channel_id, &self.client, cx)
+    pub fn report_call_event(&self, operation: &'static str, cx: &AppContext) {
+        if let Some(room) = self.room() {
+            let room = room.read(cx);
+            report_call_event_for_room(operation, room.id(), room.channel_id(), &self.client, cx);
+        }
     }
+}
 
-    pub fn report_call_event_for_room(
-        operation: &'static str,
-        room_id: Option<u64>,
-        channel_id: Option<u64>,
-        client: &Arc<Client>,
-        cx: &AppContext,
-    ) {
-        let telemetry = client.telemetry();
-        let telemetry_settings = *settings::get::<TelemetrySettings>(cx);
-        let event = ClickhouseEvent::Call {
-            operation,
-            room_id,
-            channel_id,
-        };
-        telemetry.report_clickhouse_event(event, telemetry_settings);
-    }
+pub fn report_call_event_for_room(
+    operation: &'static str,
+    room_id: u64,
+    channel_id: Option<u64>,
+    client: &Arc<Client>,
+    cx: &AppContext,
+) {
+    let telemetry = client.telemetry();
+    let telemetry_settings = *settings::get::<TelemetrySettings>(cx);
+    let event = ClickhouseEvent::Call {
+        operation,
+        room_id: Some(room_id),
+        channel_id,
+    };
+    telemetry.report_clickhouse_event(event, telemetry_settings);
+}
+
+pub fn report_call_event_for_channel(
+    operation: &'static str,
+    channel_id: u64,
+    client: &Arc<Client>,
+    cx: &AppContext,
+) {
+    let room = ActiveCall::global(cx).read(cx).room();
+
+    let telemetry = client.telemetry();
+    let telemetry_settings = *settings::get::<TelemetrySettings>(cx);
+
+    let event = ClickhouseEvent::Call {
+        operation,
+        room_id: room.map(|r| r.read(cx).id()),
+        channel_id: Some(channel_id),
+    };
+    telemetry.report_clickhouse_event(event, telemetry_settings);
 }

crates/collab_ui/src/channel_view.rs

@@ -1,5 +1,5 @@
 use anyhow::{anyhow, Result};
-use call::ActiveCall;
+use call::report_call_event_for_channel;
 use channel::{ChannelBuffer, ChannelBufferEvent, ChannelId};
 use client::proto;
 use clock::ReplicaId;
@@ -42,14 +42,9 @@ impl ChannelView {
         cx.spawn(|mut cx| async move {
             let channel_view = channel_view.await?;
             pane.update(&mut cx, |pane, cx| {
-                let room_id = ActiveCall::global(cx)
-                    .read(cx)
-                    .room()
-                    .map(|room| room.read(cx).id());
-                ActiveCall::report_call_event_for_room(
+                report_call_event_for_channel(
                     "open channel notes",
-                    room_id,
-                    Some(channel_id),
+                    channel_id,
                     &workspace.read(cx).app_state().client,
                     cx,
                 );

crates/collab_ui/src/collab_ui.rs

@@ -10,7 +10,7 @@ mod panel_settings;
 mod project_shared_notification;
 mod sharing_status_indicator;
 
-use call::{ActiveCall, Room};
+use call::{report_call_event_for_room, ActiveCall, Room};
 use gpui::{
     actions,
     geometry::{
@@ -55,18 +55,18 @@ pub fn toggle_screen_sharing(_: &ToggleScreenSharing, cx: &mut AppContext) {
         let client = call.client();
         let toggle_screen_sharing = room.update(cx, |room, cx| {
             if room.is_screen_sharing() {
-                ActiveCall::report_call_event_for_room(
+                report_call_event_for_room(
                     "disable screen share",
-                    Some(room.id()),
+                    room.id(),
                     room.channel_id(),
                     &client,
                     cx,
                 );
                 Task::ready(room.unshare_screen(cx))
             } else {
-                ActiveCall::report_call_event_for_room(
+                report_call_event_for_room(
                     "enable screen share",
-                    Some(room.id()),
+                    room.id(),
                     room.channel_id(),
                     &client,
                     cx,
@@ -83,23 +83,13 @@ pub fn toggle_mute(_: &ToggleMute, cx: &mut AppContext) {
     if let Some(room) = call.room().cloned() {
         let client = call.client();
         room.update(cx, |room, cx| {
-            if room.is_muted(cx) {
-                ActiveCall::report_call_event_for_room(
-                    "enable microphone",
-                    Some(room.id()),
-                    room.channel_id(),
-                    &client,
-                    cx,
-                );
+            let operation = if room.is_muted(cx) {
+                "enable microphone"
             } else {
-                ActiveCall::report_call_event_for_room(
-                    "disable microphone",
-                    Some(room.id()),
-                    room.channel_id(),
-                    &client,
-                    cx,
-                );
-            }
+                "disable microphone"
+            };
+            report_call_event_for_room(operation, room.id(), room.channel_id(), &client, cx);
+
             room.toggle_mute(cx)
         })
         .map(|task| task.detach_and_log_err(cx))