From 39162dcd35d41cf2d54ee9ede711b544fba99d43 Mon Sep 17 00:00:00 2001 From: Chris H-C Date: Thu, 8 Apr 2021 10:53:31 -0400 Subject: [PATCH] address review feedback --- glean-core/src/event_database/mod.rs | 2 +- glean-core/src/lib.rs | 3 ++- glean-core/src/ping/mod.rs | 8 +++++--- glean-core/tests/ping_maker.rs | 23 ++++++++++------------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/glean-core/src/event_database/mod.rs b/glean-core/src/event_database/mod.rs index 41d92eb7e0..0df60f6584 100644 --- a/glean-core/src/event_database/mod.rs +++ b/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 diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index 9d43a78913..b8a6710465 100644 --- a/glean-core/src/lib.rs +++ b/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 { 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.", diff --git a/glean-core/src/ping/mod.rs b/glean-core/src/ping/mod.rs index 75e29e926c..05ed996a28 100644 --- a/glean-core/src/ping/mod.rs +++ b/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 { - self.make_ping(glean, ping, reason, "", "") + self.collect(glean, ping, reason, "", "") .map(|ping| ::serde_json::to_string_pretty(&ping.content).unwrap()) } diff --git a/glean-core/tests/ping_maker.rs b/glean-core/tests/ping_maker.rs index 84916b3545..ec3e5e1225 100644 --- a/glean-core/tests/ping_maker.rs +++ b/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);