feat: emit oauth verification timeouts as metrics (not sentry) (#1694)

and fix the sentry middleware to use ReportableError::tags

Closes STOR-201

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() {
+                            maybe_emit_metrics(&metrics, reportable_err);
-                                debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
-                                let _ = metrics.incr(label);
-                            }
                             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::<E>() {
                     if !reportable_err.is_sentry_event() {
-                        if let Some(label) = reportable_err.metric_label() {
+                        maybe_emit_metrics(&metrics, reportable_err);
-                            debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
-                            let _ = metrics.incr(label);
-                        }
                         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<E>(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 {

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())])
             )
         );
     }

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 {
+            .map_err(|_| TokenserverError::oauth_timeout())?
-                context: "OAuth verification timeout".to_owned(),
-                ..TokenserverError::resource_unavailable()
-            })?
     }
 }
 

tokenserver-common/src/error.rs

@@ -23,7 +23,7 @@ pub struct TokenserverError {
     /// distinguish between similar errors in Sentry.
     pub context: String,
     pub backtrace: Box<Backtrace>,
-    pub tags: Option<Box<Vec<(&'static str, String)>>>,
+    pub tags: Option<Vec<(&'static str, String)>>,
     /// 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<Box<Vec<(&'static str, String)>>>,
+        tags: Option<Vec<(&'static str, String)>>,
     ) -> 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");
         }
-
+        (self.status == "invalid-client-state").then_some("request.error.invalid_client_state")
-        if matches!(
-            self,
-            TokenserverError {
-                status: "invalid-client-state",
-                ..
-            }
-        ) {
-            Some("request.error.invalid_client_state")
-        } else {
-            None
-        }
     }
 
     fn tags(&self) -> Vec<(&str, String)> {
-        *self.tags.clone().unwrap_or_default()
+        self.tags.clone().unwrap_or_default()
     }
 }