Bug 1520210 - Send 'event' ping with reason 'shutdown' at shutdown r=janerik
authorChris H-C <chutten@mozilla.com>
Thu, 24 Jan 2019 19:55:39 +0000
changeset 515333 e0c3b3edec4d276daf231f2cd5d201565f4e6055
parent 515332 f4e30f2d170aac42f38183efbecc9f00ad4b2772
child 515334 6063ab0285c78851f79646e83b490f2870d14595
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1520210
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1520210 - Send 'event' ping with reason 'shutdown' at shutdown r=janerik Instead of relying on an observer for profile-before-change, use the likely-more-reliable shutdown call from TelemetryController to trigger the shutdown "event" ping. Also, add some log lines to make diagnosing timing issues easier in the future. Differential Revision: https://phabricator.services.mozilla.com/D17412
toolkit/components/telemetry/pings/EventPing.jsm
toolkit/components/telemetry/tests/unit/test_EventPing.js
--- a/toolkit/components/telemetry/pings/EventPing.jsm
+++ b/toolkit/components/telemetry/pings/EventPing.jsm
@@ -37,17 +37,16 @@ const MS_IN_A_MINUTE = 60 * 1000;
 const DEFAULT_EVENT_LIMIT = 1000;
 const DEFAULT_MIN_FREQUENCY_MS = 60 * MS_IN_A_MINUTE;
 const DEFAULT_MAX_FREQUENCY_MS = 10 * MS_IN_A_MINUTE;
 
 const LOGGER_NAME = "Toolkit.Telemetry";
 const LOGGER_PREFIX = "TelemetryEventPing::";
 
 const EVENT_LIMIT_REACHED_TOPIC = "event-telemetry-storage-limit-reached";
-const PROFILE_BEFORE_CHANGE_TOPIC = "profile-before-change";
 
 var Policy = {
   setTimeout: (callback, delayMs) => setTimeout(callback, delayMs),
   clearTimeout: (id) => clearTimeout(id),
   sendPing: (type, payload, options) => TelemetryController.submitExternalPing(type, payload, options),
 };
 
 var TelemetryEventPing = {
@@ -70,39 +69,40 @@ var TelemetryEventPing = {
     return Telemetry.canRecordPrereleaseData ? Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN
                                              : Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT;
   },
 
   startup() {
     if (!Services.prefs.getBoolPref(Utils.Preferences.EventPingEnabled, true)) {
       return;
     }
+    this._log.trace("Starting up.");
     Services.obs.addObserver(this, EVENT_LIMIT_REACHED_TOPIC);
-    Services.obs.addObserver(this, PROFILE_BEFORE_CHANGE_TOPIC);
 
     XPCOMUtils.defineLazyPreferenceGetter(this, "maxEventsPerPing",
                                           Utils.Preferences.EventPingEventLimit,
                                           DEFAULT_EVENT_LIMIT);
     XPCOMUtils.defineLazyPreferenceGetter(this, "maxFrequency",
                                           Utils.Preferences.EventPingMaximumFrequency,
                                           DEFAULT_MAX_FREQUENCY_MS);
     XPCOMUtils.defineLazyPreferenceGetter(this, "minFrequency",
                                           Utils.Preferences.EventPingMinimumFrequency,
                                           DEFAULT_MIN_FREQUENCY_MS);
 
     this._startTimer();
   },
 
   shutdown() {
+    this._log.trace("Shutting down.");
     // removeObserver may throw, which could interrupt shutdown.
     try {
       Services.obs.removeObserver(this, EVENT_LIMIT_REACHED_TOPIC);
-      Services.obs.removeObserver(this, PROFILE_BEFORE_CHANGE_TOPIC);
     } catch (ex) {}
 
+    this._submitPing(this.Reason.SHUTDOWN, true /* discardLeftovers */);
     this._clearTimer();
   },
 
   observe(aSubject, aTopic, aData) {
     switch (aTopic) {
       case EVENT_LIMIT_REACHED_TOPIC:
         this._log.trace("event limit reached");
         let now = Utils.monotonicNow();
@@ -111,20 +111,16 @@ var TelemetryEventPing = {
           this._startTimer(this.maxFrequency - this._lastSendTime,
                            this.Reason.MAX,
                            true /* discardLeftovers*/);
         } else {
           this._log.trace("submitting ping immediately");
           this._submitPing(this.Reason.MAX);
         }
         break;
-      case PROFILE_BEFORE_CHANGE_TOPIC:
-        this._log.trace("profile before change");
-        this._submitPing(this.Reason.SHUTDOWN, true /* discardLeftovers */);
-        break;
     }
   },
 
   _startTimer(delay = this.minFrequency, reason = this.Reason.PERIODIC, discardLeftovers = false) {
     this._clearTimer();
     this._timeoutId =
       Policy.setTimeout(() => TelemetryEventPing._submitPing(reason, discardLeftovers), delay);
   },
--- a/toolkit/components/telemetry/tests/unit/test_EventPing.js
+++ b/toolkit/components/telemetry/tests/unit/test_EventPing.js
@@ -156,32 +156,16 @@ add_task(async function test_timers() {
 
   fakePolicy((callback, delay) => {
     Assert.ok(delay <= TelemetryEventPing.maxFrequency, "Timer should be at most the max frequency for a subsequent MAX ping.");
   }, pass, pass);
   recordEvents(1000);
 
 });
 
-add_task(async function test_shutdown() {
-  Telemetry.clearEvents();
-  TelemetryEventPing.testReset();
-
-  recordEvents(999);
-  fakePolicy(pass, pass, (type, payload, options) => {
-    Assert.ok(options.addClientId, "Adds the client id.");
-    Assert.ok(options.addEnvironment, "Adds the environment.");
-    Assert.ok(options.usePingSender, "Asks for pingsender.");
-    Assert.equal(payload.reason, TelemetryEventPing.Reason.SHUTDOWN, "Sending because we are shutting down");
-    Assert.equal(payload.events.parent.length, 999, "Has 999 events");
-    Assert.equal(payload.lostEventsCount, 0, "No lost events");
-  });
-  TelemetryEventPing.observe(null, "profile-before-change", null);
-});
-
 add_task(async function test_periodic() {
   Telemetry.clearEvents();
   TelemetryEventPing.testReset();
 
   fakePolicy((callback, delay) => {
     Assert.equal(delay, TelemetryEventPing.minFrequency, "Timer should default to the min frequency");
     fakePolicy(pass, pass, (type, payload, options) => {
       checkPingStructure(type, payload, options);
@@ -193,8 +177,25 @@ add_task(async function test_periodic() 
       Assert.equal(payload.lostEventsCount, 0, "Lost no events");
     });
     callback();
   }, pass, fail);
 
   recordEvents(1);
   TelemetryEventPing._startTimer();
 });
+
+// Ensure this is the final test in the suite, as it shuts things down.
+add_task(async function test_shutdown() {
+  Telemetry.clearEvents();
+  TelemetryEventPing.testReset();
+
+  recordEvents(999);
+  fakePolicy(pass, pass, (type, payload, options) => {
+    Assert.ok(options.addClientId, "Adds the client id.");
+    Assert.ok(options.addEnvironment, "Adds the environment.");
+    Assert.ok(options.usePingSender, "Asks for pingsender.");
+    Assert.equal(payload.reason, TelemetryEventPing.Reason.SHUTDOWN, "Sending because we are shutting down");
+    Assert.equal(payload.events.parent.length, 999, "Has 999 events");
+    Assert.equal(payload.lostEventsCount, 0, "No lost events");
+  });
+  TelemetryEventPing.shutdown();
+});