Merge pull request #1613 from mozilla/1704606-no-more-result-bool

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 [Full changelog](https://github.com/mozilla/glean/compare/v37.0.0...main)
 
+* Rust
+  * Don't return a result from `submit_ping`. The boolean return value indicates whether a ping was submitted ([#1613](https://github.com/mozilla/glean/pull/1613))
+
 # v37.0.0 (2021-04-30)
 
 [Full changelog](https://github.com/mozilla/glean/compare/v36.0.1...v37.0.0)

docs/user/user/collected-metrics/metrics.md

@@ -1,4 +1,4 @@
-<!-- AUTOGENERATED BY glean_parser.  DO NOT EDIT. -->
+<!-- AUTOGENERATED BY glean_parser. DO NOT EDIT. -->
 
 # Metrics
 
@@ -89,7 +89,7 @@ This ping includes the [client id](https://mozilla.github.io/glean/book/user/pin
 **Data reviews for this ping:**
 
 - <https://bugzilla.mozilla.org/show_bug.cgi?id=1587095#c6>
-- <https://bugzilla.mozilla.org/show_bug.cgi?id=1702622#REPLACEME>
+- <https://bugzilla.mozilla.org/show_bug.cgi?id=1702622#c2>
 
 **Bugs related to this ping:**
 
@@ -168,5 +168,5 @@ In addition to those built-in metrics, the following metrics are added to the pi
 
 Data categories are [defined here](https://wiki.mozilla.org/Firefox/Data_Collection).
 
-<!-- AUTOGENERATED BY glean_parser.  DO NOT EDIT. -->
+<!-- AUTOGENERATED BY glean_parser.  DO NOT EDIT. DO NOT COMMIT. -->
 

glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt

@@ -495,7 +495,7 @@ class EventMetricTypeTest {
             category = "telemetry",
             name = "test_event",
             lifetime = Lifetime.Ping,
-            sendInPings = listOf(pingName),
+            sendInPings = listOf(pingName, "events"), // also send in builtin ping
             allowedExtraKeys = listOf("someExtra")
         )
 
@@ -523,11 +523,14 @@ class EventMetricTypeTest {
             sendIfEmpty = false,
             reasonCodes = listOf())
 
-        // Trigger worker task to upload the pings in the background
+        // Trigger worker task to upload the pings in the background.
+        // Because this also triggers the builtin "events" ping
+        // we should definitely get _something_.
         triggerWorkManager(context)
 
         // We can't properly test the absence of data,
         // but we can try to receive one and that causes an exception if there is none.
+        // We also get the "events" ping, which we'll simply ignore here.
         assertNull(waitForPingContent(pingName, null, server))
 
         // Now try to manually submit the ping.

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,20 @@ 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)?;
+                    // `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 should be impossible.
+                    let content =
+                        ::serde_json::to_string(&ping.content).expect("ping serialization failed");
                     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 +678,8 @@ impl Glean {
                     "The ping '{}' was submitted and will be sent as soon as possible",
                     ping.name
                 );
-                Ok(true)
+
+                true
             }
         }
     }
@@ -699,11 +704,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 +922,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::info!("baseline ping not submitted on active");
         }
 
         self.set_dirty_flag(true);
@@ -929,12 +934,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::info!("baseline ping not submitted 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::info!("events ping not submitted on inactive");
         }
 
         self.set_dirty_flag(false);

glean-core/src/lib_unit_tests.rs

@@ -893,9 +893,8 @@ 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());
+    // But the return value is still `true` because we enqueue the ping anyway.
+    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());