address review feedback

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

@@ -613,6 +613,7 @@ impl Glean {
     /// # Errors
     ///
     /// If collecting or writing the ping to disk failed.
+    /// We may still attempt upload if collection succeeded but writing 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,7 +623,7 @@ 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.make_ping(self, &ping, reason, &doc_id, &url_path) {
+        match ping_maker.collect(self, &ping, reason, &doc_id, &url_path) {
             None => {
                 log::info!(
                     "No content for ping '{}', therefore no ping queued.",

glean-core/src/ping/mod.rs

@@ -209,12 +209,14 @@ 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 make_ping<'a>(
+    pub fn collect<'a>(
         &self,
         glean: &Glean,
         ping: &'a PingType,
@@ -277,7 +279,7 @@ impl PingMaker {
         ping: &PingType,
         reason: Option<&str>,
     ) -> Option<String> {
-        self.make_ping(glean, ping, reason, "", "")
+        self.collect(glean, ping, reason, "", "")
             .map(|ping| ::serde_json::to_string_pretty(&ping.content).unwrap())
     }
 

glean-core/tests/ping_maker.rs

@@ -35,7 +35,7 @@ fn ping_info_must_contain_a_nonempty_start_and_end_time() {
     let (glean, ping_maker, ping_type, _t) = set_up_basic_ping();
 
     let ping = ping_maker
-        .make_ping(&glean, &ping_type, None, "", "")
+        .collect(&glean, &ping_type, None, "", "")
         .unwrap();
     let ping_info = ping.content["ping_info"].as_object().unwrap();
 
@@ -53,7 +53,7 @@ fn get_ping_info_must_report_all_the_required_fields() {
     let (glean, ping_maker, ping_type, _t) = set_up_basic_ping();
 
     let ping = ping_maker
-        .make_ping(&glean, &ping_type, None, "", "")
+        .collect(&glean, &ping_type, None, "", "")
         .unwrap();
     let ping_info = ping.content["ping_info"].as_object().unwrap();
 
@@ -67,18 +67,15 @@ fn get_client_info_must_report_all_the_available_data() {
     let (glean, ping_maker, ping_type, _t) = set_up_basic_ping();
 
     let ping = ping_maker
-        .make_ping(&glean, &ping_type, None, "", "")
+        .collect(&glean, &ping_type, None, "", "")
         .unwrap();
     let client_info = ping.content["client_info"].as_object().unwrap();
 
     client_info["telemetry_sdk_build"].as_str().unwrap();
 }
 
-// SKIPPED from glean-ac: make_ping() must report a valid ping with the data from the engines
-// This test doesn't really make sense with rkv
-
 #[test]
-fn make_ping_must_report_none_when_no_data_is_stored() {
+fn collect_must_report_none_when_no_data_is_stored() {
     // NOTE: This is a behavior change from glean-ac which returned an empty
     // string in this case. As this is an implementation detail and not part of
     // the public API, it's safe to change this.
@@ -89,7 +86,7 @@ fn make_ping_must_report_none_when_no_data_is_stored() {
     glean.register_ping_type(&ping_type);
 
     assert!(ping_maker
-        .make_ping(&glean, &unknown_ping_type, None, "", "")
+        .collect(&glean, &unknown_ping_type, None, "", "")
         .is_none());
 }
 
@@ -111,7 +108,7 @@ fn seq_number_must_be_sequential() {
         for ping_name in ["store1", "store2"].iter() {
             let ping_type = PingType::new(*ping_name, true, false, vec![]);
             let ping = ping_maker
-                .make_ping(&glean, &ping_type, None, "", "")
+                .collect(&glean, &ping_type, None, "", "")
                 .unwrap();
             let seq_num = ping.content["ping_info"]["seq"].as_i64().unwrap();
             // Ensure sequence numbers in different stores are independent of
@@ -126,14 +123,14 @@ fn seq_number_must_be_sequential() {
 
         // 3rd ping of store1
         let ping = ping_maker
-            .make_ping(&glean, &ping_type, None, "", "")
+            .collect(&glean, &ping_type, None, "", "")
             .unwrap();
         let seq_num = ping.content["ping_info"]["seq"].as_i64().unwrap();
         assert_eq!(2, seq_num);
 
         // 4th ping of store1
         let ping = ping_maker
-            .make_ping(&glean, &ping_type, None, "", "")
+            .collect(&glean, &ping_type, None, "", "")
             .unwrap();
         let seq_num = ping.content["ping_info"]["seq"].as_i64().unwrap();
         assert_eq!(3, seq_num);
@@ -144,7 +141,7 @@ fn seq_number_must_be_sequential() {
 
         // 3rd ping of store2
         let ping = ping_maker
-            .make_ping(&glean, &ping_type, None, "", "")
+            .collect(&glean, &ping_type, None, "", "")
             .unwrap();
         let seq_num = ping.content["ping_info"]["seq"].as_i64().unwrap();
         assert_eq!(2, seq_num);
@@ -155,7 +152,7 @@ fn seq_number_must_be_sequential() {
 
         // 5th ping of store1
         let ping = ping_maker
-            .make_ping(&glean, &ping_type, None, "", "")
+            .collect(&glean, &ping_type, None, "", "")
             .unwrap();
         let seq_num = ping.content["ping_info"]["seq"].as_i64().unwrap();
         assert_eq!(4, seq_num);