Feat/21 extra (#23)

* feat: Add template for setting HttpRequire tags

Add convenience functions for adding tag values

Closes #21

src/metrics.rs

@@ -57,7 +57,7 @@ impl Drop for Metrics {
 impl From<&HttpRequest> for Metrics {
     fn from(req: &HttpRequest) -> Self {
         let exts = req.extensions();
-        let def_tags = Tags::from_request_head(req.head());
+        let def_tags = Tags::from(req.head());
         let tags = exts.get::<Tags>().unwrap_or(&def_tags);
         Metrics {
             client: match req.app_data::<Data<ServerState>>() {
@@ -109,7 +109,7 @@ 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.tags)
+            mtags.extend(t)
         }
 
         trace!("⌚ Starting timer... {:?}", &label; &mtags);
@@ -130,7 +130,7 @@ impl Metrics {
             let mut tagged = client.incr_with_tags(label);
             let mut mtags = self.tags.clone().unwrap_or_default();
             if let Some(tags) = tags {
-                mtags.extend(tags.tags);
+                mtags.extend(tags);
             }
             for key in mtags.tags.keys().clone() {
                 if let Some(val) = mtags.tags.get(key) {
@@ -158,7 +158,7 @@ impl Metrics {
             let mut tagged = client.count_with_tags(label, count);
             let mut mtags = self.tags.clone().unwrap_or_default();
             if let Some(tags) = tags {
-                mtags.extend(tags.tags);
+                mtags.extend(tags);
             }
             for key in mtags.tags.keys().clone() {
                 if let Some(val) = mtags.tags.get(key) {
@@ -232,7 +232,7 @@ mod tests {
             ),
         );
 
-        let tags = Tags::from_request_head(&rh);
+        let tags = Tags::from(&rh);
 
         let mut result = HashMap::<String, String>::new();
         result.insert("ua.os.ver".to_owned(), "NT 10.0".to_owned());
@@ -258,7 +258,7 @@ mod tests {
             header::HeaderValue::from_static("Mozilla/5.0 (curl) Gecko/20100101 curl"),
         );
 
-        let tags = Tags::from_request_head(&rh);
+        let tags = Tags::from(&rh);
         assert!(!tags.tags.contains_key("ua.os.ver"));
         println!("{:?}", tags);
     }

src/tags.rs

@@ -1,5 +1,7 @@
+use core::cell::RefMut;
 use std::collections::{BTreeMap, HashMap};
 
+use actix_http::Extensions;
 use actix_web::{
     dev::{Payload, RequestHead},
     http::header::USER_AGENT,
@@ -26,6 +28,10 @@ const VALID_UA_BROWSER: &[&str] = &["Chrome", "Firefox", "Safari", "Opera"];
 // field). Windows has many values and we only care that its Windows
 const VALID_UA_OS: &[&str] = &["Firefox OS", "Linux", "Mac OSX"];
 
+/// Primative User Agent parser.
+///
+/// We only care about a subset of the results for this (to avoid cardinality with
+/// metrics and logging).
 pub fn parse_user_agent(agent: &str) -> (WootheeResult<'_>, &str, &str) {
     let parser = Parser::new();
     let wresult = parser.parse(&agent).unwrap_or_else(|| WootheeResult {
@@ -84,23 +90,15 @@ impl Serialize for Tags {
     }
 }
 
+/// Insert a into a hashmap only if the `val` is not empty.
 fn insert_if_not_empty(label: &str, val: &str, tags: &mut HashMap<String, String>) {
     if !val.is_empty() {
         tags.insert(label.to_owned(), val.to_owned());
     }
 }
 
-// Tags are extra data to be recorded in metric and logging calls.
-// If additional tags are required or desired, you will need to add them to the
-// mutable extensions, e.g.
-// ```
-//      let mut tags = request.extensions_mut().get::<Tags>();
-//      tags.insert("SomeLabel".to_owned(), "whatever".to_owned());
-// ```
-// how you get the request (or the response, and it's set of `extensions`) to whatever
-// function requires it, is left as an exercise for the reader.
-impl Tags {
-    pub fn from_request_head(req_head: &RequestHead) -> Tags {
+impl From<&RequestHead> for Tags {
+    fn from(req_head: &RequestHead) -> Self {
         // Return an Option<> type because the later consumers (HandlerErrors) presume that
         // tags are optional and wrapped by an Option<> type.
         let mut tags = HashMap::new();
@@ -123,7 +121,29 @@ impl Tags {
         extra.insert("uri.path".to_owned(), req_head.uri.to_string());
         Tags { tags, extra }
     }
+}
+
+impl From<HttpRequest> for Tags {
+    fn from(request: HttpRequest) -> Self {
+        match request.extensions().get::<Self>() {
+            Some(v) => v.clone(),
+            None => Tags::from(request.head()),
+        }
+    }
+}
 
+/// Tags are extra data to be recorded in metric and logging calls.
+/// If additional tags are required or desired, you will need to add them to the
+/// mutable extensions, e.g.
+/// ```compile_fail
+///      use contile::tags::Tags;
+///
+///      let mut tags = Tags::default();
+///      tags.add_tag("SomeLabel", "whatever");
+///      tags.commit(&mut request.extensions_mut());
+/// ```
+impl Tags {
+    /// Generate a new Tag struct from a Hash of values.
     pub fn with_tags(tags: HashMap<String, String>) -> Tags {
         if tags.is_empty() {
             return Tags::default();
@@ -134,15 +154,42 @@ impl Tags {
         }
     }
 
+    /// Add an element to the "extra" data.
+    ///
+    /// Extra data is non-key storage used by sentry. It is not
+    /// distributed to metrics.
+    pub fn add_extra(&mut self, key: &str, value: &str) {
+        if !value.is_empty() {
+            self.extra.insert(key.to_owned(), value.to_owned());
+        }
+    }
+
+    /// Add an element to the "tag" data.
+    ///
+    /// Tag data is keyed info. Be careful to not include too many
+    /// unique values here otherwise you run the risk of excess
+    /// cardinality.
+    pub fn add_tag(&mut self, key: &str, value: &str) {
+        if !value.is_empty() {
+            self.tags.insert(key.to_owned(), value.to_owned());
+        }
+    }
+
+    /// Get a tag value.
     pub fn get(&self, label: &str) -> String {
         let none = "None".to_owned();
         self.tags.get(label).map(String::from).unwrap_or(none)
     }
 
-    pub fn extend(&mut self, tags: HashMap<String, String>) {
-        self.tags.extend(tags);
+    /// Extend the current tag set using another tag set.
+    ///
+    /// Useful for collating tags before distribution.
+    pub fn extend(&mut self, tags: Self) {
+        self.tags.extend(tags.tags);
+        self.extra.extend(tags.extra);
     }
 
+    /// Convert tag hash to a Binary Tree map (used by cadence and sentry)
     pub fn tag_tree(self) -> BTreeMap<String, String> {
         let mut result = BTreeMap::new();
 
@@ -152,6 +199,7 @@ impl Tags {
         result
     }
 
+    /// Convert extra hash to a Binary Tree map (used by cadence and sentry)
     pub fn extra_tree(self) -> BTreeMap<String, Value> {
         let mut result = BTreeMap::new();
 
@@ -160,6 +208,18 @@ impl Tags {
         }
         result
     }
+
+    /// Write the current tag info to the Extensions.
+    ///
+    /// Actix provides extensions for requests and responses. These allow
+    /// for arbitrary data to be stored, however note that these are
+    /// separate, and that `response.request()` is not `request`.
+    pub fn commit(self, exts: &mut RefMut<'_, Extensions>) {
+        match exts.get_mut::<Tags>() {
+            Some(t) => t.extend(self),
+            None => exts.insert(self),
+        }
+    }
 }
 
 impl FromRequest for Tags {
@@ -172,7 +232,7 @@ impl FromRequest for Tags {
             let exts = req.extensions();
             match exts.get::<Tags>() {
                 Some(t) => t.clone(),
-                None => Tags::from_request_head(req.head()),
+                None => Tags::from(req.head()),
             }
         };
 

src/web/handlers.rs

@@ -1,7 +1,7 @@
 //! API Handlers
 use std::collections::HashMap;
 
-use actix_web::{web, Error, HttpResponse};
+use actix_web::{web, Error, HttpRequest, HttpResponse};
 use serde_json::Value;
 
 use super::user_agent;
@@ -10,13 +10,15 @@ use crate::{
     error::HandlerError,
     metrics::Metrics,
     server::{cache, ServerState},
+    tags::Tags,
     web::extractors::TilesRequest,
 };
 
 pub async fn get_tiles(
     treq: TilesRequest,
     metrics: Metrics,
     state: web::Data<ServerState>,
+    request: HttpRequest,
 ) -> Result<HttpResponse, HandlerError> {
     trace!("get_tiles");
 
@@ -30,6 +32,16 @@ pub async fn get_tiles(
     };
     let stripped_ua = user_agent::strip_ua(&treq.ua);
 
+    {
+        // for demonstration purposes
+        let mut tags = Tags::default();
+        tags.add_extra("ip", fake_ip.as_str());
+        tags.add_extra("ua", &stripped_ua);
+        tags.add_extra("sub2", &treq.placement);
+        // Add/modify the existing request tags.
+        tags.commit(&mut request.extensions_mut());
+    }
+
     let audience_key = cache::AudienceKey {
         country: treq.country,
         fake_ip: fake_ip.clone(),

src/web/middleware/sentry.rs

@@ -100,7 +100,7 @@ where
     }
 
     fn call(&mut self, sreq: ServiceRequest) -> Self::Future {
-        let mut tags = Tags::from_request_head(sreq.head());
+        let mut tags = Tags::from(sreq.head());
         sreq.extensions_mut().insert(tags.clone());
 
         Box::pin(self.service.call(sreq).and_then(move |mut sresp| {
@@ -109,15 +109,11 @@ where
             // are NOT automatically passed to responses. You need to check both.
             if let Some(t) = sresp.request().extensions().get::<Tags>() {
                 trace!("Sentry: found tags in request: {:?}", &t.tags);
-                for (k, v) in t.tags.clone() {
-                    tags.tags.insert(k, v);
-                }
+                tags.extend(t.clone());
             };
             if let Some(t) = sresp.response().extensions().get::<Tags>() {
                 trace!("Sentry: found tags in response: {:?}", &t.tags);
-                for (k, v) in t.tags.clone() {
-                    tags.tags.insert(k, v);
-                }
+                tags.extend(t.clone());
             };
             match sresp.response().error() {
                 None => {