From e65486b94ae6b381ee784b4caaebc9713fc78405 Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Sat, 22 Jul 2023 00:52:29 -0700 Subject: [PATCH] feat: consolidate the sentry middlwares into autopush_common via the ReportableError trait SYNC-3825 --- autoconnect/autoconnect-web/src/lib.rs | 9 +- autoconnect/src/main.rs | 31 +-- autoconnect/src/middleware/mod.rs | 3 - autoconnect/src/middleware/sentry.rs | 201 ------------------ autoendpoint/src/error.rs | 16 +- autoendpoint/src/main.rs | 3 - autoendpoint/src/middleware/mod.rs | 3 - autoendpoint/src/server.rs | 7 +- autopush-common/src/errors.rs | 18 +- autopush-common/src/lib.rs | 1 + autopush-common/src/middleware/mod.rs | 1 + .../src/middleware/sentry.rs | 69 +++--- 12 files changed, 86 insertions(+), 276 deletions(-) delete mode 100644 autoconnect/src/middleware/mod.rs delete mode 100644 autoconnect/src/middleware/sentry.rs delete mode 100644 autoendpoint/src/middleware/mod.rs create mode 100644 autopush-common/src/middleware/mod.rs rename {autoendpoint => autopush-common}/src/middleware/sentry.rs (77%) diff --git a/autoconnect/autoconnect-web/src/lib.rs b/autoconnect/autoconnect-web/src/lib.rs index 2136f84ab..55416a306 100644 --- a/autoconnect/autoconnect-web/src/lib.rs +++ b/autoconnect/autoconnect-web/src/lib.rs @@ -23,12 +23,11 @@ macro_rules! build_app { actix_http::StatusCode::NOT_FOUND, autopush_common::errors::render_404, )) - /* - .wrap(crate::middleware::sentry::SentryWrapper::new( - $app_state.metrics.clone(), - "error".to_owned(), + .wrap(autopush_common::middleware::sentry::SentryWrapper::< + autopush_common::errors::ApcError, + >::new( + $app_state.metrics.clone(), "error".to_owned() )) - */ .configure(config) }; } diff --git a/autoconnect/src/main.rs b/autoconnect/src/main.rs index 3d6a39334..da8b0c24f 100644 --- a/autoconnect/src/main.rs +++ b/autoconnect/src/main.rs @@ -17,10 +17,6 @@ use autopush_common::{ logging, }; -mod middleware; - -pub type LocalError = autopush_common::errors::ApcError; - const USAGE: &str = " Usage: autopush_rs [options] @@ -79,21 +75,14 @@ async fn main() -> Result<()> { "Starting autoconnect on port {} (router_port: {})", port, router_port ); - HttpServer::new(move || { - let app = build_app!(app_state); - // TODO: should live in build_app! - app.wrap(crate::middleware::sentry::SentryWrapper::new( - app_state.metrics.clone(), - "error".to_owned(), - )) - }) - .bind(("0.0.0.0", port))? - .bind(("0.0.0.0", router_port))? - .run() - .await - .map_err(|e| e.into()) - .map(|v| { - info!("Shutting down autoconnect"); - v - }) + HttpServer::new(move || build_app!(app_state)) + .bind(("0.0.0.0", port))? + .bind(("0.0.0.0", router_port))? + .run() + .await + .map_err(|e| e.into()) + .map(|v| { + info!("Shutting down autoconnect"); + v + }) } diff --git a/autoconnect/src/middleware/mod.rs b/autoconnect/src/middleware/mod.rs deleted file mode 100644 index feb8ae7c6..000000000 --- a/autoconnect/src/middleware/mod.rs +++ /dev/null @@ -1,3 +0,0 @@ -//! Actix middleware - -pub mod sentry; diff --git a/autoconnect/src/middleware/sentry.rs b/autoconnect/src/middleware/sentry.rs deleted file mode 100644 index 3f69673d4..000000000 --- a/autoconnect/src/middleware/sentry.rs +++ /dev/null @@ -1,201 +0,0 @@ -use std::{ - cell::RefCell, - rc::Rc, - sync::Arc, - task::{Context, Poll}, -}; - -use actix_web::{ - dev::{Service, ServiceRequest, ServiceResponse, Transform}, - Error, HttpMessage, -}; -use cadence::{CountedExt, StatsdClient}; -use futures::{future::LocalBoxFuture, FutureExt}; -use futures_util::future::{ok, Ready}; -use sentry::{protocol::Event, Hub}; - -use crate::LocalError; -use autopush_common::tags::Tags; - -#[derive(Clone, Default)] -pub struct SentryWrapper { - metrics: Option>, - metric_label: String, -} - -impl SentryWrapper { - pub fn new(metrics: Arc, metric_label: String) -> Self { - Self { - metrics: Some(metrics), - metric_label, - } - } -} - -impl Transform for SentryWrapper -where - S: Service, Error = Error>, - S::Future: 'static, -{ - 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(), - }) - } -} - -#[derive(Debug)] -pub struct SentryWrapperMiddleware { - service: Rc>, - metrics: Option>, - metric_label: String, -} - -impl Service for SentryWrapperMiddleware -where - S: Service, Error = Error>, - S::Future: 'static, -{ - type Response = ServiceResponse; - type Error = Error; - type Future = LocalBoxFuture<'static, Result>; - - fn poll_ready(&self, cx: &mut Context<'_>) -> Poll> { - self.service.poll_ready(cx) - } - - 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 mut tags = Tags::from_request_head(sreq.head()); - let metrics = self.metrics.clone(); - let metric_label = self.metric_label.clone(); - if let Some(rtags) = sreq.request().extensions().get::() { - trace!("Sentry: found tags in request: {:?}", &rtags.tags); - for (k, v) in rtags.tags.clone() { - tags.tags.insert(k, v); - } - }; - sreq.extensions_mut().insert(tags.clone()); - - let fut = self.service.call(sreq); - - async move { - let response: Self::Response = match fut.await { - Ok(response) => response, - Err(error) => { - if let Some(api_err) = error.as_error::() { - // if it's not reportable, and we have access to the metrics, record it as a metric. - if !api_err.kind.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. - info!("Sentry: Sending error to metrics: {:?}", api_err.kind); - if let Some(metrics) = metrics { - if let Some(label) = api_err.kind.metric_label() { - 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); - event.extra.append(&mut tags.clone().extra_tree()); - event.tags.append(&mut tags.clone().tag_tree()); - 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(api_err) = error.as_error::() { - if !api_err.kind.is_sentry_event() { - debug!("Not reporting error (service error): {:?}", error); - return Ok(response); - } - } - debug!("Reporting error to Sentry (response error): {}", error); - let mut event = event_from_actix_error(error); - event.extra.append(&mut tags.clone().extra_tree()); - event.tags.append(&mut tags.clone().tag_tree()); - 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()); - } - - // TODO: Use ServiceRequest::match_pattern for the event transaction. - // Coming in Actix v3. - - Some(event) -} - -/// Convert Actix errors into a Sentry event. ApiError 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> { - // Actix errors don't have support source/cause, so to get more information - // about the error we need to downcast. - if let Some(error) = error.as_error::() { - // Use our error and associated backtrace for the event - let mut event = sentry::event_from_error(&error.kind); - event.exception.last_mut().unwrap().stacktrace = - sentry::integrations::backtrace::backtrace_to_stacktrace(&error.backtrace); - event - } else { - // Fallback to the Actix error - sentry::event_from_error(error) - } -} diff --git a/autoendpoint/src/error.rs b/autoendpoint/src/error.rs index e90f5a801..2d17146af 100644 --- a/autoendpoint/src/error.rs +++ b/autoendpoint/src/error.rs @@ -18,7 +18,7 @@ use std::fmt::{self, Display}; use thiserror::Error; use validator::{ValidationErrors, ValidationErrorsKind}; -use autopush_common::db::error::DbError; +use autopush_common::{db::error::DbError, errors::ReportableError}; /// Common `Result` type. pub type ApiResult = Result; @@ -331,6 +331,20 @@ impl Serialize for ApiError { } } +impl ReportableError for ApiError { + fn backtrace(&self) -> Option<&Backtrace> { + Some(&self.backtrace) + } + + fn is_sentry_event(&self) -> bool { + self.kind.is_sentry_event() + } + + fn metric_label(&self) -> Option<&'static str> { + self.kind.metric_label() + } +} + /// Get the error number from validation errors. If multiple errors are present, /// the first one with a valid error code is used. fn errno_from_validation_errors(e: &ValidationErrors) -> Option { diff --git a/autoendpoint/src/main.rs b/autoendpoint/src/main.rs index 047f0e8f8..4558d21ed 100644 --- a/autoendpoint/src/main.rs +++ b/autoendpoint/src/main.rs @@ -9,7 +9,6 @@ mod error; mod extractors; mod headers; mod metrics; -mod middleware; mod routers; mod routes; mod server; @@ -21,8 +20,6 @@ use std::error::Error; use autopush_common::logging; -pub type LocalError = crate::error::ApiError; - const USAGE: &str = " Usage: autoendpoint [options] diff --git a/autoendpoint/src/middleware/mod.rs b/autoendpoint/src/middleware/mod.rs deleted file mode 100644 index feb8ae7c6..000000000 --- a/autoendpoint/src/middleware/mod.rs +++ /dev/null @@ -1,3 +0,0 @@ -//! Actix middleware - -pub mod sentry; diff --git a/autoendpoint/src/server.rs b/autoendpoint/src/server.rs index 8a3c91173..9533ced62 100644 --- a/autoendpoint/src/server.rs +++ b/autoendpoint/src/server.rs @@ -11,7 +11,10 @@ use cadence::StatsdClient; use fernet::MultiFernet; use serde_json::json; -use autopush_common::db::{client::DbClient, dynamodb::DdbClientImpl, DbSettings, StorageType}; +use autopush_common::{ + db::{client::DbClient, dynamodb::DdbClientImpl, DbSettings, StorageType}, + middleware::sentry::SentryWrapper, +}; use crate::error::{ApiError, ApiErrorKind, ApiResult}; use crate::metrics; @@ -126,7 +129,7 @@ impl Server { // Middleware .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404)) // Our modified Sentry wrapper which does some blocking of non-reportable errors. - .wrap(crate::middleware::sentry::SentryWrapper::new( + .wrap(SentryWrapper::::new( metrics.clone(), "api_error".to_owned(), )) diff --git a/autopush-common/src/errors.rs b/autopush-common/src/errors.rs index 72e708145..b4ad7b129 100644 --- a/autopush-common/src/errors.rs +++ b/autopush-common/src/errors.rs @@ -171,7 +171,7 @@ impl ApcErrorKind { } } - pub fn metric_label(&self) -> Option { + pub fn metric_label(&self) -> Option<&'static str> { // TODO: add labels for skipped stuff let label = match self { Self::PongTimeout => "pong_timeout", @@ -179,7 +179,7 @@ impl ApcErrorKind { Self::PayloadError(_) => "payload", _ => return None, }; - Some(label.to_owned()) + Some(label) } } @@ -195,3 +195,17 @@ pub trait ReportableError: std::error::Error + fmt::Display { /// increment metric instead with this label fn metric_label(&self) -> Option<&'static str>; } + +impl ReportableError for ApcError { + fn backtrace(&self) -> Option<&Backtrace> { + Some(&self.backtrace) + } + + fn is_sentry_event(&self) -> bool { + self.kind.is_sentry_event() + } + + fn metric_label(&self) -> Option<&'static str> { + self.kind.metric_label() + } +} diff --git a/autopush-common/src/lib.rs b/autopush-common/src/lib.rs index 0e8f37563..b8dc4c844 100644 --- a/autopush-common/src/lib.rs +++ b/autopush-common/src/lib.rs @@ -11,6 +11,7 @@ pub mod endpoint; pub mod errors; pub mod logging; pub mod metrics; +pub mod middleware; pub mod notification; pub mod sentry; pub mod tags; diff --git a/autopush-common/src/middleware/mod.rs b/autopush-common/src/middleware/mod.rs new file mode 100644 index 000000000..2bae60991 --- /dev/null +++ b/autopush-common/src/middleware/mod.rs @@ -0,0 +1 @@ +pub mod sentry; diff --git a/autoendpoint/src/middleware/sentry.rs b/autopush-common/src/middleware/sentry.rs similarity index 77% rename from autoendpoint/src/middleware/sentry.rs rename to autopush-common/src/middleware/sentry.rs index 17d241fa5..fd467fe69 100644 --- a/autoendpoint/src/middleware/sentry.rs +++ b/autopush-common/src/middleware/sentry.rs @@ -1,9 +1,4 @@ -use std::{ - cell::RefCell, - rc::Rc, - sync::Arc, - task::{Context, Poll}, -}; +use std::{cell::RefCell, marker::PhantomData, rc::Rc, sync::Arc}; use actix_web::{ dev::{Service, ServiceRequest, ServiceResponse, Transform}, @@ -14,32 +9,34 @@ use futures::{future::LocalBoxFuture, FutureExt}; use futures_util::future::{ok, Ready}; use sentry::{protocol::Event, Hub}; -use crate::LocalError; -use autopush_common::tags::Tags; +use crate::{errors::ReportableError, tags::Tags}; #[derive(Clone)] -pub struct SentryWrapper { +pub struct SentryWrapper { metrics: Arc, metric_label: String, + phantom: PhantomData, } -impl SentryWrapper { +impl SentryWrapper { pub fn new(metrics: Arc, metric_label: String) -> Self { Self { metrics, metric_label, + phantom: PhantomData, } } } -impl Transform for SentryWrapper +impl Transform for SentryWrapper where S: Service, Error = Error>, S::Future: 'static, + E: ReportableError + actix_web::ResponseError + 'static, { type Response = ServiceResponse; type Error = Error; - type Transform = SentryWrapperMiddleware; + type Transform = SentryWrapperMiddleware; type InitError = (); type Future = Ready>; @@ -48,29 +45,30 @@ where service: Rc::new(RefCell::new(service)), metrics: self.metrics.clone(), metric_label: self.metric_label.clone(), + phantom: PhantomData, }) } } #[derive(Debug)] -pub struct SentryWrapperMiddleware { +pub struct SentryWrapperMiddleware { service: Rc>, metrics: Arc, metric_label: String, + phantom: PhantomData, } -impl Service for SentryWrapperMiddleware +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>; - fn poll_ready(&self, cx: &mut Context<'_>) -> Poll> { - self.service.poll_ready(cx) - } + actix_web::dev::forward_ready!(service); fn call(&self, sreq: ServiceRequest) -> Self::Future { // Set up the hub to add request data to events @@ -99,15 +97,15 @@ where let response: Self::Response = match fut.await { Ok(response) => response, Err(error) => { - if let Some(api_err) = error.as_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 !api_err.kind.is_sentry_event() { + 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) = api_err.kind.metric_label() { - info!("Sentry: Sending error to metrics: {:?}", api_err.kind); + if let Some(label) = reportable_err.metric_label() { + info!("Sentry: Sending error to metrics: {:?}", reportable_err); let _ = metrics.incr(&format!("{}.{}", metric_label, label)); } } @@ -115,7 +113,7 @@ where return Err(error); }; debug!("Reporting error to Sentry (service error): {}", error); - let mut event = event_from_actix_error(&error); + let mut event = event_from_actix_error::(&error); event.extra.append(&mut tags.clone().extra_tree()); event.tags.append(&mut tags.clone().tag_tree()); let event_id = hub.capture_event(event); @@ -125,10 +123,10 @@ where }; // Check for errors inside the response if let Some(error) = response.response().error() { - if let Some(api_err) = error.as_error::() { - if !api_err.kind.is_sentry_event() { - if let Some(label) = api_err.kind.metric_label() { - info!("Sentry: Sending error to metrics: {:?}", api_err.kind); + 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); @@ -136,7 +134,7 @@ where } } debug!("Reporting error to Sentry (response error): {}", error); - let mut event = event_from_actix_error(error); + let mut event = event_from_actix_error::(error); event.extra.append(&mut tags.clone().extra_tree()); event.tags.append(&mut tags.clone().tag_tree()); let event_id = hub.capture_event(event); @@ -185,17 +183,18 @@ fn process_event( Some(event) } -/// Convert Actix errors into a Sentry event. ApiError 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> { +/// 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(error) = error.as_error::() { + if let Some(reportable_err) = error.as_error::() { // Use our error and associated backtrace for the event - let mut event = sentry::event_from_error(&error.kind); - event.exception.last_mut().unwrap().stacktrace = - sentry::integrations::backtrace::backtrace_to_stacktrace(&error.backtrace); - event + crate::sentry::event_from_error(reportable_err) } else { // Fallback to the Actix error sentry::event_from_error(error)