Rust: Don't return a result from submit_ping

The `Result<bool>` value returned previously was not expressing what's
really happening anymore.
It was impossible to encounter an `Err` variant after recent changes.

Technically this is a breaking change for glean-core, but that library
is not consumed by any outsiders.

glean-core/ffi/src/lib.rs

@@ -301,9 +301,7 @@ pub extern "C" fn glean_set_upload_enabled(flag: u8) {
 #[no_mangle]
 pub extern "C" fn glean_submit_ping_by_name(ping_name: FfiStr, reason: FfiStr) -> u8 {
     with_glean(|glean| {
-        Ok(glean
-            .submit_ping_by_name(&ping_name.to_string_fallible()?, reason.as_opt_str())
-            .unwrap_or(false))
+        Ok(glean.submit_ping_by_name(&ping_name.to_string_fallible()?, reason.as_opt_str()))
     })
 }
 

glean-core/rlb/src/lib.rs

@@ -313,10 +313,7 @@ pub fn initialize(cfg: Configuration, client_info: ClientInfoMetrics) {
                     // write lock on the `glean` object.
                     // Note that unwrapping below is safe: the function will return an
                     // `Ok` value for a known ping.
-                    if glean
-                        .submit_ping_by_name("baseline", Some("dirty_startup"))
-                        .unwrap()
-                    {
+                    if glean.submit_ping_by_name("baseline", Some("dirty_startup")) {
                         state.upload_manager.trigger_upload();
                     }
                 }
@@ -553,13 +550,13 @@ pub(crate) fn submit_ping_by_name_sync(ping: &str, reason: Option<&str>) {
             // This won't actually return from `submit_ping_by_name`, but
             // returning `false` here skips spinning up the uploader below,
             // which is basically the same.
-            return Some(false);
+            return false;
         }
 
-        glean.submit_ping_by_name(&ping, reason.as_deref()).ok()
+        glean.submit_ping_by_name(&ping, reason.as_deref())
     });
 
-    if let Some(true) = submitted_ping {
+    if submitted_ping {
         let state = global_state().lock().unwrap();
         state.upload_manager.trigger_upload();
     }

glean-core/src/event_database/mod.rs

@@ -161,15 +161,7 @@ impl EventDatabase {
 
         let mut ping_sent = false;
         for store_name in store_names {
-            if let Err(err) = glean.submit_ping_by_name(&store_name, Some("startup")) {
-                log::warn!(
-                    "Error flushing existing events to the '{}' ping: {}",
-                    store_name,
-                    err
-                );
-            } else {
-                ping_sent = true;
-            }
+            ping_sent |= glean.submit_ping_by_name(&store_name, Some("startup"));
         }
 
         ping_sent
@@ -225,14 +217,7 @@ impl EventDatabase {
         // If any of the event stores reached maximum size, submit the pings
         // containing those events immediately.
         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 persist {} ping: {}",
-                    glean.get_max_events(),
-                    store_name,
-                    err
-                );
-            }
+            glean.submit_ping_by_name(store_name, Some("max_capacity"));
         }
     }
 

glean-core/src/lib.rs

@@ -161,7 +161,7 @@ pub struct Configuration {
 ///
 /// call_counter.add(&glean, 1);
 ///
-/// glean.submit_ping(&ping, None).unwrap();
+/// glean.submit_ping(&ping, None);
 /// ```
 ///
 /// ## Note
@@ -454,8 +454,8 @@ impl Glean {
         } else {
             Some("set_upload_enabled")
         };
-        if let Err(err) = self.internal_pings.deletion_request.submit(self, reason) {
-            log::error!("Failed to submit deletion-request ping on optout: {}", err);
+        if !self.internal_pings.deletion_request.submit(self, reason) {
+            log::error!("Failed to submit deletion-request ping on optout.");
         }
         self.clear_metrics();
         self.upload_enabled = false;
@@ -626,10 +626,10 @@ impl Glean {
     /// # Returns
     ///
     /// Whether the ping was succesfully assembled and queued.
-    pub fn submit_ping(&self, ping: &PingType, reason: Option<&str>) -> Result<bool> {
+    pub fn submit_ping(&self, ping: &PingType, reason: Option<&str>) -> bool {
         if !self.is_upload_enabled() {
             log::info!("Glean disabled: not submitting any pings.");
-            return Ok(false);
+            return false;
         }
 
         let ping_maker = PingMaker::new();
@@ -641,7 +641,7 @@ impl Glean {
                     "No content for ping '{}', therefore no ping queued.",
                     ping.name
                 );
-                Ok(false)
+                false
             }
             Some(ping) => {
                 // This metric is recorded *after* the ping is collected (since
@@ -656,16 +656,24 @@ impl Glean {
                 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.additional_metrics.io_errors.add(self, 1);
-                    let content = ::serde_json::to_string(&ping.content)?;
+                    let content = match ::serde_json::to_string(&ping.content) {
+                        Ok(content) => content,
+                        Err(_) => {
+                            // `serde_json::to_string` only fails if serialization of the content
+                            // fails or it contains maps with non-string keys.
+                            // However `ping.content` is already a `JsonValue`,
+                            // so both scenarios are impossible.
+                            unreachable!()
+                        }
+                    };
                     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);
+                    return true;
                 }
 
                 self.upload_manager.enqueue_ping_from_file(self, &doc_id);
@@ -674,7 +682,8 @@ impl Glean {
                     "The ping '{}' was submitted and will be sent as soon as possible",
                     ping.name
                 );
-                Ok(true)
+
+                true
             }
         }
     }
@@ -699,11 +708,11 @@ impl Glean {
     /// # Errors
     ///
     /// If collecting or writing the ping to disk failed.
-    pub fn submit_ping_by_name(&self, ping_name: &str, reason: Option<&str>) -> Result<bool> {
+    pub fn submit_ping_by_name(&self, ping_name: &str, reason: Option<&str>) -> bool {
         match self.get_ping_by_name(ping_name) {
             None => {
                 log::error!("Attempted to submit unknown ping '{}'", ping_name);
-                Ok(false)
+                false
             }
             Some(ping) => self.submit_ping(ping, reason),
         }
@@ -917,8 +926,8 @@ impl Glean {
     /// This functions generates a baseline ping with reason `active`
     /// and then sets the dirty bit.
     pub fn handle_client_active(&mut self) {
-        if let Err(err) = self.internal_pings.baseline.submit(self, Some("active")) {
-            log::warn!("Failed to submit baseline ping on active: {}", err);
+        if !self.internal_pings.baseline.submit(self, Some("active")) {
+            log::warn!("Failed to submit baseline ping on active");
         }
 
         self.set_dirty_flag(true);
@@ -929,12 +938,12 @@ impl Glean {
     /// This functions generates a baseline and an events ping with reason
     /// `inactive` and then clears the dirty bit.
     pub fn handle_client_inactive(&mut self) {
-        if let Err(err) = self.internal_pings.baseline.submit(self, Some("inactive")) {
-            log::warn!("Failed to submit baseline ping on inactive: {}", err);
+        if !self.internal_pings.baseline.submit(self, Some("inactive")) {
+            log::warn!("Failed to submit baseline ping on inactive");
         }
 
-        if let Err(err) = self.internal_pings.events.submit(self, Some("inactive")) {
-            log::warn!("Failed to submit events ping on inactive: {}", err);
+        if !self.internal_pings.events.submit(self, Some("inactive")) {
+            log::warn!("Failed to submit events ping on inactive");
         }
 
         self.set_dirty_flag(false);

glean-core/src/lib_unit_tests.rs

@@ -894,8 +894,7 @@ fn records_io_errors() {
     // Writing the ping file should fail.
     let submitted = glean.internal_pings.metrics.submit(&glean, None);
     // But the return value is still Ok(true) because we enqueue the ping anyway.
-    assert!(submitted.is_ok());
-    assert!(submitted.unwrap());
+    assert!(submitted);
 
     let metric = &glean.additional_metrics.io_errors;
     assert_eq!(
@@ -909,7 +908,7 @@ fn records_io_errors() {
 
     // Now we can submit a ping
     let submitted = glean.internal_pings.metrics.submit(&glean, None);
-    assert!(submitted.is_ok());
+    assert!(submitted);
 }
 
 #[test]

glean-core/src/metrics/ping.rs

@@ -2,7 +2,6 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-use crate::error::Result;
 use crate::Glean;
 
 /// Stores information about a ping.
@@ -60,7 +59,7 @@ impl PingType {
     /// # Returns
     ///
     /// See [`Glean::submit_ping`](crate::Glean::submit_ping) for details.
-    pub fn submit(&self, glean: &Glean, reason: Option<&str>) -> Result<bool> {
+    pub fn submit(&self, glean: &Glean, reason: Option<&str>) -> bool {
         let corrected_reason = match reason {
             Some(reason) => {
                 if self.reason_codes.contains(&reason.to_string()) {

glean-core/src/upload/directory.rs

@@ -307,7 +307,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit the ping to populate the pending_pings directory
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         let directory_manager = PingDirectoryManager::new(dir.path());
 
@@ -333,7 +333,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit the ping to populate the pending_pings directory
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         let directory_manager = PingDirectoryManager::new(&dir.path());
 
@@ -368,7 +368,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit the ping to populate the pending_pings directory
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         let directory_manager = PingDirectoryManager::new(&dir.path());
 
@@ -399,11 +399,7 @@ mod test {
         let (glean, dir) = new_glean(None);
 
         // Submit a deletion request ping to populate deletion request folder.
-        glean
-            .internal_pings
-            .deletion_request
-            .submit(&glean, None)
-            .unwrap();
+        glean.internal_pings.deletion_request.submit(&glean, None);
 
         let directory_manager = PingDirectoryManager::new(dir.path());
 

glean-core/src/upload/mod.rs

@@ -874,14 +874,10 @@ mod test {
         // Submit the ping multiple times
         let n = 10;
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
-        glean
-            .internal_pings
-            .deletion_request
-            .submit(&glean, None)
-            .unwrap();
+        glean.internal_pings.deletion_request.submit(&glean, None);
 
         // Clear the queue
         drop(glean.upload_manager.clear_ping_queue());
@@ -907,7 +903,7 @@ mod test {
         // Submit the ping multiple times
         let n = 10;
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         // Create a new upload manager pointing to the same data_path as the glean instance.
@@ -935,7 +931,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit a ping
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         // Get the pending ping directory path
         let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY);
@@ -965,7 +961,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit a ping
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         // Get the pending ping directory path
         let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY);
@@ -995,7 +991,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit a ping
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         // Get the submitted PingRequest
         match glean.get_upload_task() {
@@ -1027,7 +1023,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit a ping
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         // Get the pending ping directory path
         let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY);
@@ -1104,7 +1100,7 @@ mod test {
         glean.register_ping_type(&ping_type);
 
         // Submit a ping
-        glean.submit_ping(&ping_type, None).unwrap();
+        glean.submit_ping(&ping_type, None);
 
         // Get the submitted PingRequest
         match glean.get_upload_task() {
@@ -1152,7 +1148,7 @@ mod test {
         // Submit the ping multiple times
         let n = 5;
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         let mut upload_manager = PingUploadManager::no_policy(dir.path());
@@ -1200,7 +1196,7 @@ mod test {
         // Submit the ping multiple times
         let n = 10;
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         let directory_manager = PingDirectoryManager::new(dir.path());
@@ -1272,7 +1268,7 @@ mod test {
 
         // Submit the ping multiple times
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         let directory_manager = PingDirectoryManager::new(dir.path());
@@ -1343,7 +1339,7 @@ mod test {
 
         // Submit the ping multiple times
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         let directory_manager = PingDirectoryManager::new(dir.path());
@@ -1417,7 +1413,7 @@ mod test {
 
         // Submit the ping multiple times
         for _ in 0..n {
-            glean.submit_ping(&ping_type, None).unwrap();
+            glean.submit_ping(&ping_type, None);
         }
 
         let directory_manager = PingDirectoryManager::new(dir.path());