Skip to content
This repository has been archived by the owner on Jul 26, 2024. It is now read-only.

bug: make UA name an 'extra' value for Sentry errors. #153

Merged
merged 5 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/adm/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,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
Expand Down
4 changes: 3 additions & 1 deletion src/adm/tiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to keep the string for now and instead break down the type of error (later). Most of what we're seeing so far is err.is_timeout(), so we'd probably start there -- but probably be handy to know if it's something else before we fully break it all down.

Copy link
Member

@pjenvey pjenvey Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but the string's obviously sent as an extra (I'm not sure I noticed this before 🤷‍♂️ ), so we're not losing it, this works for me.

We could map specific e.g. err.is_timeout() errors to their own individual error if we wanted later.

err.tags.add_extra("error", &e.to_string());
err
})?
.error_for_status()?
.json()
Expand Down
26 changes: 17 additions & 9 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> = result::Result<T, HandlerError>;

Expand All @@ -23,6 +25,7 @@ pub type HandlerResult<T> = result::Result<T, HandlerError>;
pub struct HandlerError {
kind: HandlerErrorKind,
backtrace: Backtrace,
pub tags: Tags,
}

/// The specific context types of HandlerError.
Expand Down Expand Up @@ -69,12 +72,12 @@ 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: {:?}", _0)]
InvalidUA(String),
#[error("Invalid user agent")]
InvalidUA(),
}

/// A set of Error Context utilities
Expand All @@ -83,9 +86,9 @@ 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,
&HandlerErrorKind::InvalidUA() => StatusCode::FORBIDDEN,
_ => StatusCode::INTERNAL_SERVER_ERROR,
}
}
Expand All @@ -97,14 +100,14 @@ 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,
HandlerErrorKind::UnexpectedHost(_, _) => 602,
HandlerErrorKind::MissingHost(_, _) => 603,
HandlerErrorKind::UnexpectedAdvertiser(_) => 604,
HandlerErrorKind::InvalidUA(_) => 700,
HandlerErrorKind::InvalidUA() => 700,
}
}

Expand Down Expand Up @@ -166,6 +169,7 @@ where
HandlerError {
kind: HandlerErrorKind::from(item),
backtrace: Backtrace::new(),
tags: Tags::default(),
}
}
}
Expand All @@ -178,7 +182,11 @@ impl From<HandlerError> 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;
Expand Down
1 change: 1 addition & 0 deletions src/web/middleware/sentry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ where
return future::ok(sresp);
}
*/
tags.extend(herr.tags.clone());
report(&tags, sentry::event_from_error(herr));
}
}
Expand Down
19 changes: 14 additions & 5 deletions src/web/user_agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::fmt;

use woothee::parser::Parser;

use crate::error::{HandlerErrorKind, HandlerResult};
use crate::error::{HandlerError, HandlerErrorKind, HandlerResult};

/// ADM required browser format form
#[allow(dead_code)]
Expand Down Expand Up @@ -80,7 +80,11 @@ 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()) {
return Err(HandlerErrorKind::InvalidUA(ua.to_owned()).into());
let mut err: HandlerError = HandlerErrorKind::InvalidUA().into();
err.tags.add_extra("useragent", ua);
err.tags
.add_extra("name", &wresult.name.to_lowercase().as_str());
return Err(err);
}

let os = wresult.os.to_lowercase();
Expand Down Expand Up @@ -206,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() {
HandlerErrorKind::InvalidUA(_) => {}
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]
Expand Down