From 1a519d078b27d1a45b490e8c107066bc30685fe3 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 10 Jun 2021 09:08:34 -0700 Subject: [PATCH 1/3] bug: make UA name an 'extra' value for Sentry errors. Closes #147 --- src/error.rs | 8 ++++---- src/web/handlers.rs | 4 ++-- src/web/user_agent.rs | 19 +++++++++++++------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/error.rs b/src/error.rs index f89f408c..09193127 100644 --- a/src/error.rs +++ b/src/error.rs @@ -73,8 +73,8 @@ pub enum HandlerErrorKind { AdmServerError(String), /// Invalid UserAgent request - #[error("Invalid user agent: {:?}", _0)] - InvalidUA(String), + #[error("Invalid user agent")] + InvalidUA(), } /// A set of Error Context utilities @@ -85,7 +85,7 @@ impl HandlerErrorKind { HandlerErrorKind::Validation(_) => StatusCode::BAD_REQUEST, HandlerErrorKind::AdmServerError(_) => StatusCode::SERVICE_UNAVAILABLE, HandlerErrorKind::BadAdmResponse(_) => StatusCode::BAD_GATEWAY, - &HandlerErrorKind::InvalidUA(_) => StatusCode::FORBIDDEN, + &HandlerErrorKind::InvalidUA() => StatusCode::FORBIDDEN, _ => StatusCode::INTERNAL_SERVER_ERROR, } } @@ -104,7 +104,7 @@ impl HandlerErrorKind { HandlerErrorKind::UnexpectedHost(_, _) => 602, HandlerErrorKind::MissingHost(_, _) => 603, HandlerErrorKind::UnexpectedAdvertiser(_) => 604, - HandlerErrorKind::InvalidUA(_) => 700, + HandlerErrorKind::InvalidUA() => 700, } } diff --git a/src/web/handlers.rs b/src/web/handlers.rs index 324b14dc..6e2848f4 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -39,7 +39,8 @@ pub async fn get_tiles( default } }); - let (os_family, form_factor) = user_agent::get_device_info(&treq.ua)?; + let mut tags = Tags::default(); + let (os_family, form_factor) = user_agent::get_device_info(&treq.ua, &mut tags)?; let header = request.head(); let location = if state.mmdb.is_available() { @@ -58,7 +59,6 @@ pub async fn get_tiles( LocationResult::from_header(header, settings, &metrics) }; - let mut tags = Tags::default(); { tags.add_extra("country", &location.country()); tags.add_extra("region", &location.region()); diff --git a/src/web/user_agent.rs b/src/web/user_agent.rs index f56fe951..a16b0db9 100644 --- a/src/web/user_agent.rs +++ b/src/web/user_agent.rs @@ -5,6 +5,7 @@ use std::fmt; use woothee::parser::Parser; use crate::error::{HandlerErrorKind, HandlerResult}; +use crate::tags::Tags; /// ADM required browser format form #[allow(dead_code)] @@ -75,12 +76,13 @@ pub fn strip_ua(ua: &str) -> String { */ /// Convert a UserAgent header into a simplified ([OsFamily], [FormFactor]) -pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { +pub fn get_device_info(ua: &str, tags: &mut Tags) -> HandlerResult<(OsFamily, FormFactor)> { let wresult = Parser::new().parse(ua).unwrap_or_default(); // If it's not firefox, it doesn't belong here... if !["firefox"].contains(&wresult.name.to_lowercase().as_str()) { - return Err(HandlerErrorKind::InvalidUA(ua.to_owned()).into()); + tags.add_extra("name", ua); + return Err(HandlerErrorKind::InvalidUA().into()); } let os = wresult.os.to_lowercase(); @@ -106,6 +108,7 @@ pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { #[cfg(test)] mod tests { use crate::error::HandlerErrorKind; + use crate::tags::Tags; use super::{get_device_info, FormFactor, OsFamily}; @@ -117,8 +120,9 @@ mod tests { macro_rules! assert_get_device_info { ($value:expr, $os_family:expr, $form_factor:expr) => { + let mut tags = Tags::default(); assert_eq!( - get_device_info($value).expect("Error"), + get_device_info($value, &mut tags).expect("Error"), ($os_family, $form_factor) ); }; @@ -206,22 +210,25 @@ mod tests { #[test] fn chromeos() { - let result = get_device_info("Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36"); + let mut tags = Tags::default(); + let result = get_device_info("Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36", &mut tags); assert!(result.is_err()); match result.unwrap_err().kind() { - HandlerErrorKind::InvalidUA(_) => {} + HandlerErrorKind::InvalidUA() => {} _ => panic!("Incorrect error returned for test"), } } #[test] fn other_ua() { + let mut tags = Tags::default(); assert_strip_eq!( "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:?.0) Gecko/20100101 Firefox/?.0" ); assert!(get_device_info( - "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36") + "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36", + &mut tags) .is_err() ); } From 500e6a2fb3e4f51b59f9307d42e266f1b59c4b6d Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 10 Jun 2021 09:54:24 -0700 Subject: [PATCH 2/3] f add tags to HandleError so that we can pass meta info easier * Remove tag hand off for get_UA * reduce cardinality for AdmServerError in sentry --- src/adm/filter.rs | 4 +++- src/adm/tiles.rs | 4 +++- src/error.rs | 18 +++++++++++++----- src/web/handlers.rs | 4 ++-- src/web/middleware/sentry.rs | 1 + src/web/user_agent.rs | 21 ++++++++------------- 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/adm/filter.rs b/src/adm/filter.rs index ba7be6be..4eba143d 100644 --- a/src/adm/filter.rs +++ b/src/adm/filter.rs @@ -69,7 +69,9 @@ impl AdmFilter { fn report(&self, error: &HandlerError, tags: &Tags) { // trace!(&error, &tags); // TODO: if not error.is_reportable, just add to metrics. - l_sentry::report(tags, sentry::event_from_error(error)); + let mut merged_tags = error.tags.clone(); + merged_tags.extend(tags.clone()); + l_sentry::report(&merged_tags, sentry::event_from_error(error)); } /// Check the advertiser URL diff --git a/src/adm/tiles.rs b/src/adm/tiles.rs index a4f1b10f..4432d9d3 100644 --- a/src/adm/tiles.rs +++ b/src/adm/tiles.rs @@ -164,7 +164,9 @@ pub async fn get_tiles( .await .map_err(|e| { // ADM servers are down, or improperly configured - HandlerErrorKind::AdmServerError(e.to_string()) + let mut err: HandlerError = HandlerErrorKind::AdmServerError().into(); + err.tags.add_extra("error", &e.to_string()); + err })? .error_for_status()? .json() diff --git a/src/error.rs b/src/error.rs index 09193127..d9567021 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,6 +15,8 @@ use actix_web::{ }; use thiserror::Error; +use crate::tags::Tags; + /// The standard Result type for Contile (returns Error = [`HandlerError`]) pub type HandlerResult = result::Result; @@ -23,6 +25,7 @@ pub type HandlerResult = result::Result; pub struct HandlerError { kind: HandlerErrorKind, backtrace: Backtrace, + pub tags: Tags, } /// The specific context types of HandlerError. @@ -69,8 +72,8 @@ pub enum HandlerErrorKind { BadAdmResponse(String), /// ADM Servers returned an error - #[error("Adm Server Error: {:?}", _0)] - AdmServerError(String), + #[error("Adm Server Error")] + AdmServerError(), /// Invalid UserAgent request #[error("Invalid user agent")] @@ -83,7 +86,7 @@ impl HandlerErrorKind { pub fn http_status(&self) -> StatusCode { match self { HandlerErrorKind::Validation(_) => StatusCode::BAD_REQUEST, - HandlerErrorKind::AdmServerError(_) => StatusCode::SERVICE_UNAVAILABLE, + HandlerErrorKind::AdmServerError() => StatusCode::SERVICE_UNAVAILABLE, HandlerErrorKind::BadAdmResponse(_) => StatusCode::BAD_GATEWAY, &HandlerErrorKind::InvalidUA() => StatusCode::FORBIDDEN, _ => StatusCode::INTERNAL_SERVER_ERROR, @@ -97,7 +100,7 @@ impl HandlerErrorKind { HandlerErrorKind::Internal(_) => 510, HandlerErrorKind::Reqwest(_) => 520, HandlerErrorKind::BadAdmResponse(_) => 521, - HandlerErrorKind::AdmServerError(_) => 522, + HandlerErrorKind::AdmServerError() => 522, HandlerErrorKind::Location(_) => 530, HandlerErrorKind::Validation(_) => 600, HandlerErrorKind::InvalidHost(_, _) => 601, @@ -166,6 +169,7 @@ where HandlerError { kind: HandlerErrorKind::from(item), backtrace: Backtrace::new(), + tags: Tags::default(), } } } @@ -178,7 +182,11 @@ impl From for HttpResponse { impl fmt::Display for HandlerError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Error: {}\nBacktrace:\n{:?}", self.kind, self.backtrace)?; + write!( + f, + "Error: {}\nTags:{:?}\nBacktrace:\n{:?}", + self.kind, self.tags, self.backtrace + )?; // Go down the chain of errors let mut error: &dyn Error = &self.kind; diff --git a/src/web/handlers.rs b/src/web/handlers.rs index 6e2848f4..324b14dc 100644 --- a/src/web/handlers.rs +++ b/src/web/handlers.rs @@ -39,8 +39,7 @@ pub async fn get_tiles( default } }); - let mut tags = Tags::default(); - let (os_family, form_factor) = user_agent::get_device_info(&treq.ua, &mut tags)?; + let (os_family, form_factor) = user_agent::get_device_info(&treq.ua)?; let header = request.head(); let location = if state.mmdb.is_available() { @@ -59,6 +58,7 @@ pub async fn get_tiles( LocationResult::from_header(header, settings, &metrics) }; + let mut tags = Tags::default(); { tags.add_extra("country", &location.country()); tags.add_extra("region", &location.region()); diff --git a/src/web/middleware/sentry.rs b/src/web/middleware/sentry.rs index 1fe7fcdc..ed8a03b9 100644 --- a/src/web/middleware/sentry.rs +++ b/src/web/middleware/sentry.rs @@ -161,6 +161,7 @@ where return future::ok(sresp); } */ + tags.extend(herr.tags.clone()); report(&tags, sentry::event_from_error(herr)); } } diff --git a/src/web/user_agent.rs b/src/web/user_agent.rs index a16b0db9..f3ef6e58 100644 --- a/src/web/user_agent.rs +++ b/src/web/user_agent.rs @@ -4,8 +4,7 @@ use std::fmt; use woothee::parser::Parser; -use crate::error::{HandlerErrorKind, HandlerResult}; -use crate::tags::Tags; +use crate::error::{HandlerError, HandlerErrorKind, HandlerResult}; /// ADM required browser format form #[allow(dead_code)] @@ -76,13 +75,14 @@ pub fn strip_ua(ua: &str) -> String { */ /// Convert a UserAgent header into a simplified ([OsFamily], [FormFactor]) -pub fn get_device_info(ua: &str, tags: &mut Tags) -> HandlerResult<(OsFamily, FormFactor)> { +pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { let wresult = Parser::new().parse(ua).unwrap_or_default(); // If it's not firefox, it doesn't belong here... if !["firefox"].contains(&wresult.name.to_lowercase().as_str()) { - tags.add_extra("name", ua); - return Err(HandlerErrorKind::InvalidUA().into()); + let mut err: HandlerError = HandlerErrorKind::InvalidUA().into(); + err.tags.add_extra("name", ua); + return Err(err); } let os = wresult.os.to_lowercase(); @@ -108,7 +108,6 @@ pub fn get_device_info(ua: &str, tags: &mut Tags) -> HandlerResult<(OsFamily, Fo #[cfg(test)] mod tests { use crate::error::HandlerErrorKind; - use crate::tags::Tags; use super::{get_device_info, FormFactor, OsFamily}; @@ -120,9 +119,8 @@ mod tests { macro_rules! assert_get_device_info { ($value:expr, $os_family:expr, $form_factor:expr) => { - let mut tags = Tags::default(); assert_eq!( - get_device_info($value, &mut tags).expect("Error"), + get_device_info($value).expect("Error"), ($os_family, $form_factor) ); }; @@ -210,8 +208,7 @@ mod tests { #[test] fn chromeos() { - let mut tags = Tags::default(); - let result = get_device_info("Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36", &mut tags); + let result = get_device_info("Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36"); assert!(result.is_err()); match result.unwrap_err().kind() { HandlerErrorKind::InvalidUA() => {} @@ -221,14 +218,12 @@ mod tests { #[test] fn other_ua() { - let mut tags = Tags::default(); assert_strip_eq!( "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:?.0) Gecko/20100101 Firefox/?.0" ); assert!(get_device_info( - "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36", - &mut tags) + "Mozilla/5.0 (Macintosh; Intel Mac OS X 11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.150 Safari/537.36") .is_err() ); } From d65cb2ed558f77b602831befeba76f3cab158d16 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 10 Jun 2021 10:17:10 -0700 Subject: [PATCH 3/3] f add test --- src/web/user_agent.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/web/user_agent.rs b/src/web/user_agent.rs index f3ef6e58..566e0552 100644 --- a/src/web/user_agent.rs +++ b/src/web/user_agent.rs @@ -81,7 +81,9 @@ pub fn get_device_info(ua: &str) -> HandlerResult<(OsFamily, FormFactor)> { // If it's not firefox, it doesn't belong here... if !["firefox"].contains(&wresult.name.to_lowercase().as_str()) { let mut err: HandlerError = HandlerErrorKind::InvalidUA().into(); - err.tags.add_extra("name", ua); + err.tags.add_extra("useragent", ua); + err.tags + .add_extra("name", &wresult.name.to_lowercase().as_str()); return Err(err); } @@ -208,12 +210,17 @@ mod tests { #[test] fn chromeos() { - let result = get_device_info("Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36"); + let ua_str = "Mozilla/5.0 (X11; CrOS x86_64 13816.64.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.100 Safari/537.36"; + let result = get_device_info(ua_str); assert!(result.is_err()); - match result.unwrap_err().kind() { + let err = result.unwrap_err(); + match err.kind() { HandlerErrorKind::InvalidUA() => {} _ => panic!("Incorrect error returned for test"), } + assert!(err.tags.extra.get("useragent") == Some(&ua_str.to_owned())); + assert!(err.tags.extra.get("name") == Some(&"chrome".to_owned())); + dbg!(err.tags); } #[test]