diff --git a/Cargo.lock b/Cargo.lock index feabf0c81..3ac198b84 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -633,6 +633,7 @@ dependencies = [ "autoconnect_settings", "autoconnect_ws_sm", "autopush_common", + "backtrace", "ctor", "futures 0.3.28", "mockall", @@ -654,11 +655,13 @@ dependencies = [ "autoconnect_common", "autoconnect_settings", "autopush_common", + "backtrace", "cadence", "ctor", "futures 0.3.28", "mockall", "reqwest 0.11.18", + "sentry", "slog-scope", "thiserror", "tokio 1.28.2", diff --git a/autoconnect/autoconnect-ws/Cargo.toml b/autoconnect/autoconnect-ws/Cargo.toml index 78868b39e..f1fcbf848 100644 --- a/autoconnect/autoconnect-ws/Cargo.toml +++ b/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 diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/Cargo.toml b/autoconnect/autoconnect-ws/autoconnect-ws-sm/Cargo.toml index 00f7fb8c8..002e51064 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/Cargo.toml +++ b/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 diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs index 5d6e621bb..364803036 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs +++ b/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. +impl From for SMError +where + SMErrorKind: From, +{ + 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, - } - } -} diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs index 059a3071c..4b47b2aba 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs +++ b/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 diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs index 575fb6c7c..6f20867f0 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs +++ b/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, 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, - ) -> Result { + ) -> Result { 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![]) } diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs index 5e00cc524..d5d7fc47c 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs +++ b/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(()) } diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs index 5a2be676d..339633b3e 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs +++ b/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] diff --git a/autoconnect/autoconnect-ws/src/error.rs b/autoconnect/autoconnect-ws/src/error.rs index a6a15768d..977133927 100644 --- a/autoconnect/autoconnect-ws/src/error.rs +++ b/autoconnect/autoconnect-ws/src/error.rs @@ -1,11 +1,81 @@ +use std::fmt; + use actix_ws::CloseCode; +use backtrace::Backtrace; -use autoconnect_ws_sm::{SMError, WebPushClient}; +use autoconnect_ws_sm::SMError; +use autopush_common::errors::ReportableError; -// TODO: WSError should likely include a backtrace /// WebPush WebSocket Handler Errors +#[derive(Debug, thiserror::Error)] +pub struct WSError { + pub kind: WSErrorKind, + backtrace: Option, +} + +impl fmt::Display for WSError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.kind) + } +} + +// Forward From impls to WSError from WSErrorKind. Because From is reflexive, +// this impl also takes care of From. +impl From for WSError +where + WSErrorKind: From, +{ + fn from(item: T) -> Self { + let kind = WSErrorKind::from(item); + let backtrace = kind.capture_backtrace().then(Backtrace::new); + Self { kind, backtrace } + } +} + +impl WSError { + /// Return an `actix_ws::CloseCode` for the WS session Close frame + pub fn close_code(&self) -> actix_ws::CloseCode { + match &self.kind { + WSErrorKind::SM(e) => e.close_code(), + // TODO: applicable here? + //WSErrorKind::Protocol(_) => CloseCode::Protocol, + WSErrorKind::UnsupportedMessage(_) => CloseCode::Unsupported, + _ => CloseCode::Error, + } + } + + /// Return a description for the WS session Close frame. + /// + /// Control frames are limited to 125 bytes so returns just the enum + /// variant's name (via `strum::AsRefStr`) + pub fn close_description(&self) -> &str { + self.kind.as_ref() + } +} + +impl ReportableError for WSError { + fn backtrace(&self) -> Option<&Backtrace> { + self.backtrace.as_ref() + } + + fn is_sentry_event(&self) -> bool { + match &self.kind { + WSErrorKind::SM(e) => e.is_sentry_event(), + e => !matches!(e, WSErrorKind::Json(_)), + } + } + + fn metric_label(&self) -> Option<&'static str> { + match &self.kind { + WSErrorKind::SM(e) => e.metric_label(), + // TODO: + _ => None, + } + } +} + #[derive(Debug, strum::AsRefStr, thiserror::Error)] -pub enum WSError { +pub enum WSErrorKind { #[error("State error: {0}")] SM(#[from] SMError), @@ -34,58 +104,15 @@ pub enum WSError { RegistryDisconnected, } -impl WSError { - /// Return an `actix_ws::CloseCode` for the WS session Close frame - pub fn close_code(&self) -> actix_ws::CloseCode { - match self { - WSError::SM(e) => e.close_code(), - // TODO: applicable here? - //WSError::Protocol(_) => CloseCode::Protocol, - WSError::UnsupportedMessage(_) => CloseCode::Unsupported, - _ => CloseCode::Error, - } - } - - /// Return a description for the WS session Close frame. +impl WSErrorKind { + /// Whether this variant has a `Backtrace` captured /// - /// Control frames are limited to 125 bytes so returns just the enum - /// variant's name (via `strum::AsRefStr`) - pub fn close_description(&self) -> &str { - self.as_ref() - } - - /// Whether this error is reported to sentry - pub fn is_sentry_event(&self) -> bool { - !matches!(self, WSError::Json(_)) - } - - /// Create a sentry Event from this Error with `WebPushClient` information - pub fn to_sentry_event(&self, client: &WebPushClient) -> sentry::protocol::Event<'static> { - let mut event = sentry::event_from_error(self); - // TODO: - //event.exception.last_mut().unwrap().stacktrace = - // sentry::integrations::backtrace::backtrace_to_stacktrace(&self.backtrace); - - event.user = Some(sentry::User { - id: Some(client.uaid.as_simple().to_string()), - ..Default::default() - }); - let ua_info = client.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); - event + /// Some Error variants have obvious call sites and thus don't need a + /// `Backtrace` + fn capture_backtrace(&self) -> bool { + matches!( + self, + WSErrorKind::Json(_) | WSErrorKind::Protocol(_) | WSErrorKind::SessionClosed(_) + ) } } diff --git a/autoconnect/autoconnect-ws/src/handler.rs b/autoconnect/autoconnect-ws/src/handler.rs index 66508e409..eada619de 100644 --- a/autoconnect/autoconnect-ws/src/handler.rs +++ b/autoconnect/autoconnect-ws/src/handler.rs @@ -7,9 +7,10 @@ use tokio::{select, time::timeout}; use autoconnect_common::protocol::{ServerMessage, ServerNotification}; use autoconnect_settings::AppState; use autoconnect_ws_sm::{UnidentifiedClient, WebPushClient}; +use autopush_common::{errors::ReportableError, sentry::event_from_error}; use crate::{ - error::WSError, + error::{WSError, WSErrorKind}, ping::PingManager, session::{Session, SessionImpl}, }; @@ -75,7 +76,9 @@ pub(crate) async fn webpush_ws( if let Err(ref e) = result { if e.is_sentry_event() { - sentry::capture_event(e.to_sentry_event(&client)); + let mut event = event_from_error(e); + client.add_sentry_info(&mut event); + sentry::capture_event(event); } } result @@ -95,15 +98,15 @@ async fn unidentified_ws( ); let msg = match stream_with_timeout.await { Ok(Some(msg)) => msg?, - Ok(None) => return Err(WSError::StreamClosed), - Err(_) => return Err(WSError::HandshakeTimeout), + Ok(None) => return Err(WSErrorKind::StreamClosed.into()), + Err(_) => return Err(WSErrorKind::HandshakeTimeout.into()), }; trace!("unidentified_ws: Handshake msg: {:?}", msg); let client_msg = match msg { Message::Text(ref bytestring) => bytestring.parse()?, _ => { - return Err(WSError::UnsupportedMessage("Expected Text".to_owned())); + return Err(WSErrorKind::UnsupportedMessage("Expected Text".to_owned()).into()); } }; @@ -151,7 +154,7 @@ async fn identified_ws( ping_manager.on_ws_pong(client.app_settings()).await; continue; }, - _ => return Err(WSError::UnsupportedMessage("Expected Text, etc.".to_owned())) + _ => return Err(WSErrorKind::UnsupportedMessage("Expected Text, etc.".to_owned()).into()) }; for smsg in client.on_client_msg(client_msg).await? { trace!("identified_ws: msg_stream, ServerMessage -> session {:#?}", smsg); @@ -162,7 +165,7 @@ async fn identified_ws( maybe_snotif = snotif_stream.next() => { let Some(snotif) = maybe_snotif else { trace!("identified_ws: snotif_stream EOF"); - return Err(WSError::RegistryDisconnected); + return Err(WSErrorKind::RegistryDisconnected.into()); }; for smsg in client.on_server_notif(snotif).await? { trace!("identified_ws: snotif_stream, ServerMessage -> session {:#?}", smsg); diff --git a/autoconnect/autoconnect-ws/src/ping.rs b/autoconnect/autoconnect-ws/src/ping.rs index a90461e89..930482589 100644 --- a/autoconnect/autoconnect-ws/src/ping.rs +++ b/autoconnect/autoconnect-ws/src/ping.rs @@ -4,7 +4,10 @@ use autoconnect_common::{broadcast::Broadcast, protocol::ServerMessage}; use autoconnect_settings::Settings; use autoconnect_ws_sm::WebPushClient; -use crate::{error::WSError, session::Session}; +use crate::{ + error::{WSError, WSErrorKind}, + session::Session, +}; #[derive(Debug)] enum Waiting { @@ -54,7 +57,7 @@ impl PingManager { self.ping_or_timeout.tick().await; match self.waiting { Waiting::ToPing => Ok(()), - Waiting::ForPong => Err(WSError::PongTimeout), + Waiting::ForPong => Err(WSErrorKind::PongTimeout.into()), } } diff --git a/autoconnect/autoconnect-ws/src/test.rs b/autoconnect/autoconnect-ws/src/test.rs index 521d431a3..009a44405 100644 --- a/autoconnect/autoconnect-ws/src/test.rs +++ b/autoconnect/autoconnect-ws/src/test.rs @@ -10,7 +10,7 @@ use autoconnect_common::{ use autoconnect_settings::{AppState, Settings}; use autoconnect_ws_sm::UnidentifiedClient; -use crate::{error::WSError, handler::webpush_ws, session::MockSession}; +use crate::{error::WSErrorKind, handler::webpush_ws, session::MockSession}; #[ctor::ctor] fn init_test_logging() { @@ -37,7 +37,7 @@ async fn handshake_timeout() { let err = webpush_ws(client, &mut MockSession::new(), s) .await .unwrap_err(); - assert!(matches!(err, WSError::HandshakeTimeout)); + assert!(matches!(err.kind, WSErrorKind::HandshakeTimeout)); } #[actix_web::test] @@ -108,7 +108,7 @@ async fn auto_ping_timeout() { }; pin_mut!(s); let err = webpush_ws(client, &mut session, s).await.unwrap_err(); - assert!(matches!(err, WSError::PongTimeout)); + assert!(matches!(err.kind, WSErrorKind::PongTimeout)); } #[actix_web::test] @@ -134,5 +134,5 @@ async fn auto_ping_timeout_after_pong() { }; pin_mut!(s); let err = webpush_ws(client, &mut session, s).await.unwrap_err(); - assert!(matches!(err, WSError::PongTimeout)); + assert!(matches!(err.kind, WSErrorKind::PongTimeout)); } diff --git a/autopush-common/src/errors.rs b/autopush-common/src/errors.rs index 4eef2bbd2..72e708145 100644 --- a/autopush-common/src/errors.rs +++ b/autopush-common/src/errors.rs @@ -13,6 +13,8 @@ use backtrace::Backtrace; // Sentry 0.29 uses the backtrace crate, not std::back use serde::ser::{Serialize, SerializeMap, Serializer}; use thiserror::Error; +pub type Result = std::result::Result; + /// Render a 404 response pub fn render_404( res: ServiceResponse, @@ -171,17 +173,25 @@ impl ApcErrorKind { pub fn metric_label(&self) -> Option { // TODO: add labels for skipped stuff - let resp = match self { + let label = match self { Self::PongTimeout => "pong_timeout", Self::ExcessivePing => "excessive_ping", Self::PayloadError(_) => "payload", - _ => "", + _ => return None, }; - if !resp.is_empty() { - return Some(resp.to_owned()); - } - None + Some(label.to_owned()) } } -pub type Result = std::result::Result; +/// Interface for reporting our Error types to Sentry or as metrics +pub trait ReportableError: std::error::Error + fmt::Display { + /// Return a `Backtrace` for this Error if one was captured + fn backtrace(&self) -> Option<&Backtrace>; + + /// Whether this error is reported to Sentry + fn is_sentry_event(&self) -> bool; + + /// Errors that don't emit Sentry events (!is_sentry_event()) emit an + /// increment metric instead with this label + fn metric_label(&self) -> Option<&'static str>; +} diff --git a/autopush-common/src/sentry.rs b/autopush-common/src/sentry.rs index d890ac296..71c6480e1 100644 --- a/autopush-common/src/sentry.rs +++ b/autopush-common/src/sentry.rs @@ -1,3 +1,7 @@ +use std::error::Error; + +use crate::errors::ReportableError; + /// Return a `sentry::::ClientOptions` w/ the `debug-images` integration /// disabled pub fn client_options() -> sentry::ClientOptions { @@ -8,3 +12,57 @@ pub fn client_options() -> sentry::ClientOptions { opts.default_integrations = false; opts } + +/// Custom `sentry::event_from_error` for `ReportableError` +/// +/// `std::error::Error` doesn't support backtraces, thus `sentry::event_from_error` +/// doesn't either. This function works against `ReportableError` instead to +/// access its backtrace. +pub fn event_from_error(err: &E) -> sentry::protocol::Event<'static> +where + E: ReportableError + 'static, +{ + let mut exceptions = vec![exception_from_error_with_backtrace(err)]; + + let mut source = err.source(); + while let Some(err) = source { + let exception = if let Some(err) = err.downcast_ref::() { + exception_from_error_with_backtrace(err) + } else { + exception_from_error(err) + }; + exceptions.push(exception); + source = err.source(); + } + + exceptions.reverse(); + sentry::protocol::Event { + exception: exceptions.into(), + level: sentry::protocol::Level::Error, + ..Default::default() + } +} + +/// Custom `exception_from_error` support function for `ReportableError` +/// +/// Based moreso on sentry_failure's `exception_from_single_fail`. +fn exception_from_error_with_backtrace(err: &dyn ReportableError) -> sentry::protocol::Exception { + let mut exception = exception_from_error(err); + if let Some(backtrace) = err.backtrace() { + exception.stacktrace = sentry_backtrace::backtrace_to_stacktrace(backtrace) + } + exception +} + +/// Exact copy of sentry's unfortunately private `exception_from_error` +fn exception_from_error(err: &E) -> sentry::protocol::Exception +where + E: Error + ?Sized, +{ + let dbg = format!("{:?}", err); + sentry::protocol::Exception { + ty: sentry::parse_type_from_debug(&dbg).to_owned(), + value: Some(err.to_string()), + ..Default::default() + } +}