From 66221d8bec17f6134dee1b9d9005f5cdbe8121d3 Mon Sep 17 00:00:00 2001 From: JR Conlin Date: Tue, 1 Sep 2020 08:49:35 -0700 Subject: [PATCH] feat: Convert some of the validation storage errors into metrics (#810) * feat: Convert some of the validation storage errors into metrics Closes #795 * f fmt, clippy * f remove unused function, added hawk invalid_port Co-authored-by: Donovan Preston Co-authored-by: Rachel Tublitz --- src/error.rs | 25 ++++++++++-- src/web/auth.rs | 1 + src/web/error.rs | 45 ++++++++++++++++----- src/web/extractors.rs | 85 +++++++++++++++++++++++++++++++++------- src/web/middleware/db.rs | 3 ++ 5 files changed, 131 insertions(+), 28 deletions(-) diff --git a/src/error.rs b/src/error.rs index 061f5f417f..95380a207d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -57,6 +57,7 @@ pub const RETRY_AFTER: u8 = 10; pub struct ApiError { inner: Context, status: StatusCode, + metric_label: Option, } /// Top-level ErrorKind. @@ -130,8 +131,8 @@ impl ApiError { _ => (), }, _ => (), - } - true + }; + self.metric_label.is_none() } pub fn on_response(&self, state: &ServerState) { @@ -148,6 +149,7 @@ impl ApiError { ref location, name, ref _tags, + ref _metric_label, ) => { match description.as_ref() { "over-quota" => return WeaveError::OverQuota, @@ -162,7 +164,12 @@ impl ApiError { } WeaveError::UnknownError } - ValidationErrorKind::FromValidationErrors(ref _err, ref location, ref _tags) => { + ValidationErrorKind::FromValidationErrors( + ref _err, + ref location, + ref _tags, + _metric_label, + ) => { if *location == RequestErrorLocation::Body { WeaveError::InvalidWbo } else { @@ -225,7 +232,11 @@ impl From> for ApiError { ApiErrorKind::Validation(error) => error.status, }; - Self { inner, status } + Self { + inner, + status, + metric_label: None, + } } } @@ -338,3 +349,9 @@ macro_rules! from_error { from_error!(DbError, ApiError, ApiErrorKind::Db); from_error!(HawkError, ApiError, ApiErrorKind::Hawk); from_error!(ValidationError, ApiError, ApiErrorKind::Validation); + +macro_rules! label { + ($string:expr) => { + Some($string.to_string()) + }; +} diff --git a/src/web/auth.rs b/src/web/auth.rs index b531a3040a..feeac49e95 100644 --- a/src/web/auth.rs +++ b/src/web/auth.rs @@ -175,6 +175,7 @@ impl HawkPayload { RequestErrorLocation::Header, None, tags, + label!("request.validate.hawk.invalid_port"), ) })? } else if ci.scheme() == "https" { diff --git a/src/web/error.rs b/src/web/error.rs index a675211b28..719b5338de 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -90,13 +90,20 @@ impl ValidationError { #[derive(Debug, Fail)] pub enum ValidationErrorKind { #[fail(display = "{}", _0)] - FromDetails(String, RequestErrorLocation, Option, Option), + FromDetails( + String, + RequestErrorLocation, + Option, + Option, + Option, + ), #[fail(display = "{}", _0)] FromValidationErrors( #[cause] validator::ValidationErrors, RequestErrorLocation, Option, + Option, ), } @@ -119,18 +126,27 @@ impl From> for ValidationError { fn from(inner: Context) -> Self { debug!("Validation Error: {:?}", inner.get_context()); let status = match inner.get_context() { - ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name), _) - if *location == RequestErrorLocation::Header => - { + ValidationErrorKind::FromDetails( + ref _description, + ref location, + Some(ref name), + _, + _, + ) if *location == RequestErrorLocation::Header => { match name.to_ascii_lowercase().as_str() { "accept" => StatusCode::NOT_ACCEPTABLE, "content-type" => StatusCode::UNSUPPORTED_MEDIA_TYPE, _ => StatusCode::BAD_REQUEST, } } - ValidationErrorKind::FromDetails(ref _description, ref location, Some(ref name), _) - if *location == RequestErrorLocation::Path - && ["bso", "collection"].contains(&name.as_ref()) => + ValidationErrorKind::FromDetails( + ref _description, + ref location, + Some(ref name), + _, + _, + ) if *location == RequestErrorLocation::Path + && ["bso", "collection"].contains(&name.as_ref()) => { StatusCode::NOT_FOUND } @@ -185,7 +201,13 @@ impl Serialize for ValidationErrorKind { let mut seq = serializer.serialize_seq(None)?; match *self { - ValidationErrorKind::FromDetails(ref description, ref location, ref name, ref tags) => { + ValidationErrorKind::FromDetails( + ref description, + ref location, + ref name, + ref tags, + ref _metric_label, + ) => { seq.serialize_element(&SerializedValidationError { description, location, @@ -195,7 +217,12 @@ impl Serialize for ValidationErrorKind { })?; } - ValidationErrorKind::FromValidationErrors(ref errors, ref location, ref tags) => { + ValidationErrorKind::FromValidationErrors( + ref errors, + ref location, + ref tags, + ref _metric_label, + ) => { for (field, field_errors) in errors.clone().field_errors().iter() { for field_error in field_errors.iter() { seq.serialize_element(&SerializedValidationError { diff --git a/src/web/extractors.rs b/src/web/extractors.rs index bb856f5345..33fd1fc8c7 100644 --- a/src/web/extractors.rs +++ b/src/web/extractors.rs @@ -160,6 +160,8 @@ impl FromRequest for BsoBodies { /// No collection id is used, so payload checks are not done here. fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future { // Only try and parse the body if its a valid content-type + let metrics = metrics::Metrics::from(req); + let tags = Tags::from_request_head(req.head()); let ctype = match ContentType::parse(req) { Ok(v) => v, Err(e) => { @@ -168,7 +170,8 @@ impl FromRequest for BsoBodies { format!("Unreadable Content-Type: {:?}", e), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - None, + Some(tags), + label!("request.validate.bad_content_type"), ) .into(), )) @@ -178,12 +181,14 @@ impl FromRequest for BsoBodies { debug!("content_type: {:?}", &content_type); if !ACCEPTED_CONTENT_TYPES.contains(&content_type.as_ref()) { + metrics.incr("request.error.invalid_content_type"); return Box::pin(future::err( ValidationErrorKind::FromDetails( format!("Invalid Content-Type {:?}", content_type), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - None, + Some(tags), + label!("request.validate.bad_content_type"), ) .into(), )); @@ -197,17 +202,20 @@ impl FromRequest for BsoBodies { RequestErrorLocation::Header, None, None, + None, ) .into() }); // Avoid duplicating by defining our error func now, doesn't need the box wrapper - fn make_error(tags: Option) -> Error { + fn make_error(tags: Option, metrics: metrics::Metrics) -> Error { + metrics.incr_with_tags("request.error.invalid_json", tags.clone()); ValidationErrorKind::FromDetails( "Invalid JSON in request body".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), tags, + label!("request.validate.invalid_body_json"), ) .into() } @@ -227,6 +235,7 @@ impl FromRequest for BsoBodies { RequestErrorLocation::Unknown, Some("app_data".to_owned()), None, + None, ) .into(), )); @@ -240,14 +249,14 @@ impl FromRequest for BsoBodies { match u64::from_str(uid.trim()) { Ok(v) => { if v == HawkIdentifier::uid_from_path(req.uri(), None).unwrap_or(0) { - debug!("### returning quota exceeded."); error!("Returning over quota for {:?}", v); return Box::pin(future::err( ValidationErrorKind::FromDetails( "over-quota".to_owned(), RequestErrorLocation::Unknown, Some("over-quota".to_owned()), - None, + Some(tags), + label!("storage.over_quota"), ) .into(), )); @@ -273,7 +282,7 @@ impl FromRequest for BsoBodies { bsos.push(raw_json); } else { // Per Python version, BSO's must json deserialize - return future::err(make_error(None)); + return future::err(make_error(None, metrics)); } } bsos @@ -281,7 +290,7 @@ impl FromRequest for BsoBodies { json_vals } else { // Per Python version, BSO's must json deserialize - return future::err(make_error(None)); + return future::err(make_error(None, metrics)); }; // Validate all the BSO's, move invalid to our other list. Assume they'll all make @@ -301,7 +310,7 @@ impl FromRequest for BsoBodies { for bso in bsos { // Error out if its not a JSON mapping type if !bso.is_object() { - return future::err(make_error(None)); + return future::err(make_error(None, metrics)); } // Save all id's we get, check for missing id, or duplicate. let bso_id = if let Some(id) = bso.get("id").and_then(serde_json::Value::as_str) { @@ -312,7 +321,8 @@ impl FromRequest for BsoBodies { "Input BSO has duplicate ID".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - None, + Some(tags), + label!("request.store.duplicate_bso_id"), ) .into(), ); @@ -326,7 +336,8 @@ impl FromRequest for BsoBodies { "Input BSO has no ID".to_owned(), RequestErrorLocation::Body, Some("bsos".to_owned()), - None, + Some(tags), + label!("request.store.missing_bso_id"), ) .into(), ); @@ -383,6 +394,9 @@ impl FromRequest for BsoBody { fn from_request(req: &HttpRequest, payload: &mut Payload) -> Self::Future { // Only try and parse the body if its a valid content-type + let tags = Tags::from_request_head(req.head()); + let ftags = tags.clone(); + let fftags = tags.clone(); let ctype = match ContentType::parse(req) { Ok(v) => v, Err(e) => { @@ -391,7 +405,8 @@ impl FromRequest for BsoBody { format!("Unreadable Content-Type: {:?}", e), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - None, + Some(tags), + label!("request.validate.bad_content_type"), ) .into(), )) @@ -404,7 +419,8 @@ impl FromRequest for BsoBody { "Invalid Content-Type".to_owned(), RequestErrorLocation::Header, Some("Content-Type".to_owned()), - None, + Some(tags), + label!("request.validate.bad_content_type"), ) .into(), )); @@ -419,6 +435,7 @@ impl FromRequest for BsoBody { RequestErrorLocation::Unknown, Some("app_data".to_owned()), None, + None, ) .into(), )); @@ -439,7 +456,8 @@ impl FromRequest for BsoBody { "over-quota".to_owned(), RequestErrorLocation::Unknown, Some("over-quota".to_owned()), - None, + Some(tags), + label!("request.store.user_over_quota"), ) .into(), )); @@ -461,7 +479,8 @@ impl FromRequest for BsoBody { e.to_string(), RequestErrorLocation::Body, Some("bso".to_owned()), - None, + Some(tags), + label!("request.validate.bad_bso_body"), ) .into(); err.into() @@ -479,7 +498,8 @@ impl FromRequest for BsoBody { "payload too large".to_owned(), RequestErrorLocation::Body, Some("bso".to_owned()), - None, + Some(ftags), + label!("request.validate.payload_too_large"), ) .into(); return future::err(err.into()); @@ -488,6 +508,7 @@ impl FromRequest for BsoBody { let err: ApiError = ValidationErrorKind::FromValidationErrors( e, RequestErrorLocation::Body, + Some(fftags), None, ) .into(); @@ -519,6 +540,7 @@ impl BsoParam { RequestErrorLocation::Path, Some("bso".to_owned()), Some(tags.clone()), + label!("request.process.invalid_bso"), ))?; } if let Some(v) = elements.get(5) { @@ -529,6 +551,7 @@ impl BsoParam { RequestErrorLocation::Path, Some("bso".to_owned()), Some(tags.clone()), + label!("request.process.invalid_bso"), ) })?) .map_err(|e| { @@ -538,6 +561,7 @@ impl BsoParam { RequestErrorLocation::Path, Some("bso".to_owned()), Some(tags.clone()), + label!("request.process.invalid_bso"), ) })?; Ok(Self { bso: sv }) @@ -548,6 +572,7 @@ impl BsoParam { RequestErrorLocation::Path, Some("bso".to_owned()), Some(tags.clone()), + label!("request.process.missing_bso"), ))? } } @@ -564,6 +589,7 @@ impl BsoParam { e, RequestErrorLocation::Path, Some(tags.clone()), + None, ) })?; extensions.insert(bso.clone()); @@ -604,6 +630,7 @@ impl CollectionParam { RequestErrorLocation::Path, Some("collection".to_owned()), Some(tags.clone()), + label!("request.process.missing_collection"), ) })?; sv = urldecode(&sv).map_err(|_e| { @@ -612,6 +639,7 @@ impl CollectionParam { RequestErrorLocation::Path, Some("collection".to_owned()), Some(tags.clone()), + label!("request.process.invalid_collection"), ) })?; Ok(Some(Self { collection: sv })) @@ -621,6 +649,7 @@ impl CollectionParam { RequestErrorLocation::Path, Some("collection".to_owned()), Some(tags.clone()), + label!("request.process.missing_collection"), ))? } } @@ -641,6 +670,7 @@ impl CollectionParam { e, RequestErrorLocation::Path, Some(tags.clone()), + None, ) })?; Some(collection) @@ -671,6 +701,7 @@ impl FromRequest for CollectionParam { RequestErrorLocation::Path, Some("collection".to_owned()), Some(tags), + label!("request.process.missing_collection"), ))? } }) @@ -767,6 +798,7 @@ impl FromRequest for CollectionRequest { RequestErrorLocation::Header, Some("accept".to_string()), Some(tags), + label!("request.validate.invalid_accept_header"), ) .into()); } @@ -825,6 +857,7 @@ impl FromRequest for CollectionPostRequest { RequestErrorLocation::Unknown, Some("app_data".to_owned()), Some(tags), + None, ) .into()); } @@ -848,6 +881,7 @@ impl FromRequest for CollectionPostRequest { RequestErrorLocation::Body, Some("bsos".to_owned()), Some(tags), + label!("request.process.known_bad_bso"), ) .into()); } @@ -958,6 +992,7 @@ impl FromRequest for BsoPutRequest { RequestErrorLocation::Body, Some("bsos".to_owned()), Some(tags), + label!("request.process.known_bad_bso"), ) .into()); } @@ -1009,6 +1044,7 @@ impl FromRequest for HeartbeatRequest { RequestErrorLocation::Unknown, Some("state".to_owned()), Some(tags), + None, ) .into()); } @@ -1092,6 +1128,7 @@ impl HawkIdentifier { RequestErrorLocation::Path, Some("uid".to_owned()), tags, + label!("request.validate.hawk.invalid_uid"), ) .into()); } @@ -1104,6 +1141,7 @@ impl HawkIdentifier { RequestErrorLocation::Path, Some("uid".to_owned()), tags.clone(), + label!("request.validate.hawk.invalid_uid"), ) .into() }) @@ -1114,6 +1152,7 @@ impl HawkIdentifier { RequestErrorLocation::Path, Some("uid".to_owned()), tags, + label!("request.validate.hawk.missing_uid"), ))? } } @@ -1162,6 +1201,7 @@ impl HawkIdentifier { RequestErrorLocation::Path, Some("uid".to_owned()), tags, + label!("request.validate.hawk.uri_missing_uid"), ))?; } @@ -1195,6 +1235,7 @@ impl FromRequest for HawkIdentifier { RequestErrorLocation::Unknown, Some("state".to_owned()), Some(tags), + None, ) .into()); } @@ -1319,6 +1360,7 @@ impl FromRequest for BsoQueryParams { RequestErrorLocation::QueryString, None, Some(tags.clone()), + None, ) }) .await? @@ -1328,6 +1370,7 @@ impl FromRequest for BsoQueryParams { e, RequestErrorLocation::QueryString, Some(tags.clone()), + None, ) })?; // issue559: Dead code (timestamp always None) @@ -1403,6 +1446,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::QueryString, None, Some(tags.clone()), + None, ) }) .await? @@ -1416,6 +1460,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::Unknown, Some("state".to_owned()), Some(tags), + None, ) .into()); } @@ -1437,6 +1482,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::Header, Some((*header).to_owned()), Some(tags.clone()), + None, ) .into(); err @@ -1449,6 +1495,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::Header, Some((*header).to_owned()), Some(tags.clone()), + label!("request.validate.batch.invalid_x_weave"), ) .into(); err @@ -1459,6 +1506,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::Header, None, Some(tags.clone()), + label!("request.validate.batch.size_exceeded"), ) .into()); } @@ -1474,6 +1522,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::Path, None, Some(tags), + label!("request.validate.batch.missing_id"), ) .into()); } @@ -1483,6 +1532,7 @@ impl FromRequest for BatchRequestOpt { e, RequestErrorLocation::QueryString, Some(tags.clone()), + None, ) .into(); err @@ -1501,6 +1551,7 @@ impl FromRequest for BatchRequestOpt { RequestErrorLocation::QueryString, Some("batch".to_owned()), Some(ftags), + label!("request.validate.batch.invalid_id"), ) .into()); } @@ -1547,6 +1598,7 @@ impl PreConditionHeaderOpt { RequestErrorLocation::Header, Some("X-If-Unmodified-Since".to_owned()), tags, + label!("request.validate.mod_header.conflict"), ) .into()); }; @@ -1570,6 +1622,7 @@ impl PreConditionHeaderOpt { RequestErrorLocation::Header, Some("X-If-Modified-Since".to_owned()), tags, + label!("request.validate.mod_header.negative"), ) .into()); } @@ -1581,6 +1634,7 @@ impl PreConditionHeaderOpt { RequestErrorLocation::Header, Some(field_name.to_owned()), tags.clone(), + None, ) .into() }) @@ -1591,6 +1645,7 @@ impl PreConditionHeaderOpt { RequestErrorLocation::Header, Some(field_name.to_owned()), tags.clone(), + None, ) .into() }) diff --git a/src/web/middleware/db.rs b/src/web/middleware/db.rs index c6f40c2e30..02dd262dc4 100644 --- a/src/web/middleware/db.rs +++ b/src/web/middleware/db.rs @@ -176,6 +176,9 @@ where if apie.is_reportable() { report(&tags, sentry::integrations::failure::event_from_fail(&apie)); } else { + if let Some(label) = apie.metric_label { + state.metrics.incr_with_tags(apie.metric_label, tags); + } debug!("Not reporting error: {:?}", apie); } return Err(apie.into());