From 5e6a9af49a8454b3da0af5a5ce9bdbe1e177295c Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 15 Apr 2024 11:18:03 -0700 Subject: [PATCH 1/2] feat: Ensure that status and labels are propagated for DB errors Closes: SYNC-4233 --- Cargo.lock | 1 + Cargo.toml | 1 + .../autoconnect-ws-sm/src/error.rs | 19 ++++++----- autoendpoint/Cargo.toml | 1 + autoendpoint/src/error.rs | 34 +++++++++++++++---- autoendpoint/src/routers/mod.rs | 6 +++- autopush-common/Cargo.toml | 2 +- .../src/db/bigtable/bigtable_client/error.rs | 26 ++++++++++++++ autopush-common/src/db/bigtable/pool.rs | 1 + autopush-common/src/db/error.rs | 13 +++++++ 10 files changed, 88 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5bce8c1be..e4e62307e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -719,6 +719,7 @@ dependencies = [ "bytebuffer", "cadence", "config", + "deadpool", "docopt", "fernet", "futures 0.3.30", diff --git a/Cargo.toml b/Cargo.toml index 858cfd6fa..e683cdc2f 100644 --- a/Cargo.toml +++ b/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" diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs index 44a304a9f..815723205 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs +++ b/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 diff --git a/autoendpoint/Cargo.toml b/autoendpoint/Cargo.toml index c23c6f77c..bc0e48d17 100644 --- a/autoendpoint/Cargo.toml +++ b/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" diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index 039462495..b60b20b6a 100644 --- a/autoendpoint/src/error.rs +++ b/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) => e.metric_label().unwrap_or("database"), ApiErrorKind::Conditional(_) => "conditional", - ApiErrorKind::EndpointUrl(_) => "endpoint_url", + ApiErrorKind::EndpointUrl(e) => e.metric_label().unwrap_or("endpoint_url"), 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) + } } diff --git a/autoendpoint/src/routers/mod.rs b/autoendpoint/src/routers/mod.rs index f82a926a3..ac5e27778 100644 --- a/autoendpoint/src/routers/mod.rs +++ b/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,6 +202,7 @@ impl RouterError { | RouterError::RequestTimeout | RouterError::TooMuchData(_) | RouterError::Upstream { .. } => false, + RouterError::SaveDb(e) => e.is_sentry_event(), _ => true, } } @@ -207,6 +210,7 @@ impl RouterError { pub fn extras(&self) -> Vec<(&str, String)> { match self { RouterError::Fcm(e) => e.extras(), + RouterError::SaveDb(e) => e.extras(), _ => vec![], } } diff --git a/autopush-common/Cargo.toml b/autopush-common/Cargo.toml index a716e0f54..4a999d086 100644 --- a/autopush-common/Cargo.toml +++ b/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" diff --git a/autopush-common/src/db/bigtable/bigtable_client/error.rs b/autopush-common/src/db/bigtable/bigtable_client/error.rs index 16f70536d..25f3a5035 100644 --- a/autopush-common/src/db/bigtable/bigtable_client/error.rs +++ b/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 diff --git a/autopush-common/src/db/bigtable/pool.rs b/autopush-common/src/db/bigtable/pool.rs index 8aa993706..c216107de 100644 --- a/autopush-common/src/db/bigtable/pool.rs +++ b/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. diff --git a/autopush-common/src/db/error.rs b/autopush-common/src/db/error.rs index eabdee1cc..373b8986c 100644 --- a/autopush-common/src/db/error.rs +++ b/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 { From dd35d32d148bbbcee195afc8653af70aaa6bd213 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 16 Apr 2024 11:17:29 -0700 Subject: [PATCH 2/2] f r's --- autoendpoint/src/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index b60b20b6a..61ab12750 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -194,9 +194,9 @@ impl ApiErrorKind { ApiErrorKind::General(_) => "general", ApiErrorKind::Io(_) => "io", ApiErrorKind::Metrics(_) => "metrics", - ApiErrorKind::Database(e) => e.metric_label().unwrap_or("database"), + ApiErrorKind::Database(e) => return e.metric_label(), ApiErrorKind::Conditional(_) => "conditional", - ApiErrorKind::EndpointUrl(e) => e.metric_label().unwrap_or("endpoint_url"), + ApiErrorKind::EndpointUrl(e) => return e.metric_label(), ApiErrorKind::RegistrationSecretHash(_) => "registration_secret_hash", }) }