diff --git a/Cargo.lock b/Cargo.lock index 69da39adac..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", @@ -2632,6 +2633,7 @@ name = "syncserver-common" version = "0.15.8" dependencies = [ "actix-web", + "backtrace", "cadence", "futures 0.3.30", "hkdf", 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/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..1ff1529cc5 100644 --- a/syncserver-common/src/lib.rs +++ b/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; + + 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. diff --git a/syncserver-db-common/src/error.rs b/syncserver-db-common/src/error.rs index e49e89fde7..46f52f282d 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,28 @@ impl From for MysqlError { } } +impl ReportableError for MysqlError { + 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/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/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/server/mod.rs b/syncserver/src/server/mod.rs index a27bd9e8ba..63d9baa88d 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. @@ -314,7 +323,8 @@ impl Server { tokenserver_state.clone(), Arc::clone(&secrets), limits, - build_cors(&settings_copy) + build_cors(&settings_copy), + metrics.clone() ) }); @@ -368,7 +378,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/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 2de91d066f..13fccf38ec 100644 --- a/syncserver/src/web/auth.rs +++ b/syncserver/src/web/auth.rs @@ -206,6 +206,7 @@ fn verify_hmac(info: &[u8], key: &[u8], expected: &[u8]) -> ApiResult<()> { #[cfg(test)] mod tests { + use std::fmt::{self, Display, Formatter}; use super::{HawkPayload, Secrets}; @@ -533,10 +534,10 @@ mod tests { } } - impl std::fmt::Display for HawkHeader { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { + impl Display for HawkHeader { + fn fmt(&self, fmt: &mut Formatter<'_>) -> Result<(), fmt::Error> { write!( - f, + fmt, "Hawk id=\"{}\", mac=\"{}\", nonce=\"{}\", ts=\"{}\"", self.id, self.mac, self.nonce, self.ts ) diff --git a/syncserver/src/web/middleware/sentry.rs b/syncserver/src/web/middleware/sentry.rs index bc21d40007..a81fb293b6 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(&err.error_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); } + 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() } } 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 62419daa68..3e35febe78 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 std::fmt::Display for Offset { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { +impl Display for Offset { + fn fmt(&self, fmt: &mut Formatter) -> Result<(), fmt::Error> { // issue559: Disable ':' support for now. - write!(f, "{}", 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 826837beff..31b29956dd 100644 --- a/syncstorage-mysql/src/diesel_ext.rs +++ b/syncstorage-mysql/src/diesel_ext.rs @@ -38,31 +38,6 @@ impl QueryFragment for LockInShareMode { } } -// May be used for certain legacy MySQL versions -#[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/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/support.rs b/syncstorage-spanner/src/support.rs index 6b09543e54..59ea702361 100644 --- a/syncstorage-spanner/src/support.rs +++ b/syncstorage-spanner/src/support.rs @@ -449,44 +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)) - } -} - -// this is legacy, but may be used by the Stand Alone MySQL server. Allow it -// as dead code for now. -#[allow(dead_code)] -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, - } - } -} - -#[allow(dead_code)] -impl MapAndThenTrait for I where I: Sized + Iterator> {} diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 67c0586252..bfa7b579d1 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() } } @@ -257,8 +263,8 @@ impl From for HttpResponse { } impl ReportableError for TokenserverError { - fn error_backtrace(&self) -> String { - format!("{:#?}", self.backtrace) + fn backtrace(&self) -> Option<&Backtrace> { + Some(&self.backtrace) } fn is_sentry_event(&self) -> bool { @@ -283,6 +289,10 @@ impl ReportableError for TokenserverError { None } } + + fn tags(&self) -> Vec<(&str, String)> { + *self.tags.clone().unwrap_or_default() + } } impl InternalError for TokenserverError {