Merge branch 'master' into release/0.17

Author: pjenvey

syncserver-common/src/lib.rs

@@ -130,18 +130,18 @@ impl BlockingThreadpool {
         T: Send + 'static,
         E: fmt::Debug + Send + InternalError + 'static,
     {
-        self.spawned_tasks.fetch_add(1, Ordering::Relaxed);
+        self.spawned_tasks.fetch_add(1, Ordering::SeqCst);
         // Ensure the counter's always decremented (whether the task completed,
         // was cancelled or panicked)
         scopeguard::defer! {
-            self.spawned_tasks.fetch_sub(1, Ordering::Relaxed);
+            self.spawned_tasks.fetch_sub(1, Ordering::SeqCst);
         }
 
         let active_threads = Arc::clone(&self.active_threads);
         let f_with_metrics = move || {
-            active_threads.fetch_add(1, Ordering::Relaxed);
+            active_threads.fetch_add(1, Ordering::SeqCst);
             scopeguard::defer! {
-               active_threads.fetch_sub(1, Ordering::Relaxed);
+               active_threads.fetch_sub(1, Ordering::SeqCst);
             }
             f()
         };
@@ -154,8 +154,11 @@ impl BlockingThreadpool {
 
     /// Return the pool's current metrics
     pub fn metrics(&self) -> BlockingThreadpoolMetrics {
-        let spawned_tasks = self.spawned_tasks.load(Ordering::Relaxed);
-        let active_threads = self.active_threads.load(Ordering::Relaxed);
+        // active_threads is decremented on a separate thread so we need a
+        // strong Ordering to ensure it's in sync w/ spawned_tasks (otherwise
+        // it could underflow queued_tasks)
+        let spawned_tasks = self.spawned_tasks.load(Ordering::SeqCst);
+        let active_threads = self.active_threads.load(Ordering::SeqCst);
         BlockingThreadpoolMetrics {
             queued_tasks: spawned_tasks - active_threads,
             active_threads,

syncserver-common/src/middleware/sentry.rs

@@ -14,15 +14,13 @@ use crate::{ReportableError, Taggable};
 #[derive(Clone)]
 pub struct SentryWrapper<E> {
     metrics: Arc<StatsdClient>,
-    metric_label: String,
     phantom: PhantomData<E>,
 }
 
 impl<E> SentryWrapper<E> {
-    pub fn new(metrics: Arc<StatsdClient>, metric_label: String) -> Self {
+    pub fn new(metrics: Arc<StatsdClient>) -> Self {
         Self {
             metrics,
-            metric_label,
             phantom: PhantomData,
         }
     }
@@ -44,7 +42,6 @@ where
         ok(SentryWrapperMiddleware {
             service: Rc::new(RefCell::new(service)),
             metrics: self.metrics.clone(),
-            metric_label: self.metric_label.clone(),
             phantom: PhantomData,
         })
     }
@@ -54,7 +51,6 @@ where
 pub struct SentryWrapperMiddleware<S, E> {
     service: Rc<RefCell<S>>,
     metrics: Arc<StatsdClient>,
-    metric_label: String,
     phantom: PhantomData<E>,
 }
 
@@ -81,7 +77,6 @@ where
 
         // get the tag information
         let metrics = self.metrics.clone();
-        let metric_label = self.metric_label.clone();
         let tags = sreq.get_tags();
         let extras = sreq.get_extras();
 
@@ -100,7 +95,7 @@ where
                             // capture it, and then turn it off before we run out of money.
                             if let Some(label) = reportable_err.metric_label() {
                                 debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
-                                let _ = metrics.incr(&format!("{}.{}", metric_label, label));
+                                let _ = metrics.incr(&label);
                             }
                             debug!("Sentry: Not reporting error (service error): {:?}", error);
                             return Err(error);
@@ -122,7 +117,7 @@ where
                     if !reportable_err.is_sentry_event() {
                         if let Some(label) = reportable_err.metric_label() {
                             debug!("Sentry: Sending error to metrics: {:?}", reportable_err);
-                            let _ = metrics.incr(&format!("{}.{}", metric_label, label));
+                            let _ = metrics.incr(&label);
                         }
                         debug!("Not reporting error (service error): {:?}", error);
                         return Ok(response);

syncserver/src/server/mod.rs

@@ -88,10 +88,7 @@ macro_rules! build_app {
             // These will wrap all outbound responses with matching status codes.
             .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404))
             // These are our wrappers
-            .wrap(SentryWrapper::<ApiError>::new(
-                $metrics.clone(),
-                "api_error".to_owned(),
-            ))
+            .wrap(SentryWrapper::<ApiError>::new($metrics.clone()))
             .wrap_fn(middleware::weave::set_weave_timestamp)
             .wrap_fn(tokenserver::logging::handle_request_log_line)
             .wrap_fn(middleware::rejectua::reject_user_agent)
@@ -199,7 +196,6 @@ macro_rules! build_app_without_syncstorage {
             .wrap(ErrorHandlers::new().handler(StatusCode::NOT_FOUND, ApiError::render_404))
             .wrap(SentryWrapper::<tokenserver_common::TokenserverError>::new(
                 $metrics.clone(),
-                "api_error".to_owned(),
             ))
             // These are our wrappers
             .wrap_fn(tokenserver::logging::handle_request_log_line)

syncserver/src/web/handlers.rs

@@ -3,7 +3,12 @@ use std::collections::HashMap;
 use std::convert::Into;
 use std::time::{Duration, Instant};
 
-use actix_web::{http::StatusCode, web::Data, HttpRequest, HttpResponse, HttpResponseBuilder};
+use crate::server::user_agent::get_device_info;
+use actix_web::{
+    http::{header, StatusCode},
+    web::Data,
+    HttpRequest, HttpResponse, HttpResponseBuilder,
+};
 use serde::Serialize;
 use serde_json::{json, Value};
 use syncserver_common::{X_LAST_MODIFIED, X_WEAVE_NEXT_OFFSET, X_WEAVE_RECORDS};
@@ -32,6 +37,18 @@ pub async fn get_collections(
     db_pool: DbTransactionPool,
     request: HttpRequest,
 ) -> Result<HttpResponse, ApiError> {
+    // The values below, prefixed by `_`, are temporarily and intentionally ignored at present.
+    // They will be passed to the Glean logic we will implement to emit metrics.
+    // We'd like for the data to be ready and in place to pass to that logic.
+    let _hashed_fxa_uid: String = meta.user_id.hashed_fxa_uid.clone();
+    let _hashed_device_id: String = meta.user_id.hashed_device_id.clone();
+    let user_agent = request
+        .headers()
+        .get(header::USER_AGENT)
+        .and_then(|header| header.to_str().ok())
+        .unwrap_or("none");
+    let _device_info = get_device_info(user_agent);
+
     db_pool
         .transaction_http(request, |db| async move {
             meta.emit_api_metric("request.get_collections");