feat: Ensure that status and labels are propagated for DB errors

Cargo.toml

@@ -29,6 +29,7 @@ edition = "2021"
 # there are some lingering code interdependencies that prevent that, but it should
 # remain a goal for the overall refactor.
 
+deadpool = { version = "0.10", features = ["managed", "rt_tokio_1"] }
 actix = "0.13"
 actix-cors = "0.7"
 actix-http = "3.2"

autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs

@@ -71,7 +71,11 @@ impl ReportableError for SMError {
 
     fn metric_label(&self) -> Option<&'static str> {
         // TODO:
-        None
+        match &self.kind {
+            SMErrorKind::Database(e) => e.metric_label(),
+            SMErrorKind::MakeEndpoint(e) => e.metric_label(),
+            _ => None,
+        }
     }
 }
 
@@ -108,13 +112,12 @@ pub enum SMErrorKind {
 impl SMErrorKind {
     /// Whether this error is reported to Sentry
     fn is_sentry_event(&self) -> bool {
-        matches!(
-            self,
-            SMErrorKind::Database(_)
-                | SMErrorKind::Internal(_)
-                | SMErrorKind::Reqwest(_)
-                | SMErrorKind::MakeEndpoint(_)
-        )
+        match self {
+            SMErrorKind::Database(e) => e.is_sentry_event(),
+            SMErrorKind::MakeEndpoint(e) => e.is_sentry_event(),
+            SMErrorKind::Reqwest(_) | SMErrorKind::Internal(_) => true,
+            _ => false,
+        }
     }
 
     /// Whether this variant has a `Backtrace` captured

autoendpoint/Cargo.toml

@@ -61,6 +61,7 @@ yup-oauth2 = "8.1"
 #ureq={ version="2.4", features=["json"] }
 
 [dev-dependencies]
+deadpool = { workspace = true }
 mockall.workspace = true
 mockito = "0.31"
 tempfile = "3.2.0"

autoendpoint/src/error.rs

@@ -152,14 +152,13 @@ impl ApiErrorKind {
 
             ApiErrorKind::LogCheck => StatusCode::IM_A_TEAPOT,
 
-            ApiErrorKind::Database(DbError::Backoff(_)) | ApiErrorKind::Conditional(_) => {
-                StatusCode::SERVICE_UNAVAILABLE
-            }
+            ApiErrorKind::Conditional(_) => StatusCode::SERVICE_UNAVAILABLE,
+
+            ApiErrorKind::Database(e) => e.status(),
 
             ApiErrorKind::General(_)
             | ApiErrorKind::Io(_)
             | ApiErrorKind::Metrics(_)
-            | ApiErrorKind::Database(_)
             | ApiErrorKind::EndpointUrl(_)
             | ApiErrorKind::RegistrationSecretHash(_) => StatusCode::INTERNAL_SERVER_ERROR,
         }
@@ -195,9 +194,9 @@ impl ApiErrorKind {
             ApiErrorKind::General(_) => "general",
             ApiErrorKind::Io(_) => "io",
             ApiErrorKind::Metrics(_) => "metrics",
-            ApiErrorKind::Database(_) => "database",
+            ApiErrorKind::Database(e) => return e.metric_label(),
             ApiErrorKind::Conditional(_) => "conditional",
-            ApiErrorKind::EndpointUrl(_) => "endpoint_url",
+            ApiErrorKind::EndpointUrl(e) => return e.metric_label(),
             ApiErrorKind::RegistrationSecretHash(_) => "registration_secret_hash",
         })
     }
@@ -207,6 +206,7 @@ impl ApiErrorKind {
         match self {
             // ignore selected validation errors.
             ApiErrorKind::Router(e) => e.is_sentry_event(),
+            ApiErrorKind::Database(e) => e.is_sentry_event(),
             // Ignore common webpush errors
             ApiErrorKind::NoTTL | ApiErrorKind::InvalidEncryption(_) |
             // Ignore common VAPID erros
@@ -409,4 +409,26 @@ mod tests {
         assert_eq!(event.exception[1].ty, "ApiError");
         assert_eq!(event.extra.get("row"), Some(&"bar".into()));
     }
+
+    /// Ensure that Pool error metric labels are specified and that they return a 503 status code.
+    #[cfg(feature = "bigtable")]
+    #[test]
+    fn test_label_for_metrics() {
+        // specifically test for a timeout on pool entry creation.
+        let e: ApiError = ApiErrorKind::Database(DbError::BTError(
+            autopush_common::db::bigtable::BigTableError::PoolTimeout(
+                deadpool::managed::TimeoutType::Create,
+            ),
+        ))
+        .into();
+
+        // Remember, `autoendpoint` is prefixed to this metric label.
+        assert_eq!(
+            e.kind.metric_label(),
+            Some("storage.bigtable.error.pool_timeout")
+        );
+
+        // "Retry-After" is applied on any 503 response (See ApiError::error_response)
+        assert_eq!(e.kind.status(), actix_http::StatusCode::SERVICE_UNAVAILABLE)
+    }
 }

autoendpoint/src/routers/mod.rs

@@ -12,6 +12,7 @@ use autopush_common::db::error::DbError;
 use actix_web::http::StatusCode;
 use actix_web::HttpResponse;
 use async_trait::async_trait;
+use autopush_common::errors::ReportableError;
 use std::collections::HashMap;
 use thiserror::Error;
 
@@ -121,7 +122,7 @@ impl RouterError {
             RouterError::Apns(e) => e.status(),
             RouterError::Fcm(e) => StatusCode::from_u16(e.status().as_u16()).unwrap_or_default(),
 
-            RouterError::SaveDb(_) => StatusCode::SERVICE_UNAVAILABLE,
+            RouterError::SaveDb(e) => e.status(),
 
             RouterError::UserWasDeleted | RouterError::NotFound => StatusCode::GONE,
 
@@ -177,6 +178,7 @@ impl RouterError {
                 "notification.bridge.error.fcm.badappid"
             }
             RouterError::TooMuchData(_) => "notification.bridge.error.too_much_data",
+            RouterError::SaveDb(e) => e.metric_label().unwrap_or_default(),
             _ => "",
         };
         if !err.is_empty() {
@@ -200,13 +202,15 @@ impl RouterError {
             | RouterError::RequestTimeout
             | RouterError::TooMuchData(_)
             | RouterError::Upstream { .. } => false,
+            RouterError::SaveDb(e) => e.is_sentry_event(),
             _ => true,
         }
     }
 
     pub fn extras(&self) -> Vec<(&str, String)> {
         match self {
             RouterError::Fcm(e) => e.extras(),
+            RouterError::SaveDb(e) => e.extras(),
             _ => vec![],
         }
     }

autopush-common/Cargo.toml

@@ -16,6 +16,7 @@ base64.workspace = true
 cadence.workspace = true
 chrono.workspace = true
 config.workspace = true
+deadpool.workspace = true
 fernet.workspace = true
 futures.workspace = true
 futures-util.workspace = true
@@ -56,7 +57,6 @@ url.workspace = true
 
 again = "0.1"
 async-trait = "0.1"
-deadpool = { version = "0.10", features = ["managed", "rt_tokio_1"] }
 gethostname = "0.4"
 futures-backoff = "0.1.0"
 num_cpus = "1.16"

autopush-common/src/db/bigtable/bigtable_client/error.rs

@@ -1,5 +1,6 @@
 use std::fmt::{self, Display};
 
+use actix_web::http::StatusCode;
 use backtrace::Backtrace;
 use deadpool::managed::{PoolError, TimeoutType};
 use thiserror::Error;
@@ -82,6 +83,21 @@ impl Display for MutateRowStatus {
     }
 }
 
+impl MutateRowStatus {
+    pub fn status(&self) -> StatusCode {
+        match self {
+            MutateRowStatus::OK => StatusCode::OK,
+            // Some of these were taken from the java-bigtable-hbase retry handlers
+            MutateRowStatus::Aborted
+            | MutateRowStatus::DeadlineExceeded
+            | MutateRowStatus::Internal
+            | MutateRowStatus::ResourceExhausted
+            | MutateRowStatus::Unavailable => StatusCode::SERVICE_UNAVAILABLE,
+            _ => StatusCode::INTERNAL_SERVER_ERROR,
+        }
+    }
+}
+
 #[derive(Debug, Error)]
 pub enum BigTableError {
     #[error("Invalid Row Response: {0}")]
@@ -122,6 +138,16 @@ pub enum BigTableError {
     Config(String),
 }
 
+impl BigTableError {
+    pub fn status(&self) -> StatusCode {
+        match self {
+            BigTableError::PoolTimeout(_) => StatusCode::SERVICE_UNAVAILABLE,
+            BigTableError::Status(e, _) => e.status(),
+            _ => StatusCode::INTERNAL_SERVER_ERROR,
+        }
+    }
+}
+
 impl ReportableError for BigTableError {
     fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
         None

autopush-common/src/db/bigtable/pool.rs

@@ -179,6 +179,7 @@ impl fmt::Debug for BigtableClientManager {
 impl Manager for BigtableClientManager {
     type Error = BigTableError;
     type Type = BigtableDb;
+    // TODO: Deadpool 0.11+ introduces new lifetime constratints.
 
     /// Create a new Bigtable Client with it's own channel.
     /// `BigtableClient` is the most atomic we can go.

autopush-common/src/db/error.rs

@@ -1,3 +1,5 @@
+use actix_web::http::StatusCode;
+
 use backtrace::Backtrace;
 #[cfg(feature = "dynamodb")]
 use rusoto_core::RusotoError;
@@ -81,6 +83,17 @@ pub enum DbError {
     Backoff(String),
 }
 
+impl DbError {
+    pub fn status(&self) -> StatusCode {
+        match self {
+            #[cfg(feature = "bigtable")]
+            Self::BTError(e) => e.status(),
+            Self::Backoff(_) => StatusCode::SERVICE_UNAVAILABLE,
+            _ => StatusCode::INTERNAL_SERVER_ERROR,
+        }
+    }
+}
+
 impl ReportableError for DbError {
     fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
         match &self {