diff --git a/syncserver-common/src/middleware/sentry.rs b/syncserver-common/src/middleware/sentry.rs index c320cc02..988ee765 100644 --- a/syncserver-common/src/middleware/sentry.rs +++ b/syncserver-common/src/middleware/sentry.rs @@ -93,10 +93,7 @@ where // but we may need that information to debug a production issue. We can // add an info here, temporarily turn on info level debugging on a given server, // 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); - } + maybe_emit_metrics(&metrics, reportable_err); debug!("Sentry: Not reporting error (service error): {:?}", error); return Err(error); } @@ -115,10 +112,7 @@ where if let Some(error) = response.response().error() { if let Some(reportable_err) = error.as_error::() { 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); - } + maybe_emit_metrics(&metrics, reportable_err); debug!("Not reporting error (service error): {:?}", error); return Ok(response); } @@ -134,6 +128,23 @@ where } } +/// Emit metrics when a [ReportableError::metric_label] is returned +fn maybe_emit_metrics(metrics: &StatsdClient, err: &E) +where + E: ReportableError, +{ + let Some(label) = err.metric_label() else { + return; + }; + debug!("Sending error to metrics: {:?}", err); + let mut builder = metrics.incr_with_tags(label); + let tags = err.tags(); + for (key, val) in &tags { + builder = builder.with_tag(key, val); + } + builder.send(); +} + /// Build a Sentry request struct from the HTTP request fn sentry_request_from_http(request: &ServiceRequest) -> sentry::protocol::Request { sentry::protocol::Request { diff --git a/syncserver/src/tokenserver/extractors.rs b/syncserver/src/tokenserver/extractors.rs index 15bcfd60..f41f6793 100644 --- a/syncserver/src/tokenserver/extractors.rs +++ b/syncserver/src/tokenserver/extractors.rs @@ -115,7 +115,7 @@ impl TokenserverRequest { warn!("Client attempted stale value"; "uid"=> self.user.uid, "client_state"=> self.user.client_state.clone()); return Err(TokenserverError::invalid_client_state( error_message, - Some(Box::new(vec![("is_stale", "true".to_owned())])), + Some(vec![("is_stale", "true".to_owned())]), )); } @@ -1200,7 +1200,7 @@ mod tests { error, TokenserverError::invalid_client_state( error_message, - Some(Box::new(vec![("is_stale", "true".to_owned())])) + Some(vec![("is_stale", "true".to_owned())]) ) ); } diff --git a/tokenserver-auth/src/oauth/py.rs b/tokenserver-auth/src/oauth/py.rs index 8377b272..a434868e 100644 --- a/tokenserver-auth/src/oauth/py.rs +++ b/tokenserver-auth/src/oauth/py.rs @@ -176,10 +176,7 @@ impl VerifyToken for Verifier { // than the specified number of seconds. time::timeout(Duration::from_secs(self.timeout), fut) .await - .map_err(|_| TokenserverError { - context: "OAuth verification timeout".to_owned(), - ..TokenserverError::resource_unavailable() - })? + .map_err(|_| TokenserverError::oauth_timeout())? } } diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 26395a7b..814c4a22 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -23,7 +23,7 @@ pub struct TokenserverError { /// distinguish between similar errors in Sentry. pub context: String, pub backtrace: Box, - pub tags: Option>>, + pub tags: Option>, /// TODO: refactor TokenserverError to include a TokenserverErrorKind, w/ /// variants for sources (currently just DbError). May require moving /// TokenserverError out of common (into syncserver) @@ -107,7 +107,7 @@ impl TokenserverError { pub fn invalid_client_state( description: String, - tags: Option>>, + tags: Option>, ) -> Self { Self { status: "invalid-client-state", @@ -147,7 +147,15 @@ impl TokenserverError { description: "Resource is not available".to_owned(), http_status: StatusCode::SERVICE_UNAVAILABLE, context: "Resource is not available".to_owned(), - ..Default::default() + ..Self::default() + } + } + + pub fn oauth_timeout() -> Self { + Self { + context: "OAuth verification timeout".to_owned(), + tags: Some(vec![("reason", "oauth_verify_timeout".to_owned())]), + ..Self::resource_unavailable() } } @@ -284,6 +292,9 @@ impl ReportableError for TokenserverError { if let Some(source) = &self.source { return source.is_sentry_event(); } + if self.http_status == StatusCode::SERVICE_UNAVAILABLE { + return false; + } self.http_status.is_server_error() && self.metric_label().is_none() } @@ -291,25 +302,18 @@ impl ReportableError for TokenserverError { if let Some(source) = &self.source { return source.metric_label(); } + + if self.http_status == StatusCode::SERVICE_UNAVAILABLE { + return Some("request.error.resource_unavailable"); + } if self.http_status.is_client_error() { return Some("request.error.oauth"); } - - if matches!( - self, - TokenserverError { - status: "invalid-client-state", - .. - } - ) { - Some("request.error.invalid_client_state") - } else { - None - } + (self.status == "invalid-client-state").then_some("request.error.invalid_client_state") } fn tags(&self) -> Vec<(&str, String)> { - *self.tags.clone().unwrap_or_default() + self.tags.clone().unwrap_or_default() } }