diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fec9152d..93d17006b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ [Full changelog](https://github.com/mozilla/glean/compare/v36.0.1...main) +* General + * **Breaking Change**: "deletion-request" pings now include the reason upload was disabled: `at_init` (Glean detected a change between runs) or `set_upload_enabled` (Glean was told of a change as it happened). ([#1593](https://github.com/mozilla/glean/pull/1593)). + # v36.0.1 (2021-04-09) [Full changelog](https://github.com/mozilla/glean/compare/v36.0.0...v36.0.1) diff --git a/docs/user/user/collected-metrics/metrics.md b/docs/user/user/collected-metrics/metrics.md index 7557709bc..0a70ad33c 100644 --- a/docs/user/user/collected-metrics/metrics.md +++ b/docs/user/user/collected-metrics/metrics.md @@ -89,10 +89,23 @@ This ping includes the [client id](https://mozilla.github.io/glean/book/user/pin **Data reviews for this ping:** - +- **Bugs related to this ping:** - +- + +**Reasons this ping may be sent:** + +- `at_init`: The ping was submitted at startup. + Glean discovered that between the last time it was run and this time, + upload of data has been disabled. + +- `set_upload_enabled`: The ping was submitted between Glean init and Glean shutdown. + Glean was told after init but before shutdown that upload has changed + from enabled to disabled. + All Glean pings contain built-in metrics in the [`ping_info`](https://mozilla.github.io/glean/book/user/pings/index.html#the-ping_info-section) and [`client_info`](https://mozilla.github.io/glean/book/user/pings/index.html#the-client_info-section) sections. diff --git a/glean-core/pings.yaml b/glean-core/pings.yaml index 2f5b728c2..9a6657019 100644 --- a/glean-core/pings.yaml +++ b/glean-core/pings.yaml @@ -114,7 +114,18 @@ deletion-request: send_if_empty: true bugs: - https://bugzilla.mozilla.org/1587095 + - https://bugzilla.mozilla.org/1702622 data_reviews: - https://bugzilla.mozilla.org/show_bug.cgi?id=1587095#c6 + - https://bugzilla.mozilla.org/show_bug.cgi?id=1702622#c2 notification_emails: - glean-team@mozilla.com + reasons: + at_init: | + The ping was submitted at startup. + Glean discovered that between the last time it was run and this time, + upload of data has been disabled. + set_upload_enabled: | + The ping was submitted between Glean init and Glean shutdown. + Glean was told after init but before shutdown that upload has changed + from enabled to disabled. diff --git a/glean-core/src/internal_pings.rs b/glean-core/src/internal_pings.rs index b7c53a97b..e78584f1e 100644 --- a/glean-core/src/internal_pings.rs +++ b/glean-core/src/internal_pings.rs @@ -53,7 +53,12 @@ impl InternalPings { "max_capacity".to_string(), ], ), - deletion_request: PingType::new("deletion-request", true, true, vec![]), + deletion_request: PingType::new( + "deletion-request", + true, + true, + vec!["at_init".to_string(), "set_upload_enabled".to_string()], + ), } } } diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index 27dcfdff0..14c0be1f2 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -282,7 +282,7 @@ impl Glean { // Temporarily enable uploading so we can submit a // deletion request ping. glean.upload_enabled = true; - glean.on_upload_disabled(); + glean.on_upload_disabled(true); } } } @@ -412,7 +412,7 @@ impl Glean { if flag { self.on_upload_enabled(); } else { - self.on_upload_disabled(); + self.on_upload_disabled(false); } true } else { @@ -446,10 +446,15 @@ impl Glean { /// A deletion_request ping is sent, all pending metrics, events and queued /// pings are cleared, and the client_id is set to KNOWN_CLIENT_ID. /// Afterward, the upload_enabled flag is set to false. - fn on_upload_disabled(&mut self) { + fn on_upload_disabled(&mut self, during_init: bool) { // The upload_enabled flag should be true here, or the deletion ping // won't be submitted. - if let Err(err) = self.internal_pings.deletion_request.submit(self, None) { + let reason = if during_init { + Some("at_init") + } 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); } self.clear_metrics(); diff --git a/glean-core/tests/ping.rs b/glean-core/tests/ping.rs index 467321b88..d0437bf96 100644 --- a/glean-core/tests/ping.rs +++ b/glean-core/tests/ping.rs @@ -55,7 +55,15 @@ fn disabling_upload_clears_pending_pings() { glean.set_upload_enabled(false); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); // Disabling upload generates a deletion ping - assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); + let dpings = get_deletion_pings(glean.get_data_path()).unwrap(); + assert_eq!(1, dpings.len()); + let payload = &dpings[0].1; + assert_eq!( + "set_upload_enabled", + payload["ping_info"].as_object().unwrap()["reason"] + .as_str() + .unwrap() + ); glean.set_upload_enabled(true); assert_eq!(0, get_queued_pings(glean.get_data_path()).unwrap().len()); @@ -71,7 +79,15 @@ fn deletion_request_only_when_toggled_from_on_to_off() { // Disabling upload generates a deletion ping glean.set_upload_enabled(false); - assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len()); + let dpings = get_deletion_pings(glean.get_data_path()).unwrap(); + assert_eq!(1, dpings.len()); + let payload = &dpings[0].1; + assert_eq!( + "set_upload_enabled", + payload["ping_info"].as_object().unwrap()["reason"] + .as_str() + .unwrap() + ); // Re-setting it to `false` should not generate an additional ping. // As we didn't clear the pending ping, that's the only one that sticks around.