From 72782b8389c102acf157dd2af7bac5a25b0e95f4 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 17 Feb 2025 14:29:08 +1100 Subject: [PATCH] fix(gui-client): update telemetry context on new session (#8152) Every time we start a new session, our telemetry context potentially changes, i.e. the user may sign into a new account. This should ensure that both the IPC service and the GUI always use the most up-to-date `account_slug` as part of Sentry events. In addition, this will also set the `account_slug` for clients that just signed in. Previously, the `account_slug` would only get populated on the next start of the client. --- rust/connlib/clients/android/src/lib.rs | 2 +- rust/connlib/clients/apple/src/lib.rs | 4 +- rust/gateway/src/main.rs | 6 +-- rust/gui-client/src-common/src/controller.rs | 47 +++++++++++--------- rust/gui-client/src-tauri/src/client.rs | 12 ++--- rust/gui-client/src-tauri/src/client/gui.rs | 1 - rust/headless-client/src/ipc_service.rs | 6 +-- rust/headless-client/src/main.rs | 2 +- rust/telemetry/src/lib.rs | 22 ++++----- 9 files changed, 52 insertions(+), 50 deletions(-) diff --git a/rust/connlib/clients/android/src/lib.rs b/rust/connlib/clients/android/src/lib.rs index 06acb1f8f..d0bdc1a2b 100644 --- a/rust/connlib/clients/android/src/lib.rs +++ b/rust/connlib/clients/android/src/lib.rs @@ -328,7 +328,7 @@ fn connect( let mut telemetry = Telemetry::default(); telemetry.start(&api_url, RELEASE, ANDROID_DSN); - telemetry.set_firezone_id(device_id.clone()); + Telemetry::set_firezone_id(device_id.clone()); init_logging(&PathBuf::from(log_dir), log_filter)?; install_rustls_crypto_provider(); diff --git a/rust/connlib/clients/apple/src/lib.rs b/rust/connlib/clients/apple/src/lib.rs index d270b23c9..c5f1df776 100644 --- a/rust/connlib/clients/apple/src/lib.rs +++ b/rust/connlib/clients/apple/src/lib.rs @@ -239,8 +239,8 @@ impl WrappedSession { ) -> Result { let mut telemetry = Telemetry::default(); telemetry.start(&api_url, RELEASE, APPLE_DSN); - telemetry.set_firezone_id(device_id.clone()); - telemetry.set_account_slug(account_slug); + Telemetry::set_firezone_id(device_id.clone()); + Telemetry::set_account_slug(account_slug); init_logging(log_dir.into(), log_filter)?; install_rustls_crypto_provider(); diff --git a/rust/gateway/src/main.rs b/rust/gateway/src/main.rs index d8c3b65c1..3032ad8b4 100644 --- a/rust/gateway/src/main.rs +++ b/rust/gateway/src/main.rs @@ -61,7 +61,7 @@ fn main() -> ExitCode { .expect("Failed to create tokio runtime"); match runtime - .block_on(try_main(cli, &mut telemetry)) + .block_on(try_main(cli)) .context("Failed to start Gateway") { Ok(ExitCode::SUCCESS) => { @@ -97,13 +97,13 @@ fn has_necessary_permissions() -> bool { is_root || has_net_admin } -async fn try_main(cli: Cli, telemetry: &mut Telemetry) -> Result { +async fn try_main(cli: Cli) -> Result { firezone_logging::setup_global_subscriber(layer::Identity::default()) .context("Failed to set up logging")?; let firezone_id = get_firezone_id(cli.firezone_id).await .context("Couldn't read FIREZONE_ID or write it to disk: Please provide it through the env variable or provide rw access to /var/lib/firezone/")?; - telemetry.set_firezone_id(firezone_id.clone()); + Telemetry::set_firezone_id(firezone_id.clone()); let login = LoginUrl::gateway( cli.api_url, diff --git a/rust/gui-client/src-common/src/controller.rs b/rust/gui-client/src-common/src/controller.rs index dac5ca773..53ece9049 100644 --- a/rust/gui-client/src-common/src/controller.rs +++ b/rust/gui-client/src-common/src/controller.rs @@ -31,7 +31,7 @@ mod ran_before; pub type CtlrTx = mpsc::Sender; -pub struct Controller<'a, I: GuiIntegration> { +pub struct Controller { /// Debugging-only settings like API URL, auth URL, log filter advanced_settings: AdvancedSettings, // Sign-in state with the portal / deep links @@ -46,7 +46,6 @@ pub struct Controller<'a, I: GuiIntegration> { release: Option, rx: ReceiverStream, status: Status, - telemetry: &'a mut Telemetry, updates_rx: ReceiverStream>, uptime: crate::uptime::Tracker, @@ -179,14 +178,13 @@ enum EventloopTick { UpdateNotification(Option>), } -impl Controller<'_, I> { +impl Controller { pub async fn start( ctlr_tx: CtlrTx, integration: I, rx: mpsc::Receiver, advanced_settings: AdvancedSettings, log_filter_reloader: LogFilterReloader, - telemetry: &mut Telemetry, updates_rx: mpsc::Receiver>, ) -> Result<(), Error> { tracing::debug!("Starting new instance of `Controller`"); @@ -208,7 +206,6 @@ impl Controller<'_, I> { release: None, rx: ReceiverStream::new(rx), status: Default::default(), - telemetry, updates_rx: ReceiverStream::new(updates_rx), uptime: Default::default(), dns_notifier, @@ -221,23 +218,7 @@ impl Controller<'_, I> { } pub async fn main_loop(mut self) -> Result<(), Error> { - let account_slug = self.auth.session().map(|s| s.account_slug.to_owned()); - - // Tell IPC service to start telemetry. - { - let environment = self.advanced_settings.api_url.to_string(); - if let Some(account_slug) = account_slug.clone() { - self.telemetry.set_account_slug(account_slug); - } - - self.ipc_client - .send_msg(&IpcClientMsg::StartTelemetry { - environment, - release: crate::RELEASE.to_string(), - account_slug, - }) - .await?; - } + self.update_telemetry_context().await?; if let Some(token) = self .auth @@ -371,6 +352,25 @@ impl Controller<'_, I> { Ok(()) } + async fn update_telemetry_context(&mut self) -> Result<()> { + let environment = self.advanced_settings.api_url.to_string(); + let account_slug = self.auth.session().map(|s| s.account_slug.to_owned()); + + if let Some(account_slug) = account_slug.clone() { + Telemetry::set_account_slug(account_slug); + } + + self.ipc_client + .send_msg(&IpcClientMsg::StartTelemetry { + environment, + release: crate::RELEASE.to_string(), + account_slug, + }) + .await?; + + Ok(()) + } + async fn handle_deep_link(&mut self, url: &SecretString) -> Result<(), Error> { let auth_response = deep_link::parse_auth_callback(url).context("Couldn't parse scheme request")?; @@ -382,7 +382,10 @@ impl Controller<'_, I> { .auth .handle_response(auth_response) .context("Couldn't handle auth response")?; + + self.update_telemetry_context().await?; self.start_session(token).await?; + Ok(()) } diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index 393271368..70c7de022 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -3,7 +3,7 @@ use clap::{Args, Parser}; use firezone_gui_client_common::{ self as common, controller::Failure, deep_link, settings::AdvancedSettings, }; -use firezone_telemetry as telemetry; +use firezone_telemetry::Telemetry; use tracing::instrument; use tracing_subscriber::EnvFilter; @@ -60,11 +60,11 @@ pub(crate) fn run() -> Result<()> { Some(Cmd::SmokeTest) => { // Can't check elevation here because the Windows CI is always elevated let settings = common::settings::load_advanced_settings().unwrap_or_default(); - let mut telemetry = telemetry::Telemetry::default(); + let mut telemetry = Telemetry::default(); telemetry.start( settings.api_url.as_ref(), firezone_gui_client_common::RELEASE, - telemetry::GUI_DSN, + firezone_telemetry::GUI_DSN, ); // Don't fix the log filter for smoke tests let common::logging::Handles { @@ -91,17 +91,17 @@ pub(crate) fn run() -> Result<()> { // Can't `instrument` this because logging isn't running when we enter it. fn run_gui(cli: Cli) -> Result<()> { let mut settings = common::settings::load_advanced_settings().unwrap_or_default(); - let mut telemetry = telemetry::Telemetry::default(); + let mut telemetry = Telemetry::default(); // In the future telemetry will be opt-in per organization, that's why this isn't just at the top of `main` telemetry.start( settings.api_url.as_ref(), firezone_gui_client_common::RELEASE, - telemetry::GUI_DSN, + firezone_telemetry::GUI_DSN, ); // Get the device ID before starting Tokio, so that all the worker threads will inherit the correct scope. // Technically this means we can fail to get the device ID on a newly-installed system, since the IPC service may not have fully started up when the GUI process reaches this point, but in practice it's unlikely. if let Ok(id) = firezone_headless_client::device_id::get() { - telemetry.set_firezone_id(id.id); + Telemetry::set_firezone_id(id.id); } fix_log_filter(&mut settings)?; let common::logging::Handles { diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index cf48fb528..1704313a7 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -242,7 +242,6 @@ pub(crate) fn run( ctlr_rx, advanced_settings, reloader, - &mut telemetry, updates_rx, )).catch_unwind().await; diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index d6951ba41..ea9bd5cb2 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -257,7 +257,7 @@ async fn ipc_listen( .context("Failed to read / create device ID")? .id; - telemetry.set_firezone_id(firezone_id); + Telemetry::set_firezone_id(firezone_id); let mut server = IpcServer::new(ServiceId::Prod).await?; let mut dns_controller = DnsController { dns_control_method }; @@ -564,7 +564,7 @@ impl<'a> Handler<'a> { .start(&environment, &release, firezone_telemetry::GUI_DSN); if let Some(account_slug) = account_slug { - self.telemetry.set_account_slug(account_slug); + Telemetry::set_account_slug(account_slug); } } } @@ -581,7 +581,7 @@ impl<'a> Handler<'a> { assert!(self.session.is_none()); let device_id = device_id::get_or_create().context("Failed to get-or-create device ID")?; - self.telemetry.set_firezone_id(device_id.id.clone()); + Telemetry::set_firezone_id(device_id.id.clone()); let url = LoginUrl::client( Url::parse(api_url).context("Failed to parse URL")?, diff --git a/rust/headless-client/src/main.rs b/rust/headless-client/src/main.rs index 8ecb29660..372504f4a 100644 --- a/rust/headless-client/src/main.rs +++ b/rust/headless-client/src/main.rs @@ -179,7 +179,7 @@ fn main() -> Result<()> { Some(id) => id, None => device_id::get_or_create().context("Could not get `firezone_id` from CLI, could not read it from disk, could not generate it and save it to disk")?.id, }; - telemetry.set_firezone_id(firezone_id.clone()); + Telemetry::set_firezone_id(firezone_id.clone()); let url = LoginUrl::client( cli.api_url, diff --git a/rust/telemetry/src/lib.rs b/rust/telemetry/src/lib.rs index 137adb6b9..26181607b 100644 --- a/rust/telemetry/src/lib.rs +++ b/rust/telemetry/src/lib.rs @@ -143,26 +143,26 @@ impl Telemetry { .await; } - pub fn set_account_slug(&mut self, slug: String) { - self.update_user(|user| { + pub fn set_account_slug(slug: String) { + update_user(|user| { user.other.insert("account_slug".to_owned(), slug.into()); }); } - pub fn set_firezone_id(&mut self, id: String) { - self.update_user(|user| { + pub fn set_firezone_id(id: String) { + update_user(|user| { user.id = Some(id); }); } +} - fn update_user(&mut self, update: impl FnOnce(&mut sentry::User)) { - sentry::Hub::main().configure_scope(|scope| { - let mut user = scope.user().cloned().unwrap_or_default(); - update(&mut user); +fn update_user(update: impl FnOnce(&mut sentry::User)) { + sentry::Hub::main().configure_scope(|scope| { + let mut user = scope.user().cloned().unwrap_or_default(); + update(&mut user); - scope.set_user(Some(user)); - }); - } + scope.set_user(Some(user)); + }); } fn set_current_user(user: Option) {