feat: Make more errors use ReportableError trait (#632)

Closes SYNC-4145

autoconnect/autoconnect-web/src/error.rs

@@ -1,6 +1,5 @@
 use actix_http::ws::HandshakeError;
 use actix_web::{error::ResponseError, http::StatusCode, HttpResponse};
-use backtrace::Backtrace;
 use serde_json::json;
 
 use autopush_common::errors::ReportableError;
@@ -34,21 +33,13 @@ impl ResponseError for ApiError {
 }
 
 impl ReportableError for ApiError {
-    fn backtrace(&self) -> Option<&Backtrace> {
-        None
-    }
-
     fn is_sentry_event(&self) -> bool {
         match self {
             // Ignore failing upgrade to WebSocket
             ApiError::Actix(e) => e.as_error::<HandshakeError>().is_none(),
             _ => true,
         }
     }
-
-    fn metric_label(&self) -> Option<&'static str> {
-        None
-    }
 }
 
 impl ApiError {

autoendpoint/src/routers/apns/error.rs

@@ -1,6 +1,7 @@
 use crate::error::ApiErrorKind;
 use crate::routers::RouterError;
 use actix_web::http::StatusCode;
+use autopush_common::errors::ReportableError;
 use std::io;
 
 /// Errors that may occur in the Apple Push Notification Service router
@@ -86,3 +87,24 @@ impl From<ApnsError> for ApiErrorKind {
         ApiErrorKind::Router(RouterError::Apns(e))
     }
 }
+
+impl ReportableError for ApnsError {
+    fn is_sentry_event(&self) -> bool {
+        !matches!(self, ApnsError::SizeLimit(_) | ApnsError::Unregistered)
+    }
+
+    fn metric_label(&self) -> Option<&'static str> {
+        match &self {
+            ApnsError::SizeLimit(_) => Some("notification.bridge.error.apns.oversized"),
+            ApnsError::ApnsUpstream(_) => Some("notification.bridge.error.apns.upstream"),
+            _ => None,
+        }
+    }
+
+    fn extras(&self) -> Vec<(&str, String)> {
+        match self {
+            ApnsError::ApnsUpstream(e) => vec![("error", e.to_string())],
+            _ => vec![],
+        }
+    }
+}

autoendpoint/src/routers/fcm/error.rs

@@ -1,5 +1,7 @@
 use crate::error::ApiErrorKind;
 use crate::routers::RouterError;
+
+use autopush_common::errors::ReportableError;
 use reqwest::StatusCode;
 
 /// Errors that may occur in the Firebase Cloud Messaging router
@@ -71,9 +73,33 @@ impl FcmError {
             | FcmError::NoOAuthToken => None,
         }
     }
+}
 
-    pub fn extras(&self) -> Vec<(&str, String)> {
+impl From<FcmError> for ApiErrorKind {
+    fn from(e: FcmError) -> Self {
+        ApiErrorKind::Router(RouterError::Fcm(e))
+    }
+}
+
+impl ReportableError for FcmError {
+    fn is_sentry_event(&self) -> bool {
+        matches!(&self, FcmError::InvalidAppId(_) | FcmError::NoAppId)
+    }
+
+    fn metric_label(&self) -> Option<&'static str> {
+        match &self {
+            FcmError::InvalidAppId(_) | FcmError::NoAppId => {
+                Some("notification.bridge.error.fcm.badappid")
+            }
+            _ => None,
+        }
+    }
+
+    fn extras(&self) -> Vec<(&str, String)> {
         match self {
+            FcmError::InvalidAppId(appid) => {
+                vec![("app_id", appid.to_string())]
+            }
             FcmError::EmptyResponse(status) => {
                 vec![("status", status.to_string())]
             }
@@ -84,9 +110,3 @@ impl FcmError {
         }
     }
 }
-
-impl From<FcmError> for ApiErrorKind {
-    fn from(e: FcmError) -> Self {
-        ApiErrorKind::Router(RouterError::Fcm(e))
-    }
-}

autoendpoint/src/routers/mod.rs

@@ -167,40 +167,25 @@ impl RouterError {
             RouterError::Upstream { .. } => None,
         }
     }
+}
 
-    pub fn metric_label(&self) -> Option<&'static str> {
-        // NOTE: Some metrics are emitted for other Errors via handle_error
-        // callbacks, whereas some are emitted via this method. These 2 should
-        // be consoliated: https://mozilla-hub.atlassian.net/browse/SYNC-3695
-        let err = match self {
-            #[cfg(feature = "adm")]
-            RouterError::Adm(AdmError::InvalidProfile | AdmError::NoProfile) => {
-                "notification.bridge.error.adm.profile"
-            }
-            RouterError::Apns(ApnsError::SizeLimit(_)) => {
-                "notification.bridge.error.apns.oversized"
-            }
-            RouterError::Fcm(FcmError::InvalidAppId(_) | FcmError::NoAppId) => {
-                "notification.bridge.error.fcm.badappid"
-            }
-            RouterError::TooMuchData(_) => "notification.bridge.error.too_much_data",
-            RouterError::SaveDb(e) => e.metric_label().unwrap_or_default(),
-            _ => "",
-        };
-        if !err.is_empty() {
-            return Some(err);
+impl ReportableError for RouterError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        match &self {
+            RouterError::Apns(e) => Some(e),
+            RouterError::Fcm(e) => Some(e),
+            RouterError::SaveDb(e) => Some(e),
+            _ => None,
         }
-        None
     }
 
-    pub fn is_sentry_event(&self) -> bool {
+    fn is_sentry_event(&self) -> bool {
         match self {
             #[cfg(feature = "adm")]
             RouterError::Adm(e) => !matches!(e, AdmError::InvalidProfile | AdmError::NoProfile),
             // apns handle_error emits a metric for ApnsError::Unregistered
-            RouterError::Apns(ApnsError::SizeLimit(_))
-            | RouterError::Apns(ApnsError::Unregistered) => false,
-            RouterError::Fcm(e) => !matches!(e, FcmError::InvalidAppId(_) | FcmError::NoAppId),
+            RouterError::Apns(e) => e.is_sentry_event(),
+            RouterError::Fcm(e) => e.is_sentry_event(),
             // common handle_error emits metrics for these
             RouterError::Authentication
             | RouterError::GCMAuthentication
@@ -214,8 +199,25 @@ impl RouterError {
         }
     }
 
-    pub fn extras(&self) -> Vec<(&str, String)> {
+    fn metric_label(&self) -> Option<&'static str> {
+        // NOTE: Some metrics are emitted for other Errors via handle_error
+        // callbacks, whereas some are emitted via this method. These 2 should
+        // be consoliated: https://mozilla-hub.atlassian.net/browse/SYNC-3695
         match self {
+            #[cfg(feature = "adm")]
+            RouterError::Adm(AdmError::InvalidProfile | AdmError::NoProfile) => {
+                Some("notification.bridge.error.adm.profile")
+            }
+            RouterError::Apns(e) => e.metric_label(),
+            RouterError::Fcm(e) => e.metric_label(),
+            RouterError::TooMuchData(_) => Some("notification.bridge.error.too_much_data"),
+            _ => None,
+        }
+    }
+
+    fn extras(&self) -> Vec<(&str, String)> {
+        match &self {
+            RouterError::Apns(e) => e.extras(),
             RouterError::Fcm(e) => e.extras(),
             RouterError::SaveDb(e) => e.extras(),
             _ => vec![],

autopush-common/src/db/bigtable/bigtable_client/error.rs

@@ -1,7 +1,6 @@
 use std::fmt::{self, Display};
 
 use actix_web::http::StatusCode;
-use backtrace::Backtrace;
 use deadpool::managed::{PoolError, TimeoutType};
 use thiserror::Error;
 
@@ -149,14 +148,6 @@ impl BigTableError {
 }
 
 impl ReportableError for BigTableError {
-    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
-        None
-    }
-
-    fn backtrace(&self) -> Option<&Backtrace> {
-        None
-    }
-
     fn is_sentry_event(&self) -> bool {
         #[allow(clippy::match_like_matches_macro)]
         match self {

autopush-common/src/db/error.rs

@@ -1,6 +1,5 @@
 use actix_web::http::StatusCode;
 
-use backtrace::Backtrace;
 #[cfg(feature = "dynamodb")]
 use rusoto_core::RusotoError;
 #[cfg(feature = "dynamodb")]
@@ -103,10 +102,6 @@ impl ReportableError for DbError {
         }
     }
 
-    fn backtrace(&self) -> Option<&Backtrace> {
-        None
-    }
-
     fn is_sentry_event(&self) -> bool {
         match &self {
             #[cfg(feature = "bigtable")]

autopush-common/src/errors.rs

@@ -165,13 +165,12 @@ impl ApcErrorKind {
 
     pub fn metric_label(&self) -> Option<&'static str> {
         // TODO: add labels for skipped stuff
-        let label = match self {
-            Self::PongTimeout => "pong_timeout",
-            Self::ExcessivePing => "excessive_ping",
-            Self::PayloadError(_) => "payload",
-            _ => return None,
-        };
-        Some(label)
+        match self {
+            Self::PongTimeout => Some("pong_timeout"),
+            Self::ExcessivePing => Some("excessive_ping"),
+            Self::PayloadError(_) => Some("payload"),
+            _ => None,
+        }
     }
 }
 
@@ -186,14 +185,19 @@ pub trait ReportableError: std::error::Error {
     }
 
     /// Return a `Backtrace` for this Error if one was captured
-    fn backtrace(&self) -> Option<&Backtrace>;
-
+    fn backtrace(&self) -> Option<&Backtrace> {
+        None
+    }
     /// Whether this error is reported to Sentry
-    fn is_sentry_event(&self) -> bool;
+    fn is_sentry_event(&self) -> bool {
+        true
+    }
 
     /// 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>;
+    fn metric_label(&self) -> Option<&'static str> {
+        None
+    }
 
     /// Experimental: return tag key value pairs for metrics and Sentry
     fn tags(&self) -> Vec<(&str, String)> {