From 0a0ce20f9a87daeb81c791593545ec53fd1a0192 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 9 May 2024 16:34:01 -0700 Subject: [PATCH 1/5] feat: Add normalized ReportableError to errors We want to do things like add tags and other features to sync errors the way that we do in other packages. To do so, we're backporting ReportableError from Autopush to Syncstorage. This also addresses some clippy fixes required by 1.78 Closes SYNC-4262 --- Cargo.lock | 1 + syncserver-common/Cargo.toml | 2 +- syncserver-common/src/lib.rs | 28 +++++++++++++++- syncserver-db-common/src/error.rs | 30 +++++++++++++++++- syncserver/src/error.rs | 4 +-- syncserver/src/web/middleware/sentry.rs | 2 +- syncserver/src/web/transaction.rs | 8 ++--- syncstorage-db-common/src/error.rs | 10 ++++-- syncstorage-db-common/src/params.rs | 14 +++++--- syncstorage-mysql/src/error.rs | 30 ++++++++++++++---- syncstorage-spanner/src/error.rs | 11 +++++-- syncstorage-spanner/src/manager/session.rs | 2 +- syncstorage-spanner/src/models.rs | 3 +- syncstorage-spanner/src/support.rs | 37 ---------------------- tokenserver-auth/src/crypto.rs | 1 + tokenserver-common/src/error.rs | 8 +++-- 16 files changed, 123 insertions(+), 68 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69da39adac..9128ab1bf5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2632,6 +2632,7 @@ name = "syncserver-common" version = "0.15.8" dependencies = [ "actix-web", + "backtrace", "cadence", "futures 0.3.30", "hkdf", diff --git a/syncserver-common/Cargo.toml b/syncserver-common/Cargo.toml index 1f5f507c24..f6fdce94ef 100644 --- a/syncserver-common/Cargo.toml +++ b/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 - diff --git a/syncserver-common/src/lib.rs b/syncserver-common/src/lib.rs index b0762bd396..e0e278e0a4 100644 --- a/syncserver-common/src/lib.rs +++ b/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; + + 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. diff --git a/syncserver-db-common/src/error.rs b/syncserver-db-common/src/error.rs index e49e89fde7..d3c0de5b9f 100644 --- a/syncserver-db-common/src/error.rs +++ b/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 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 { + 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!( diff --git a/syncserver/src/error.rs b/syncserver/src/error.rs index fc0c53b11f..f03069ac3e 100644 --- a/syncserver/src/error.rs +++ b/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 { diff --git a/syncserver/src/web/middleware/sentry.rs b/syncserver/src/web/middleware/sentry.rs index bc21d40007..8738aa014e 100644 --- a/syncserver/src/web/middleware/sentry.rs +++ b/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 } diff --git a/syncserver/src/web/transaction.rs b/syncserver/src/web/transaction.rs index 3975a315b1..361584b901 100644 --- a/syncserver/src/web/transaction.rs +++ b/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>), 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, diff --git a/syncstorage-db-common/src/error.rs b/syncstorage-db-common/src/error.rs index aa1716a5b2..1d69aea932 100644 --- a/syncstorage-db-common/src/error.rs +++ b/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 { - 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) } } diff --git a/syncstorage-db-common/src/params.rs b/syncstorage-db-common/src/params.rs index abd142079b..a8a2447744 100644 --- a/syncstorage-db-common/src/params.rs +++ b/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(), diff --git a/syncstorage-mysql/src/error.rs b/syncstorage-mysql/src/error.rs index 683854578d..2216844690 100644 --- a/syncstorage-mysql/src/error.rs +++ b/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 { - 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(), + } } } diff --git a/syncstorage-spanner/src/error.rs b/syncstorage-spanner/src/error.rs index d0aa3ad195..6e64348e71 100644 --- a/syncstorage-spanner/src/error.rs +++ b/syncstorage-spanner/src/error.rs @@ -112,6 +112,13 @@ impl DbErrorIntrospect for DbError { } impl ReportableError for DbError { + fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> { + match &self.kind { + DbErrorKind::Common(e) => Some(e), + _ => None, + } + } + fn is_sentry_event(&self) -> bool { match &self.kind { DbErrorKind::Common(e) => e.is_sentry_event(), @@ -127,8 +134,8 @@ impl ReportableError for DbError { } } - fn error_backtrace(&self) -> String { - format!("{:#?}", self.backtrace) + fn backtrace(&self) -> Option<&Backtrace> { + Some(&self.backtrace) } } diff --git a/syncstorage-spanner/src/manager/session.rs b/syncstorage-spanner/src/manager/session.rs index dae2e404cf..3e666acf13 100644 --- a/syncstorage-spanner/src/manager/session.rs +++ b/syncstorage-spanner/src/manager/session.rs @@ -255,7 +255,7 @@ async fn create_session( settings: &SpannerSessionSettings, ) -> Result { let mut req = CreateSessionRequest::new(); - req.database = settings.database.clone(); + req.database.clone_from(&settings.database); let meta = settings .metadata_builder() .routing_param("database", &settings.database) diff --git a/syncstorage-spanner/src/models.rs b/syncstorage-spanner/src/models.rs index 6a0a19dac4..0ad4c7e336 100644 --- a/syncstorage-spanner/src/models.rs +++ b/syncstorage-spanner/src/models.rs @@ -222,8 +222,7 @@ impl SpannerDb { .session .borrow() .coll_locks - .get(&(params.user_id.clone(), collection_id)) - .is_some() + .contains_key(&(params.user_id.clone(), collection_id)) { return Ok(()); } diff --git a/syncstorage-spanner/src/support.rs b/syncstorage-spanner/src/support.rs index cc2255f8b4..59ea702361 100644 --- a/syncstorage-spanner/src/support.rs +++ b/syncstorage-spanner/src/support.rs @@ -449,40 +449,3 @@ pub fn bso_to_update_row( row.set_values(RepeatedField::from_vec(values)); Ok((columns, row)) } - -#[derive(Clone)] -pub struct MapAndThenIterator { - iter: I, - f: F, -} - -impl Iterator for MapAndThenIterator -where - F: FnMut(A) -> Result, - I: Iterator>, -{ - type Item = Result; - - fn next(&mut self) -> Option { - self.iter.next().map(|result| result.and_then(&mut self.f)) - } -} - -pub trait MapAndThenTrait { - /// Return an iterator adaptor that applies the provided closure to every - /// DbResult::Ok value. DbResult::Err values are unchanged. - /// - /// The closure can be used for control flow based on result values - fn map_and_then(self, func: F) -> MapAndThenIterator - where - Self: Sized + Iterator>, - F: FnMut(A) -> Result, - { - MapAndThenIterator { - iter: self, - f: func, - } - } -} - -impl MapAndThenTrait for I where I: Sized + Iterator> {} diff --git a/tokenserver-auth/src/crypto.rs b/tokenserver-auth/src/crypto.rs index 4e3a882468..01e92dd942 100644 --- a/tokenserver-auth/src/crypto.rs +++ b/tokenserver-auth/src/crypto.rs @@ -22,6 +22,7 @@ pub trait Crypto { /// Signs the `payload` using HMAC given the `key` fn hmac_sign(&self, key: &[u8], payload: &[u8]) -> Result, Self::Error>; + #[allow(dead_code)] /// Verify an HMAC signature on a payload given a shared key fn hmac_verify(&self, key: &[u8], payload: &[u8], signature: &[u8]) -> Result<(), Self::Error>; diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 67c0586252..31f5612111 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -257,8 +257,12 @@ impl From for HttpResponse { } impl ReportableError for TokenserverError { - fn error_backtrace(&self) -> String { - format!("{:#?}", self.backtrace) + fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> { + None + } + + fn backtrace(&self) -> Option<&Backtrace> { + Some(&self.backtrace) } fn is_sentry_event(&self) -> bool { From 36620d627a0180f98e124d86fc9f379cd00025a1 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 10 May 2024 09:56:41 -0700 Subject: [PATCH 2/5] f add tag handler * More clippy updates --- syncserver/src/tokenserver/extractors.rs | 29 ++++++++++++++++++------ syncserver/src/web/auth.rs | 9 +++++--- syncstorage-mysql/src/diesel_ext.rs | 3 +++ syncstorage-mysql/src/models.rs | 3 +-- tokenserver-common/src/error.rs | 12 +++++++++- 5 files changed, 43 insertions(+), 13 deletions(-) diff --git a/syncserver/src/tokenserver/extractors.rs b/syncserver/src/tokenserver/extractors.rs index 385f5f3828..359458f7ba 100644 --- a/syncserver/src/tokenserver/extractors.rs +++ b/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 { diff --git a/syncserver/src/web/auth.rs b/syncserver/src/web/auth.rs index 228d55af12..13fccf38ec 100644 --- a/syncserver/src/web/auth.rs +++ b/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 ) diff --git a/syncstorage-mysql/src/diesel_ext.rs b/syncstorage-mysql/src/diesel_ext.rs index ecaac4b800..d35c713007 100644 --- a/syncstorage-mysql/src/diesel_ext.rs +++ b/syncstorage-mysql/src/diesel_ext.rs @@ -38,6 +38,8 @@ impl QueryFragment for LockInShareMode { } } +// TODO: can we kill this? +#[allow(dead_code)] /// Emit 'ON DUPLICATE KEY UPDATE' pub trait IntoDuplicateValueClause { type ValueClause; @@ -45,6 +47,7 @@ pub trait IntoDuplicateValueClause { fn into_value_clause(self) -> Self::ValueClause; } +#[allow(dead_code)] pub trait OnDuplicateKeyUpdateDsl { fn on_duplicate_key_update(self, expression: X) -> OnDuplicateKeyUpdate where diff --git a/syncstorage-mysql/src/models.rs b/syncstorage-mysql/src/models.rs index 0514f8ea17..5598b479c9 100644 --- a/syncstorage-mysql/src/models.rs +++ b/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(()); } diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 31f5612111..7fd2a7f508 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -22,6 +22,7 @@ pub struct TokenserverError { pub context: String, pub backtrace: Box, pub token_type: TokenType, + pub tags: Option>>, } #[derive(Clone, Debug)] @@ -62,6 +63,7 @@ impl Default for TokenserverError { context: "Unauthorized".to_owned(), backtrace: Box::new(Backtrace::new()), token_type: TokenType::Oauth, + tags: None, } } } @@ -104,12 +106,16 @@ impl TokenserverError { } } - pub fn invalid_client_state(description: String) -> Self { + pub fn invalid_client_state( + description: String, + tags: Option>>, + ) -> Self { Self { status: "invalid-client-state", context: description.clone(), description, name: "X-Client-State".to_owned(), + tags, ..Self::default() } } @@ -287,6 +293,10 @@ impl ReportableError for TokenserverError { None } } + + fn tags(&self) -> Vec<(&str, String)> { + *self.tags.clone().unwrap_or_default() + } } impl InternalError for TokenserverError { From fe29a08117f55a2b40bf71c00e1adede9512cd1c Mon Sep 17 00:00:00 2001 From: jrconlin Date: Mon, 13 May 2024 16:04:11 -0700 Subject: [PATCH 3/5] f add sentry handler for ReportableError This continues to use the `Taggable` trait, which we may want to port to autopush. --- Cargo.lock | 1 + Cargo.toml | 6 + syncserver-common/src/lib.rs | 5 +- syncserver/Cargo.toml | 3 +- syncserver/src/server/mod.rs | 23 +- syncserver/src/server/tags.rs | 18 +- syncserver/src/server/test.rs | 12 +- syncserver/src/web/middleware/sentry.rs | 428 ++++++++++++------------ 8 files changed, 275 insertions(+), 221 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9128ab1bf5..9a2c11d5e8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2588,6 +2588,7 @@ dependencies = [ "docopt", "dyn-clone", "futures 0.3.30", + "futures-util", "hawk", "hex", "hmac", diff --git a/Cargo.toml b/Cargo.toml index 9bc62f462e..ae9e0c3be2 100644 --- a/Cargo.toml +++ b/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" diff --git a/syncserver-common/src/lib.rs b/syncserver-common/src/lib.rs index e0e278e0a4..1ff1529cc5 100644 --- a/syncserver-common/src/lib.rs +++ b/syncserver-common/src/lib.rs @@ -11,6 +11,7 @@ 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}; @@ -59,7 +60,7 @@ macro_rules! impl_fmt_display { }; } -pub trait ReportableError { +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 @@ -85,7 +86,7 @@ pub trait ReportableError { /// 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)> { + fn extras(&self) -> Vec<(&str, Value)> { vec![] } } diff --git a/syncserver/Cargo.toml b/syncserver/Cargo.toml index 57c1aaf147..67583a0883 100644 --- a/syncserver/Cargo.toml +++ b/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" } diff --git a/syncserver/src/server/mod.rs b/syncserver/src/server/mod.rs index 2c05999ccf..9cbc16e566 100644 --- a/syncserver/src/server/mod.rs +++ b/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::::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::::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() ) }); diff --git a/syncserver/src/server/tags.rs b/syncserver/src/server/tags.rs index 933644c9d9..a85b053cb2 100644 --- a/syncserver/src/server/tags.rs +++ b/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; + /// 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; } impl Taggable for T @@ -61,10 +64,17 @@ where } } - fn get_extras(&self) -> HashMap { + fn get_extras(&self) -> HashMap { self.extensions() .get::() - .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() } } diff --git a/syncserver/src/server/test.rs b/syncserver/src/server/test.rs index d9dfa05443..8598e36cba 100644 --- a/syncserver/src/server/test.rs +++ b/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::, 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::, 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::, Arc::clone(&SECRETS), limits, - build_cors(&settings) + build_cors(&settings), + metrics )) .await; let req = create_request(method, path, None, Some(body)).to_request(); diff --git a/syncserver/src/web/middleware/sentry.rs b/syncserver/src/web/middleware/sentry.rs index 8738aa014e..9f0a0700dc 100644 --- a/syncserver/src/web/middleware/sentry.rs +++ b/syncserver/src/web/middleware/sentry.rs @@ -1,245 +1,263 @@ -use std::collections::HashMap; -use std::error::Error as StdError; -use std::future::Future; +use std::{cell::RefCell, collections::BTreeMap, marker::PhantomData, rc::Rc, sync::Arc}; -use actix_http::HttpMessage; use actix_web::{ - dev::{Service, ServiceRequest, ServiceResponse}, - http::header::USER_AGENT, - FromRequest, + dev::{Service, ServiceRequest, ServiceResponse, Transform}, + Error, }; -use sentry::protocol::Event; -use sentry_backtrace::parse_stacktrace; -use serde_json::value::Value; -use syncserver_common::{Metrics, ReportableError}; -use tokenserver_common::TokenserverError; - -use crate::error::ApiError; -use crate::server::{tags::Taggable, user_agent, MetricsWrapper}; - -pub fn report( - tags: HashMap, - extra: HashMap, - mut event: Event<'static>, -) { - event.tags.extend(tags); - event - .extra - .extend(extra.into_iter().map(|(k, v)| (k, Value::from(v)))); - trace!("Sentry: Sending error: {:?}", &event); - sentry::capture_event(event); -} +use cadence::{CountedExt, StatsdClient}; +use futures::{future::LocalBoxFuture, FutureExt}; +use futures_util::future::{ok, Ready}; +use sentry::{protocol::Event, Hub}; -pub fn report_error( - request: ServiceRequest, - service: &impl Service, Error = actix_web::Error>, -) -> impl Future, actix_web::Error>> { - add_initial_tags(&request, request.head().method.to_string()); - add_initial_extras(&request, request.head().uri.to_string()); - - let fut = service.call(request); - - async move { - let mut sresp = fut.await?; - let tags = sresp.request().get_tags(); - let extras = sresp.request().get_extras(); - - match sresp.response().error() { - None => { - // Middleware errors are eaten by current versions of Actix. Errors are now added - // to the extensions. Need to check both for any errors and report them. - if let Some(events) = sresp - .request() - .extensions_mut() - .remove::>>() - { - for event in events { - trace!("Sentry: found an error stored in request: {:?}", &event); - report(tags.clone(), extras.clone(), event); - } - } - if let Some(events) = sresp - .response_mut() - .extensions_mut() - .remove::>>() - { - for event in events { - trace!("Sentry: Found an error stored in response: {:?}", &event); - report(tags.clone(), extras.clone(), event); - } - } - } - Some(e) => { - let metrics = MetricsWrapper::extract(sresp.request()).await.unwrap().0; +use crate::server::tags::Taggable; +use syncserver_common::ReportableError; - if let Some(apie) = e.as_error::() { - process_error(apie, metrics, tags, extras); - } else if let Some(tokenserver_error) = e.as_error::() { - process_error(tokenserver_error, metrics, tags, extras); - } - } - } - Ok(sresp) - } +#[derive(Clone)] +pub struct SentryWrapper { + metrics: Arc, + metric_label: String, + phantom: PhantomData, } -fn process_error( - err: &E, - metrics: Metrics, - tags: HashMap, - extras: HashMap, -) where - E: ReportableError + StdError + 'static, -{ - if let Some(label) = err.metric_label() { - metrics.incr(&label); - } - - if err.is_sentry_event() { - report(tags, extras, event_from_error(err)); - } else { - trace!("Sentry: Not reporting error: {:?}", err); +impl SentryWrapper { + pub fn new(metrics: Arc, metric_label: String) -> Self { + Self { + metrics, + metric_label, + phantom: PhantomData, + } } } -/// Custom `sentry::event_from_error` for `ReportableError` -/// -/// `sentry::event_from_error` can't access `std::Error` backtraces as its -/// `backtrace()` method is currently Rust nightly only. This function works -/// against `ReportableError` instead to access its backtrace. -pub fn event_from_error(err: &E) -> Event<'static> +impl Transform for SentryWrapper where - E: ReportableError + StdError + 'static, + S: Service, Error = Error>, + S::Future: 'static, + E: ReportableError + actix_web::ResponseError + 'static, { - let mut exceptions = vec![exception_from_error_with_backtrace(err)]; + type Response = ServiceResponse; + type Error = Error; + type Transform = SentryWrapperMiddleware; + type InitError = (); + type Future = Ready>; - let mut source = err.source(); - while let Some(err) = source { - let exception = if let Some(err) = err.downcast_ref::() { - exception_from_error_with_backtrace(err) - } else { - exception_from_error(err) - }; - exceptions.push(exception); - source = err.source(); + fn new_transform(&self, service: S) -> Self::Future { + ok(SentryWrapperMiddleware { + service: Rc::new(RefCell::new(service)), + metrics: self.metrics.clone(), + metric_label: self.metric_label.clone(), + phantom: PhantomData, + }) } +} - exceptions.reverse(); - Event { - exception: exceptions.into(), - level: sentry::protocol::Level::Error, - ..Default::default() - } +#[derive(Debug)] +pub struct SentryWrapperMiddleware { + service: Rc>, + metrics: Arc, + metric_label: String, + phantom: PhantomData, } -/// Custom `exception_from_error` support function for `ReportableError` -/// -/// Based moreso on sentry_failure's `exception_from_single_fail`. -fn exception_from_error_with_backtrace(err: &E) -> sentry::protocol::Exception +impl Service for SentryWrapperMiddleware where - E: ReportableError + StdError, + S: Service, Error = Error>, + S::Future: 'static, + E: ReportableError + actix_web::ResponseError + 'static, { - let mut exception = exception_from_error(err); - exception.stacktrace = parse_stacktrace(&format!("{:?}", &err.backtrace())); - exception + type Response = ServiceResponse; + type Error = Error; + type Future = LocalBoxFuture<'static, Result>; + + actix_web::dev::forward_ready!(service); + + fn call(&self, sreq: ServiceRequest) -> Self::Future { + // Set up the hub to add request data to events + let hub = Hub::new_from_top(Hub::main()); + let _ = hub.push_scope(); + let sentry_request = sentry_request_from_http(&sreq); + hub.configure_scope(|scope| { + scope.add_event_processor(Box::new(move |event| process_event(event, &sentry_request))) + }); + + // get the tag information + let metrics = self.metrics.clone(); + let metric_label = self.metric_label.clone(); + let tags = sreq.get_tags(); + let extras = sreq.get_extras(); + + let fut = self.service.call(sreq); + + async move { + let response: Self::Response = match fut.await { + Ok(response) => response, + Err(error) => { + if let Some(reportable_err) = error.as_error::() { + // if it's not reportable, and we have access to the metrics, record it as a metric. + if !reportable_err.is_sentry_event() { + // The error (e.g. VapidErrorKind::InvalidKey(String)) might be too cardinal, + // 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() { + info!("Sentry: Sending error to metrics: {:?}", reportable_err); + let _ = metrics.incr(&format!("{}.{}", metric_label, label)); + } + debug!("Sentry: Not reporting error (service error): {:?}", error); + return Err(error); + } + }; + debug!("Reporting error to Sentry (service error): {}", error); + let mut event = event_from_actix_error::(&error); + // Add in the tags from the request + event.tags.extend(tags); + event.extra.extend(extras); + let event_id = hub.capture_event(event); + trace!("event_id = {}", event_id); + return Err(error); + } + }; + // Check for errors inside the response + 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() { + info!("Sentry: Sending error to metrics: {:?}", reportable_err); + let _ = metrics.incr(&format!("{}.{}", metric_label, label)); + } + debug!("Not reporting error (service error): {:?}", error); + return Ok(response); + } + } + debug!("Reporting error to Sentry (response error): {}", error); + let event = event_from_actix_error::(error); + let event_id = hub.capture_event(event); + trace!("event_id = {}", event_id); + } + Ok(response) + } + .boxed_local() + } } -/// Exact copy of sentry's unfortunately private `exception_from_error` -fn exception_from_error(err: &E) -> sentry::protocol::Exception { - let dbg = format!("{:?}", err); - sentry::protocol::Exception { - ty: sentry::parse_type_from_debug(&dbg).to_owned(), - value: Some(err.to_string()), +/// Build a Sentry request struct from the HTTP request +fn sentry_request_from_http(request: &ServiceRequest) -> sentry::protocol::Request { + sentry::protocol::Request { + url: format!( + "{}://{}{}", + request.connection_info().scheme(), + request.connection_info().host(), + request.uri() + ) + .parse() + .ok(), + method: Some(request.method().to_string()), + headers: request + .headers() + .iter() + .map(|(k, v)| (k.to_string(), v.to_str().unwrap_or_default().to_string())) + .collect(), ..Default::default() } } -/// Adds HTTP-related tags to be included in every syncstorage or tokenserver request. -fn add_initial_tags(msg: &T, method: String) -where - T: Taggable + HttpMessage, -{ - msg.add_tag("uri.method".to_owned(), method); +/// Add request data to a Sentry event +#[allow(clippy::unnecessary_wraps)] +fn process_event( + mut event: Event<'static>, + request: &sentry::protocol::Request, +) -> Option> { + if event.request.is_none() { + event.request = Some(request.clone()); + } + + // TODO: Use ServiceRequest::match_pattern for the event transaction. + // Coming in Actix v3. + + Some(event) } -/// Adds HTTP-related extras to be included in every syncstorage or tokenserver request. -fn add_initial_extras(msg: &T, uri: String) +/// Convert Actix errors into a Sentry event. ReportableError is handled +/// explicitly so the event can include a backtrace and source error +/// information. +fn event_from_actix_error(error: &actix_web::Error) -> sentry::protocol::Event<'static> where - T: Taggable + HttpMessage, + E: ReportableError + actix_web::ResponseError + 'static, { - if let Some(ua) = msg.headers().get(USER_AGENT) { - if let Ok(uas) = ua.to_str() { - let (ua_result, metrics_os, metrics_browser) = user_agent::parse_user_agent(uas); - msg.add_extra("ua.os.family".to_owned(), metrics_os.to_owned()); - msg.add_extra("ua.browser.family".to_owned(), metrics_browser.to_owned()); - msg.add_extra("ua.name".to_owned(), ua_result.name.to_owned()); - msg.add_extra("ua.os.ver".to_owned(), ua_result.os_version.to_string()); - msg.add_extra("ua.browser.ver".to_owned(), ua_result.version.to_owned()); - msg.add_extra("ua".to_owned(), uas.to_string()); - } + // Actix errors don't have support source/cause, so to get more information + // about the error we need to downcast. + if let Some(reportable_err) = error.as_error::() { + // Use our error and associated backtrace for the event + event_from_error(reportable_err) + } else { + // Fallback to the Actix error + sentry::event_from_error(error) } - - msg.add_extra("uri.path".to_owned(), uri); } -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_tags() { - use actix_web::{http::header, test::TestRequest}; - use std::collections::HashMap; - - let uri = "/1.5/42/storage/meta/global".to_owned(); - let ua = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0"; - let req = TestRequest::default() - .uri(&uri) - .insert_header((header::USER_AGENT, header::HeaderValue::from_static(ua))) - .to_http_request(); - - add_initial_tags(&req, "GET".to_owned()); - add_initial_extras(&req, uri.clone()); - - let mut tags = HashMap::::new(); - tags.insert("uri.method".to_owned(), "GET".to_owned()); +/// Custom `sentry::event_from_error` for `ReportableError` +/// +/// `std::error::Error` doesn't support backtraces, thus `sentry::event_from_error` +/// doesn't either. This function works against `ReportableError` instead to +/// extract backtraces, etc. from it and its chain of `reportable_source's. +/// +/// A caveat of this function is that it cannot extract +/// `ReportableError`s/backtraces, etc. that occur in a chain after a +/// `std::error::Error` occurs: as `std::error::Error::source` only allows +/// downcasting to a concrete type, not `dyn ReportableError`. +pub fn event_from_error( + mut reportable_err: &dyn ReportableError, +) -> sentry::protocol::Event<'static> { + let mut exceptions = vec![]; + let mut tags = BTreeMap::new(); + let mut extra = BTreeMap::new(); - for tag in tags.clone() { - req.add_tag(tag.0.clone(), tag.1.clone()); + // Gather reportable_source()'s for their backtraces, etc + loop { + exceptions.push(exception_from_reportable_error(reportable_err)); + for (k, v) in reportable_err.tags() { + // NOTE: potentially overwrites other tags/extras from this chain + tags.insert(k.to_owned(), v); } - - let mut extras = HashMap::::new(); - extras.insert("ua.os.ver".to_owned(), "NT 10.0".to_owned()); - extras.insert("ua.os.family".to_owned(), "Windows".to_owned()); - extras.insert("ua.browser.ver".to_owned(), "72.0".to_owned()); - extras.insert("ua.name".to_owned(), "Firefox".to_owned()); - extras.insert("ua.browser.family".to_owned(), "Firefox".to_owned()); - extras.insert("ua".to_owned(), ua.to_owned()); - extras.insert("uri.path".to_owned(), uri); - - for extra in extras.clone() { - req.add_extra(extra.0.clone(), extra.1.clone()) + for (k, v) in reportable_err.extras() { + extra.insert(k.to_owned(), v.into()); } + reportable_err = match reportable_err.reportable_source() { + Some(reportable_err) => reportable_err, + None => break, + }; + } - assert_eq!(req.get_tags(), tags); - assert_eq!(req.get_extras(), extras); + // Then fallback to source() for remaining Errors + let mut source = reportable_err.reportable_source(); + while let Some(err) = source { + exceptions.push(exception_from_reportable_error(err)); + source = err.reportable_source(); + } + + exceptions.reverse(); + sentry::protocol::Event { + exception: exceptions.into(), + level: sentry::protocol::Level::Error, + tags, + extra, + ..Default::default() } +} - #[test] - fn no_empty_tags() { - use actix_web::{http::header, test::TestRequest}; - - let uri = "/1.5/42/storage/meta/global".to_owned(); - let req = TestRequest::default() - .uri(&uri) - .insert_header(( - header::USER_AGENT, - header::HeaderValue::from_static("Mozilla/5.0 (curl) Gecko/20100101 curl"), - )) - .to_http_request(); - add_initial_tags(&req, "GET".to_owned()); - add_initial_extras(&req, uri); - - assert!(!req.get_tags().contains_key("ua.os.ver")); +/// Custom `exception_from_error` support function for `ReportableError` +/// +/// Based moreso on sentry_failure's `exception_from_single_fail`. Includes a +/// stacktrace if available. +fn exception_from_reportable_error(err: &dyn ReportableError) -> sentry::protocol::Exception { + let dbg = format!("{:?}", &err.to_string()); + sentry::protocol::Exception { + ty: sentry::parse_type_from_debug(&dbg).to_owned(), + value: Some(dbg), + stacktrace: err + .backtrace() + .map(sentry_backtrace::backtrace_to_stacktrace) + .unwrap_or_default(), + ..Default::default() } } From aa1a30a68e9bf85a98a8681c29a24dd6686dbcf7 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 14 May 2024 07:49:18 -0700 Subject: [PATCH 4/5] f clippy --- syncserver/src/web/middleware/sentry.rs | 2 +- tokenserver-auth/src/oauth/py.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/syncserver/src/web/middleware/sentry.rs b/syncserver/src/web/middleware/sentry.rs index 9f0a0700dc..a81fb293b6 100644 --- a/syncserver/src/web/middleware/sentry.rs +++ b/syncserver/src/web/middleware/sentry.rs @@ -220,7 +220,7 @@ pub fn event_from_error( tags.insert(k.to_owned(), v); } for (k, v) in reportable_err.extras() { - extra.insert(k.to_owned(), v.into()); + extra.insert(k.to_owned(), v); } reportable_err = match reportable_err.reportable_source() { Some(reportable_err) => reportable_err, diff --git a/tokenserver-auth/src/oauth/py.rs b/tokenserver-auth/src/oauth/py.rs index 5e3d816a9f..a485fcd8c6 100644 --- a/tokenserver-auth/src/oauth/py.rs +++ b/tokenserver-auth/src/oauth/py.rs @@ -68,7 +68,7 @@ impl Verifier { ("alg", &alg), ("kid", kid), ("use", "sig"), - ("n", &n), + ("n", n), ("e", e), ] .into_py_dict(py); From ae0bb4b8e048671cc281c89f9a379886d6f13969 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 14 May 2024 09:04:33 -0700 Subject: [PATCH 5/5] f r's --- syncserver-db-common/src/error.rs | 6 ------ syncstorage-db-common/src/params.rs | 2 +- syncstorage-mysql/src/diesel_ext.rs | 25 ------------------------- tokenserver-common/src/error.rs | 4 ---- 4 files changed, 1 insertion(+), 36 deletions(-) diff --git a/syncserver-db-common/src/error.rs b/syncserver-db-common/src/error.rs index d3c0de5b9f..46f52f282d 100644 --- a/syncserver-db-common/src/error.rs +++ b/syncserver-db-common/src/error.rs @@ -40,12 +40,6 @@ impl From 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 } diff --git a/syncstorage-db-common/src/params.rs b/syncstorage-db-common/src/params.rs index a8a2447744..3e35febe78 100644 --- a/syncstorage-db-common/src/params.rs +++ b/syncstorage-db-common/src/params.rs @@ -70,7 +70,7 @@ pub struct Offset { impl Display for Offset { fn fmt(&self, fmt: &mut Formatter) -> Result<(), fmt::Error> { // issue559: Disable ':' support for now. - write!(fmt, "{:?}", self.offset) + write!(fmt, "{}", self.offset) /* match self.timestamp { None => self.offset.to_string(), diff --git a/syncstorage-mysql/src/diesel_ext.rs b/syncstorage-mysql/src/diesel_ext.rs index d35c713007..31b29956dd 100644 --- a/syncstorage-mysql/src/diesel_ext.rs +++ b/syncstorage-mysql/src/diesel_ext.rs @@ -38,31 +38,6 @@ impl QueryFragment 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 { - fn on_duplicate_key_update(self, expression: X) -> OnDuplicateKeyUpdate - where - X: Expression; -} - -impl OnDuplicateKeyUpdateDsl for InsertStatement { - fn on_duplicate_key_update(self, expression: X) -> OnDuplicateKeyUpdate - where - X: Expression, - { - OnDuplicateKeyUpdate(Box::new(self), expression) - } -} - #[derive(Debug, Clone)] pub struct OnDuplicateKeyUpdate(Box>, X); diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 7fd2a7f508..bfa7b579d1 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -263,10 +263,6 @@ impl From for HttpResponse { } impl ReportableError for TokenserverError { - fn reportable_source(&self) -> Option<&(dyn ReportableError + 'static)> { - None - } - fn backtrace(&self) -> Option<&Backtrace> { Some(&self.backtrace) }