From 96e2c6e14998f38e419850d55d8a3d32a3fc244a Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Thu, 5 Nov 2020 02:44:05 +0000 Subject: [PATCH] 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 | 7 +- toolkit/components/telemetry/Scalars.yaml | 15 ++++ .../app/TelemetryControllerParent.jsm | 37 ++++++++++ .../tests/unit/test_SyncPingIntegration.js | 69 +++++++++++++++++++ .../telemetry/tests/unit/xpcshell.ini | 2 + 5 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 toolkit/components/telemetry/tests/unit/test_SyncPingIntegration.js diff --git a/services/sync/modules/telemetry.js b/services/sync/modules/telemetry.js index 4c3a0bc67bd3..12150ade91ad 100644 --- a/services/sync/modules/telemetry.js +++ b/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": diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml index 78d7b52b1f98..04e455c093ea 100644 --- a/toolkit/components/telemetry/Scalars.yaml +++ b/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: diff --git a/toolkit/components/telemetry/app/TelemetryControllerParent.jsm b/toolkit/components/telemetry/app/TelemetryControllerParent.jsm index e76483d861f8..832b9e0b280b 100644 --- a/toolkit/components/telemetry/app/TelemetryControllerParent.jsm +++ b/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(); diff --git a/toolkit/components/telemetry/tests/unit/test_SyncPingIntegration.js b/toolkit/components/telemetry/tests/unit/test_SyncPingIntegration.js new file mode 100644 index 000000000000..9acb18bb6746 --- /dev/null +++ b/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(); +}); diff --git a/toolkit/components/telemetry/tests/unit/xpcshell.ini b/toolkit/components/telemetry/tests/unit/xpcshell.ini index f50412219a21..b4969400a80f 100644 --- a/toolkit/components/telemetry/tests/unit/xpcshell.ini +++ b/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