From f795eb0813b4ee37463add5391c829c906fdb35d Mon Sep 17 00:00:00 2001
From: JR Conlin <jconlin+git@mozilla.com>
Date: Mon, 5 Apr 2021 14:45:39 -0700
Subject: [PATCH] bug: Restore hawk error metrics (#1033)

* chore: clippy updates for rust 1.51

Closes: #1032

* bug: Restore hawk error metrics

(Includes clippy changes for rust 1.51)

Hawk errors should be returned as metrics. During the middleware purge, this broke. This PR includes metric reporting into the sentry handler.

Closes #812.
---
 Dockerfile                     |  2 +-
 src/db/error.rs                |  2 +-
 src/error.rs                   |  8 --------
 src/server/test.rs             |  2 --
 src/web/error.rs               |  6 +-----
 src/web/middleware/rejectua.rs |  1 -
 src/web/middleware/sentry.rs   | 15 +++++++++++----
 src/web/tags.rs                |  1 -
 8 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/Dockerfile b/Dockerfile
index 286c3fd6..bea15f7f 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,4 +1,4 @@
-FROM rust:1.50-buster as builder
+FROM rust:1.51-buster as builder
 WORKDIR /app
 ADD . /app
 ENV PATH=$PATH:/root/.cargo/bin
diff --git a/src/db/error.rs b/src/db/error.rs
index 181f06c5..22e8163f 100644
--- a/src/db/error.rs
+++ b/src/db/error.rs
@@ -75,7 +75,7 @@ impl DbError {
 
     pub fn metric_label(&self) -> Option<String> {
         match self.inner.get_context() {
-            DbErrorKind::Conflict => Some("request.error.db.conflict".to_owned()),
+            DbErrorKind::Conflict => Some("storage.conflict".to_owned()),
             _ => None,
         }
     }
diff --git a/src/error.rs b/src/error.rs
index 629e4b0c..5d64c029 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -22,8 +22,6 @@ use serde::{
 };
 
 use crate::db::error::{DbError, DbErrorKind};
-use crate::server::metrics::Metrics;
-use crate::server::ServerState;
 use crate::web::error::{HawkError, ValidationError, ValidationErrorKind};
 use crate::web::extractors::RequestErrorLocation;
 
@@ -137,12 +135,6 @@ impl ApiError {
         self.kind().metric_label().is_none()
     }
 
-    pub fn on_response(&self, state: &ServerState) {
-        if self.is_conflict() {
-            Metrics::from(state).incr("storage.confict")
-        }
-    }
-
     fn weave_error_code(&self) -> WeaveError {
         match self.kind() {
             ApiErrorKind::Validation(ver) => match ver.kind() {
diff --git a/src/server/test.rs b/src/server/test.rs
index fad2e2b5..594d8ffd 100644
--- a/src/server/test.rs
+++ b/src/server/test.rs
@@ -250,9 +250,7 @@ async fn test_endpoint_with_body(
         .call(req)
         .await
         .expect("Could not get sresponse in test_endpoint_with_body");
-    dbg!("got response", sresponse.response().status());
     assert!(sresponse.response().status().is_success());
-    dbg!("all good");
     test::read_body(sresponse).await
 }
 
diff --git a/src/web/error.rs b/src/web/error.rs
index 422a10ae..25c2cabf 100644
--- a/src/web/error.rs
+++ b/src/web/error.rs
@@ -29,10 +29,7 @@ impl HawkError {
     }
 
     pub fn is_reportable(&self) -> bool {
-        matches!(
-            &self.kind(),
-            HawkErrorKind::TruncatedId | HawkErrorKind::Parse(_)
-        )
+        matches!(&self.kind(), HawkErrorKind::TruncatedId)
     }
 
     pub fn metric_label(&self) -> Option<String> {
@@ -207,7 +204,6 @@ impl Serialize for ValidationErrorKind {
         S: Serializer,
     {
         let mut seq = serializer.serialize_seq(None)?;
-
         match *self {
             ValidationErrorKind::FromDetails(
                 ref description,
diff --git a/src/web/middleware/rejectua.rs b/src/web/middleware/rejectua.rs
index 4c0db467..622b7556 100644
--- a/src/web/middleware/rejectua.rs
+++ b/src/web/middleware/rejectua.rs
@@ -54,7 +54,6 @@ where
     }
 }
 #[allow(clippy::clippy::upper_case_acronyms)]
-
 pub struct RejectUAMiddleware<S> {
     service: S,
 }
diff --git a/src/web/middleware/sentry.rs b/src/web/middleware/sentry.rs
index c7c95619..4a4ca95b 100644
--- a/src/web/middleware/sentry.rs
+++ b/src/web/middleware/sentry.rs
@@ -15,7 +15,7 @@ use sentry::protocol::Event;
 use std::task::Poll;
 
 use crate::error::ApiError;
-use crate::server::ServerState;
+use crate::server::{metrics::Metrics, ServerState};
 use crate::web::tags::Tags;
 
 pub struct SentryWrapper;
@@ -100,6 +100,11 @@ where
     fn call(&mut self, sreq: ServiceRequest) -> Self::Future {
         let mut tags = Tags::from(sreq.head());
         sreq.extensions_mut().insert(tags.clone());
+        let metrics = if let Some(state) = sreq.app_data::<Data<ServerState>>() {
+            Some(Metrics::from(state.get_ref()))
+        } else {
+            None
+        };
 
         Box::pin(self.service.call(sreq).and_then(move |mut sresp| {
             // handed an actix_error::error::Error;
@@ -151,9 +156,11 @@ where
                 }
                 Some(e) => {
                     if let Some(apie) = e.as_error::<ApiError>() {
-                        if let Some(state) = sresp.request().app_data::<Data<ServerState>>() {
-                            apie.on_response(state.as_ref());
-                        };
+                        if let Some(metrics) = metrics {
+                            if let Some(label) = apie.kind().metric_label() {
+                                metrics.incr(&label);
+                            }
+                        }
                         if !apie.is_reportable() {
                             trace!("Sentry: Not reporting error: {:?}", apie);
                             return future::ok(sresp);
diff --git a/src/web/tags.rs b/src/web/tags.rs
index 12aa19b0..df352664 100644
--- a/src/web/tags.rs
+++ b/src/web/tags.rs
@@ -172,7 +172,6 @@ impl From<Tags> for BTreeMap<String, String> {
         for (k, v) in tags.tags {
             result.insert(k.clone(), v.clone());
         }
-
         result
     }
 }