bug: Add better error messaging for GCM/FCM processing (#445)

* bug: Add better error messaging for GCM/FCM processing Attempt to determine the cause of the JSON parse fail for GCM messages Issue: SYNC-3931 --------- Co-authored-by: Philip Jenvey <pjenvey@underboss.org>
mozilla-services · Sep 27, 2023 · 2e48f50 · 2e48f50
1 parent fb7546a
commit 2e48f50
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 7 deletions.
diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs
@@ -343,6 +343,14 @@ impl ReportableError for ApiError {
     fn metric_label(&self) -> Option<&'static str> {
         self.kind.metric_label()
     }
+
+    fn extras(&self) -> Vec<(&str, String)> {
+        match &self.kind {
+            ApiErrorKind::Router(e) => e.extras(),
+            ApiErrorKind::LogCheck => vec![("coffee", "Unsupported".to_owned())],
+            _ => vec![],
+        }
+    }
 }
 
 /// Get the error number from validation errors. If multiple errors are present,

diff --git a/autoendpoint/src/routers/fcm/client.rs b/autoendpoint/src/routers/fcm/client.rs
@@ -154,10 +154,18 @@ impl FcmClient {
 
         // Handle error
         let status = response.status();
-        let data: GcmResponse = response
-            .json()
+        let raw_data = response
+            .bytes()
             .await
             .map_err(FcmError::DeserializeResponse)?;
+        if raw_data.is_empty() {
+            return Err(FcmError::EmptyResponse(status).into());
+        }
+        let data: GcmResponse = serde_json::from_slice(&raw_data).map_err(|e| {
+            let s = String::from_utf8(raw_data.to_vec()).unwrap_or_else(|e| e.to_string());
+            FcmError::InvalidResponse(e, s, status)
+        })?;
+
         if status.is_client_error() || status.is_server_error() || data.failure > 0 {
             let invalid = GcmResult::invalid();
             return Err(
@@ -238,10 +246,17 @@ impl FcmClient {
         // Handle error
         let status = response.status();
         if status.is_client_error() || status.is_server_error() {
-            let data: FcmResponse = response
-                .json()
+            let raw_data = response
+                .bytes()
                 .await
                 .map_err(FcmError::DeserializeResponse)?;
+            if raw_data.is_empty() {
+                return Err(FcmError::EmptyResponse(status).into());
+            }
+            let data: FcmResponse = serde_json::from_slice(&raw_data).map_err(|e| {
+                let s = String::from_utf8(raw_data.to_vec()).unwrap_or_else(|e| e.to_string());
+                FcmError::InvalidResponse(e, s, status)
+            })?;
 
             // we only ever send one.
             return Err(match (status, data.error) {

diff --git a/autoendpoint/src/routers/fcm/error.rs b/autoendpoint/src/routers/fcm/error.rs
@@ -17,6 +17,12 @@ pub enum FcmError {
     #[error("Unable to deserialize FCM response")]
     DeserializeResponse(#[source] reqwest::Error),
 
+    #[error("Invalid JSON response from FCM")]
+    InvalidResponse(#[source] serde_json::Error, String, StatusCode),
+
+    #[error("Empty response from FCM")]
+    EmptyResponse(StatusCode),
+
     #[error("No OAuth token was present")]
     NoOAuthToken,
 
@@ -43,7 +49,9 @@ impl FcmError {
             | FcmError::OAuthToken(_)
             | FcmError::NoOAuthToken => StatusCode::INTERNAL_SERVER_ERROR,
 
-            FcmError::DeserializeResponse(_) => StatusCode::BAD_GATEWAY,
+            FcmError::DeserializeResponse(_)
+            | FcmError::EmptyResponse(_)
+            | FcmError::InvalidResponse(_, _, _) => StatusCode::BAD_GATEWAY,
         }
     }
 
@@ -58,9 +66,21 @@ impl FcmError {
             | FcmError::OAuthClientBuild(_)
             | FcmError::OAuthToken(_)
             | FcmError::DeserializeResponse(_)
+            | FcmError::EmptyResponse(_)
+            | FcmError::InvalidResponse(_, _, _)
             | FcmError::NoOAuthToken => None,
         }
     }
+
+    pub fn extras(&self) -> Vec<(&str, String)> {
+        match self {
+            FcmError::EmptyResponse(status) => vec![("status", status.to_string())],
+            FcmError::InvalidResponse(_, body, status) => {
+                vec![("status", status.to_string()), ("body", body.to_owned())]
+            }
+            _ => vec![],
+        }
+    }
 }
 
 impl From<FcmError> for ApiErrorKind {

diff --git a/autoendpoint/src/routers/mod.rs b/autoendpoint/src/routers/mod.rs
@@ -203,4 +203,11 @@ impl RouterError {
             _ => true,
         }
     }
+
+    pub fn extras(&self) -> Vec<(&str, String)> {
+        match self {
+            RouterError::Fcm(e) => e.extras(),
+            _ => vec![],
+        }
+    }
 }
diff --git a/autopush-common/src/errors.rs b/autopush-common/src/errors.rs
@@ -194,6 +194,17 @@ pub trait ReportableError: std::error::Error + fmt::Display {
     /// 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>;
+
+    /// Experimental: return tag key value pairs for metrics and Sentry
+    fn tags(&self) -> Vec<(&str, String)> {
+        vec![]
+    }
+
+    /// Experimental: return key value pairs for Sentry Event's extra data
+    /// TODO: should probably return Vec<(&str, Value)> or Vec<(String, Value)>
+    fn extras(&self) -> Vec<(&str, String)> {
+        vec![]
+    }
 }
 
 impl ReportableError for ApcError {

diff --git a/autopush-common/src/middleware/sentry.rs b/autopush-common/src/middleware/sentry.rs
@@ -8,6 +8,7 @@ use cadence::{CountedExt, StatsdClient};
 use futures::{future::LocalBoxFuture, FutureExt};
 use futures_util::future::{ok, Ready};
 use sentry::{protocol::Event, Hub};
+use serde_json::value::Value;
 
 use crate::{errors::ReportableError, tags::Tags};
 
@@ -108,14 +109,22 @@ where
                                 info!("Sentry: Sending error to metrics: {:?}", reportable_err);
                                 let _ = metrics.incr(&format!("{}.{}", metric_label, label));
                             }
+                            debug!("Sentry: Not reporting error (service error): {:?}", error);
+                            return Err(error);
                         }
-                        debug!("Sentry: Not reporting error (service error): {:?}", error);
-                        return Err(error);
                     };
                     debug!("Reporting error to Sentry (service error): {}", error);
                     let mut event = event_from_actix_error::<E>(&error);
                     event.extra.append(&mut tags.clone().extra_tree());
                     event.tags.append(&mut tags.clone().tag_tree());
+                    if let Some(reportable_err) = error.as_error::<E>() {
+                        for (key, val) in reportable_err.extras() {
+                            event.extra.insert(key.to_owned(), Value::from(val));
+                        }
+                        for (key, val) in reportable_err.tags() {
+                            event.tags.insert(key.to_owned(), val);
+                        }
+                    }
                     let event_id = hub.capture_event(event);
                     trace!("event_id = {}", event_id);
                     return Err(error);
@@ -137,6 +146,14 @@ where
                 let mut event = event_from_actix_error::<E>(error);
                 event.extra.append(&mut tags.clone().extra_tree());
                 event.tags.append(&mut tags.clone().tag_tree());
+                if let Some(reportable_err) = error.as_error::<E>() {
+                    for (key, val) in reportable_err.extras() {
+                        event.extra.insert(key.to_owned(), Value::from(val));
+                    }
+                    for (key, val) in reportable_err.tags() {
+                        event.tags.insert(key.to_owned(), val);
+                    }
+                }
                 let event_id = hub.capture_event(event);
                 trace!("event_id = {}", event_id);
             }