feat: add tags to ADM errors. (#385)

* feat: add tags to ADM errors.

Closes #384

src/adm/filter.rs

@@ -408,7 +408,7 @@ impl AdmFilter {
                     return None;
                 }
                 // match to the version that we switched over from built in image management
-                // to CDN image fetch. Note: iOS does not use the standard firefox version number
+                // to CDN image fetch.
 
                 if device_info.legacy_only()
                     && !self.legacy_list.contains(&tile.name.to_lowercase())

src/adm/tiles.rs

@@ -213,7 +213,7 @@ pub async fn get_tiles(
                     // We still want to track this as a server error later.
                     //
                     // TODO: Remove this after the shared cache is implemented.
-                    let mut err: HandlerError = if e.is_timeout()
+                    let err: HandlerError = if e.is_timeout()
                         && Instant::now()
                             .checked_duration_since(state.start_up)
                             .unwrap_or_else(|| Duration::from_secs(0))
@@ -224,18 +224,22 @@ pub async fn get_tiles(
                         HandlerErrorKind::AdmServerError().into()
                     };
                     // ADM servers are down, or improperly configured
-                    err.tags.add_extra("error", &e.to_string());
+                    // be sure to write the error to the provided mut tags.
+                    tags.add_extra("error", &e.to_string());
                     err
                 })?
                 .error_for_status()?
                 .json()
                 .await
                 .map_err(|e| {
                     // ADM servers are not returning correct information
-                    HandlerErrorKind::BadAdmResponse(format!(
+                    let err: HandlerError = HandlerErrorKind::BadAdmResponse(format!(
                         "ADM provided invalid response: {:?}",
                         e
                     ))
+                    .into();
+                    tags.add_extra("error", &e.to_string());
+                    err
                 })?
         }
     };

src/web/handlers.rs

@@ -82,14 +82,15 @@ pub async fn get_tiles(
         legacy_only: device_info.legacy_only(),
     };
 
-    let mut tags = Tags::default();
+    let mut tags = Tags::from_head(request.head(), settings);
     {
         tags.add_extra("audience_key", &format!("{:#?}", audience_key));
         // Add/modify the existing request tags.
         // tags.clone().commit(&mut request.extensions_mut());
     }
 
     let mut expired = false;
+
     if settings.test_mode != crate::settings::TestModes::TestFakeResponse {
         // First make a cheap read from the cache
         if let Some(tiles_state) = state.tiles_cache.get(&audience_key) {
@@ -168,19 +169,24 @@ pub async fn get_tiles(
             // Add some kind of stats to Retrieving or RetrievingFirst?
             // do we need a kill switch if we're restricting like this already?
             match e.kind() {
-                HandlerErrorKind::BadAdmResponse(es) => {
+                // Handle a bad response from ADM specially.
+                // Report it to metrics and sentry, but also store an empty record
+                // into the cache so that we don't stampede the ADM servers.
+                HandlerErrorKind::BadAdmResponse(_es) => {
                     warn!("Bad response from ADM: {:?}", e);
+                    // Merge in the error tags, which should already include the
+                    // error string as `error`
+                    tags.extend(e.tags.clone());
+                    tags.add_tag("level", "warning");
                     metrics.incr_with_tags("tiles.invalid", Some(&tags));
+                    // write an empty tile set into the cache for this result.
                     handle.insert(TilesState::Fresh {
                         tiles: Tiles::empty(add_jitter(&state.settings)),
                     });
-                    // Report directly to sentry
-                    // (This is starting to become a pattern. 🤔)
-                    let mut tags = Tags::from_head(request.head(), settings);
-                    tags.add_extra("err", es);
-                    tags.add_tag("level", "warning");
+                    // Report the error directly to sentry
                     l_sentry::report(sentry::event_from_error(&e), &tags);
                     warn!("ADM Server error: {:?}", e);
+                    // Return a 204 to the client.
                     Ok(HttpResponse::NoContent().finish())
                 }
                 _ => Err(e),

src/web/test.rs

@@ -28,7 +28,7 @@ const UA_91: &str =
 const UA_90: &str =
     "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/90.0";
 const UA_IPHONE: &str =
-    "Mozilla/5.0 (iPhone; CPU iPhone OS 14_8_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) FxiOS/40.2 Mobile/15E148 Safari/605.1.15";
+    "Mozilla/5.0 (iPhone; CPU iPhone OS 14_8_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) FxiOS/91.0 Mobile/15E148 Safari/605.1.15";
 const MMDB_LOC: &str = "mmdb/GeoLite2-City-Test.mmdb";
 const TEST_ADDR: &str = "216.160.83.56";
 

src/web/user_agent.rs

@@ -54,9 +54,11 @@ pub struct DeviceInfo {
 }
 
 impl DeviceInfo {
+    /// "Legacy" means that it can only display tiles that are available from
+    /// remote settings. Currently, that's limited to just desktop devices that
+    /// are before v. 91
     pub fn legacy_only(&self) -> bool {
-        self.os_family == OsFamily::IOs && self.ff_version < 36
-            || self.os_family != OsFamily::IOs && self.ff_version < 91
+        matches!(self.form_factor, FormFactor::Desktop | FormFactor::Other) && self.ff_version < 91
     }
 
     /// Determine if the device is a mobile phone based on either the form factor or OS.