From 771813087c8eccc448530cea2d323f8de8ee81a3 Mon Sep 17 00:00:00 2001 From: JR Conlin Date: Mon, 20 May 2024 08:51:58 -0700 Subject: [PATCH] feat: Add normalized ReportableError to errors (#1559) * 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 This continues to use the `Taggable` trait, which we may want to port to autopush. Closes SYNC-4262 --- Cargo.lock | 2 + Cargo.toml | 6 + syncserver-common/Cargo.toml | 2 +- syncserver-common/src/lib.rs | 31 +- syncserver-db-common/src/error.rs | 24 +- syncserver/Cargo.toml | 3 +- syncserver/src/error.rs | 4 +- syncserver/src/server/mod.rs | 23 +- syncserver/src/server/tags.rs | 18 +- syncserver/src/server/test.rs | 12 +- syncserver/src/tokenserver/extractors.rs | 29 +- syncserver/src/web/auth.rs | 7 +- syncserver/src/web/middleware/sentry.rs | 432 ++++++++++++----------- syncstorage-db-common/src/error.rs | 10 +- syncstorage-db-common/src/params.rs | 14 +- syncstorage-mysql/src/diesel_ext.rs | 25 -- syncstorage-mysql/src/error.rs | 30 +- syncstorage-spanner/src/error.rs | 11 +- syncstorage-spanner/src/support.rs | 41 --- tokenserver-common/src/error.rs | 16 +- 20 files changed, 418 insertions(+), 322 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69da39ad..9a2c11d5 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 9bc62f46..ae9e0c3b 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 1f5f507c..f6fdce94 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 b0762bd3..1ff1529c 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 e49e89fd..46f52f28 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 57c1aaf1..67583a08 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 fc0c53b1..f03069ac 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 a27bd9e8..63d9baa8 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 933644c9..a85b053c 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 d9dfa054..8598e36c 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 385f5f38..359458f7 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 2de91d06..13fccf38 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 bc21d400..a81fb293 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 cadence::{CountedExt, StatsdClient}; +use futures::{future::LocalBoxFuture, FutureExt}; +use futures_util::future::{ok, Ready}; +use sentry::{protocol::Event, Hub}; -use crate::error::ApiError; -use crate::server::{tags::Taggable, user_agent, MetricsWrapper}; +use crate::server::tags::Taggable; +use syncserver_common::ReportableError; -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); +#[derive(Clone)] +pub struct SentryWrapper { + metrics: Arc, + metric_label: String, + phantom: PhantomData, } -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; - - 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); - } - } +impl SentryWrapper { + pub fn new(metrics: Arc, metric_label: String) -> Self { + Self { + metrics, + metric_label, + phantom: PhantomData, } - Ok(sresp) } } -fn process_error( - err: &E, - metrics: Metrics, - tags: HashMap, - extras: HashMap, -) where - E: ReportableError + StdError + 'static, +impl Transform for SentryWrapper +where + S: Service, Error = Error>, + S::Future: 'static, + E: ReportableError + actix_web::ResponseError + 'static, { - if let Some(label) = err.metric_label() { - metrics.incr(&label); + type Response = ServiceResponse; + type Error = Error; + type Transform = SentryWrapperMiddleware; + type InitError = (); + type Future = Ready>; + + 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, + }) + } +} + +#[derive(Debug)] +pub struct SentryWrapperMiddleware { + service: Rc>, + metrics: Arc, + metric_label: String, + phantom: PhantomData, +} + +impl Service for SentryWrapperMiddleware +where + S: Service, Error = Error>, + S::Future: 'static, + E: ReportableError + actix_web::ResponseError + 'static, +{ + 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() + } +} + +/// 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() + } +} + +/// 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()); } - if err.is_sentry_event() { - report(tags, extras, event_from_error(err)); + // TODO: Use ServiceRequest::match_pattern for the event transaction. + // Coming in Actix v3. + + Some(event) +} + +/// 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 + E: ReportableError + actix_web::ResponseError + 'static, +{ + // 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 { - trace!("Sentry: Not reporting error: {:?}", err); + // Fallback to the Actix error + sentry::event_from_error(error) } } /// 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> -where - E: ReportableError + StdError + 'static, -{ - let mut exceptions = vec![exception_from_error_with_backtrace(err)]; +/// `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(); - 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) + // 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); + } + 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, }; - exceptions.push(exception); - source = err.source(); + } + + // 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(); - Event { + sentry::protocol::Event { exception: exceptions.into(), level: sentry::protocol::Level::Error, + tags, + extra, ..Default::default() } } /// 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 -where - E: ReportableError + StdError, -{ - let mut exception = exception_from_error(err); - exception.stacktrace = parse_stacktrace(&err.error_backtrace()); - exception -} - -/// Exact copy of sentry's unfortunately private `exception_from_error` -fn exception_from_error(err: &E) -> sentry::protocol::Exception { - let dbg = format!("{:?}", err); +/// 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(err.to_string()), + value: Some(dbg), + stacktrace: err + .backtrace() + .map(sentry_backtrace::backtrace_to_stacktrace) + .unwrap_or_default(), ..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); -} - -/// Adds HTTP-related extras to be included in every syncstorage or tokenserver request. -fn add_initial_extras(msg: &T, uri: String) -where - T: Taggable + HttpMessage, -{ - 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()); - } - } - - 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()); - - for tag in tags.clone() { - req.add_tag(tag.0.clone(), tag.1.clone()); - } - - 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()) - } - - assert_eq!(req.get_tags(), tags); - assert_eq!(req.get_extras(), extras); - } - - #[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")); - } -} diff --git a/syncstorage-db-common/src/error.rs b/syncstorage-db-common/src/error.rs index aa1716a5..1d69aea9 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 62419daa..3e35febe 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 826837be..31b29956 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 68385457..22168446 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 d0aa3ad1..6e64348e 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 6b09543e..59ea7023 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 67c05862..bfa7b579 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 {