Bug 1685745 - Upload a ping even if we can't save it to disk (#1576)

* bug 1685745 - Revamp PingMaker to make pings, not just payloads

* bug 1685745 - Try to upload a ping even if we fail to persist to disk

* update CHANGELOG

* rustfmt

* address review feedback

* everything is Ok

* remove obsolete comment, assert Ok(true)

CHANGELOG.md

@@ -15,6 +15,8 @@
     Use `return@measure <val>` for early returns.
 * Python
   * The Glean Python bindings now use rkv's safe mode backend. This should avoid intermittent segfaults in the LMDB backend.
+* General
+  * Attempt to upload a ping even in the face of IO Errors ([#1576](https://github.com/mozilla/glean/pull/1576)).
 
 # v36.0.0 (2021-03-16)
 

glean-core/src/event_database/mod.rs

@@ -227,7 +227,7 @@ impl EventDatabase {
         for store_name in stores_to_submit {
             if let Err(err) = glean.submit_ping_by_name(store_name, Some("max_capacity")) {
                 log::warn!(
-                    "Got more than {} events, but could not send {} ping: {}",
+                    "Got more than {} events, but could not persist {} ping: {}",
                     glean.get_max_events(),
                     store_name,
                     err

glean-core/src/lib.rs

@@ -609,10 +609,6 @@ impl Glean {
     /// # Returns
     ///
     /// Whether the ping was succesfully assembled and queued.
-    ///
-    /// # Errors
-    ///
-    /// If collecting or writing the ping to disk failed.
     pub fn submit_ping(&self, ping: &PingType, reason: Option<&str>) -> Result<bool> {
         if !self.is_upload_enabled() {
             log::info!("Glean disabled: not submitting any pings.");
@@ -622,15 +618,15 @@ impl Glean {
         let ping_maker = PingMaker::new();
         let doc_id = Uuid::new_v4().to_string();
         let url_path = self.make_path(&ping.name, &doc_id);
-        match ping_maker.collect(self, &ping, reason) {
+        match ping_maker.collect(self, &ping, reason, &doc_id, &url_path) {
             None => {
                 log::info!(
                     "No content for ping '{}', therefore no ping queued.",
                     ping.name
                 );
                 Ok(false)
             }
-            Some(content) => {
+            Some(ping) => {
                 // This metric is recorded *after* the ping is collected (since
                 // that is the only way to know *if* it will be submitted). The
                 // implication of this is that the count for a metrics ping will
@@ -640,17 +636,19 @@ impl Glean {
                     .get(&ping.name)
                     .add(&self, 1);
 
-                if let Err(e) = ping_maker.store_ping(
-                    self,
-                    &doc_id,
-                    &ping.name,
-                    &self.get_data_path(),
-                    &url_path,
-                    &content,
-                ) {
-                    log::warn!("IO error while writing ping to file: {}", e);
+                if let Err(e) = ping_maker.store_ping(&self.get_data_path(), &ping) {
+                    log::warn!("IO error while writing ping to file: {}. Enqueuing upload of what we have in memory.", e);
                     self.core_metrics.io_errors.add(self, 1);
-                    return Err(e.into());
+                    let content = ::serde_json::to_string(&ping.content)?;
+                    self.upload_manager.enqueue_ping(
+                        self,
+                        ping.doc_id,
+                        ping.url_path,
+                        &content,
+                        Some(ping.headers),
+                    );
+                    // Not actually 100% 'Ok'. bug 1704606
+                    return Ok(true);
                 }
 
                 self.upload_manager.enqueue_ping_from_file(self, &doc_id);

glean-core/src/lib_unit_tests.rs

@@ -893,7 +893,9 @@ fn records_io_errors() {
 
     // Writing the ping file should fail.
     let submitted = glean.internal_pings.metrics.submit(&glean, None);
-    assert!(submitted.is_err());
+    // But the return value is still Ok(true) because we enqueue the ping anyway.
+    assert!(submitted.is_ok());
+    assert!(submitted.unwrap());
 
     let metric = &glean.core_metrics.io_errors;
     assert_eq!(

glean-core/src/ping/mod.rs

@@ -14,11 +14,26 @@ use serde_json::{json, Value as JsonValue};
 use crate::common_metric_data::{CommonMetricData, Lifetime};
 use crate::metrics::{CounterMetric, DatetimeMetric, Metric, MetricType, PingType, TimeUnit};
 use crate::storage::StorageManager;
+use crate::upload::HeaderMap;
 use crate::util::{get_iso_time_string, local_now_with_offset};
 use crate::{
     Glean, Result, DELETION_REQUEST_PINGS_DIRECTORY, INTERNAL_STORAGE, PENDING_PINGS_DIRECTORY,
 };
 
+/// Holds everything you need to store or send a ping.
+pub struct Ping<'a> {
+    /// The unique document id.
+    pub doc_id: &'a str,
+    /// The ping's name.
+    pub name: &'a str,
+    /// The path on the server to use when uplaoding this ping.
+    pub url_path: &'a str,
+    /// The payload, including `*_info` fields.
+    pub content: JsonValue,
+    /// The headers to upload with the payload.
+    pub headers: HeaderMap,
+}
+
 /// Collect a ping's data, assemble it into its full payload and store it on disk.
 pub struct PingMaker;
 
@@ -160,58 +175,31 @@ impl PingMaker {
         json!(map)
     }
 
-    /// Build the metadata JSON to be persisted with a ping.
+    /// Build the headers to be persisted and sent with a ping.
     ///
-    /// Currently the only type of metadata we need to persist is the value of the `X-Debug-ID` header.
+    /// Currently the only headers we persist are `X-Debug-ID` and `X-Source-Tags`.
     ///
     /// # Arguments
     ///
-    /// * `glean` - the [`Glean`] instance to collect metadata from.
+    /// * `glean` - the [`Glean`] instance to collect headers from.
     ///
     /// # Returns
     ///
-    /// A JSON object representing the metadata that needs to be persisted with this ping.
-    ///
-    /// The structure of the metadata json is:
-    ///
-    /// ```json
-    /// {
-    ///     "headers": {
-    ///         "X-Debug-ID": "test-tag"
-    ///     }
-    /// }
+    /// A map of header names to header values.
+    /// Might be empty if there are no extra headers to send.
     /// ```
-    fn get_metadata(&self, glean: &Glean) -> Option<JsonValue> {
-        let mut headers_map = json!({});
+    fn get_headers(&self, glean: &Glean) -> HeaderMap {
+        let mut headers_map = HeaderMap::new();
 
         if let Some(debug_view_tag) = glean.debug_view_tag() {
-            headers_map
-                .as_object_mut()
-                .unwrap() // safe unwrap, we created the object above
-                .insert(
-                    "X-Debug-ID".to_string(),
-                    JsonValue::String(debug_view_tag.to_string()),
-                );
+            headers_map.insert("X-Debug-ID".to_string(), debug_view_tag.to_string());
         }
 
         if let Some(source_tags) = glean.source_tags() {
-            headers_map
-                .as_object_mut()
-                .unwrap() // safe unwrap, we created the object above
-                .insert(
-                    "X-Source-Tags".to_string(),
-                    JsonValue::String(source_tags.join(",")),
-                );
+            headers_map.insert("X-Source-Tags".to_string(), source_tags.join(","));
         }
 
-        // safe unwrap, we created the object above
-        if !headers_map.as_object().unwrap().is_empty() {
-            Some(json!({
-                "headers": headers_map,
-            }))
-        } else {
-            None
-        }
+        headers_map
     }
 
     /// Collects a snapshot for the given ping from storage and attach required meta information.
@@ -221,17 +209,21 @@ impl PingMaker {
     /// * `glean` - the [`Glean`] instance to collect data from.
     /// * `ping` - the ping to collect for.
     /// * `reason` - an optional reason code to include in the ping.
+    /// * `doc_id` - the ping's unique document identifier.
+    /// * `url_path` - the path on the server to upload this ping to.
     ///
     /// # Returns
     ///
-    /// A fully assembled JSON representation of the ping payload.
+    /// A fully assembled representation of the ping payload and associated metadata.
     /// If there is no data stored for the ping, `None` is returned.
-    pub fn collect(
+    pub fn collect<'a>(
         &self,
         glean: &Glean,
-        ping: &PingType,
+        ping: &'a PingType,
         reason: Option<&str>,
-    ) -> Option<JsonValue> {
+        doc_id: &'a str,
+        url_path: &'a str,
+    ) -> Option<Ping<'a>> {
         info!("Collecting {}", ping.name);
 
         let metrics_data = StorageManager.snapshot_as_json(glean.storage(), &ping.name, true);
@@ -260,7 +252,13 @@ impl PingMaker {
             json_obj.insert("events".to_string(), events_data);
         }
 
-        Some(json)
+        Some(Ping {
+            content: json,
+            name: &ping.name,
+            doc_id,
+            url_path,
+            headers: self.get_headers(glean),
+        })
     }
 
     /// Collects a snapshot for the given ping from storage and attach required meta information.
@@ -281,8 +279,8 @@ impl PingMaker {
         ping: &PingType,
         reason: Option<&str>,
     ) -> Option<String> {
-        self.collect(glean, ping, reason)
-            .map(|ping| ::serde_json::to_string_pretty(&ping).unwrap())
+        self.collect(glean, ping, reason, "", "")
+            .map(|ping| ::serde_json::to_string_pretty(&ping.content).unwrap())
     }
 
     /// Gets the path to a directory for ping storage.
@@ -313,33 +311,30 @@ impl PingMaker {
     }
 
     /// Stores a ping to disk in the pings directory.
-    pub fn store_ping(
-        &self,
-        glean: &Glean,
-        doc_id: &str,
-        ping_name: &str,
-        data_path: &Path,
-        url_path: &str,
-        ping_content: &JsonValue,
-    ) -> std::io::Result<()> {
-        let pings_dir = self.get_pings_dir(data_path, Some(ping_name))?;
+    pub fn store_ping(&self, data_path: &Path, ping: &Ping) -> std::io::Result<()> {
+        let pings_dir = self.get_pings_dir(data_path, Some(ping.name))?;
         let temp_dir = self.get_tmp_dir(data_path)?;
 
         // Write to a temporary location and then move when done,
         // for transactional writes.
-        let temp_ping_path = temp_dir.join(doc_id);
-        let ping_path = pings_dir.join(doc_id);
+        let temp_ping_path = temp_dir.join(ping.doc_id);
+        let ping_path = pings_dir.join(ping.doc_id);
 
-        log::debug!("Storing ping '{}' at '{}'", doc_id, ping_path.display());
+        log::debug!(
+            "Storing ping '{}' at '{}'",
+            ping.doc_id,
+            ping_path.display()
+        );
 
         {
             let mut file = File::create(&temp_ping_path)?;
-            file.write_all(url_path.as_bytes())?;
+            file.write_all(ping.url_path.as_bytes())?;
             file.write_all(b"\n")?;
-            file.write_all(::serde_json::to_string(ping_content)?.as_bytes())?;
-            if let Some(metadata) = self.get_metadata(glean) {
-                file.write_all(b"\n")?;
-                file.write_all(::serde_json::to_string(&metadata)?.as_bytes())?;
+            file.write_all(::serde_json::to_string(&ping.content)?.as_bytes())?;
+            if !ping.headers.is_empty() {
+                file.write_all(b"\n{\"headers\":")?;
+                file.write_all(::serde_json::to_string(&ping.headers)?.as_bytes())?;
+                file.write_all(b"}")?;
             }
         }
 

glean-core/src/upload/mod.rs

@@ -321,7 +321,8 @@ impl PingUploadManager {
         }
     }
 
-    fn enqueue_ping(
+    /// Enqueue a ping for upload.
+    pub fn enqueue_ping(
         &self,
         glean: &Glean,
         document_id: &str,