feat: Add metrics for user creation/deletion. (#506)

* feat: Add metrics for user creation/deletion. There were a few questions raised about possible UAID churn occuring with autoconnect. Adding some metrics Add new metrics for: * `ua.expiration` - a UAID has been reset for * `reason`:(`reset_uaid`|`too_many_messages`|`no_current_mo`|`invalid_current_mo`) * added `autoconnect`:`true` for consistency * `ua.existing_user` - A user was found in the database * `ua.new_user` - no record was found, assinging a new UAID. * `reassined`: Was a UAID offered by the UA? * f rename `reset_uaid` to `old_record_version` This is more indicitive of what the flag is for and what it does. Closes: SYNC-3994 --------- Co-authored-by: Philip Jenvey <[email protected]>
mozilla-services · Nov 9, 2023 · bd2e011 · bd2e011
1 parent 8f6e03e
commit bd2e011
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 21 deletions.
diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/mod.rs
@@ -183,7 +183,7 @@ impl WebPushClient {
         // Log out the final stats message
         info!("Session";
             "uaid_hash" => self.uaid.as_simple().to_string(),
-            "uaid_reset" => self.flags.reset_uaid,
+            "uaid_reset" => self.flags.old_record_version,
             "existing_uaid" => stats.existing_uaid,
             "connection_type" => "webpush",
             "ua_name" => &ua_info.browser_name,
@@ -297,6 +297,7 @@ pub async fn process_existing_user(
                 .metrics
                 .incr_with_tags("ua.expiration")
                 .with_tag("code", "104")
+                .with_tag("reason", "no_current_month")
                 .send();
             app_state.db.remove_user(&user.uaid).await?;
             return Ok(None);
@@ -311,6 +312,7 @@ pub async fn process_existing_user(
                 .metrics
                 .incr_with_tags("ua.expiration")
                 .with_tag("code", "105")
+                .with_tag("reason", "invalid_current_month")
                 .send();
             app_state.db.remove_user(&user.uaid).await?;
             return Ok(None);
@@ -319,7 +321,7 @@ pub async fn process_existing_user(
 
     let flags = ClientFlags {
         check_storage: true,
-        reset_uaid: user
+        old_record_version: user
             .record_version
             .map_or(true, |rec_ver| rec_ver < USER_RECORD_VERSION),
         ..Default::default()
@@ -336,7 +338,7 @@ pub struct ClientFlags {
     /// Whether this client needs to check storage for messages
     check_storage: bool,
     /// Flags the need to drop the user record
-    reset_uaid: bool,
+    old_record_version: bool,
 }
 
 impl Default for ClientFlags {
@@ -345,7 +347,7 @@ impl Default for ClientFlags {
             include_topic: true,
             increment_storage: false,
             check_storage: false,
-            reset_uaid: false,
+            old_record_version: false,
         }
     }
 }

diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_client_msg.rs
@@ -314,8 +314,13 @@ impl WebPushClient {
         // All Ack'd and finished checking/incrementing storage
         debug_assert!(!self.ack_state.unacked_notifs());
         let flags = &self.flags;
-        if flags.reset_uaid {
-            debug!("▶️ WebPushClient:post_process_all_acked reset_uaid");
+        if flags.old_record_version {
+            debug!("▶️ WebPushClient:post_process_all_acked; resetting uaid");
+            self.app_state
+                .metrics
+                .incr_with_tags("ua.expiration")
+                .with_tag("reason", "old_record_version")
+                .send();
             self.app_state.db.remove_user(&self.uaid).await?;
             Err(SMErrorKind::UaidReset.into())
         } else {

diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/identified/on_server_notif.rs
@@ -295,6 +295,11 @@ impl WebPushClient {
         if self.sent_from_storage > self.app_state.settings.msg_limit {
             // Exceeded the max limit of stored messages: drop the user to
             // trigger a re-register
+            self.app_state
+                .metrics
+                .incr_with_tags("ua.expiration")
+                .with_tag("reason", "too_many_messages")
+                .send();
             self.app_state.db.remove_user(&self.uaid).await?;
             return Err(SMErrorKind::UaidReset.into());
         }

diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/unidentified.rs
@@ -67,19 +67,31 @@ impl UnidentifiedClient {
         );
 
         // Ignore invalid uaids (treat as None) so they'll be issued a new one
-        let uaid = uaid.as_deref().and_then(|uaid| Uuid::try_parse(uaid).ok());
+        let original_uaid = uaid.as_deref().and_then(|uaid| Uuid::try_parse(uaid).ok());
 
         let GetOrCreateUser {
             user,
             existing_user,
             flags,
-        } = self.get_or_create_user(uaid).await?;
+        } = self.get_or_create_user(original_uaid).await?;
         let uaid = user.uaid;
         debug!(
             "💬UnidentifiedClient::on_client_msg Hello! uaid: {} existing_user: {}",
             uaid, existing_user,
         );
-        let _ = self.app_state.metrics.incr("ua.command.hello");
+        self.app_state
+            .metrics
+            .incr_with_tags("ua.command.hello")
+            .with_tag("uaid", {
+                if existing_user {
+                    "existing"
+                } else if original_uaid.unwrap_or(uaid) != uaid {
+                    "reassigned"
+                } else {
+                    "new"
+                }
+            })
+            .send();
 
         let (broadcast_subs, broadcasts) = self
             .broadcast_init(&Broadcast::from_hashmap(broadcasts.unwrap_or_default()))

diff --git a/autopush/src/client.rs b/autopush/src/client.rs
@@ -215,8 +215,10 @@ pub struct ClientFlags {
     increment_storage: bool,
     /// Whether this client needs to check storage for messages
     check: bool,
-    /// Flags the need to drop the user record
-    reset_uaid: bool,
+    /// Flags the need to drop the user record if the User record_version is less
+    /// than the USER_RECORD_VERSION constant. The reset is done after all existing
+    /// message traffic is sent over.
+    old_record_version: bool,
     rotate_message_table: bool,
 }
 
@@ -226,7 +228,7 @@ impl ClientFlags {
             include_topic: true,
             increment_storage: false,
             check: false,
-            reset_uaid: false,
+            old_record_version: false,
             rotate_message_table: false,
         }
     }
@@ -421,7 +423,7 @@ where
             uaid,
             message_month,
             check_storage,
-            reset_uaid,
+            old_record_version,
             rotate_message_table,
             connected_at,
             deferred_user_registration,
@@ -432,7 +434,7 @@ where
                     uaid: Some(uaid),
                     message_month,
                     check_storage,
-                    reset_uaid,
+                    old_record_version,
                     rotate_message_table,
                     connected_at,
                     deferred_user_registration,
@@ -442,7 +444,7 @@ where
                         uaid,
                         message_month,
                         check_storage,
-                        reset_uaid,
+                        old_record_version,
                         rotate_message_table,
                         connected_at,
                         deferred_user_registration,
@@ -484,7 +486,7 @@ where
         // Setup the objects and such needed for a WebPushClient
         let mut flags = ClientFlags::new();
         flags.check = check_storage;
-        flags.reset_uaid = reset_uaid;
+        flags.old_record_version = old_record_version;
         flags.rotate_message_table = rotate_message_table;
         let (initialized_subs, broadcasts) = srv.broadcast_init(&desired_broadcasts);
         broadcast_subs.replace(initialized_subs);
@@ -498,7 +500,7 @@ where
             connected_at,
             stats: SessionStatistics {
                 uaid: uaid.as_simple().to_string(),
-                uaid_reset: reset_uaid,
+                uaid_reset: old_record_version,
                 existing_uaid: check_storage,
                 connection_type: String::from("webpush"),
                 ..Default::default()
@@ -949,8 +951,8 @@ where
                     .migrate_user(&webpush.uaid, &webpush.message_month),
             );
             transition!(AwaitMigrateUser { response, data });
-        } else if all_acked && webpush.flags.reset_uaid {
-            debug!("Dropping user: flagged reset_uaid");
+        } else if all_acked && webpush.flags.old_record_version {
+            debug!("Dropping user: flagged old_record_version");
             let response = Box::new(data.srv.ddb.drop_uaid(&webpush.uaid));
             transition!(AwaitDropUser { response, data });
         }

diff --git a/autopush/src/db/commands.rs b/autopush/src/db/commands.rs
@@ -484,7 +484,7 @@ fn handle_user_result(
     hello_response.check_storage = true;
     hello_response.rotate_message_table = user_month != *cur_month;
     hello_response.message_month = user_month;
-    hello_response.reset_uaid = user
+    hello_response.old_record_version = user
         .record_version
         .map_or(true, |rec_ver| rec_ver < USER_RECORD_VERSION);
 

diff --git a/autopush/src/db/mod.rs b/autopush/src/db/mod.rs
@@ -44,7 +44,7 @@ pub struct HelloResponse {
     pub uaid: Option<Uuid>,
     pub message_month: String,
     pub check_storage: bool,
-    pub reset_uaid: bool,
+    pub old_record_version: bool,
     pub rotate_message_table: bool,
     pub connected_at: u64,
     // Exists when we didn't register this user during HELLO