feat: Add normalized ReportableError to errors

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,6 +9,7 @@ use std::{
 };
 
 use actix_web::web;
+use backtrace::Backtrace;
 use hkdf::Hkdf;
 use sha2::Sha256;
 
@@ -59,9 +60,34 @@ macro_rules! impl_fmt_display {
 }
 
 pub trait ReportableError {
-    fn error_backtrace(&self) -> String;
+    /// 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, String)> {
+        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,34 @@ impl From<MysqlErrorKind> for MysqlError {
     }
 }
 
+impl ReportableError for MysqlError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        // There are no further local errors, therefore no need to
+        // look deeper
+        None
+    }
+
+    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/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/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 {

syncserver/src/web/auth.rs

@@ -206,6 +206,8 @@ fn verify_hmac(info: &[u8], key: &[u8], expected: &[u8]) -> ApiResult<()> {
 
 #[cfg(test)]
 mod tests {
+    use std::fmt::{self, Display, Formatter};
+
     use super::{HawkPayload, Secrets};
 
     #[test]
@@ -532,9 +534,10 @@ mod tests {
         }
     }
 
-    impl ToString for HawkHeader {
-        fn to_string(&self) -> String {
-            format!(
+    impl Display for HawkHeader {
+        fn fmt(&self, fmt: &mut Formatter<'_>) -> Result<(), fmt::Error> {
+            write!(
+                fmt,
                 "Hawk id=\"{}\", mac=\"{}\", nonce=\"{}\", ts=\"{}\"",
                 self.id, self.mac, self.nonce, self.ts
             )

syncserver/src/web/middleware/sentry.rs

@@ -140,7 +140,7 @@ where
     E: ReportableError + StdError,
 {
     let mut exception = exception_from_error(err);
-    exception.stacktrace = parse_stacktrace(&err.error_backtrace());
+    exception.stacktrace = parse_stacktrace(&format!("{:?}", &err.backtrace()));
     exception
 }
 

syncserver/src/web/transaction.rs

@@ -44,8 +44,8 @@ impl DbTransactionPool {
     /// transaction is rolled back. If the action succeeds, the transaction is
     /// NOT committed. Further processing is required before we are sure the
     /// action has succeeded (ex. check HTTP response for internal error).
-    async fn transaction_internal<'a, A: 'a, R, F>(
-        &'a self,
+    async fn transaction_internal<'a, A, R, F>(
+        &self,
         request: HttpRequest,
         action: A,
     ) -> Result<(R, Box<dyn Db<Error = DbError>>), ApiError>
@@ -90,7 +90,7 @@ impl DbTransactionPool {
     }
 
     /// Perform an action inside of a DB transaction.
-    pub async fn transaction<'a, A: 'a, R, F>(
+    pub async fn transaction<'a, A, R, F>(
         &'a self,
         request: HttpRequest,
         action: A,
@@ -108,7 +108,7 @@ impl DbTransactionPool {
 
     /// Perform an action inside of a DB transaction. This method will rollback
     /// if the HTTP response is an error.
-    pub async fn transaction_http<'a, A: 'a, F>(
+    pub async fn transaction_http<'a, A, F>(
         &'a self,
         request: HttpRequest,
         action: A,

syncstorage-db-common/src/error.rs

@@ -93,19 +93,23 @@ impl DbErrorIntrospect for SyncstorageDbError {
 }
 
 impl ReportableError for SyncstorageDbError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        None
+    }
+
     fn is_sentry_event(&self) -> bool {
         !matches!(&self.kind, SyncstorageDbErrorKind::Conflict)
     }
 
     fn metric_label(&self) -> Option<String> {
-        match &self.kind {
+        match self.kind {
             SyncstorageDbErrorKind::Conflict => Some("storage.conflict".to_owned()),
             _ => None,
         }
     }
 
-    fn error_backtrace(&self) -> String {
-        format!("{:#?}", self.backtrace)
+    fn backtrace(&self) -> Option<&Backtrace> {
+        Some(&self.backtrace)
     }
 }
 

syncstorage-db-common/src/params.rs

@@ -1,5 +1,11 @@
 //! Parameter types for database methods.
-use std::{collections::HashMap, num::ParseIntError, str::FromStr};
+use core::fmt;
+use std::{
+    collections::HashMap,
+    fmt::{Display, Formatter},
+    num::ParseIntError,
+    str::FromStr,
+};
 
 use diesel::Queryable;
 use serde::{Deserialize, Serialize};
@@ -61,10 +67,10 @@ pub struct Offset {
     pub offset: u64,
 }
 
-impl ToString for Offset {
-    fn to_string(&self) -> String {
+impl Display for Offset {
+    fn fmt(&self, fmt: &mut Formatter) -> Result<(), fmt::Error> {
         // issue559: Disable ':' support for now.
-        self.offset.to_string()
+        write!(fmt, "{:?}", self.offset)
         /*
         match self.timestamp {
             None => self.offset.to_string(),

syncstorage-mysql/src/diesel_ext.rs

@@ -38,13 +38,16 @@ impl QueryFragment<Mysql> for LockInShareMode {
     }
 }
 
+// TODO: can we kill this?
+#[allow(dead_code)]
 /// Emit 'ON DUPLICATE KEY UPDATE'
 pub trait IntoDuplicateValueClause {
     type ValueClause;
 
     fn into_value_clause(self) -> Self::ValueClause;
 }
 
+#[allow(dead_code)]
 pub trait OnDuplicateKeyUpdateDsl<T, U, Op, Ret> {
     fn on_duplicate_key_update<X>(self, expression: X) -> OnDuplicateKeyUpdate<T, U, Op, Ret, X>
     where

syncstorage-mysql/src/error.rs

@@ -92,23 +92,39 @@ impl DbErrorIntrospect for DbError {
 }
 
 impl ReportableError for DbError {
+    fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> {
+        Some(match &self.kind {
+            DbErrorKind::Common(e) => e,
+            DbErrorKind::Mysql(e) => e,
+        })
+    }
+
     fn is_sentry_event(&self) -> bool {
         match &self.kind {
             DbErrorKind::Common(e) => e.is_sentry_event(),
-            _ => true,
+            DbErrorKind::Mysql(e) => e.is_sentry_event(),
         }
     }
 
     fn metric_label(&self) -> Option<String> {
-        if let DbErrorKind::Common(e) = &self.kind {
-            e.metric_label()
-        } else {
-            None
+        match &self.kind {
+            DbErrorKind::Common(e) => e.metric_label(),
+            DbErrorKind::Mysql(e) => e.metric_label(),
         }
     }
 
-    fn error_backtrace(&self) -> String {
-        format!("{:#?}", self.backtrace)
+    fn backtrace(&self) -> Option<&Backtrace> {
+        match &self.kind {
+            DbErrorKind::Common(e) => e.backtrace(),
+            DbErrorKind::Mysql(e) => e.backtrace(),
+        }
+    }
+
+    fn tags(&self) -> Vec<(&str, String)> {
+        match &self.kind {
+            DbErrorKind::Common(e) => e.tags(),
+            DbErrorKind::Mysql(e) => e.tags(),
+        }
     }
 }
 

syncstorage-mysql/src/models.rs

@@ -167,8 +167,7 @@ impl MysqlDb {
             .session
             .borrow()
             .coll_locks
-            .get(&(user_id as u32, collection_id))
-            .is_some()
+            .contains_key(&(user_id as u32, collection_id))
         {
             return Ok(());
         }