feat: Remove magic strings and add enum variants instead (#903)

Closes: #897

- Added a `BroadcastErrorType` enum
- Added a `MessageType` enum with a conversion from
`ClientMessage`/`ServerMessage` to `MessageType`
- Added a `MetricName` enum
- To work easier with Metrics, I added an extension trait in
`autopush-common/metrics.rs`, with methods `inc`, `inc_raw`,
`inc_with_tags`. The `inc` and `inc_with_tags` methods accept a
`MetricName`
- This replaces the `cadence::CountedExt` trait with our own
`StatsdClientExt` trait with almost the same methods.

Since it replaces a lot of text, and it's my first PR, I might have
missed something. But the local tests run through.

Author: gruberb

Cargo.lock

@@ -1,24 +1,24 @@
 [workspace]
 members = [
-  "autopush-common",
-  "autoendpoint",
-  "autoconnect",
-  "autoconnect/autoconnect-common",
-  "autoconnect/autoconnect-settings",
-  "autoconnect/autoconnect-web",
-  "autoconnect/autoconnect-ws",
-  "autoconnect/autoconnect-ws/autoconnect-ws-sm",
+    "autopush-common",
+    "autoendpoint",
+    "autoconnect",
+    "autoconnect/autoconnect-common",
+    "autoconnect/autoconnect-settings",
+    "autoconnect/autoconnect-web",
+    "autoconnect/autoconnect-ws",
+    "autoconnect/autoconnect-ws/autoconnect-ws-sm",
 ]
 resolver = "2"
 
 [workspace.package]
 version = "1.74.3"
 authors = [
-  "Ben Bangert <[email protected]>",
-  "JR Conlin <[email protected]>",
-  "Phil Jenvey <[email protected]>",
-  "Alex Crichton <[email protected]>",
-  "Mark Drobnak <[email protected]>",
+    "Ben Bangert <[email protected]>",
+    "JR Conlin <[email protected]>",
+    "Phil Jenvey <[email protected]>",
+    "Alex Crichton <[email protected]>",
+    "Mark Drobnak <[email protected]>",
 ]
 edition = "2021"
 rust-version = "1.86.0"
@@ -49,19 +49,19 @@ env_logger = "0.11"
 fernet = "0.2.0"
 futures = { version = "0.3", features = ["compat"] }
 futures-util = { version = "0.3", features = [
-  "async-await",
-  "compat",
-  "sink",
-  "io",
+    "async-await",
+    "compat",
+    "sink",
+    "io",
 ] }
 futures-locks = "0.7"
 hex = "0.4.2"
 httparse = "1.3"
 hyper = "1.6"
 lazy_static = "1.4"
 log = { version = "0.4", features = [
-  "max_level_debug",
-  "release_max_level_info",
+    "max_level_debug",
+    "release_max_level_info",
 ] }
 mockall = "0.13"
 mozsvc-common = "0.2"
@@ -70,7 +70,7 @@ rand = "0.9"
 regex = "1.4"
 reqwest = { version = "0.12", features = ["json", "blocking"] }
 sentry = { version = "0.36", features = [
-  "debug-logs",
+    "debug-logs",
 ] } # Using debug-logs avoids https://github.com/getsentry/sentry-rust/issues/237
 sentry-core = { version = "0.36" }
 sentry-actix = "0.36"
@@ -79,9 +79,9 @@ serde = "1.0"
 serde_derive = "1.0"
 serde_json = "1.0"
 slog = { version = "2.7", features = [
-  "dynamic-keys",
-  "max_level_trace",
-  "release_max_level_info",
+    "dynamic-keys",
+    "max_level_trace",
+    "release_max_level_info",
 ] }
 slog-async = "2.6"
 slog-envlogger = "2.2.0"
@@ -90,6 +90,7 @@ slog-scope = "4.4"
 slog-stdlog = "4.1"
 slog-term = "2.6"
 strum = { version = "0.27", features = ["derive"] }
+strum_macros = "0.27"
 thiserror = "2.0"
 tokio = "1.38"
 tokio-compat-02 = "0.2"

Cargo.toml

@@ -8,6 +8,7 @@ version.workspace = true
 
 [dependencies]
 actix-web.workspace = true
+bytestring.workspace = true
 cadence.workspace = true
 futures.workspace = true
 futures-locks.workspace = true
@@ -20,6 +21,8 @@ serde_derive.workspace = true
 serde_json.workspace = true
 slog.workspace = true
 slog-scope.workspace = true
+strum.workspace = true
+strum_macros.workspace = true
 uuid.workspace = true
 
 

autoconnect/autoconnect-common/Cargo.toml

@@ -13,6 +13,7 @@
 use std::collections::HashMap;
 
 use serde_derive::{Deserialize, Serialize};
+use strum_macros::{AsRefStr, Display};
 
 use autopush_common::errors::{ApcErrorKind, Result};
 
@@ -72,6 +73,12 @@ struct BroadcastRevision {
     broadcast: BroadcastKey,
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq, AsRefStr, Display)]
+pub enum BroadcastErrorKind {
+    #[strum(serialize = "Broadcast not found")]
+    NotFound,
+}
+
 /// A provided Broadcast/Version used for `BroadcastSubsInit`, client comparisons, and outgoing
 /// deltas
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
@@ -85,7 +92,7 @@ impl Broadcast {
     pub fn error(self) -> Broadcast {
         Broadcast {
             broadcast_id: self.broadcast_id,
-            version: "Broadcast not found".to_string(),
+            version: BroadcastErrorKind::NotFound.to_string(),
         }
     }
 }
@@ -191,7 +198,9 @@ impl BroadcastChangeTracker {
         let key = self
             .broadcast_registry
             .lookup_key(&broadcast.broadcast_id)
-            .ok_or(ApcErrorKind::BroadcastError("Broadcast not found".into()))?;
+            .ok_or(ApcErrorKind::BroadcastError(
+                BroadcastErrorKind::NotFound.to_string(),
+            ))?;
 
         if let Some(ver) = self.broadcast_versions.get_mut(&key) {
             if *ver == broadcast.version {
@@ -200,7 +209,9 @@ impl BroadcastChangeTracker {
             *ver = broadcast.version;
         } else {
             trace!("📢 Not found: {b_id}");
-            return Err(ApcErrorKind::BroadcastError("Broadcast not found".into()).into());
+            return Err(
+                ApcErrorKind::BroadcastError(BroadcastErrorKind::NotFound.to_string()).into(),
+            );
         }
 
         trace!("📢 New version of {b_id}");

autoconnect/autoconnect-common/src/broadcast.rs

@@ -1,7 +1,9 @@
 use std::{collections::HashMap, error::Error, io, sync::Arc, time::Duration};
 
 use actix_web::rt;
-use cadence::{CountedExt, StatsdClient};
+use autopush_common::metric_name::MetricName;
+use autopush_common::metrics::StatsdClientExt;
+use cadence::StatsdClient;
 use serde_derive::Deserialize;
 use tokio::sync::RwLock;
 
@@ -39,7 +41,9 @@ pub async fn init_and_spawn_megaphone_updater(
             if let Err(e) = updater(&broadcaster, &http, &url, &token).await {
                 report_updater_error(&metrics, e);
             } else {
-                metrics.incr_with_tags("megaphone.updater.ok").send();
+                metrics
+                    .incr_with_tags(MetricName::MegaphoneUpdaterOk)
+                    .send();
             }
         }
     });
@@ -59,7 +63,7 @@ fn report_updater_error(metrics: &Arc<StatsdClient>, err: reqwest::Error) {
         "unknown"
     };
     metrics
-        .incr_with_tags("megaphone.updater.error")
+        .incr_with_tags(MetricName::MegaphoneUpdaterError)
         .with_tag("reason", reason)
         .send();
     if reason == "unknown" {

autoconnect/autoconnect-common/src/megaphone.rs

@@ -10,10 +10,44 @@ use std::collections::HashMap;
 use std::str::FromStr;
 
 use serde_derive::{Deserialize, Serialize};
+use strum_macros::{AsRefStr, Display, EnumString};
 use uuid::Uuid;
 
 use autopush_common::notification::Notification;
 
+/// Message types for WebPush protocol messages.
+///
+/// This enum should be used instead of string literals when referring to message types.
+/// String serialization is handled automatically via the strum traits.
+///
+/// Example:
+/// ```
+///  use autoconnect_common::protocol::MessageType;
+///
+/// let message_type = MessageType::Hello;
+/// let message_str = message_type.as_ref();  // Returns "hello"
+/// ```
+#[derive(Debug, Clone, Copy, PartialEq, Eq, AsRefStr, Display, EnumString)]
+#[strum(serialize_all = "snake_case")]
+pub enum MessageType {
+    Hello,
+    Register,
+    Unregister,
+    BroadcastSubscribe,
+    Ack,
+    Nack,
+    Ping,
+    Notification,
+    Broadcast,
+}
+
+impl MessageType {
+    /// Returns the expected message type string for error messages
+    pub fn expected_msg(&self) -> String {
+        format!(r#"Expected messageType="{}""#, self.as_ref())
+    }
+}
+
 #[derive(Debug, Eq, PartialEq, Serialize)]
 #[serde(untagged)]
 pub enum BroadcastValue {
@@ -69,6 +103,21 @@ pub enum ClientMessage {
     Ping,
 }
 
+impl ClientMessage {
+    /// Get the message type of this message
+    pub fn message_type(&self) -> MessageType {
+        match self {
+            ClientMessage::Hello { .. } => MessageType::Hello,
+            ClientMessage::Register { .. } => MessageType::Register,
+            ClientMessage::Unregister { .. } => MessageType::Unregister,
+            ClientMessage::BroadcastSubscribe { .. } => MessageType::BroadcastSubscribe,
+            ClientMessage::Ack { .. } => MessageType::Ack,
+            ClientMessage::Nack { .. } => MessageType::Nack,
+            ClientMessage::Ping => MessageType::Ping,
+        }
+    }
+}
+
 impl FromStr for ClientMessage {
     type Err = serde_json::error::Error;
 
@@ -141,11 +190,23 @@ pub enum ServerMessage {
 }
 
 impl ServerMessage {
+    /// Get the message type of this message
+    pub fn message_type(&self) -> MessageType {
+        match self {
+            ServerMessage::Hello { .. } => MessageType::Hello,
+            ServerMessage::Register { .. } => MessageType::Register,
+            ServerMessage::Unregister { .. } => MessageType::Unregister,
+            ServerMessage::Broadcast { .. } => MessageType::Broadcast,
+            ServerMessage::Notification(..) => MessageType::Notification,
+            ServerMessage::Ping => MessageType::Ping,
+        }
+    }
+
     pub fn to_json(&self) -> Result<String, serde_json::error::Error> {
         match self {
-            // clients recognize {"messageType": "ping"} but traditionally both
-            // client/server send the empty object version
-            ServerMessage::Ping => Ok("{}".to_owned()),
+            // Both client and server understand the verbose `{"messageType": "ping"}` and the abbreviated `{}`
+            // as valid ping messages. The server defaults to the shorter `{}` form.
+            ServerMessage::Ping => Ok("{}".to_string()),
             _ => serde_json::to_string(self),
         }
     }

autoconnect/autoconnect-common/src/protocol.rs

@@ -1,5 +1,7 @@
+use bytestring::ByteString;
 use uuid::Uuid;
 
+use crate::protocol::MessageType;
 use autopush_common::{
     db::{mock::MockDbClient, User},
     util::timing::ms_since_epoch,
@@ -13,10 +15,23 @@ pub const DUMMY_CHID: Uuid = Uuid::from_u128(0xdeadbeef_0000_0000_abad_1dea00000
 
 /// A minimal websocket Push "hello" message, used by an unregistered UA with
 /// no existing channel subscriptions
-pub const HELLO: &str = r#"{"messageType": "hello", "use_webpush": true}"#;
-/// A post initial registration response
-pub const HELLO_AGAIN: &str = r#"{"messageType": "hello", "use_webpush": true,
-                                  "uaid": "deadbeef-0000-0000-deca-fbad00000000"}"#;
+pub fn hello_json() -> ByteString {
+    format!(
+        r#"{{"messageType": "{}", "use_webpush": true}}"#,
+        MessageType::Hello.as_ref()
+    )
+    .into()
+}
+
+pub fn hello_again_json() -> ByteString {
+    format!(
+        r#"{{"messageType": "{}", "use_webpush": true,
+                "uaid": "{}"}}"#,
+        MessageType::Hello.as_ref(),
+        DUMMY_UAID
+    )
+    .into()
+}
 
 pub const CURRENT_MONTH: &str = "message_2018_06";
 

autoconnect/autoconnect-common/src/test_support.rs

@@ -5,10 +5,11 @@ use actix_web::{
     web::{self, Data, Json},
     HttpResponse, ResponseError,
 };
-use cadence::CountedExt;
 use serde_json::json;
 
 use autoconnect_settings::AppState;
+use autopush_common::metric_name::MetricName;
+use autopush_common::metrics::StatsdClientExt;
 
 use crate::error::ApiError;
 
@@ -47,7 +48,7 @@ pub async fn health_route(state: Data<AppState>) -> Json<serde_json::Value> {
         health["reliability"] = json!(state.reliability.health_check().await.unwrap_or_else(|e| {
             state
                 .metrics
-                .incr_with_tags("error.redis.unavailable")
+                .incr_with_tags(MetricName::ErrorRedisUnavailable)
                 .with_tag("application", "autoconnect")
                 .send();
             error!("🔍🟥 Reliability reporting down: {:?}", e);