From 5cc84a990c060c81ea53bbe2fed399af31c5b6d9 Mon Sep 17 00:00:00 2001
From: bodymindarts <justin@galoy.io>
Date: Tue, 13 Feb 2024 14:29:01 +0100
Subject: [PATCH] refactor: better tracing when notification fails

---
 core/notifications/src/executor/mod.rs       | 47 ++++++++++++++------
 core/notifications/src/notification_event.rs | 31 +++++++++++++
 2 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/core/notifications/src/executor/mod.rs b/core/notifications/src/executor/mod.rs
index 1f507fc4ff..fa37beb76f 100644
--- a/core/notifications/src/executor/mod.rs
+++ b/core/notifications/src/executor/mod.rs
@@ -3,6 +3,7 @@ pub mod error;
 mod fcm;
 
 use fcm::{error::FcmError, FcmClient};
+use tracing::{error, instrument};
 
 use crate::{notification_event::*, primitives::*, user_notification_settings::*};
 
@@ -28,35 +29,53 @@ impl Executor {
         })
     }
 
+    #[instrument(
+        name = "executor.notify",
+        skip(self),
+        fields(n_errors, n_removed_tokens),
+        err
+    )]
     pub async fn notify<T: NotificationEvent>(&self, event: &T) -> Result<(), ExecutorError> {
         let mut settings = self.settings.find_for_user_id(event.user_id()).await?;
-        if !settings.should_send_notification(
-            UserNotificationChannel::Push,
-            UserNotificationCategory::Circles,
-        ) {
+        if !settings.should_send_notification(UserNotificationChannel::Push, event.category()) {
             return Ok(());
         }
 
         let msg = event.to_localized_msg(settings.locale().unwrap_or_default());
 
         let mut should_persist = false;
+        let mut last_err = None;
+        let mut n_errs = 0;
+        let mut n_removed_tokens = 0;
         for device_token in settings.push_device_tokens() {
-            let res = self.fcm.send(&device_token, &msg, event.deep_link()).await;
-            if let Err(e) = res {
-                match e {
-                    FcmError::GoogleFcm1Error(google_fcm1::Error::BadRequest(_)) => {
-                        should_persist = true;
-                        settings.remove_push_device_token(device_token)
-                    }
-                    _ => return Err(ExecutorError::Fcm(e)),
+            match self.fcm.send(&device_token, &msg, event.deep_link()).await {
+                Err(FcmError::GoogleFcm1Error(google_fcm1::Error::BadRequest(e))) => {
+                    n_errs += 1;
+                    n_removed_tokens += 1;
+                    error!("BadRequest sending to device: {}", e);
+                    should_persist = true;
+                    settings.remove_push_device_token(device_token)
                 }
+                Err(e) => {
+                    n_errs += 1;
+                    error!("Unexpected error sending to device: {}", e);
+                    last_err = Some(e.into())
+                }
+                _ => continue,
             }
         }
 
         if should_persist {
-            self.settings.persist(&mut settings).await?;
+            let _ = self.settings.persist(&mut settings).await;
         }
 
-        Ok(())
+        tracing::Span::current().record("n_errors", n_errs);
+        tracing::Span::current().record("n_removed_tokens", n_removed_tokens);
+
+        if let Some(e) = last_err {
+            Err(e)
+        } else {
+            Ok(())
+        }
     }
 }
diff --git a/core/notifications/src/notification_event.rs b/core/notifications/src/notification_event.rs
index e30447310a..cf55cf5073 100644
--- a/core/notifications/src/notification_event.rs
+++ b/core/notifications/src/notification_event.rs
@@ -8,6 +8,7 @@ pub enum DeepLink {
 }
 
 pub trait NotificationEvent: std::fmt::Debug + Into<NotificationEventPayload> {
+    fn category(&self) -> UserNotificationCategory;
     fn user_id(&self) -> &GaloyUserId;
     fn deep_link(&self) -> DeepLink;
     fn to_localized_msg(&self, locale: GaloyLocale) -> LocalizedMessage;
@@ -24,6 +25,16 @@ pub enum NotificationEventPayload {
 }
 
 impl NotificationEvent for NotificationEventPayload {
+    fn category(&self) -> UserNotificationCategory {
+        match self {
+            NotificationEventPayload::CircleGrew(e) => e.category(),
+            NotificationEventPayload::CircleThresholdReached(e) => e.category(),
+            NotificationEventPayload::IdentityVerificationApproved(e) => e.category(),
+            NotificationEventPayload::IdentityVerificationDeclined(e) => e.category(),
+            NotificationEventPayload::IdentityVerificationReviewPending(e) => e.category(),
+        }
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         match self {
             NotificationEventPayload::CircleGrew(event) => event.user_id(),
@@ -72,6 +83,10 @@ pub struct CircleGrew {
 }
 
 impl NotificationEvent for CircleGrew {
+    fn category(&self) -> UserNotificationCategory {
+        UserNotificationCategory::Circles
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         &self.user_id
     }
@@ -100,6 +115,10 @@ pub struct CircleThresholdReached {
 }
 
 impl NotificationEvent for CircleThresholdReached {
+    fn category(&self) -> UserNotificationCategory {
+        UserNotificationCategory::Circles
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         &self.user_id
     }
@@ -125,6 +144,10 @@ pub struct IdentityVerificationApproved {
 }
 
 impl NotificationEvent for IdentityVerificationApproved {
+    fn category(&self) -> UserNotificationCategory {
+        UserNotificationCategory::AdminNotification
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         &self.user_id
     }
@@ -157,6 +180,10 @@ pub struct IdentityVerificationDeclined {
 }
 
 impl NotificationEvent for IdentityVerificationDeclined {
+    fn category(&self) -> UserNotificationCategory {
+        UserNotificationCategory::AdminNotification
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         &self.user_id
     }
@@ -182,6 +209,10 @@ pub struct IdentityVerificationReviewPending {
 }
 
 impl NotificationEvent for IdentityVerificationReviewPending {
+    fn category(&self) -> UserNotificationCategory {
+        UserNotificationCategory::AdminNotification
+    }
+
     fn user_id(&self) -> &GaloyUserId {
         &self.user_id
     }