From 2e48f504748211a0259b4049934dde7a99efdda6 Mon Sep 17 00:00:00 2001 From: JR Conlin Date: Wed, 27 Sep 2023 09:41:59 -0700 Subject: [PATCH] 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 --- autoendpoint/src/error.rs | 8 ++++++++ autoendpoint/src/routers/fcm/client.rs | 23 +++++++++++++++++++---- autoendpoint/src/routers/fcm/error.rs | 22 +++++++++++++++++++++- autoendpoint/src/routers/mod.rs | 7 +++++++ autopush-common/src/errors.rs | 11 +++++++++++ autopush-common/src/middleware/sentry.rs | 21 +++++++++++++++++++-- 6 files changed, 85 insertions(+), 7 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index 2d17146af..fe171ee47 100644 --- 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 index bfc436d1c..00982ae24 100644 --- 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 index 51c953aa4..af2251e14 100644 --- 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 for ApiErrorKind { diff --git a/autoendpoint/src/routers/mod.rs b/autoendpoint/src/routers/mod.rs index c30c6ae3b..71e434637 100644 --- 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 index b4ad7b129..ee08b7ac6 100644 --- 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 index fd467fe69..3ad9435a1 100644 --- 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::(&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::() { + 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::(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::() { + 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); }