feat: Add normalized ReportableError to errors

Cargo.toml

@@ -37,6 +37,12 @@ chrono = "0.4"
 docopt = "1.1"
 env_logger = "0.10"
 futures = { version = "0.3", features = ["compat"] }
+futures-util = { version = "0.3", features = [
+  "async-await",
+  "compat",
+  "sink",
+  "io",
+] }
 hex = "0.4"
 hkdf = "0.12"
 hmac = "0.12"

syncserver-common/Cargo.toml

@@ -6,6 +6,7 @@ authors.workspace = true
 edition.workspace = true
 
 [dependencies]
+backtrace.workspace = true
 cadence.workspace = true
 futures.workspace = true
 sha2.workspace = true
@@ -15,4 +16,3 @@ slog.workspace = true
 slog-scope.workspace = true
 actix-web.workspace = true
 hkdf.workspace = true
-

syncserver-common/src/lib.rs

@@ -9,7 +9,9 @@ use std::{
 };
 
 use actix_web::web;
+use backtrace::Backtrace;
 use hkdf::Hkdf;
+use serde_json::Value;
 use sha2::Sha256;
 
 pub use metrics::{metrics_from_opts, MetricError, Metrics};
@@ -58,10 +60,35 @@ macro_rules! impl_fmt_display {
     };
 }
 
-pub trait ReportableError {
-    fn error_backtrace(&self) -> String;
+pub trait ReportableError: std::fmt::Display {
+    /// Like [Error::source] but returns the source (if any) of this error as a
+    /// [ReportableError] if it implements the trait. Otherwise callers of this
+    /// method will likely subsequently call [Error::source] to return the
+    /// source (if any) as the parent [Error] trait.
+
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        None
+    }
+
+    /// Return a `Backtrace` for this Error if one was captured
+    fn backtrace(&self) -> Option<&Backtrace>;
+
+    /// Whether this error is reported to Sentry
     fn is_sentry_event(&self) -> bool;
+
+    /// Errors that don't emit Sentry events (!is_sentry_event()) emit an
+    /// increment metric instead with this label
     fn metric_label(&self) -> Option<String>;
+
+    fn tags(&self) -> Vec<(&str, String)> {
+        vec![]
+    }
+
+    /// Experimental: return key value pairs for Sentry Event's extra data
+    /// TODO: should probably return Vec<(&str, Value)> or Vec<(String, Value)>
+    fn extras(&self) -> Vec<(&str, Value)> {
+        vec![]
+    }
 }
 
 /// Types that implement this trait can represent internal errors.

syncserver-db-common/src/error.rs

@@ -2,7 +2,7 @@ use std::fmt;
 
 use backtrace::Backtrace;
 use http::StatusCode;
-use syncserver_common::{from_error, impl_fmt_display};
+use syncserver_common::{from_error, impl_fmt_display, ReportableError};
 use thiserror::Error;
 
 /// Error specific to any MySQL database backend. These errors are not related to the syncstorage
@@ -39,6 +39,28 @@ impl From<MysqlErrorKind> for MysqlError {
     }
 }
 
+impl ReportableError for MysqlError {
+    fn is_sentry_event(&self) -> bool {
+        true
+    }
+
+    fn metric_label(&self) -> Option<String> {
+        Some(
+            match self.kind {
+                MysqlErrorKind::DieselQuery(_) => "diesel_query",
+                MysqlErrorKind::DieselConnection(_) => "diesel_connection",
+                MysqlErrorKind::Pool(_) => "pool",
+                MysqlErrorKind::Migration(_) => "migration",
+            }
+            .to_string(),
+        )
+    }
+
+    fn backtrace(&self) -> Option<&Backtrace> {
+        Some(&self.backtrace)
+    }
+}
+
 impl_fmt_display!(MysqlError, MysqlErrorKind);
 
 from_error!(

syncserver/Cargo.toml

@@ -14,6 +14,7 @@ cadence.workspace = true
 chrono.workspace = true
 docopt.workspace = true
 futures.workspace = true
+futures-util.workspace = true
 hex.workspace = true
 lazy_static.workspace = true
 rand.workspace = true
@@ -49,7 +50,7 @@ syncserver-settings = { path = "../syncserver-settings" }
 syncstorage-db = { path = "../syncstorage-db" }
 syncstorage-settings = { path = "../syncstorage-settings" }
 time = "^0.3"
-tokenserver-auth = { path = "../tokenserver-auth", default-features = false}
+tokenserver-auth = { path = "../tokenserver-auth", default-features = false }
 tokenserver-common = { path = "../tokenserver-common" }
 tokenserver-db = { path = "../tokenserver-db" }
 tokenserver-settings = { path = "../tokenserver-settings" }

syncserver/src/error.rs

@@ -270,8 +270,8 @@ from_error!(HawkError, ApiError, ApiErrorKind::Hawk);
 from_error!(ValidationError, ApiError, ApiErrorKind::Validation);
 
 impl ReportableError for ApiError {
-    fn error_backtrace(&self) -> String {
-        format!("{:#?}", self.backtrace)
+    fn backtrace(&self) -> Option<&Backtrace> {
+        Some(&self.backtrace)
     }
 
     fn is_sentry_event(&self) -> bool {

syncserver/src/server/mod.rs

@@ -23,6 +23,7 @@ use tokio::{sync::RwLock, time};
 use crate::error::ApiError;
 use crate::server::tags::Taggable;
 use crate::tokenserver;
+use crate::web::middleware::sentry::SentryWrapper;
 use crate::web::{handlers, middleware};
 
 pub const BSO_ID_REGEX: &str = r"[ -~]{1,64}";
@@ -72,7 +73,7 @@ pub struct Server;
 
 #[macro_export]
 macro_rules! build_app {
-    ($syncstorage_state: expr, $tokenserver_state: expr, $secrets: expr, $limits: expr, $cors: expr) => {
+    ($syncstorage_state: expr, $tokenserver_state: expr, $secrets: expr, $limits: expr, $cors: expr, $metrics: expr) => {
         App::new()
             .configure(|cfg| {
                 cfg.app_data(Data::new($syncstorage_state));
@@ -87,9 +88,13 @@ macro_rules! build_app {
             // These will wrap all outbound responses with matching status codes.
             .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404))
             // These are our wrappers
+            .wrap(SentryWrapper::<ApiError>::new(
+                $metrics.clone(),
+                "api_error".to_owned(),
+            ))
             .wrap_fn(middleware::weave::set_weave_timestamp)
             .wrap_fn(tokenserver::logging::handle_request_log_line)
-            .wrap_fn(middleware::sentry::report_error)
+            //.wrap_fn(middleware::sentry::report_error)
             .wrap_fn(middleware::rejectua::reject_user_agent)
             .wrap($cors)
             .wrap_fn(middleware::emit_http_status_with_tokenserver_origin)
@@ -186,15 +191,19 @@ macro_rules! build_app {
 
 #[macro_export]
 macro_rules! build_app_without_syncstorage {
-    ($state: expr, $secrets: expr, $cors: expr) => {
+    ($state: expr, $secrets: expr, $cors: expr, $metrics: expr) => {
         App::new()
             .app_data(Data::new($state))
             .app_data(Data::new($secrets))
             // Middleware is applied LIFO
             // These will wrap all outbound responses with matching status codes.
             .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404))
+            .wrap(SentryWrapper::<ApiError>::new(
+                $metrics.clone(),
+                "api_error".to_owned(),
+            ))
             // These are our wrappers
-            .wrap_fn(middleware::sentry::report_error)
+            //.wrap_fn(middleware::sentry::report_error)
             .wrap_fn(tokenserver::logging::handle_request_log_line)
             .wrap_fn(middleware::rejectua::reject_user_agent)
             // Followed by the "official middleware" so they run first.
@@ -309,7 +318,8 @@ impl Server {
                 tokenserver_state.clone(),
                 Arc::clone(&secrets),
                 limits,
-                build_cors(&settings_copy)
+                build_cors(&settings_copy),
+                metrics.clone()
             )
         });
 
@@ -353,7 +363,8 @@ impl Server {
             build_app_without_syncstorage!(
                 tokenserver_state.clone(),
                 Arc::clone(&secrets),
-                build_cors(&settings_copy)
+                build_cors(&settings_copy),
+                tokenserver_state.metrics.clone()
             )
         });
 

syncserver/src/server/tags.rs

@@ -1,6 +1,7 @@
 use std::collections::HashMap;
 
 use actix_web::HttpMessage;
+use serde_json::Value;
 
 pub trait Taggable {
     /// Adds a tag to be included in any metric or Sentry error emitted from this point in the
@@ -16,8 +17,10 @@ pub trait Taggable {
     /// cardinality that is too high for tags. Note that extras will not be included with metrics.
     fn add_extra(&self, key: String, value: String);
 
-    /// Gets all the extras associated with `Self`.
-    fn get_extras(&self) -> HashMap<String, String>;
+    /// Gets all the extras associated with `Self`. This converts the values to `serde_json::Value`
+    /// because the only caller / consumer for this function is the Sentry middleware, which uses
+    /// `Value` for extras.
+    fn get_extras(&self) -> HashMap<String, Value>;
 }
 
 impl<T> Taggable for T
@@ -61,10 +64,17 @@ where
         }
     }
 
-    fn get_extras(&self) -> HashMap<String, String> {
+    fn get_extras(&self) -> HashMap<String, Value> {
         self.extensions()
             .get::<Extras>()
-            .map(|extras_ref| extras_ref.0.clone())
+            .map(|extras_ref| {
+                extras_ref
+                    .0
+                    .clone()
+                    .into_iter()
+                    .map(|(k, v)| (k, Value::from(v)))
+                    .collect()
+            })
             .unwrap_or_default()
     }
 }

syncserver/src/server/test.rs

@@ -114,12 +114,14 @@ macro_rules! init_app {
             crate::logging::init_logging(false).unwrap();
             let limits = Arc::new($settings.syncstorage.limits.clone());
             let state = get_test_state(&$settings).await;
+            let metrics = state.metrics.clone();
             test::init_service(build_app!(
                 state,
                 None::<tokenserver::ServerState>,
                 Arc::clone(&SECRETS),
                 limits,
-                build_cors(&$settings)
+                build_cors(&$settings),
+                metrics
             ))
             .await
         }
@@ -232,12 +234,14 @@ where
     let settings = get_test_settings();
     let limits = Arc::new(settings.syncstorage.limits.clone());
     let state = get_test_state(&settings).await;
+    let metrics = state.metrics.clone();
     let app = test::init_service(build_app!(
         state,
         None::<tokenserver::ServerState>,
         Arc::clone(&SECRETS),
         limits,
-        build_cors(&settings)
+        build_cors(&settings),
+        metrics
     ))
     .await;
 
@@ -274,12 +278,14 @@ async fn test_endpoint_with_body(
     let settings = get_test_settings();
     let limits = Arc::new(settings.syncstorage.limits.clone());
     let state = get_test_state(&settings).await;
+    let metrics = state.metrics.clone();
     let app = test::init_service(build_app!(
         state,
         None::<tokenserver::ServerState>,
         Arc::clone(&SECRETS),
         limits,
-        build_cors(&settings)
+        build_cors(&settings),
+        metrics
     ))
     .await;
     let req = create_request(method, path, None, Some(body)).to_request();

syncserver/src/tokenserver/extractors.rs

@@ -103,7 +103,7 @@ impl TokenserverRequest {
         // always include a client state.
         if !self.user.client_state.is_empty() && self.auth_data.client_state.is_empty() {
             let error_message = "Unacceptable client-state value empty string".to_owned();
-            return Err(TokenserverError::invalid_client_state(error_message));
+            return Err(TokenserverError::invalid_client_state(error_message, None));
         }
 
         // The client state on the request must not have been used in the past.
@@ -114,7 +114,10 @@ impl TokenserverRequest {
         {
             let error_message = "Unacceptable client-state value stale value".to_owned();
             warn!("Client attempted stale value"; "uid"=> self.user.uid, "client_state"=> self.user.client_state.clone());
-            return Err(TokenserverError::invalid_client_state(error_message));
+            return Err(TokenserverError::invalid_client_state(
+                error_message,
+                Some(Box::new(vec![("is_stale", "true".to_owned())])),
+            ));
         }
 
         // If the client state on the request differs from the most recently-used client state, it must
@@ -124,7 +127,7 @@ impl TokenserverRequest {
         {
             let error_message =
                 "Unacceptable client-state value new value with no generation change".to_owned();
-            return Err(TokenserverError::invalid_client_state(error_message));
+            return Err(TokenserverError::invalid_client_state(error_message, None));
         }
 
         // If the client state on the request differs from the most recently-used client state, it must
@@ -135,7 +138,7 @@ impl TokenserverRequest {
             let error_message =
                 "Unacceptable client-state value new value with no keys_changed_at change"
                     .to_owned();
-            return Err(TokenserverError::invalid_client_state(error_message));
+            return Err(TokenserverError::invalid_client_state(error_message, None));
         }
 
         // The generation on the request cannot be earlier than the generation stored on the user
@@ -1237,7 +1240,13 @@ mod tests {
 
         let error = tokenserver_request.validate().unwrap_err();
         let error_message = "Unacceptable client-state value stale value".to_owned();
-        assert_eq!(error, TokenserverError::invalid_client_state(error_message));
+        assert_eq!(
+            error,
+            TokenserverError::invalid_client_state(
+                error_message,
+                Some(Box::new(vec![("is_stale", "true".to_owned())]))
+            )
+        );
     }
 
     #[actix_rt::test]
@@ -1275,7 +1284,10 @@ mod tests {
         let error = tokenserver_request.validate().unwrap_err();
         let error_message =
             "Unacceptable client-state value new value with no generation change".to_owned();
-        assert_eq!(error, TokenserverError::invalid_client_state(error_message));
+        assert_eq!(
+            error,
+            TokenserverError::invalid_client_state(error_message, None),
+        );
     }
 
     #[actix_rt::test]
@@ -1313,7 +1325,10 @@ mod tests {
         let error = tokenserver_request.validate().unwrap_err();
         let error_message =
             "Unacceptable client-state value new value with no keys_changed_at change".to_owned();
-        assert_eq!(error, TokenserverError::invalid_client_state(error_message));
+        assert_eq!(
+            error,
+            TokenserverError::invalid_client_state(error_message, None)
+        );
     }
 
     fn extract_body_as_str(sresponse: ServiceResponse) -> String {