bug: add tag info to sentry error messages

Cargo.toml

@@ -43,6 +43,7 @@ mozsvc-common = "0.1"
 num_cpus = "1.11"
 # must match what's used by googleapis-raw
 protobuf = "2.7.0"
+openssl ="0.10"
 rand = "0.7"
 regex = "1.3"
 sentry = { version = "0.17.0", features = ["with_curl_transport"] }

README.md

@@ -34,7 +34,10 @@ Mozilla Sync Storage built with [Rust](https://rust-lang.org).
 - Pkg-config
 - Openssl
 
-Depending on your OS, you may also need to install `libgrpcdev`, and `protobuf-compiler-grpc`.
+Depending on your OS, you may also need to install `libgrpcdev`,
+`libcurl4-openssl-dev`, and `protobuf-compiler-grpc`. *Note*: if the
+code complies cleanly, but generates a Segmentation Fault within
+Sentry init, you probably are missing `libcurl4-openssl-dev`.
 
 ## Local Setup
 

db-tests/src/batch.rs

@@ -55,7 +55,11 @@ async fn create_delete() -> Result<()> {
     let uid = 1;
     let coll = "clients";
     let id = db.create_batch(cb(uid, coll, vec![])).compat().await?;
-    assert!(db.validate_batch(vb(uid, coll, id.clone())).compat().await?);
+    assert!(
+        db.validate_batch(vb(uid, coll, id.clone()))
+            .compat()
+            .await?
+    );
 
     db.delete_batch(params::DeleteBatch {
         user_id: hid(uid),
@@ -77,7 +81,11 @@ async fn expiry() -> Result<()> {
     let id = with_delta!(db, -(BATCH_LIFETIME + 11), {
         db.create_batch(cb(uid, coll, vec![])).compat().await
     })?;
-    assert!(!db.validate_batch(vb(uid, coll, id.clone())).compat().await?);
+    assert!(
+        !db.validate_batch(vb(uid, coll, id.clone()))
+            .compat()
+            .await?
+    );
     let result = db.get_batch(gb(uid, coll, id.clone())).compat().await?;
     assert!(result.is_none());
 
@@ -101,15 +109,21 @@ async fn update() -> Result<()> {
     let uid = 1;
     let coll = "clients";
     let id = db.create_batch(cb(uid, coll, vec![])).compat().await?;
-    assert!(db.get_batch(gb(uid, coll, id.clone())).compat().await?.is_some());
+    assert!(db
+        .get_batch(gb(uid, coll, id.clone()))
+        .compat()
+        .await?
+        .is_some());
     // XXX: now bogus under spanner
     //assert_eq!(batch.bsos, "".to_owned());
 
     let bsos = vec![
         postbso("b0", Some("payload 0"), Some(10), None),
         postbso("b1", Some("payload 1"), Some(1_000_000_000), None),
     ];
-    db.append_to_batch(ab(uid, coll, id.clone(), bsos)).compat().await?;
+    db.append_to_batch(ab(uid, coll, id.clone(), bsos))
+        .compat()
+        .await?;
 
     assert!(db.get_batch(gb(uid, coll, id)).compat().await?.is_some());
     // XXX: now bogus under spanner

db-tests/src/support.rs

@@ -4,8 +4,8 @@ use futures::compat::Future01CompatExt;
 use syncstorage::{
     db::{params, util::SyncTimestamp, Db, Sorting},
     error::ApiError,
-    settings::{Secrets, ServerLimits, Settings},
     server::metrics,
+    settings::{Secrets, ServerLimits, Settings},
     web::extractors::{BsoQueryParams, HawkIdentifier},
 };
 

src/db/error.rs

@@ -81,13 +81,7 @@ impl From<Context<DbErrorKind>> for DbError {
             _ => StatusCode::INTERNAL_SERVER_ERROR,
         };
 
-        let error = Self { inner, status };
-
-        if status == StatusCode::INTERNAL_SERVER_ERROR {
-            sentry::integrations::failure::capture_fail(&error);
-        }
-
-        error
+        Self { inner, status }
     }
 }
 

src/db/spanner/models.rs

@@ -1266,8 +1266,7 @@ impl SpannerDb {
                 } else {
                     "".to_string()
                 }
-            )
-            .to_string();
+            );
             q = format!(
                 "{}{}",
                 q,
@@ -1279,8 +1278,7 @@ impl SpannerDb {
                 } else {
                     "".to_string()
                 }
-            )
-            .to_string();
+            );
             q = format!(
                 "{}{}",
                 q,
@@ -1291,8 +1289,7 @@ impl SpannerDb {
                 } else {
                     "".to_string()
                 }
-            )
-            .to_string();
+            );
             q = format!(
                 "{}{}",
                 q,
@@ -1302,8 +1299,7 @@ impl SpannerDb {
                 } else {
                     "".to_string()
                 }
-            )
-            .to_string();
+            );
 
             if q.is_empty() {
                 // Nothing to update

src/error.rs

@@ -111,7 +111,12 @@ impl ApiError {
     fn weave_error_code(&self) -> WeaveError {
         match self.kind() {
             ApiErrorKind::Validation(ver) => match ver.kind() {
-                ValidationErrorKind::FromDetails(ref description, ref location, name) => {
+                ValidationErrorKind::FromDetails(
+                    ref description,
+                    ref location,
+                    name,
+                    ref _tags,
+                ) => {
                     if description == "size-limit-exceeded" {
                         return WeaveError::SizeLimitExceeded;
                     }
@@ -123,7 +128,7 @@ impl ApiError {
                     }
                     WeaveError::UnknownError
                 }
-                ValidationErrorKind::FromValidationErrors(ref _err, ref location) => {
+                ValidationErrorKind::FromValidationErrors(ref _err, ref location, ref _tags) => {
                     if *location == RequestErrorLocation::Body {
                         WeaveError::InvalidWbo
                     } else {

src/main.rs

@@ -34,7 +34,8 @@ fn main() -> Result<(), Box<dyn Error>> {
     // Avoid its default reqwest transport for now due to issues w/
     // likely grpcio's boringssl
     let curl_transport_factory = |options: &sentry::ClientOptions| {
-        Box::new(sentry::transports::CurlHttpTransport::new(options))
+        // Note: set options.debug = true when diagnosing sentry issues.
+        Box::new(sentry::transports::CurlHttpTransport::new(&options))
             as Box<dyn sentry::internals::Transport>
     };
     let sentry = sentry::init(sentry::ClientOptions {

src/server/metrics.rs

@@ -1,24 +1,21 @@
-use std::collections::HashMap;
 use std::net::UdpSocket;
 use std::time::Instant;
 
-use actix_web::{error::ErrorInternalServerError, http, Error, HttpRequest};
+use actix_web::{error::ErrorInternalServerError, Error, HttpRequest};
 use cadence::{
     BufferedUdpMetricSink, Counted, Metric, NopMetricSink, QueuingMetricSink, StatsdClient, Timed,
 };
 
 use crate::error::ApiError;
-use crate::server::user_agent::parse_user_agent;
 use crate::server::ServerState;
 use crate::settings::Settings;
-
-pub type Tags = HashMap<String, String>;
+use crate::web::tags::Tags;
 
 #[derive(Debug, Clone)]
 pub struct MetricTimer {
     pub label: String,
     pub start: Instant,
-    pub tags: Option<Tags>,
+    pub tags: Tags,
 }
 
 #[derive(Debug, Clone)]
@@ -30,14 +27,20 @@ pub struct Metrics {
 
 impl Drop for Metrics {
     fn drop(&mut self) {
+        let tags = self.tags.clone().unwrap_or_default();
         if let Some(client) = self.client.as_ref() {
             if let Some(timer) = self.timer.as_ref() {
                 let lapse = (Instant::now() - timer.start).as_millis() as u64;
-                trace!("⌚ Ending timer at nanos: {:?} : {:?}", &timer.label, lapse);
+                trace!("⌚ Ending timer at nanos: {:?} : {:?}", &timer.label, lapse;
+                "ua.os.family" => tags.get("ua.os.family"),
+                "ua.browser.family" => tags.get("ua.browser.family"),
+                "ua.name" => tags.get("ua.name"),
+                "ua.os.ver" => tags.get("ua.os.ver"),
+                "ua.browser.ver" => tags.get("ua.browser.ver"));
                 let mut tagged = client.time_with_tags(&timer.label, lapse);
                 // Include any "hard coded" tags.
                 // tagged = tagged.with_tag("version", env!("CARGO_PKG_VERSION"));
-                let tags = timer.tags.clone().unwrap_or_default();
+                let tags = timer.tags.tags.clone();
                 let keys = tags.keys();
                 for tag in keys {
                     tagged = tagged.with_tag(tag, &tags.get(tag).unwrap())
@@ -58,11 +61,9 @@ impl Drop for Metrics {
 
 impl From<&HttpRequest> for Metrics {
     fn from(req: &HttpRequest) -> Self {
-        let ua = req.headers().get(http::header::USER_AGENT);
-        let mut tags: Option<Tags> = None;
-        if let Some(ua_string) = ua {
-            tags = Some(Self::default_tags(ua_string.to_str().unwrap_or("")));
-        }
+        let exts = req.extensions();
+        let def_tags = Tags::from_request_head(req.head());
+        let tags = exts.get::<Tags>().unwrap_or_else(|| &def_tags);
         Metrics {
             client: match req.app_data::<ServerState>() {
                 Some(v) => Some(*v.metrics.clone()),
@@ -71,7 +72,7 @@ impl From<&HttpRequest> for Metrics {
                     None
                 }
             },
-            tags,
+            tags: Some(tags.clone()),
             timer: None,
         }
     }
@@ -102,22 +103,6 @@ impl Metrics {
         StatsdClient::builder("", NopMetricSink).build()
     }
 
-    pub fn default_tags(user_agent: &str) -> Tags {
-        let mut tags = Tags::new();
-
-        let (ua_result, metrics_os, metrics_browser) = parse_user_agent(user_agent);
-
-        tags.insert("ua.os.family".to_owned(), metrics_os.to_owned());
-        tags.insert("ua.browser.family".to_owned(), metrics_browser.to_owned());
-        tags.insert("ua.name".to_owned(), ua_result.name.to_owned());
-        tags.insert(
-            "ua.os.ver".to_owned(),
-            ua_result.os_version.to_owned().to_string(),
-        );
-        tags.insert("ua.browser.ver".to_owned(), ua_result.version.to_owned());
-        tags
-    }
-
     pub fn noop() -> Self {
         Self {
             client: Some(Self::sink()),
@@ -129,13 +114,19 @@ impl Metrics {
     pub fn start_timer(&mut self, label: &str, tags: Option<Tags>) {
         let mut mtags = self.tags.clone().unwrap_or_default();
         if let Some(t) = tags {
-            mtags.extend(t)
+            mtags.extend(t.tags)
         }
-        trace!("⌚ Starting timer... {:?}", &label);
+
+        trace!("⌚ Starting timer... {:?}", &label;
+            "ua.os.family" => mtags.get("ua.os.family"),
+            "ua.browser.family" => mtags.get("ua.browser.family"),
+            "ua.name" => mtags.get("ua.name"),
+            "ua.os.ver" => mtags.get("ua.os.ver"),
+            "ua.browser.ver" => mtags.get("ua.browser.ver"));
         self.timer = Some(MetricTimer {
             label: label.to_owned(),
             start: Instant::now(),
-            tags: if !mtags.is_empty() { Some(mtags) } else { None },
+            tags: mtags,
         });
     }
 
@@ -147,18 +138,27 @@ impl Metrics {
     pub fn incr_with_tags(self, label: &str, tags: Option<Tags>) {
         if let Some(client) = self.client.as_ref() {
             let mut tagged = client.incr_with_tags(label);
-            let mut mtags = self.tags.clone().unwrap_or_default();
-            mtags.extend(tags.unwrap_or_default());
-            let keys = mtags.keys();
-            for tag in keys {
-                tagged = tagged.with_tag(tag, &mtags.get(tag).unwrap())
+            let mut mtags = self.tags.clone().unwrap_or_default().tags;
+            if let Some(t) = tags {
+                mtags.extend(t.tags)
+            }
+            let tag_keys = mtags.keys();
+            for key in tag_keys.clone() {
+                // REALLY wants a static here, or at least a well defined ref.
+                tagged = tagged.with_tag(&key, &mtags.get(key).unwrap());
             }
             // Include any "hard coded" tags.
             // incr = incr.with_tag("version", env!("CARGO_PKG_VERSION"));
             match tagged.try_send() {
                 Err(e) => {
                     // eat the metric, but log the error
-                    debug!("⚠️ Metric {} error: {:?} ", label, e);
+                    debug!("⚠️ Metric {} error: {:?} ", label, e;
+                        "ua.os.family" => mtags.get("ua.os.family"),
+                        "ua.browser.family" => mtags.get("ua.browser.family"),
+                        "ua.name" => mtags.get("ua.name"),
+                        "ua.os.ver" => mtags.get("ua.os.ver"),
+                        "ua.browser.ver" => mtags.get("ua.browser.ver")
+                    );
                 }
                 Ok(v) => trace!("☑️ {:?}", v.as_metric_str()),
             }
@@ -201,16 +201,31 @@ mod tests {
 
     #[test]
     fn test_tags() {
-        let tags = Metrics::default_tags(
-            "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0",
+        use actix_web::dev::RequestHead;
+        use actix_web::http::{header, uri::Uri};
+        use std::collections::HashMap;
+
+        let mut rh = RequestHead::default();
+        let path = "/1.5/42/storage/meta/global";
+        rh.uri = Uri::from_static(path);
+        rh.headers.insert(
+            header::USER_AGENT,
+            header::HeaderValue::from_static(
+                "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0",
+            ),
         );
+
+        let tags = Tags::from_request_head(&rh);
+
         let mut result = HashMap::<String, String>::new();
         result.insert("ua.os.ver".to_owned(), "NT 10.0".to_owned());
         result.insert("ua.os.family".to_owned(), "Windows".to_owned());
         result.insert("ua.browser.ver".to_owned(), "72.0".to_owned());
         result.insert("ua.name".to_owned(), "Firefox".to_owned());
         result.insert("ua.browser.family".to_owned(), "Firefox".to_owned());
+        result.insert("uri.path".to_owned(), path.to_owned());
+        result.insert("uri.method".to_owned(), "GET".to_owned());
 
-        assert_eq!(tags, result)
+        assert_eq!(tags.tags, result)
     }
 }

src/server/mod.rs

@@ -67,6 +67,7 @@ macro_rules! build_app {
             .wrap(middleware::PreConditionCheck::new())
             .wrap(middleware::DbTransaction::new())
             .wrap(middleware::WeaveTimestamp::new())
+            .wrap(middleware::SentryWrapper::new())
             // Followed by the "official middleware" so they run first.
             .wrap(Cors::default())
             .service(
@@ -148,6 +149,7 @@ macro_rules! build_app {
                         .body(include_str!("../../version.json"))
                 })),
             )
+            .service(web::resource("/__error__").route(web::get().to_async(handlers::test_error)))
     };
 }
 

src/server/test.rs

@@ -55,7 +55,7 @@ fn get_test_settings() -> Settings {
         debug: true,
         port,
         host,
-        database_url: settings.database_url.clone(),
+        database_url: settings.database_url,
         database_pool_max_size: Some(pool_size + 1),
         database_use_test_transactions: true,
         limits: ServerLimits::default(),
@@ -113,7 +113,7 @@ fn create_hawk_header(method: &str, port: u16, path: &str) -> String {
     let host = TEST_HOST;
     let payload = HawkPayload {
         expires: (Utc::now().timestamp() + 5) as f64,
-        node: format!("http://{}:{}", host, port).to_string(),
+        node: format!("http://{}:{}", host, port),
         salt: "wibble".to_string(),
         user_id: 42,
         fxa_uid: "xxx_test".to_owned(),