refactor: simplify metric_label to return a &str

Issue STOR-64
This commit is contained in:
Philip Jenvey 2025-04-18 22:59:52 -07:00
parent 1d355ec7b5
commit 0ca435fb1a
No known key found for this signature in database
GPG Key ID: 5B9F83DE4F7EB7FA
13 changed files with 80 additions and 92 deletions

View File

@ -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 /// Errors that don't emit Sentry events (!is_sentry_event()) emit an
/// increment metric instead with this label /// increment metric instead with this label
fn metric_label(&self) -> Option<String>; fn metric_label(&self) -> Option<&str> {
None
}
/// Experimental: return tag key value pairs for metrics and Sentry
fn tags(&self) -> Vec<(&str, String)> { fn tags(&self) -> Vec<(&str, String)> {
vec![] vec![]
} }

View File

@ -95,7 +95,7 @@ where
// capture it, and then turn it off before we run out of money. // capture it, and then turn it off before we run out of money.
if let Some(label) = reportable_err.metric_label() { if let Some(label) = reportable_err.metric_label() {
debug!("Sentry: Sending error to metrics: {:?}", reportable_err); debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
let _ = metrics.incr(&label); let _ = metrics.incr(label);
} }
debug!("Sentry: Not reporting error (service error): {:?}", error); debug!("Sentry: Not reporting error (service error): {:?}", error);
return Err(error); return Err(error);
@ -117,7 +117,7 @@ where
if !reportable_err.is_sentry_event() { if !reportable_err.is_sentry_event() {
if let Some(label) = reportable_err.metric_label() { if let Some(label) = reportable_err.metric_label() {
debug!("Sentry: Sending error to metrics: {:?}", reportable_err); debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
let _ = metrics.incr(&label); let _ = metrics.incr(label);
} }
debug!("Not reporting error (service error): {:?}", error); debug!("Not reporting error (service error): {:?}", error);
return Ok(response); return Ok(response);

View File

@ -48,16 +48,13 @@ impl ReportableError for SqlError {
} }
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&'static str> {
Some( Some(match self.kind {
match self.kind { SqlErrorKind::DieselQuery(_) => "storage.sql.error.diesel_query",
SqlErrorKind::DieselQuery(_) => "storage.sql.error.diesel_query", SqlErrorKind::DieselConnection(_) => "storage.sql.error.diesel_connection",
SqlErrorKind::DieselConnection(_) => "storage.sql.error.diesel_connection", SqlErrorKind::Pool(_) => "storage.sql.error.pool",
SqlErrorKind::Pool(_) => "storage.sql.error.pool", SqlErrorKind::Migration(_) => "storage.sql.error.migration",
SqlErrorKind::Migration(_) => "storage.sql.error.migration", })
}
.to_string(),
)
} }
fn backtrace(&self) -> Option<&Backtrace> { fn backtrace(&self) -> Option<&Backtrace> {

View File

@ -83,7 +83,7 @@ pub enum ApiErrorKind {
} }
impl ApiErrorKind { impl ApiErrorKind {
pub fn metric_label(&self) -> Option<String> { pub fn metric_label(&self) -> Option<&str> {
match self { match self {
ApiErrorKind::Hawk(err) => err.metric_label(), ApiErrorKind::Hawk(err) => err.metric_label(),
ApiErrorKind::Db(err) => err.metric_label(), ApiErrorKind::Db(err) => err.metric_label(),
@ -285,7 +285,7 @@ impl ReportableError for ApiError {
} }
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&str> {
self.kind.metric_label() self.kind.metric_label()
} }
} }

View File

@ -27,7 +27,6 @@ use super::{
extractors::RequestErrorLocation, extractors::RequestErrorLocation,
}; };
use crate::error::{ApiErrorKind, ApiResult}; use crate::error::{ApiErrorKind, ApiResult};
use crate::label;
/// A parsed and authenticated JSON payload /// A parsed and authenticated JSON payload
/// extracted from the signed `id` property /// extracted from the signed `id` property
@ -182,7 +181,7 @@ impl HawkPayload {
"Invalid port (hostname:port) specified".to_owned(), "Invalid port (hostname:port) specified".to_owned(),
RequestErrorLocation::Header, RequestErrorLocation::Header,
None, None,
label!("request.validate.hawk.invalid_port"), Some("request.validate.hawk.invalid_port"),
) )
})? })?
} else if ci.scheme() == "https" { } else if ci.scheme() == "https" {

View File

@ -32,22 +32,22 @@ impl HawkError {
&self.kind &self.kind
} }
pub fn metric_label(&self) -> Option<String> { pub fn metric_label(&self) -> Option<&'static str> {
match self.kind() { Some(match self.kind() {
HawkErrorKind::Base64(_) => Some("request.error.hawk.decode_error".to_owned()), HawkErrorKind::Base64(_) => "request.error.hawk.decode_error",
HawkErrorKind::Expired => Some("request.error.hawk.expired".to_owned()), HawkErrorKind::Expired => "request.error.hawk.expired",
HawkErrorKind::Header(_) => Some("request.error.hawk.header".to_owned()), HawkErrorKind::Header(_) => "request.error.hawk.header",
HawkErrorKind::Hmac(_) => Some("request.error.hawk.hmac".to_owned()), HawkErrorKind::Hmac(_) => "request.error.hawk.hmac",
HawkErrorKind::InvalidHeader => Some("request.error.hawk.invalid_header".to_owned()), HawkErrorKind::InvalidHeader => "request.error.hawk.invalid_header",
HawkErrorKind::InvalidKeyLength(_) => Some("request.error.hawk.expired".to_owned()), HawkErrorKind::InvalidKeyLength(_) => "request.error.hawk.expired",
HawkErrorKind::Json(_) => Some("request.error.hawk.invalid_json".to_owned()), HawkErrorKind::Json(_) => "request.error.hawk.invalid_json",
HawkErrorKind::MissingHeader => Some("request.error.hawk.missing_header".to_owned()), HawkErrorKind::MissingHeader => "request.error.hawk.missing_header",
HawkErrorKind::MissingId => Some("request.error.hawk.missing_id".to_owned()), HawkErrorKind::MissingId => "request.error.hawk.missing_id",
HawkErrorKind::MissingPrefix => Some("request.error.hawk.missing_prefix".to_owned()), HawkErrorKind::MissingPrefix => "request.error.hawk.missing_prefix",
HawkErrorKind::Parse(_) => Some("request.error.hawk.parse_error".to_owned()), HawkErrorKind::Parse(_) => "request.error.hawk.parse_error",
HawkErrorKind::TruncatedId => Some("request.error.hawk.id_too_short".to_owned()), HawkErrorKind::TruncatedId => "request.error.hawk.id_too_short",
_ => None, _ => return None,
} })
} }
} }
@ -102,18 +102,10 @@ pub struct ValidationError {
} }
impl ValidationError { impl ValidationError {
pub fn metric_label(&self) -> Option<String> { pub fn metric_label(&self) -> Option<&'static str> {
match &self.kind { match &self.kind {
ValidationErrorKind::FromDetails( ValidationErrorKind::FromDetails(.., metric_label)
_description, | ValidationErrorKind::FromValidationErrors(.., metric_label) => *metric_label,
ref _location,
Some(ref _name),
metric_label,
) => metric_label.clone(),
ValidationErrorKind::FromValidationErrors(_errors, _location, metric_label) => {
metric_label.clone()
}
_ => None,
} }
} }
@ -153,13 +145,18 @@ impl ValidationError {
#[derive(Debug, Error)] #[derive(Debug, Error)]
pub enum ValidationErrorKind { pub enum ValidationErrorKind {
#[error("{}", _0)] #[error("{}", _0)]
FromDetails(String, RequestErrorLocation, Option<String>, Option<String>), FromDetails(
String,
RequestErrorLocation,
Option<String>,
Option<&'static str>,
),
#[error("{}", _0)] #[error("{}", _0)]
FromValidationErrors( FromValidationErrors(
validator::ValidationErrors, validator::ValidationErrors,
RequestErrorLocation, RequestErrorLocation,
Option<String>, Option<&'static str>,
), ),
} }

View File

@ -35,7 +35,6 @@ use tokenserver_auth::TokenserverOrigin;
use validator::{Validate, ValidationError}; use validator::{Validate, ValidationError};
use crate::error::{ApiError, ApiErrorKind}; use crate::error::{ApiError, ApiErrorKind};
use crate::label;
use crate::server::{MetricsWrapper, ServerState, BSO_ID_REGEX, COLLECTION_ID_REGEX}; use crate::server::{MetricsWrapper, ServerState, BSO_ID_REGEX, COLLECTION_ID_REGEX};
use crate::web::{ use crate::web::{
auth::HawkPayload, auth::HawkPayload,
@ -185,7 +184,7 @@ impl FromRequest for BsoBodies {
format!("Unreadable Content-Type: {:?}", e), format!("Unreadable Content-Type: {:?}", e),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("Content-Type".to_owned()), Some("Content-Type".to_owned()),
label!("request.error.invalid_content_type"), Some("request.error.invalid_content_type"),
) )
.into(), .into(),
)) ))
@ -200,7 +199,7 @@ impl FromRequest for BsoBodies {
format!("Invalid Content-Type {:?}", content_type), format!("Invalid Content-Type {:?}", content_type),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("Content-Type".to_owned()), Some("Content-Type".to_owned()),
label!("request.error.invalid_content_type"), Some("request.error.invalid_content_type"),
) )
.into(), .into(),
)); ));
@ -224,7 +223,7 @@ impl FromRequest for BsoBodies {
"Invalid JSON in request body".to_owned(), "Invalid JSON in request body".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bsos".to_owned()), Some("bsos".to_owned()),
label!("request.validate.invalid_body_json"), Some("request.validate.invalid_body_json"),
) )
.into() .into()
} }
@ -302,7 +301,7 @@ impl FromRequest for BsoBodies {
"Input BSO has duplicate ID".to_owned(), "Input BSO has duplicate ID".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bsos".to_owned()), Some("bsos".to_owned()),
label!("request.store.duplicate_bso_id"), Some("request.store.duplicate_bso_id"),
) )
.into(), .into(),
); );
@ -316,7 +315,7 @@ impl FromRequest for BsoBodies {
"Input BSO has no ID".to_owned(), "Input BSO has no ID".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bsos".to_owned()), Some("bsos".to_owned()),
label!("request.store.missing_bso_id"), Some("request.store.missing_bso_id"),
) )
.into(), .into(),
); );
@ -387,7 +386,7 @@ impl FromRequest for BsoBody {
format!("Unreadable Content-Type: {:?}", e), format!("Unreadable Content-Type: {:?}", e),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("Content-Type".to_owned()), Some("Content-Type".to_owned()),
label!("request.error.invalid_content_type"), Some("request.error.invalid_content_type"),
) )
.into()) .into())
} }
@ -399,7 +398,7 @@ impl FromRequest for BsoBody {
"Invalid Content-Type".to_owned(), "Invalid Content-Type".to_owned(),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("Content-Type".to_owned()), Some("Content-Type".to_owned()),
label!("request.error.invalid_content_type"), Some("request.error.invalid_content_type"),
) )
.into()); .into());
} }
@ -427,7 +426,7 @@ impl FromRequest for BsoBody {
e.to_string(), e.to_string(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.validate.bad_bso_body"), Some("request.validate.bad_bso_body"),
) )
.into(); .into();
err err
@ -445,7 +444,7 @@ impl FromRequest for BsoBody {
"payload too large".to_owned(), "payload too large".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.validate.payload_too_large"), Some("request.validate.payload_too_large"),
) )
.into()); .into());
} }
@ -480,7 +479,7 @@ impl BsoParam {
"Invalid BSO".to_owned(), "Invalid BSO".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.process.invalid_bso"), Some("request.process.invalid_bso"),
))?; ))?;
} }
if let Some(v) = elements.get(5) { if let Some(v) = elements.get(5) {
@ -490,7 +489,7 @@ impl BsoParam {
"Invalid BSO".to_owned(), "Invalid BSO".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.process.invalid_bso"), Some("request.process.invalid_bso"),
) )
})?) })?)
.map_err(|e| { .map_err(|e| {
@ -499,7 +498,7 @@ impl BsoParam {
"Invalid BSO".to_owned(), "Invalid BSO".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.process.invalid_bso"), Some("request.process.invalid_bso"),
) )
})?; })?;
Ok(Self { bso: sv }) Ok(Self { bso: sv })
@ -509,7 +508,7 @@ impl BsoParam {
"Missing BSO".to_owned(), "Missing BSO".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("bso".to_owned()), Some("bso".to_owned()),
label!("request.process.missing_bso"), Some("request.process.missing_bso"),
))? ))?
} }
} }
@ -559,7 +558,7 @@ impl CollectionParam {
"Missing Collection".to_owned(), "Missing Collection".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("collection".to_owned()), Some("collection".to_owned()),
label!("request.process.missing_collection"), Some("request.process.missing_collection"),
) )
})?; })?;
sv = urldecode(&sv).map_err(|_e| { sv = urldecode(&sv).map_err(|_e| {
@ -567,7 +566,7 @@ impl CollectionParam {
"Invalid Collection".to_owned(), "Invalid Collection".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("collection".to_owned()), Some("collection".to_owned()),
label!("request.process.invalid_collection"), Some("request.process.invalid_collection"),
) )
})?; })?;
Ok(Some(Self { collection: sv })) Ok(Some(Self { collection: sv }))
@ -576,7 +575,7 @@ impl CollectionParam {
"Missing Collection".to_owned(), "Missing Collection".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("collection".to_owned()), 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(), "Missing Collection".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("collection".to_owned()), 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), format!("Invalid Accept header specified: {:?}", accept),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("accept".to_string()), Some("accept".to_string()),
label!("request.validate.invalid_accept_header"), Some("request.validate.invalid_accept_header"),
) )
.into()); .into());
} }
@ -779,7 +778,7 @@ impl FromRequest for CollectionPostRequest {
"Known-bad BSO payload".to_owned(), "Known-bad BSO payload".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bsos".to_owned()), Some("bsos".to_owned()),
label!("request.process.known_bad_bso"), Some("request.process.known_bad_bso"),
) )
.into()); .into());
} }
@ -896,7 +895,7 @@ impl FromRequest for BsoPutRequest {
"Known-bad BSO payload".to_owned(), "Known-bad BSO payload".to_owned(),
RequestErrorLocation::Body, RequestErrorLocation::Body,
Some("bsos".to_owned()), Some("bsos".to_owned()),
label!("request.process.known_bad_bso"), Some("request.process.known_bad_bso"),
) )
.into()); .into());
} }
@ -1024,7 +1023,7 @@ impl HawkIdentifier {
"Invalid UID".to_owned(), "Invalid UID".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("uid".to_owned()), Some("uid".to_owned()),
label!("request.validate.hawk.invalid_uid"), Some("request.validate.hawk.invalid_uid"),
) )
.into()); .into());
} }
@ -1036,7 +1035,7 @@ impl HawkIdentifier {
"Invalid UID".to_owned(), "Invalid UID".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("uid".to_owned()), Some("uid".to_owned()),
label!("request.validate.hawk.invalid_uid"), Some("request.validate.hawk.invalid_uid"),
) )
.into() .into()
}) })
@ -1046,7 +1045,7 @@ impl HawkIdentifier {
"Missing UID".to_owned(), "Missing UID".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("uid".to_owned()), 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(), "conflicts with payload".to_owned(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
Some("uid".to_owned()), 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), format!("Invalid integer value: {}", value),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some((*header).to_owned()), Some((*header).to_owned()),
label!("request.validate.batch.invalid_x_weave"), Some("request.validate.batch.invalid_x_weave"),
) )
.into(); .into();
err err
@ -1403,7 +1402,7 @@ impl FromRequest for BatchRequestOpt {
"size-limit-exceeded".to_owned(), "size-limit-exceeded".to_owned(),
RequestErrorLocation::Header, RequestErrorLocation::Header,
None, None,
label!("request.validate.batch.size_exceeded"), Some("request.validate.batch.size_exceeded"),
) )
.into()); .into());
} }
@ -1418,7 +1417,7 @@ impl FromRequest for BatchRequestOpt {
"Commit with no batch specified".to_string(), "Commit with no batch specified".to_string(),
RequestErrorLocation::Path, RequestErrorLocation::Path,
None, None,
label!("request.validate.batch.missing_id"), Some("request.validate.batch.missing_id"),
) )
.into()); .into());
} }
@ -1445,7 +1444,7 @@ impl FromRequest for BatchRequestOpt {
format!(r#"Invalid batch ID: "{}""#, batch), format!(r#"Invalid batch ID: "{}""#, batch),
RequestErrorLocation::QueryString, RequestErrorLocation::QueryString,
Some("batch".to_owned()), Some("batch".to_owned()),
label!("request.validate.batch.invalid_id"), Some("request.validate.batch.invalid_id"),
) )
.into()); .into());
} }
@ -1491,7 +1490,7 @@ impl PreConditionHeaderOpt {
"conflicts with X-If-Modified-Since".to_owned(), "conflicts with X-If-Modified-Since".to_owned(),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("X-If-Unmodified-Since".to_owned()), Some("X-If-Unmodified-Since".to_owned()),
label!("request.validate.mod_header.conflict"), Some("request.validate.mod_header.conflict"),
) )
.into()); .into());
}; };
@ -1514,7 +1513,7 @@ impl PreConditionHeaderOpt {
"value is negative".to_owned(), "value is negative".to_owned(),
RequestErrorLocation::Header, RequestErrorLocation::Header,
Some("X-If-Modified-Since".to_owned()), Some("X-If-Modified-Since".to_owned()),
label!("request.validate.mod_header.negative"), Some("request.validate.mod_header.negative"),
) )
.into()); .into());
} }

View File

@ -13,10 +13,3 @@ pub const DOCKER_FLOW_ENDPOINTS: [&str; 4] = [
"/__version__", "/__version__",
"/__error__", "/__error__",
]; ];
#[macro_export]
macro_rules! label {
($string:expr) => {
Some($string.to_string())
};
}

View File

@ -101,9 +101,9 @@ impl ReportableError for SyncstorageDbError {
!matches!(&self.kind, SyncstorageDbErrorKind::Conflict) !matches!(&self.kind, SyncstorageDbErrorKind::Conflict)
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&'static str> {
match self.kind { match self.kind {
SyncstorageDbErrorKind::Conflict => Some("storage.conflict".to_owned()), SyncstorageDbErrorKind::Conflict => Some("storage.conflict"),
_ => None, _ => None,
} }
} }

View File

@ -106,7 +106,7 @@ impl ReportableError for DbError {
} }
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&str> {
match &self.kind { match &self.kind {
DbErrorKind::Common(e) => e.metric_label(), DbErrorKind::Common(e) => e.metric_label(),
DbErrorKind::Mysql(e) => e.metric_label(), DbErrorKind::Mysql(e) => e.metric_label(),

View File

@ -126,7 +126,7 @@ impl ReportableError for DbError {
} }
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&str> {
if let DbErrorKind::Common(e) = &self.kind { if let DbErrorKind::Common(e) = &self.kind {
e.metric_label() e.metric_label()
} else { } else {

View File

@ -294,13 +294,13 @@ impl ReportableError for TokenserverError {
self.http_status.is_server_error() && self.metric_label().is_none() self.http_status.is_server_error() && self.metric_label().is_none()
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&str> {
if let Some(source) = &self.source { if let Some(source) = &self.source {
return source.metric_label(); return source.metric_label();
} }
if self.http_status.is_client_error() { if self.http_status.is_client_error() {
match self.token_type { match self.token_type {
TokenType::Oauth => Some("request.error.oauth".to_owned()), TokenType::Oauth => Some("request.error.oauth"),
} }
} else if matches!( } else if matches!(
self, self,
@ -309,7 +309,7 @@ impl ReportableError for TokenserverError {
.. ..
} }
) { ) {
Some("request.error.invalid_client_state".to_owned()) Some("request.error.invalid_client_state")
} else { } else {
None None
} }

View File

@ -40,7 +40,7 @@ impl ReportableError for DbError {
} }
} }
fn metric_label(&self) -> Option<String> { fn metric_label(&self) -> Option<&str> {
match &self.kind { match &self.kind {
DbErrorKind::Sql(e) => e.metric_label(), DbErrorKind::Sql(e) => e.metric_label(),
_ => None, _ => None,