Bug 1673795 - Integrate sync ping shutdown handling with TelemetryController and record if a sync shutdown ping was sent. r=chutten

Differential Revision: https://phabricator.services.mozilla.com/D94929

services/sync/modules/telemetry.js

@@ -57,8 +57,6 @@ XPCOMUtils.defineLazyGetter(
 const log = Log.repository.getLogger("Sync.Telemetry");
 
 const TOPICS = [
-  "profile-before-change",
-
   // For tracking change to account/device identifiers.
   "fxaccounts:new_device_id",
   "fxaccounts:onlogout",
@@ -581,6 +579,7 @@ class SyncTelemetryImpl {
     this.sessionStartDate = TelemetryUtils.toLocalTimeISOString(
       TelemetryUtils.truncateToHours(sessionStartDate)
     );
+    TelemetryController.registerSyncPingShutdown(() => this.shutdown());
   }
 
   sanitizeFxaDeviceId(deviceId) {
@@ -952,10 +951,6 @@ class SyncTelemetryImpl {
     log.trace(`observed ${topic} ${data}`);
 
     switch (topic) {
-      case "profile-before-change":
-        this.shutdown();
-        break;
-
       case "weave:service:ready":
       case "weave:service:login:change":
       case "fxaccounts:new_device_id":

toolkit/components/telemetry/Scalars.yaml

@@ -3218,6 +3218,21 @@ telemetry:
     record_in_processes:
       - main
 
+  sync_shutdown_ping_sent:
+    bug_numbers:
+      - 1673795
+    description: >
+      Whether a sync shutdown ping has been sent in this session.
+    expires: "90"
+    kind: boolean
+    notification_emails:
+      - sync-team@mozilla.com
+    products:
+      - 'firefox'
+    release_channel_collection: opt-out
+    record_in_processes:
+      - main
+
 telemetry.discarded:
   accumulations:
     bug_numbers:

toolkit/components/telemetry/app/TelemetryControllerParent.jsm

@@ -268,6 +268,17 @@ var TelemetryController = Object.freeze({
     return Impl.saveUninstallPing();
   },
 
+  /**
+   * Allows the sync ping to tell the controller that it is initializing, so
+   * should be included in the orderly shutdown process.
+   *
+   * @param {Function} aFnShutdown The function to call as telemetry shuts down.
+
+   */
+  registerSyncPingShutdown(afnShutdown) {
+    Impl.registerSyncPingShutdown(afnShutdown);
+  },
+
   /**
    * Allows waiting for TelemetryControllers delayed initialization to complete.
    * The returned promise is guaranteed to resolve before TelemetryController is shutting down.
@@ -312,6 +323,9 @@ var Impl = {
   _probeRegistrationPromise: null,
   // The promise of any outstanding task sending the "deletion-request" ping.
   _deletionRequestPingSubmittedPromise: null,
+  // A function to shutdown the sync/fxa ping, or null if that ping has not
+  // self-initialized.
+  _fnSyncPingShutdown: null,
 
   get _log() {
     return TelemetryControllerBase.log;
@@ -619,6 +633,12 @@ var Impl = {
       return Promise.reject(new Error("Invalid payload type submitted."));
     }
 
+    // We're trying to track down missing sync pings (bug 1663573), so record
+    // a temporary cross-checking counter.
+    if (aType == "sync" && aPayload.why == "shutdown") {
+      Telemetry.scalarSet("telemetry.sync_shutdown_ping_sent", true);
+    }
+
     let promise = this._submitPingLogic(aType, aPayload, aOptions);
     this._trackPendingPingTask(promise);
     return promise;
@@ -936,6 +956,12 @@ var Impl = {
       EcosystemTelemetry.shutdown();
       await TelemetryPrioPing.shutdown();
 
+      // Shutdown the sync ping if it is initialized - this is likely, but not
+      // guaranteed, to submit a "shutdown" sync ping.
+      if (this._fnSyncPingShutdown) {
+        this._fnSyncPingShutdown();
+      }
+
       // Stop the datachoices infobar display.
       TelemetryReportingPolicy.shutdown();
       TelemetryEnvironment.shutdown();
@@ -1025,6 +1051,16 @@ var Impl = {
     return undefined;
   },
 
+  /**
+   * Register the sync ping's shutdown handler.
+   */
+  registerSyncPingShutdown(fnShutdown) {
+    if (this._fnSyncPingShutdown) {
+      throw new Error("The sync ping shutdown handler is already registered.");
+    }
+    this._fnSyncPingShutdown = fnShutdown;
+  },
+
   /**
    * Get an object describing the current state of this module for AsyncShutdown diagnostics.
    */
@@ -1186,6 +1222,7 @@ var Impl = {
 
   async reset() {
     this._clientID = null;
+    this._fnSyncPingShutdown = null;
     this._detachObservers();
 
     let sessionReset = TelemetrySession.testReset();

toolkit/components/telemetry/tests/unit/test_SyncPingIntegration.js

@@ -0,0 +1,69 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/
+*/
+
+add_task(async function test_setup() {
+  // Addon manager needs a profile directory
+  do_get_profile();
+});
+
+add_task(async function test_register_twice_fails() {
+  TelemetryController.registerSyncPingShutdown(() => {});
+  Assert.throws(
+    () => TelemetryController.registerSyncPingShutdown(() => {}),
+    /The sync ping shutdown handler is already registered./
+  );
+  await TelemetryController.testReset();
+});
+
+add_task(async function test_reset_clears_handler() {
+  await TelemetryController.testSetup();
+  TelemetryController.registerSyncPingShutdown(() => {});
+  await TelemetryController.testReset();
+  // If this works the reset must have cleared it.
+  TelemetryController.registerSyncPingShutdown(() => {});
+  await TelemetryController.testReset();
+});
+
+add_task(async function test_shutdown_handler_submits() {
+  let handlerCalled = false;
+  await TelemetryController.testSetup();
+  TelemetryController.registerSyncPingShutdown(() => {
+    handlerCalled = true;
+    // and submit a ping.
+    let ping = {
+      why: "shutdown",
+    };
+    TelemetryController.submitExternalPing("sync", ping);
+  });
+
+  await TelemetryController.testShutdown();
+  Assert.ok(handlerCalled);
+  // and check we recorded telemetry about it.
+  let snapshot = Telemetry.getSnapshotForScalars("main", true).parent || {};
+  Assert.equal(
+    snapshot["telemetry.sync_shutdown_ping_sent"],
+    true,
+    "recorded that we sent a ping."
+  );
+  await TelemetryController.testReset();
+});
+
+add_task(async function test_shutdown_handler_no_submit() {
+  let handlerCalled = false;
+  await TelemetryController.testSetup();
+  TelemetryController.registerSyncPingShutdown(() => {
+    handlerCalled = true;
+    // but don't submit a ping.
+  });
+
+  await TelemetryController.testShutdown();
+  Assert.ok(handlerCalled);
+  // and check we didn't record our scalar.
+  let snapshot = Telemetry.getSnapshotForScalars("main", true).parent || {};
+  Assert.ok(
+    !("telemetry.sync_shutdown_ping_sent" in snapshot),
+    "should not have recorded we sent a ping"
+  );
+  await TelemetryController.testReset();
+});

toolkit/components/telemetry/tests/unit/xpcshell.ini

@@ -31,6 +31,8 @@ skip-if = debug || asan || tsan || os == "android" || release_or_beta
 [test_TelemetryHistograms.js]
 [test_SubsessionChaining.js]
 tags = addons
+[test_SyncPingIntegration.js]
+skip-if = os == "android"
 [test_TelemetryEnvironment.js]
 skip-if = os == "android"
 tags = addons