fix: add ReportableError::reportable_source (#500)

* fix: add ReportableError::reportable_source

for event_from_error to workaround its issues w/ downcast_ref

SYNC-3981

Author: pjenvey

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

@@ -103,3 +103,18 @@ pub enum SMErrorKind {
     #[error("Client sent too many pings too often")]
     ExcessivePing,
 }
+
+#[cfg(debug_assertions)]
+/// Return a [SMErrorKind::Reqwest] [SMError] for tests
+pub async fn __test_sm_reqwest_error() -> SMError {
+    // An easily constructed reqwest::Error
+    let e = reqwest::Client::builder()
+        .https_only(true)
+        .build()
+        .unwrap()
+        .get("http://example.com")
+        .send()
+        .await
+        .unwrap_err();
+    SMErrorKind::Reqwest(e).into()
+}

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

@@ -8,3 +8,6 @@ mod unidentified;
 pub use error::SMError;
 pub use identified::WebPushClient;
 pub use unidentified::UnidentifiedClient;
+
+#[cfg(debug_assertions)]
+pub use error::__test_sm_reqwest_error;

autoconnect/autoconnect-ws/src/error.rs

@@ -72,15 +72,17 @@ impl WSError {
 }
 
 impl ReportableError for WSError {
-    fn backtrace(&self) -> Option<&Backtrace> {
-        // XXX: dumb hack: return SMError's backtrace for now as our
-        // sentry::event_from_error doesn't capture it
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
         match &self.kind {
-            WSErrorKind::SM(e) => e.backtrace(),
-            _ => self.backtrace.as_ref(),
+            WSErrorKind::SM(e) => Some(e),
+            _ => None,
         }
     }
 
+    fn backtrace(&self) -> Option<&Backtrace> {
+        self.backtrace.as_ref()
+    }
+
     fn is_sentry_event(&self) -> bool {
         self.kind.is_sentry_event()
     }
@@ -147,3 +149,31 @@ impl WSErrorKind {
         false
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use autoconnect_ws_sm::__test_sm_reqwest_error;
+    use autopush_common::sentry::event_from_error;
+
+    use super::{WSError, WSErrorKind};
+
+    #[actix_web::test]
+    async fn sentry_event() {
+        // A chain of errors: SMError -> WSError -> reqwest::Error -> BadScheme
+        let e: WSError = WSErrorKind::SM(__test_sm_reqwest_error().await).into();
+        let event = event_from_error(&e);
+        assert_eq!(event.exception.len(), 4);
+
+        // Source of the reqwest::Error (BadScheme)
+        assert_eq!(event.exception[0].stacktrace, None);
+        // reqwest::Error
+        assert_eq!(event.exception[1].ty, "reqwest::Error");
+        assert_eq!(event.exception[1].stacktrace, None);
+        // SMError w/ ReportableError::backtrace
+        assert_eq!(event.exception[2].ty, "SMError");
+        assert!(event.exception[2].stacktrace.is_some());
+        // WSError
+        assert_eq!(event.exception[3].ty, "WSError");
+        assert_eq!(event.exception[3].stacktrace, None);
+    }
+}

autoendpoint/src/error.rs

@@ -332,6 +332,13 @@ impl Serialize for ApiError {
 }
 
 impl ReportableError for ApiError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        match &self.kind {
+            ApiErrorKind::EndpointUrl(e) => Some(e),
+            _ => None,
+        }
+    }
+
     fn backtrace(&self) -> Option<&Backtrace> {
         Some(&self.backtrace)
     }

autopush-common/src/errors.rs

@@ -181,6 +181,14 @@ impl ApcErrorKind {
 
 /// Interface for reporting our Error types to Sentry or as metrics
 pub trait ReportableError: std::error::Error {
+    /// Like [Error::source] but returns the source (if any) of this error as a
+    /// [ReportableError] if it implements the trait. Otherwise callers of this
+    /// method will likely subsequently call [Error::source] to return the
+    /// source (if any) as the parent [Error] trait.
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        None
+    }
+
     /// Return a `Backtrace` for this Error if one was captured
     fn backtrace(&self) -> Option<&Backtrace>;
 

autopush-common/src/sentry.rs

@@ -17,21 +17,30 @@ pub fn client_options() -> sentry::ClientOptions {
 ///
 /// `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<E>(err: &E) -> sentry::protocol::Event<'static>
-where
-    E: ReportableError + 'static,
-{
-    let mut exceptions = vec![exception_from_error_with_backtrace(err)];
+/// extract backtraces from it and its chain of `reportable_source's.
+///
+/// A caveat of this function is that it cannot extract
+/// `ReportableError`s/backtraces that occur in a chain after a
+/// `std::error::Error` occurs: as `std::error::Error::source` only allows
+/// downcasting to a concrete type, not `dyn ReportableError`.
+pub fn event_from_error(
+    mut reportable_err: &dyn ReportableError,
+) -> sentry::protocol::Event<'static> {
+    let mut exceptions = vec![];
 
-    let mut source = err.source();
-    while let Some(err) = source {
-        let exception = if let Some(err) = err.downcast_ref::<E>() {
-            exception_from_error_with_backtrace(err)
-        } else {
-            exception_from_error(err)
+    // Gather reportable_source()'s for their backtraces
+    loop {
+        exceptions.push(exception_from_reportable_error(reportable_err));
+        reportable_err = match reportable_err.reportable_source() {
+            Some(reportable_err) => reportable_err,
+            None => break,
         };
-        exceptions.push(exception);
+    }
+
+    // Then fallback to source() for remaining Errors
+    let mut source = reportable_err.source();
+    while let Some(err) = source {
+        exceptions.push(exception_from_error(err));
         source = err.source();
     }
 
@@ -45,8 +54,9 @@ where
 
 /// 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 {
+/// Based moreso on sentry_failure's `exception_from_single_fail`. Includes a
+/// stacktrace if available.
+fn exception_from_reportable_error(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)