Bug 1301289 - send accumulated sync pings earlier in the shutdown process. r=tcsc draft
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 08 Sep 2016 15:05:11 +1000
changeset 411962 16d16ab53bfc512c8c4f514aec3825486c9b61df
parent 410919 a6b6a93eb41a05e310a11f0172f01ba9b21d3eac
child 530848 e5b44ed53225518a203b9d3405e83c2c2317d5ce
push id29019
push userbmo:markh@mozilla.com
push dateFri, 09 Sep 2016 01:33:56 +0000
reviewerstcsc
bugs1301289
milestone51.0a1
Bug 1301289 - send accumulated sync pings earlier in the shutdown process. r=tcsc MozReview-Commit-ID: Ls6HeieFi1H
services/sync/modules/telemetry.js
services/sync/tests/unit/head_helpers.js
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -28,17 +28,17 @@ Cu.import("resource://gre/modules/FxAcco
 
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
                                    "@mozilla.org/base/telemetry;1",
                                    "nsITelemetry");
 
 const log = Log.repository.getLogger("Sync.Telemetry");
 
 const TOPICS = [
-  "xpcom-shutdown",
+  "profile-before-change",
   "weave:service:sync:start",
   "weave:service:sync:finish",
   "weave:service:sync:error",
 
   "weave:engine:sync:start",
   "weave:engine:sync:finish",
   "weave:engine:sync:error",
   "weave:engine:sync:applied",
@@ -356,16 +356,17 @@ class SyncTelemetryImpl {
       Observers.remove(topic, this, this);
     }
   }
 
   submit(record) {
     // We still call submit() with possibly illegal payloads so that tests can
     // know that the ping was built. We don't end up submitting them, however.
     if (record.syncs.length) {
+      log.trace(`submitting ${record.syncs.length} sync record(s) to telemetry`);
       TelemetryController.submitExternalPing("sync", record);
     }
   }
 
 
   onSyncStarted() {
     if (this.current) {
       log.warn("Observed weave:service:sync:start, but we're already recording a sync!");
@@ -400,17 +401,17 @@ class SyncTelemetryImpl {
       this.lastSubmissionTime = Telemetry.msSinceProcessStart();
     }
   }
 
   observe(subject, topic, data) {
     log.trace(`observed ${topic} ${data}`);
 
     switch (topic) {
-      case "xpcom-shutdown":
+      case "profile-before-change":
         this.shutdown();
         break;
 
       /* sync itself state changes */
       case "weave:service:sync:start":
         this.onSyncStarted();
         break;
 
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -247,17 +247,21 @@ function get_sync_test_telemetry() {
   for (let engineName of testEngines) {
     ns.SyncTelemetry.allowedEngines.add(engineName);
   }
   ns.SyncTelemetry.submissionInterval = -1;
   return ns.SyncTelemetry;
 }
 
 function assert_valid_ping(record) {
-  if (record) {
+  // This is called as the test harness tears down due to shutdown. This
+  // will typically have no recorded syncs, and the validator complains about
+  // it. So ignore such records (but only ignore when *both* shutdown and
+  // no Syncs - either of them not being true might be an actual problem)
+  if (record && (record.why != "shutdown" || record.syncs.length != 0)) {
     if (!SyncPingValidator(record)) {
       deepEqual([], SyncPingValidator.errors, "Sync telemetry ping validation failed");
     }
     equal(record.version, 1);
     record.syncs.forEach(p => {
       lessOrEqual(p.when, Date.now());
     });
   }