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

--HG--
extra : moz-landing-system : lando

toolkit/components/telemetry/pings/EventPing.jsm

@@ -42,7 +42,6 @@ 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),
@@ -75,8 +74,8 @@ var TelemetryEventPing = {
     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,
@@ -92,12 +91,13 @@ var TelemetryEventPing = {
   },
 
   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();
   },
 
@@ -116,10 +116,6 @@ var TelemetryEventPing = {
           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;
     }
   },
 

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

@@ -161,22 +161,6 @@ add_task(async function test_timers() {
 
 });
 
-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();
@@ -198,3 +182,20 @@ add_task(async function test_periodic() {
   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();
+});