feat: Make more errors use ReportableError trait

autoendpoint/src/routers/apns/error.rs

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

autoendpoint/src/routers/fcm/error.rs

@@ -1,5 +1,8 @@
 use crate::error::ApiErrorKind;
 use crate::routers::RouterError;
+
+use autopush_common::errors::ReportableError;
+use backtrace::Backtrace;
 use reqwest::StatusCode;
 
 /// Errors that may occur in the Firebase Cloud Messaging router
@@ -71,9 +74,47 @@ 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 reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        None
+    }
+
+    fn backtrace(&self) -> Option<&Backtrace> {
+        None
+    }
+
+    fn is_sentry_event(&self) -> bool {
+        matches!(
+            &self,
+            FcmError::InvalidAppId(_)
+                | FcmError::NoAppId
+                | FcmError::EmptyResponse(_)
+                | FcmError::InvalidResponse(_, _, _)
+        )
+    }
+
+    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 +125,3 @@ impl FcmError {
         }
     }
 }
-
-impl From<FcmError> for ApiErrorKind {
-    fn from(e: FcmError) -> Self {
-        ApiErrorKind::Router(RouterError::Fcm(e))
-    }
-}

autoendpoint/src/routers/mod.rs

@@ -162,38 +162,28 @@ 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 {
-            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,
         }
+    }
+
+    fn backtrace(&self) -> Option<&backtrace::Backtrace> {
         None
     }
 
-    pub fn is_sentry_event(&self) -> bool {
+    fn is_sentry_event(&self) -> bool {
         match self {
             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
@@ -207,8 +197,24 @@ 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 {
+            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![],

autoendpoint/src/routes/health.rs

@@ -1,4 +1,5 @@
 //! Health and Dockerflow routes
+use std::collections::HashMap;
 use std::thread;
 
 use actix_web::{
@@ -17,18 +18,19 @@ use crate::server::AppState;
 pub async fn health_route(state: Data<AppState>) -> Json<serde_json::Value> {
     let router_health = interpret_table_health(state.db.router_table_exists().await);
     let message_health = interpret_table_health(state.db.message_table_exists().await);
+    let mut routers: HashMap<&str, bool> = HashMap::new();
+    routers.insert("adm", state.adm_router.active());
+    routers.insert("apns", state.apns_router.active());
+    routers.insert("fcm", state.fcm_router.active());
 
-    Json(json!({
-        "status": "OK",
-        "version": env!("CARGO_PKG_VERSION"),
-        "router_table": router_health,
-        "message_table": message_health,
-        "routers": {
-            "adm": state.adm_router.active(),
-            "apns": state.apns_router.active(),
-            "fcm": state.fcm_router.active(),
-        }
-    }))
+    let health = json!({
+    "status": "OK",
+    "version": env!("CARGO_PKG_VERSION"),
+    "router_table": router_health,
+    "message_table": message_health,
+    "routers": routers});
+
+    Json(health)
 }
 
 /// Convert the result of a DB health check to JSON

autoendpoint/src/server.rs

@@ -24,7 +24,8 @@ use autopush_common::{
 
 use crate::error::{ApiError, ApiErrorKind, ApiResult};
 use crate::metrics;
-use crate::routers::{adm::router::AdmRouter, apns::router::ApnsRouter, fcm::router::FcmRouter};
+use crate::routers::adm::router::AdmRouter;
+use crate::routers::{apns::router::ApnsRouter, fcm::router::FcmRouter};
 use crate::routes::{
     health::{health_route, lb_heartbeat_route, log_check, status_route, version_route},
     registration::{

autopush-common/src/errors.rs

@@ -139,7 +139,7 @@ pub enum ApcErrorKind {
     #[error("Database Error: {0}")]
     DatabaseError(String),
 
-    // TODO: option this.
+    // TODO: Feature flag this.
     #[error("Rusoto Error: {0}")]
     RusotoError(String),
 }
@@ -167,13 +167,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,
+        }
     }
 }
 
@@ -210,6 +209,9 @@ pub trait ReportableError: std::error::Error {
 }
 
 impl ReportableError for ApcError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        None
+    }
     fn backtrace(&self) -> Option<&Backtrace> {
         Some(&self.backtrace)
     }