diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index d6f07fe2af..77737eddf7 100644 --- a/glean-core/ffi/src/lib.rs +++ b/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())) }) } diff --git a/glean-core/rlb/src/lib.rs b/glean-core/rlb/src/lib.rs index 438acd6853..671d383f48 100644 --- a/glean-core/rlb/src/lib.rs +++ b/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(); } diff --git a/glean-core/src/event_database/mod.rs b/glean-core/src/event_database/mod.rs index 9b14d748cc..245f533130 100644 --- a/glean-core/src/event_database/mod.rs +++ b/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")); } } diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index fb40c1de63..cf5f5af0b2 100644 --- a/glean-core/src/lib.rs +++ b/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 { + 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,7 +656,16 @@ 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, @@ -664,8 +673,7 @@ impl Glean { &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 { + 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); diff --git a/glean-core/src/lib_unit_tests.rs b/glean-core/src/lib_unit_tests.rs index 52e4a8bf9d..fbf5ca7acd 100644 --- a/glean-core/src/lib_unit_tests.rs +++ b/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] diff --git a/glean-core/src/metrics/ping.rs b/glean-core/src/metrics/ping.rs index fd44b06dec..2aacbcf3a1 100644 --- a/glean-core/src/metrics/ping.rs +++ b/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 { + 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()) { diff --git a/glean-core/src/upload/directory.rs b/glean-core/src/upload/directory.rs index 6b4a58156b..6c7722aabd 100644 --- a/glean-core/src/upload/directory.rs +++ b/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()); diff --git a/glean-core/src/upload/mod.rs b/glean-core/src/upload/mod.rs index adefc2448b..877acb3d88 100644 --- a/glean-core/src/upload/mod.rs +++ b/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()); diff --git a/glean-core/tests/ping.rs b/glean-core/tests/ping.rs index d0437bf963..20d034ff8b 100644 --- a/glean-core/tests/ping.rs +++ b/glean-core/tests/ping.rs @@ -25,7 +25,7 @@ fn write_ping_to_disk() { }); counter.add(&glean, 1); - assert!(ping.submit(&glean, None).unwrap()); + assert!(ping.submit(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); } @@ -46,7 +46,7 @@ fn disabling_upload_clears_pending_pings() { }); counter.add(&glean, 1); - assert!(ping.submit(&glean, None).unwrap()); + assert!(ping.submit(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); // At this point no deletion_request ping should exist // (that is: it's directory should not exist at all) @@ -69,7 +69,7 @@ fn disabling_upload_clears_pending_pings() { assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); counter.add(&glean, 1); - assert!(ping.submit(&glean, None).unwrap()); + assert!(ping.submit(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); } @@ -111,11 +111,11 @@ fn empty_pings_with_flag_are_sent() { // No data is stored in either of the custom pings // Sending this should succeed. - assert_eq!(true, ping1.submit(&glean, None).unwrap()); + assert_eq!(true, ping1.submit(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); // Sending this should fail. - assert_eq!(false, ping2.submit(&glean, None).unwrap()); + assert_eq!(false, ping2.submit(&glean, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); } @@ -152,7 +152,7 @@ fn test_pings_submitted_metric() { }); counter.add(&glean, 1); - assert!(metrics_ping.submit(&glean, None).unwrap()); + assert!(metrics_ping.submit(&glean, None)); // Check recording in the metrics ping assert_eq!( @@ -186,8 +186,8 @@ fn test_pings_submitted_metric() { // This should record a count of 2 baseline pings in the metrics ping, but // it resets each time on the baseline ping, so we should only ever get 1 // baseline ping recorded in the baseline ping itsef. - assert!(baseline_ping.submit(&glean, None).unwrap()); - assert!(baseline_ping.submit(&glean, None).unwrap()); + assert!(baseline_ping.submit(&glean, None)); + assert!(baseline_ping.submit(&glean, None)); // Check recording in the metrics ping assert_eq!( diff --git a/glean-core/tests/ping_maker.rs b/glean-core/tests/ping_maker.rs index ec3e5e1225..8781ad494d 100644 --- a/glean-core/tests/ping_maker.rs +++ b/glean-core/tests/ping_maker.rs @@ -177,7 +177,7 @@ fn clear_pending_pings() { }); metric.set(&glean, true); - assert!(glean.submit_ping(&ping_type, None).is_ok()); + assert!(glean.submit_ping(&ping_type, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); assert!(ping_maker @@ -194,17 +194,17 @@ fn no_pings_submitted_if_upload_disabled() { let ping_type = PingType::new("store1", true, true, vec![]); glean.register_ping_type(&ping_type); - assert!(glean.submit_ping(&ping_type, None).is_ok()); + assert!(glean.submit_ping(&ping_type, None)); assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len()); // Disable upload, then try to sumbit glean.set_upload_enabled(false); - assert!(glean.submit_ping(&ping_type, None).is_ok()); + assert!(!glean.submit_ping(&ping_type, None)); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); // Test again through the direct call - assert!(ping_type.submit(&glean, None).is_ok()); + assert!(!ping_type.submit(&glean, None)); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); } @@ -215,7 +215,7 @@ fn metadata_is_correctly_added_when_necessary() { let ping_type = PingType::new("store1", true, true, vec![]); glean.register_ping_type(&ping_type); - assert!(glean.submit_ping(&ping_type, None).is_ok()); + assert!(glean.submit_ping(&ping_type, None)); let (_, _, metadata) = &get_queued_pings(glean.get_data_path()).unwrap()[0]; let headers = metadata.as_ref().unwrap().get("headers").unwrap();