From 0ca435fb1a05f073d1e78ed420d953a00c8d0d53 Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Fri, 18 Apr 2025 22:59:52 -0700 Subject: [PATCH] refactor: simplify metric_label to return a &str Issue STOR-64 --- syncserver-common/src/lib.rs | 5 +- syncserver-common/src/middleware/sentry.rs | 4 +- syncserver-db-common/src/error.rs | 17 +++--- syncserver/src/error.rs | 4 +- syncserver/src/web/auth.rs | 3 +- syncserver/src/web/error.rs | 55 +++++++++---------- syncserver/src/web/extractors.rs | 61 +++++++++++----------- syncserver/src/web/mod.rs | 7 --- syncstorage-db-common/src/error.rs | 4 +- syncstorage-mysql/src/error.rs | 2 +- syncstorage-spanner/src/error.rs | 2 +- tokenserver-common/src/error.rs | 6 +-- tokenserver-db/src/error.rs | 2 +- 13 files changed, 80 insertions(+), 92 deletions(-) diff --git a/syncserver-common/src/lib.rs b/syncserver-common/src/lib.rs index 54830063..b6ed89b7 100644 --- a/syncserver-common/src/lib.rs +++ b/syncserver-common/src/lib.rs @@ -82,8 +82,11 @@ pub trait ReportableError: std::fmt::Display + std::fmt::Debug { /// Errors that don't emit Sentry events (!is_sentry_event()) emit an /// increment metric instead with this label - fn metric_label(&self) -> Option; + fn metric_label(&self) -> Option<&str> { + None + } + /// Experimental: return tag key value pairs for metrics and Sentry fn tags(&self) -> Vec<(&str, String)> { vec![] } diff --git a/syncserver-common/src/middleware/sentry.rs b/syncserver-common/src/middleware/sentry.rs index 8da8d4ff..c320cc02 100644 --- a/syncserver-common/src/middleware/sentry.rs +++ b/syncserver-common/src/middleware/sentry.rs @@ -95,7 +95,7 @@ where // capture it, and then turn it off before we run out of money. if let Some(label) = reportable_err.metric_label() { debug!("Sentry: Sending error to metrics: {:?}", reportable_err); - let _ = metrics.incr(&label); + let _ = metrics.incr(label); } debug!("Sentry: Not reporting error (service error): {:?}", error); return Err(error); @@ -117,7 +117,7 @@ where if !reportable_err.is_sentry_event() { if let Some(label) = reportable_err.metric_label() { debug!("Sentry: Sending error to metrics: {:?}", reportable_err); - let _ = metrics.incr(&label); + let _ = metrics.incr(label); } debug!("Not reporting error (service error): {:?}", error); return Ok(response); diff --git a/syncserver-db-common/src/error.rs b/syncserver-db-common/src/error.rs index a4149856..5b8731a4 100644 --- a/syncserver-db-common/src/error.rs +++ b/syncserver-db-common/src/error.rs @@ -48,16 +48,13 @@ impl ReportableError for SqlError { } } - fn metric_label(&self) -> Option { - Some( - match self.kind { - SqlErrorKind::DieselQuery(_) => "storage.sql.error.diesel_query", - SqlErrorKind::DieselConnection(_) => "storage.sql.error.diesel_connection", - SqlErrorKind::Pool(_) => "storage.sql.error.pool", - SqlErrorKind::Migration(_) => "storage.sql.error.migration", - } - .to_string(), - ) + fn metric_label(&self) -> Option<&'static str> { + Some(match self.kind { + SqlErrorKind::DieselQuery(_) => "storage.sql.error.diesel_query", + SqlErrorKind::DieselConnection(_) => "storage.sql.error.diesel_connection", + SqlErrorKind::Pool(_) => "storage.sql.error.pool", + SqlErrorKind::Migration(_) => "storage.sql.error.migration", + }) } fn backtrace(&self) -> Option<&Backtrace> { diff --git a/syncserver/src/error.rs b/syncserver/src/error.rs index 86d6a3a5..c89ed0a1 100644 --- a/syncserver/src/error.rs +++ b/syncserver/src/error.rs @@ -83,7 +83,7 @@ pub enum ApiErrorKind { } impl ApiErrorKind { - pub fn metric_label(&self) -> Option { + pub fn metric_label(&self) -> Option<&str> { match self { ApiErrorKind::Hawk(err) => err.metric_label(), ApiErrorKind::Db(err) => err.metric_label(), @@ -285,7 +285,7 @@ impl ReportableError for ApiError { } } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&str> { self.kind.metric_label() } } diff --git a/syncserver/src/web/auth.rs b/syncserver/src/web/auth.rs index 9153d38b..24f5bba9 100644 --- a/syncserver/src/web/auth.rs +++ b/syncserver/src/web/auth.rs @@ -27,7 +27,6 @@ use super::{ extractors::RequestErrorLocation, }; use crate::error::{ApiErrorKind, ApiResult}; -use crate::label; /// A parsed and authenticated JSON payload /// extracted from the signed `id` property @@ -182,7 +181,7 @@ impl HawkPayload { "Invalid port (hostname:port) specified".to_owned(), RequestErrorLocation::Header, None, - label!("request.validate.hawk.invalid_port"), + Some("request.validate.hawk.invalid_port"), ) })? } else if ci.scheme() == "https" { diff --git a/syncserver/src/web/error.rs b/syncserver/src/web/error.rs index cddd343c..87893db0 100644 --- a/syncserver/src/web/error.rs +++ b/syncserver/src/web/error.rs @@ -32,22 +32,22 @@ impl HawkError { &self.kind } - pub fn metric_label(&self) -> Option { - match self.kind() { - HawkErrorKind::Base64(_) => Some("request.error.hawk.decode_error".to_owned()), - HawkErrorKind::Expired => Some("request.error.hawk.expired".to_owned()), - HawkErrorKind::Header(_) => Some("request.error.hawk.header".to_owned()), - HawkErrorKind::Hmac(_) => Some("request.error.hawk.hmac".to_owned()), - HawkErrorKind::InvalidHeader => Some("request.error.hawk.invalid_header".to_owned()), - HawkErrorKind::InvalidKeyLength(_) => Some("request.error.hawk.expired".to_owned()), - HawkErrorKind::Json(_) => Some("request.error.hawk.invalid_json".to_owned()), - HawkErrorKind::MissingHeader => Some("request.error.hawk.missing_header".to_owned()), - HawkErrorKind::MissingId => Some("request.error.hawk.missing_id".to_owned()), - HawkErrorKind::MissingPrefix => Some("request.error.hawk.missing_prefix".to_owned()), - HawkErrorKind::Parse(_) => Some("request.error.hawk.parse_error".to_owned()), - HawkErrorKind::TruncatedId => Some("request.error.hawk.id_too_short".to_owned()), - _ => None, - } + pub fn metric_label(&self) -> Option<&'static str> { + Some(match self.kind() { + HawkErrorKind::Base64(_) => "request.error.hawk.decode_error", + HawkErrorKind::Expired => "request.error.hawk.expired", + HawkErrorKind::Header(_) => "request.error.hawk.header", + HawkErrorKind::Hmac(_) => "request.error.hawk.hmac", + HawkErrorKind::InvalidHeader => "request.error.hawk.invalid_header", + HawkErrorKind::InvalidKeyLength(_) => "request.error.hawk.expired", + HawkErrorKind::Json(_) => "request.error.hawk.invalid_json", + HawkErrorKind::MissingHeader => "request.error.hawk.missing_header", + HawkErrorKind::MissingId => "request.error.hawk.missing_id", + HawkErrorKind::MissingPrefix => "request.error.hawk.missing_prefix", + HawkErrorKind::Parse(_) => "request.error.hawk.parse_error", + HawkErrorKind::TruncatedId => "request.error.hawk.id_too_short", + _ => return None, + }) } } @@ -102,18 +102,10 @@ pub struct ValidationError { } impl ValidationError { - pub fn metric_label(&self) -> Option { + pub fn metric_label(&self) -> Option<&'static str> { match &self.kind { - ValidationErrorKind::FromDetails( - _description, - ref _location, - Some(ref _name), - metric_label, - ) => metric_label.clone(), - ValidationErrorKind::FromValidationErrors(_errors, _location, metric_label) => { - metric_label.clone() - } - _ => None, + ValidationErrorKind::FromDetails(.., metric_label) + | ValidationErrorKind::FromValidationErrors(.., metric_label) => *metric_label, } } @@ -153,13 +145,18 @@ impl ValidationError { #[derive(Debug, Error)] pub enum ValidationErrorKind { #[error("{}", _0)] - FromDetails(String, RequestErrorLocation, Option, Option), + FromDetails( + String, + RequestErrorLocation, + Option, + Option<&'static str>, + ), #[error("{}", _0)] FromValidationErrors( validator::ValidationErrors, RequestErrorLocation, - Option, + Option<&'static str>, ), } diff --git a/syncserver/src/web/extractors.rs b/syncserver/src/web/extractors.rs index 4bd617b0..f53f34e5 100644 --- a/syncserver/src/web/extractors.rs +++ b/syncserver/src/web/extractors.rs @@ -35,7 +35,6 @@ use tokenserver_auth::TokenserverOrigin; use validator::{Validate, ValidationError}; use crate::error::{ApiError, ApiErrorKind}; -use crate::label; use crate::server::{MetricsWrapper, ServerState, BSO_ID_REGEX, COLLECTION_ID_REGEX}; use crate::web::{ auth::HawkPayload, @@ -185,7 +184,7 @@ impl FromRequest for BsoBodies { format!("Unreadable Content-Type: {:?}", e), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - label!("request.error.invalid_content_type"), + Some("request.error.invalid_content_type"), ) .into(), )) @@ -200,7 +199,7 @@ impl FromRequest for BsoBodies { format!("Invalid Content-Type {:?}", content_type), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - label!("request.error.invalid_content_type"), + Some("request.error.invalid_content_type"), ) .into(), )); @@ -224,7 +223,7 @@ impl FromRequest for BsoBodies { "Invalid JSON in request body".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - label!("request.validate.invalid_body_json"), + Some("request.validate.invalid_body_json"), ) .into() } @@ -302,7 +301,7 @@ impl FromRequest for BsoBodies { "Input BSO has duplicate ID".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - label!("request.store.duplicate_bso_id"), + Some("request.store.duplicate_bso_id"), ) .into(), ); @@ -316,7 +315,7 @@ impl FromRequest for BsoBodies { "Input BSO has no ID".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - label!("request.store.missing_bso_id"), + Some("request.store.missing_bso_id"), ) .into(), ); @@ -387,7 +386,7 @@ impl FromRequest for BsoBody { format!("Unreadable Content-Type: {:?}", e), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - label!("request.error.invalid_content_type"), + Some("request.error.invalid_content_type"), ) .into()) } @@ -399,7 +398,7 @@ impl FromRequest for BsoBody { "Invalid Content-Type".to_owned(), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - label!("request.error.invalid_content_type"), + Some("request.error.invalid_content_type"), ) .into()); } @@ -427,7 +426,7 @@ impl FromRequest for BsoBody { e.to_string(), RequestErrorLocation::Body, Some("bso".to_owned()), - label!("request.validate.bad_bso_body"), + Some("request.validate.bad_bso_body"), ) .into(); err @@ -445,7 +444,7 @@ impl FromRequest for BsoBody { "payload too large".to_owned(), RequestErrorLocation::Body, Some("bso".to_owned()), - label!("request.validate.payload_too_large"), + Some("request.validate.payload_too_large"), ) .into()); } @@ -480,7 +479,7 @@ impl BsoParam { "Invalid BSO".to_owned(), RequestErrorLocation::Path, Some("bso".to_owned()), - label!("request.process.invalid_bso"), + Some("request.process.invalid_bso"), ))?; } if let Some(v) = elements.get(5) { @@ -490,7 +489,7 @@ impl BsoParam { "Invalid BSO".to_owned(), RequestErrorLocation::Path, Some("bso".to_owned()), - label!("request.process.invalid_bso"), + Some("request.process.invalid_bso"), ) })?) .map_err(|e| { @@ -499,7 +498,7 @@ impl BsoParam { "Invalid BSO".to_owned(), RequestErrorLocation::Path, Some("bso".to_owned()), - label!("request.process.invalid_bso"), + Some("request.process.invalid_bso"), ) })?; Ok(Self { bso: sv }) @@ -509,7 +508,7 @@ impl BsoParam { "Missing BSO".to_owned(), RequestErrorLocation::Path, Some("bso".to_owned()), - label!("request.process.missing_bso"), + Some("request.process.missing_bso"), ))? } } @@ -559,7 +558,7 @@ impl CollectionParam { "Missing Collection".to_owned(), RequestErrorLocation::Path, Some("collection".to_owned()), - label!("request.process.missing_collection"), + Some("request.process.missing_collection"), ) })?; sv = urldecode(&sv).map_err(|_e| { @@ -567,7 +566,7 @@ impl CollectionParam { "Invalid Collection".to_owned(), RequestErrorLocation::Path, Some("collection".to_owned()), - label!("request.process.invalid_collection"), + Some("request.process.invalid_collection"), ) })?; Ok(Some(Self { collection: sv })) @@ -576,7 +575,7 @@ impl CollectionParam { "Missing Collection".to_owned(), RequestErrorLocation::Path, Some("collection".to_owned()), - label!("request.process.missing_collection"), + Some("request.process.missing_collection"), ))? } } @@ -615,7 +614,7 @@ impl FromRequest for CollectionParam { "Missing Collection".to_owned(), RequestErrorLocation::Path, Some("collection".to_owned()), - label!("request.process.missing_collection"), + Some("request.process.missing_collection"), ))? } }) @@ -697,7 +696,7 @@ impl FromRequest for CollectionRequest { format!("Invalid Accept header specified: {:?}", accept), RequestErrorLocation::Header, Some("accept".to_string()), - label!("request.validate.invalid_accept_header"), + Some("request.validate.invalid_accept_header"), ) .into()); } @@ -779,7 +778,7 @@ impl FromRequest for CollectionPostRequest { "Known-bad BSO payload".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - label!("request.process.known_bad_bso"), + Some("request.process.known_bad_bso"), ) .into()); } @@ -896,7 +895,7 @@ impl FromRequest for BsoPutRequest { "Known-bad BSO payload".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - label!("request.process.known_bad_bso"), + Some("request.process.known_bad_bso"), ) .into()); } @@ -1024,7 +1023,7 @@ impl HawkIdentifier { "Invalid UID".to_owned(), RequestErrorLocation::Path, Some("uid".to_owned()), - label!("request.validate.hawk.invalid_uid"), + Some("request.validate.hawk.invalid_uid"), ) .into()); } @@ -1036,7 +1035,7 @@ impl HawkIdentifier { "Invalid UID".to_owned(), RequestErrorLocation::Path, Some("uid".to_owned()), - label!("request.validate.hawk.invalid_uid"), + Some("request.validate.hawk.invalid_uid"), ) .into() }) @@ -1046,7 +1045,7 @@ impl HawkIdentifier { "Missing UID".to_owned(), RequestErrorLocation::Path, Some("uid".to_owned()), - label!("request.validate.hawk.missing_uid"), + Some("request.validate.hawk.missing_uid"), ))? } } @@ -1099,7 +1098,7 @@ impl HawkIdentifier { "conflicts with payload".to_owned(), RequestErrorLocation::Path, Some("uid".to_owned()), - label!("request.validate.hawk.uri_missing_uid"), + Some("request.validate.hawk.uri_missing_uid"), ))?; } @@ -1393,7 +1392,7 @@ impl FromRequest for BatchRequestOpt { format!("Invalid integer value: {}", value), RequestErrorLocation::Header, Some((*header).to_owned()), - label!("request.validate.batch.invalid_x_weave"), + Some("request.validate.batch.invalid_x_weave"), ) .into(); err @@ -1403,7 +1402,7 @@ impl FromRequest for BatchRequestOpt { "size-limit-exceeded".to_owned(), RequestErrorLocation::Header, None, - label!("request.validate.batch.size_exceeded"), + Some("request.validate.batch.size_exceeded"), ) .into()); } @@ -1418,7 +1417,7 @@ impl FromRequest for BatchRequestOpt { "Commit with no batch specified".to_string(), RequestErrorLocation::Path, None, - label!("request.validate.batch.missing_id"), + Some("request.validate.batch.missing_id"), ) .into()); } @@ -1445,7 +1444,7 @@ impl FromRequest for BatchRequestOpt { format!(r#"Invalid batch ID: "{}""#, batch), RequestErrorLocation::QueryString, Some("batch".to_owned()), - label!("request.validate.batch.invalid_id"), + Some("request.validate.batch.invalid_id"), ) .into()); } @@ -1491,7 +1490,7 @@ impl PreConditionHeaderOpt { "conflicts with X-If-Modified-Since".to_owned(), RequestErrorLocation::Header, Some("X-If-Unmodified-Since".to_owned()), - label!("request.validate.mod_header.conflict"), + Some("request.validate.mod_header.conflict"), ) .into()); }; @@ -1514,7 +1513,7 @@ impl PreConditionHeaderOpt { "value is negative".to_owned(), RequestErrorLocation::Header, Some("X-If-Modified-Since".to_owned()), - label!("request.validate.mod_header.negative"), + Some("request.validate.mod_header.negative"), ) .into()); } diff --git a/syncserver/src/web/mod.rs b/syncserver/src/web/mod.rs index 671c8289..dbd3171e 100644 --- a/syncserver/src/web/mod.rs +++ b/syncserver/src/web/mod.rs @@ -13,10 +13,3 @@ pub const DOCKER_FLOW_ENDPOINTS: [&str; 4] = [ "/__version__", "/__error__", ]; - -#[macro_export] -macro_rules! label { - ($string:expr) => { - Some($string.to_string()) - }; -} diff --git a/syncstorage-db-common/src/error.rs b/syncstorage-db-common/src/error.rs index 1d69aea9..8e4e678e 100644 --- a/syncstorage-db-common/src/error.rs +++ b/syncstorage-db-common/src/error.rs @@ -101,9 +101,9 @@ impl ReportableError for SyncstorageDbError { !matches!(&self.kind, SyncstorageDbErrorKind::Conflict) } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&'static str> { match self.kind { - SyncstorageDbErrorKind::Conflict => Some("storage.conflict".to_owned()), + SyncstorageDbErrorKind::Conflict => Some("storage.conflict"), _ => None, } } diff --git a/syncstorage-mysql/src/error.rs b/syncstorage-mysql/src/error.rs index d9a66d96..7ab200a4 100644 --- a/syncstorage-mysql/src/error.rs +++ b/syncstorage-mysql/src/error.rs @@ -106,7 +106,7 @@ impl ReportableError for DbError { } } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&str> { match &self.kind { DbErrorKind::Common(e) => e.metric_label(), DbErrorKind::Mysql(e) => e.metric_label(), diff --git a/syncstorage-spanner/src/error.rs b/syncstorage-spanner/src/error.rs index 6e64348e..64c45adf 100644 --- a/syncstorage-spanner/src/error.rs +++ b/syncstorage-spanner/src/error.rs @@ -126,7 +126,7 @@ impl ReportableError for DbError { } } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&str> { if let DbErrorKind::Common(e) = &self.kind { e.metric_label() } else { diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index d794062b..3bde6b33 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -294,13 +294,13 @@ impl ReportableError for TokenserverError { self.http_status.is_server_error() && self.metric_label().is_none() } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&str> { if let Some(source) = &self.source { return source.metric_label(); } if self.http_status.is_client_error() { match self.token_type { - TokenType::Oauth => Some("request.error.oauth".to_owned()), + TokenType::Oauth => Some("request.error.oauth"), } } else if matches!( self, @@ -309,7 +309,7 @@ impl ReportableError for TokenserverError { .. } ) { - Some("request.error.invalid_client_state".to_owned()) + Some("request.error.invalid_client_state") } else { None } diff --git a/tokenserver-db/src/error.rs b/tokenserver-db/src/error.rs index bff809f5..6b7e2ddc 100644 --- a/tokenserver-db/src/error.rs +++ b/tokenserver-db/src/error.rs @@ -40,7 +40,7 @@ impl ReportableError for DbError { } } - fn metric_label(&self) -> Option { + fn metric_label(&self) -> Option<&str> { match &self.kind { DbErrorKind::Sql(e) => e.metric_label(), _ => None,