bug 1702622 - Add reasons to 'deletion-request' ping r?janerik

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)

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:**
 
 - <https://bugzilla.mozilla.org/show_bug.cgi?id=1587095#c6>
+- <https://bugzilla.mozilla.org/show_bug.cgi?id=1702622#REPLACEME>
 
 **Bugs related to this ping:**
 
 - <https://bugzilla.mozilla.org/1587095>
+- <https://bugzilla.mozilla.org/1702622>
+
+**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.
 

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.

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()],
+            ),
         }
     }
 }

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();

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.