feat: add stacktraces to some sentry events (#406)

SYNC-3822

autoconnect/autoconnect-ws/Cargo.toml

@@ -11,6 +11,7 @@ actix-http.workspace = true
 actix-rt.workspace = true
 actix-web.workspace = true
 actix-ws.workspace = true
+backtrace.workspace = true
 futures.workspace = true
 mockall.workspace = true
 serde_json.workspace = true
@@ -25,10 +26,10 @@ strum = { version = "0.24", features = ["derive"] }
 autoconnect_common.workspace = true
 autoconnect_settings.workspace = true
 autoconnect_ws_sm = { path = "./autoconnect-ws-sm" }
+autopush_common.workspace = true
 
 [dev-dependencies]
 async-stream = "0.3"
 ctor.workspace = true
 
 autoconnect_common = { workspace = true, features = ["test-support"] }
-autopush_common.workspace = true

autoconnect/autoconnect-ws/autoconnect-ws-sm/Cargo.toml

@@ -9,9 +9,11 @@ authors.workspace = true
 [dependencies]
 actix-web.workspace = true
 actix-ws.workspace = true
+backtrace.workspace = true
 cadence.workspace = true
 futures.workspace = true
 reqwest.workspace = true
+sentry.workspace = true
 slog-scope.workspace = true
 uuid.workspace = true
 thiserror.workspace = true

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs

@@ -1,10 +1,81 @@
+use std::fmt;
+
 use actix_ws::CloseCode;
+use backtrace::Backtrace;
+
+use autopush_common::{db::error::DbError, errors::ReportableError};
 
-use autopush_common::db::error::DbError;
+pub type NonStdBacktrace = backtrace::Backtrace;
 
 /// WebSocket state machine errors
+#[derive(Debug, thiserror::Error)]
+pub struct SMError {
+    pub kind: SMErrorKind,
+    /// Avoid thiserror's automatic `std::backtrace::Backtrace` integration by
+    /// not using the type name "Backtrace". The older `backtrace::Backtrace`
+    /// is still preferred for Sentry integration:
+    /// https://github.com/getsentry/sentry-rust/issues/600
+    backtrace: NonStdBacktrace,
+}
+
+impl fmt::Display for SMError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{}", self.kind)
+    }
+}
+
+// Forward From impls to SMError from SMErrorKind. Because From is reflexive,
+// this impl also takes care of From<SMErrorKind>.
+impl<T> From<T> for SMError
+where
+    SMErrorKind: From<T>,
+{
+    fn from(item: T) -> Self {
+        Self {
+            kind: SMErrorKind::from(item),
+            backtrace: Backtrace::new(),
+        }
+    }
+}
+
+impl SMError {
+    pub fn close_code(&self) -> actix_ws::CloseCode {
+        match self.kind {
+            // TODO: applicable here?
+            //SMErrorKind::InvalidMessage(_) => CloseCode::Invalid,
+            SMErrorKind::UaidReset => CloseCode::Normal,
+            _ => CloseCode::Error,
+        }
+    }
+
+    pub fn invalid_message(description: String) -> Self {
+        SMErrorKind::InvalidMessage(description).into()
+    }
+}
+
+impl ReportableError for SMError {
+    fn backtrace(&self) -> Option<&Backtrace> {
+        Some(&self.backtrace)
+    }
+
+    fn is_sentry_event(&self) -> bool {
+        matches!(
+            self.kind,
+            SMErrorKind::Database(_)
+                | SMErrorKind::Internal(_)
+                | SMErrorKind::Reqwest(_)
+                | SMErrorKind::MakeEndpoint(_)
+        )
+    }
+
+    fn metric_label(&self) -> Option<&'static str> {
+        // TODO:
+        None
+    }
+}
+
 #[derive(thiserror::Error, Debug)]
-pub enum SMError {
+pub enum SMErrorKind {
     #[error("Database error: {0}")]
     Database(#[from] DbError),
 
@@ -32,14 +103,3 @@ pub enum SMError {
     #[error("Client sent too many pings too often")]
     ExcessivePing,
 }
-
-impl SMError {
-    pub fn close_code(&self) -> actix_ws::CloseCode {
-        match self {
-            // TODO: applicable here?
-            //SMError::InvalidMessage(_) => CloseCode::Invalid,
-            SMError::UaidReset => CloseCode::Normal,
-            _ => CloseCode::Error,
-        }
-    }
-}

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs

@@ -17,7 +17,7 @@ use autopush_common::{
     util::{ms_since_epoch, user_agent::UserAgentInfo},
 };
 
-use crate::error::SMError;
+use crate::error::{SMError, SMErrorKind};
 
 mod on_client_msg;
 mod on_server_notif;
@@ -224,7 +224,7 @@ impl WebPushClient {
             app_state.db.save_messages(&uaid, notifs).await?;
             debug!("Finished saving unacked direct notifs, checking for reconnect");
             let Some(user) = app_state.db.get_user(&uaid).await? else {
-                return Err(SMError::Internal(format!("User not found for unacked direct notifs: {uaid}")));
+                return Err(SMErrorKind::Internal(format!("User not found for unacked direct notifs: {uaid}")));
             };
             if connected_at == user.connected_at {
                 return Ok(());
@@ -241,6 +241,30 @@ impl WebPushClient {
         });
     }
 
+    /// Add User information and tags for this Client to a Sentry Event
+    pub fn add_sentry_info(&self, event: &mut sentry::protocol::Event) {
+        event.user = Some(sentry::User {
+            id: Some(self.uaid.as_simple().to_string()),
+            ..Default::default()
+        });
+        let ua_info = self.ua_info.clone();
+        event
+            .tags
+            .insert("ua_name".to_owned(), ua_info.browser_name);
+        event
+            .tags
+            .insert("ua_os_family".to_owned(), ua_info.metrics_os);
+        event
+            .tags
+            .insert("ua_os_ver".to_owned(), ua_info.os_version);
+        event
+            .tags
+            .insert("ua_browser_family".to_owned(), ua_info.metrics_browser);
+        event
+            .tags
+            .insert("ua_browser_ver".to_owned(), ua_info.browser_version);
+    }
+
     /// Return a reference to `AppState`'s `Settings`
     pub fn app_settings(&self) -> &Settings {
         &self.app_state.settings

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs

@@ -10,7 +10,7 @@ use autoconnect_common::{
 use autopush_common::{endpoint::make_endpoint, util::sec_since_epoch};
 
 use super::WebPushClient;
-use crate::error::SMError;
+use crate::error::{SMError, SMErrorKind};
 
 impl WebPushClient {
     /// Handle a WebPush `ClientMessage` sent from the user agent over the
@@ -21,7 +21,7 @@ impl WebPushClient {
     ) -> Result<Vec<ServerMessage>, SMError> {
         match msg {
             ClientMessage::Hello { .. } => {
-                Err(SMError::InvalidMessage("Already Hello'd".to_owned()))
+                Err(SMError::invalid_message("Already Hello'd".to_owned()))
             }
             ClientMessage::Register { channel_id, key } => {
                 Ok(vec![self.register(channel_id, key).await?])
@@ -53,10 +53,11 @@ impl WebPushClient {
                "channel_id" => &channel_id_str,
                "key" => &key,
         );
-        let channel_id = Uuid::try_parse(&channel_id_str)
-            .map_err(|_| SMError::InvalidMessage(format!("Invalid channelID: {channel_id_str}")))?;
+        let channel_id = Uuid::try_parse(&channel_id_str).map_err(|_| {
+            SMError::invalid_message(format!("Invalid channelID: {channel_id_str}"))
+        })?;
         if channel_id.as_hyphenated().to_string() != channel_id_str {
-            return Err(SMError::InvalidMessage(format!(
+            return Err(SMError::invalid_message(format!(
                 "Invalid UUID format, not lower-case/dashed: {channel_id}",
             )));
         }
@@ -67,7 +68,7 @@ impl WebPushClient {
                 self.stats.registers += 1;
                 (200, endpoint)
             }
-            Err(SMError::MakeEndpoint(msg)) => {
+            Err(SMErrorKind::MakeEndpoint(msg)) => {
                 error!("WebPushClient::register make_endpoint failed: {}", msg);
                 (400, "Failed to generate endpoint".to_owned())
             }
@@ -87,7 +88,7 @@ impl WebPushClient {
         &mut self,
         channel_id: &Uuid,
         key: Option<String>,
-    ) -> Result<String, SMError> {
+    ) -> Result<String, SMErrorKind> {
         if let Some(user) = &self.deferred_add_user {
             debug!(
                 "💬WebPushClient::register: User not yet registered: {}",
@@ -104,7 +105,7 @@ impl WebPushClient {
             &self.app_state.endpoint_url,
             &self.app_state.fernet,
         )
-        .map_err(|e| SMError::MakeEndpoint(e.to_string()))?;
+        .map_err(|e| SMErrorKind::MakeEndpoint(e.to_string()))?;
         self.app_state
             .db
             .add_channel(&self.uaid, channel_id)
@@ -264,7 +265,7 @@ impl WebPushClient {
             self.last_ping = sec_since_epoch();
             Ok(ServerMessage::Ping)
         } else {
-            Err(SMError::ExcessivePing)
+            Err(SMErrorKind::ExcessivePing.into())
         }
     }
 
@@ -301,7 +302,7 @@ impl WebPushClient {
         if flags.reset_uaid {
             debug!("▶️ WebPushClient:post_process_all_acked reset_uaid");
             self.app_state.db.remove_user(&self.uaid).await?;
-            Err(SMError::UaidReset)
+            Err(SMErrorKind::UaidReset.into())
         } else {
             Ok(vec![])
         }

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs

@@ -6,7 +6,7 @@ use autopush_common::{
 };
 
 use super::WebPushClient;
-use crate::error::SMError;
+use crate::error::{SMError, SMErrorKind};
 
 impl WebPushClient {
     /// Handle a `ServerNotification` for this user
@@ -23,7 +23,7 @@ impl WebPushClient {
         match snotif {
             ServerNotification::Notification(notif) => Ok(vec![self.notif(notif)?]),
             ServerNotification::CheckStorage => self.check_storage().await,
-            ServerNotification::Disconnect => Err(SMError::Ghost),
+            ServerNotification::Disconnect => Err(SMErrorKind::Ghost.into()),
         }
     }
 
@@ -240,7 +240,7 @@ impl WebPushClient {
             self.ack_state.unacked_stored_highest
         );
         let Some(timestamp) = self.ack_state.unacked_stored_highest else {
-            return Err(SMError::Internal("increment_storage w/ no unacked_stored_highest".to_owned()));
+            return Err(SMErrorKind::Internal("increment_storage w/ no unacked_stored_highest".to_owned()).into());
         };
         self.app_state
             .db
@@ -253,7 +253,7 @@ impl WebPushClient {
     /// Ensure this user hasn't exceeded the maximum allowed number of messages
     /// read from storage (`Settings::msg_limit`)
     ///
-    /// Drops the user record and returns the `SMError::UaidReset` error if
+    /// Drops the user record and returns the `SMErrorKind::UaidReset` error if
     /// they have
     async fn check_msg_limit(&mut self) -> Result<(), SMError> {
         trace!(
@@ -265,7 +265,7 @@ impl WebPushClient {
             // Exceeded the max limit of stored messages: drop the user to
             // trigger a re-register
             self.app_state.db.remove_user(&self.uaid).await?;
-            return Err(SMError::UaidReset);
+            return Err(SMErrorKind::UaidReset.into());
         }
         Ok(())
     }

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs

@@ -54,7 +54,7 @@ impl UnidentifiedClient {
             broadcasts,
             ..
         } = msg else {
-            return Err(SMError::InvalidMessage(
+            return Err(SMError::invalid_message(
                 r#"Expected messageType="hello", "use_webpush": true"#.to_owned()
             ));
         };
@@ -67,7 +67,7 @@ impl UnidentifiedClient {
             .as_deref()
             .map(Uuid::try_parse)
             .transpose()
-            .map_err(|_| SMError::InvalidMessage("Invalid uaid".to_owned()))?;
+            .map_err(|_| SMError::invalid_message("Invalid uaid".to_owned()))?;
 
         let GetOrCreateUser {
             user,
@@ -194,7 +194,7 @@ mod tests {
     };
     use autoconnect_settings::AppState;
 
-    use crate::error::SMError;
+    use crate::error::SMErrorKind;
 
     use super::UnidentifiedClient;
 
@@ -210,17 +210,23 @@ mod tests {
     #[tokio::test]
     async fn reject_not_hello() {
         let client = uclient(Default::default());
-        let result = client.on_client_msg(ClientMessage::Ping).await;
-        assert!(matches!(result, Err(SMError::InvalidMessage(_))));
+        let err = client
+            .on_client_msg(ClientMessage::Ping)
+            .await
+            .err()
+            .unwrap();
+        assert!(matches!(err.kind, SMErrorKind::InvalidMessage(_)));
 
         let client = uclient(Default::default());
-        let result = client
+        let err = client
             .on_client_msg(ClientMessage::Register {
                 channel_id: DUMMY_CHID.to_string(),
                 key: None,
             })
-            .await;
-        assert!(matches!(result, Err(SMError::InvalidMessage(_))));
+            .await
+            .err()
+            .unwrap();
+        assert!(matches!(err.kind, SMErrorKind::InvalidMessage(_)));
     }
 
     #[tokio::test]