Bug 1583897 - Send a telemetry event for new sendtab. r=tcsc,eoger,lina
authorMark Hammond <mhammond@skippinet.com.au>
Mon, 14 Oct 2019 22:17:28 +0000
changeset 497549 15f007ec4cacb3114aaa3dd76ed8c1d72e20cde9
parent 497548 ebd88beae3d8b1c6c2b7e1f3ac96a0a2ec4c32be
child 497550 8ea163ea6721438aa8a352b3d3f297fe2895373c
push id114152
push userdvarga@mozilla.com
push dateTue, 15 Oct 2019 11:14:34 +0000
treeherdermozilla-inbound@28fe88d0915b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, eoger, lina
bugs1583897
milestone71.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 1583897 - Send a telemetry event for new sendtab. r=tcsc,eoger,lina Differential Revision: https://phabricator.services.mozilla.com/D48153
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/FxAccountsCommands.js
services/fxaccounts/FxAccountsCommon.js
services/fxaccounts/FxAccountsTelemetry.jsm
services/fxaccounts/moz.build
services/fxaccounts/tests/xpcshell/test_commands.js
services/sync/modules/telemetry.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/test_corrupt_keys.js
services/sync/tests/unit/test_errorhandler_1.js
services/sync/tests/unit/test_errorhandler_2.js
services/sync/tests/unit/test_telemetry.js
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -87,16 +87,22 @@ ChromeUtils.defineModuleGetter(
 );
 
 ChromeUtils.defineModuleGetter(
   this,
   "FxAccountsProfile",
   "resource://gre/modules/FxAccountsProfile.jsm"
 );
 
+ChromeUtils.defineModuleGetter(
+  this,
+  "FxAccountsTelemetry",
+  "resource://gre/modules/FxAccountsTelemetry.jsm"
+);
+
 XPCOMUtils.defineLazyModuleGetters(this, {
   Preferences: "resource://gre/modules/Preferences.jsm",
 });
 
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "FXA_ENABLED",
   "identity.fxaccounts.enabled",
@@ -404,16 +410,20 @@ class FxAccounts {
   get device() {
     return this._internal.device;
   }
 
   get keys() {
     return this._internal.keys;
   }
 
+  get telemetry() {
+    return this._internal.telemetry;
+  }
+
   _withCurrentAccountState(func) {
     return this._internal.withCurrentAccountState(func);
   }
 
   _withVerifiedAccountState(func) {
     return this._internal.withVerifiedAccountState(func);
   }
 
@@ -853,16 +863,24 @@ FxAccountsInternal.prototype = {
   _device: null,
   get device() {
     if (!this._device) {
       this._device = new FxAccountsDevice(this);
     }
     return this._device;
   },
 
+  _telemetry: null,
+  get telemetry() {
+    if (!this._telemetry) {
+      this._telemetry = new FxAccountsTelemetry();
+    }
+    return this._telemetry;
+  },
+
   // A hook-point for tests who may want a mocked AccountState or mocked storage.
   newAccountState(credentials) {
     let storage = new FxAccountsStorageManager();
     storage.initialize(credentials);
     return new AccountState(storage);
   },
 
   notifyDevices(deviceIds, excludedIds, payload, TTL) {
--- a/services/fxaccounts/FxAccountsCommands.js
+++ b/services/fxaccounts/FxAccountsCommands.js
@@ -1,15 +1,15 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const EXPORTED_SYMBOLS = ["SendTab", "FxAccountsCommands"];
 
-const { COMMAND_SENDTAB, log } = ChromeUtils.import(
+const { COMMAND_SENDTAB, COMMAND_SENDTAB_TAIL, log } = ChromeUtils.import(
   "resource://gre/modules/FxAccountsCommon.js"
 );
 ChromeUtils.defineModuleGetter(
   this,
   "PushCrypto",
   "resource://gre/modules/PushCrypto.jsm"
 );
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
@@ -144,17 +144,17 @@ class FxAccountsCommands {
       if (!sender) {
         log.warn(
           "Incoming command is from an unknown device (maybe disconnected?)"
         );
       }
       switch (command) {
         case COMMAND_SENDTAB:
           try {
-            const { title, uri } = await this.sendTab.handle(payload);
+            const { title, uri } = await this.sendTab.handle(senderId, payload);
             log.info(
               `Tab received with FxA commands: ${title} from ${
                 sender ? sender.name : "Unknown device"
               }.`
             );
             tabsReceived.push({ title, uri, sender });
           } catch (e) {
             log.error(`Error while handling incoming Send Tab payload.`, e);
@@ -187,30 +187,35 @@ class SendTab {
    * @param {Object} tab
    * @param {string} tab.url
    * @param {string} tab.title
    * @returns A report object, in the shape of
    *          {succeded: [Device], error: [{device: Device, error: Exception}]}
    */
   async send(to, tab) {
     log.info(`Sending a tab to ${to.length} devices.`);
+    const flowID = this._fxai.telemetry.generateFlowID();
     const encoder = new TextEncoder("utf8");
-    const data = {
-      entries: [{ title: tab.title, url: tab.url }],
-    };
+    const data = { entries: [{ title: tab.title, url: tab.url }] };
     const bytes = encoder.encode(JSON.stringify(data));
     const report = {
       succeeded: [],
       failed: [],
     };
     for (let device of to) {
       try {
         const encrypted = await this._encrypt(bytes, device);
-        const payload = { encrypted };
+        const payload = { encrypted, flowID };
         await this._commands.invoke(COMMAND_SENDTAB, device, payload); // FxA needs an object.
+        this._fxai.telemetry.recordEvent(
+          "command-sent",
+          COMMAND_SENDTAB_TAIL,
+          this._fxai.telemetry.sanitizeDeviceId(device.id),
+          { flowID }
+        );
         report.succeeded.push(device);
       } catch (error) {
         log.error("Error while invoking a send tab command.", error);
         report.failed.push({ device, error });
       }
     }
     return report;
   }
@@ -223,24 +228,31 @@ class SendTab {
         true
       ) &&
       device.availableCommands &&
       device.availableCommands[COMMAND_SENDTAB]
     );
   }
 
   // Handle incoming send tab payload, called by FxAccountsCommands.
-  async handle({ encrypted }) {
+  async handle(senderID, { encrypted, flowID }) {
     const bytes = await this._decrypt(encrypted);
     const decoder = new TextDecoder("utf8");
     const data = JSON.parse(decoder.decode(bytes));
     const current = data.hasOwnProperty("current")
       ? data.current
       : data.entries.length - 1;
     const { title, url: uri } = data.entries[current];
+    this._fxai.telemetry.recordEvent(
+      "command-received",
+      COMMAND_SENDTAB_TAIL,
+      this._fxai.telemetry.sanitizeDeviceId(senderID),
+      { flowID }
+    );
+
     return {
       title,
       uri,
     };
   }
 
   async _encrypt(bytes, device) {
     let bundle = device.availableCommands[COMMAND_SENDTAB];
--- a/services/fxaccounts/FxAccountsCommon.js
+++ b/services/fxaccounts/FxAccountsCommon.js
@@ -73,17 +73,22 @@ exports.ON_VERIFY_LOGIN_NOTIFICATION = "
 exports.ON_COMMAND_RECEIVED_NOTIFICATION = "fxaccounts:command_received";
 
 exports.FXA_PUSH_SCOPE_ACCOUNT_UPDATE = "chrome://fxa-device-update";
 
 exports.ON_PROFILE_CHANGE_NOTIFICATION = "fxaccounts:profilechange"; // WebChannel
 exports.ON_ACCOUNT_STATE_CHANGE_NOTIFICATION = "fxaccounts:statechange";
 exports.ON_NEW_DEVICE_ID = "fxaccounts:new_device_id";
 
-exports.COMMAND_SENDTAB = "https://identity.mozilla.com/cmd/open-uri";
+// The common prefix for all commands.
+exports.COMMAND_PREFIX = "https://identity.mozilla.com/cmd/";
+
+// The commands we support - only the _TAIL values are recorded in telemetry.
+exports.COMMAND_SENDTAB_TAIL = "open-uri";
+exports.COMMAND_SENDTAB = exports.COMMAND_PREFIX + exports.COMMAND_SENDTAB_TAIL;
 
 // OAuth
 exports.FX_OAUTH_CLIENT_ID = "5882386c6d801776";
 exports.SCOPE_PROFILE = "profile";
 exports.SCOPE_OLD_SYNC = "https://identity.mozilla.com/apps/oldsync";
 
 // UI Requests.
 exports.UI_REQUEST_SIGN_IN_FLOW = "signInFlow";
new file mode 100644
--- /dev/null
+++ b/services/fxaccounts/FxAccountsTelemetry.jsm
@@ -0,0 +1,55 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+// FxA Telemetry support. For hysterical raisins, the actual implementation
+// is inside "sync". We should move the core implementation somewhere that's
+// sanely shared (eg, services-common?), but let's wait and see where we end up
+// first...
+
+// We use this observers module because we leverage its support for richer
+// "subject" data.
+const { Observers } = ChromeUtils.import(
+  "resource://services-common/observers.js"
+);
+
+class FxAccountsTelemetry {
+  recordEvent(object, method, value, extra = undefined) {
+    // We need to ensure the telemetry module is loaded.
+    ChromeUtils.import("resource://services-sync/telemetry.js");
+    // Now it will be listening for the notifications...
+    Observers.notify("fxa:telemetry:event", { object, method, value, extra });
+  }
+
+  // A flow ID can be anything that's "probably" unique, so for now use a UUID.
+  generateFlowID() {
+    return Cc["@mozilla.org/uuid-generator;1"]
+      .getService(Ci.nsIUUIDGenerator)
+      .generateUUID()
+      .toString()
+      .slice(1, -1);
+  }
+
+  // Sanitize the ID of a device into something suitable for including in the
+  // ping. Returns null if no transformation is possible.
+  sanitizeDeviceId(deviceId) {
+    // We only know how to hash it for sync users, which kinda sucks.
+    let xps =
+      this._weaveXPCOM ||
+      Cc["@mozilla.org/weave/service;1"].getService(Ci.nsISupports)
+        .wrappedJSObject;
+    if (!xps.enabled) {
+      return null;
+    }
+    try {
+      return xps.Weave.Service.identity.hashedDeviceID(deviceId);
+    } catch {
+      // sadly this can happen in various scenarios, so don't complain.
+    }
+    return null;
+  }
+}
+
+var EXPORTED_SYMBOLS = ["FxAccountsTelemetry"];
--- a/services/fxaccounts/moz.build
+++ b/services/fxaccounts/moz.build
@@ -25,14 +25,15 @@ EXTRA_JS_MODULES += [
   'FxAccountsDevice.jsm',
   'FxAccountsKeys.jsm',
   'FxAccountsPairing.jsm',
   'FxAccountsPairingChannel.js',
   'FxAccountsProfile.jsm',
   'FxAccountsProfileClient.jsm',
   'FxAccountsPush.jsm',
   'FxAccountsStorage.jsm',
+  'FxAccountsTelemetry.jsm',
   'FxAccountsWebChannel.jsm',
 ]
 
 XPCOM_MANIFESTS += [
     'components.conf',
 ]
--- a/services/fxaccounts/tests/xpcshell/test_commands.js
+++ b/services/fxaccounts/tests/xpcshell/test_commands.js
@@ -2,16 +2,46 @@
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 const { FxAccountsCommands, SendTab } = ChromeUtils.import(
   "resource://gre/modules/FxAccountsCommands.js"
 );
 
+const { COMMAND_SENDTAB, COMMAND_SENDTAB_TAIL } = ChromeUtils.import(
+  "resource://gre/modules/FxAccountsCommon.js"
+);
+
+class TelemetryMock {
+  constructor() {
+    this._events = [];
+    this._uuid_counter = 0;
+  }
+
+  recordEvent(object, method, value, extra = undefined) {
+    this._events.push({ object, method, value, extra });
+  }
+
+  generateFlowID() {
+    this._uuid_counter += 1;
+    return this._uuid_counter.toString();
+  }
+
+  sanitizeDeviceId(id) {
+    return id + "-san";
+  }
+}
+
+function FxaInternalMock() {
+  return {
+    telemetry: new TelemetryMock(),
+  };
+}
+
 add_task(async function test_sendtab_isDeviceCompatible() {
   const sendTab = new SendTab(null, null);
   let device = { name: "My device" };
   Assert.ok(!sendTab.isDeviceCompatible(device));
   device = { name: "My device", availableCommands: {} };
   Assert.ok(!sendTab.isDeviceCompatible(device));
   device = {
     name: "My device",
@@ -26,34 +56,95 @@ add_task(async function test_sendtab_sen
   const commands = {
     invoke: sinon.spy((cmd, device, payload) => {
       if (device.name == "Device 1") {
         throw new Error("Invoke error!");
       }
       Assert.equal(payload.encrypted, "encryptedpayload");
     }),
   };
-  const sendTab = new SendTab(commands, null);
+  const fxai = FxaInternalMock();
+  const sendTab = new SendTab(commands, fxai);
   sendTab._encrypt = (bytes, device) => {
     if (device.name == "Device 2") {
       throw new Error("Encrypt error!");
     }
     return "encryptedpayload";
   };
-  const to = [{ name: "Device 1" }, { name: "Device 2" }, { name: "Device 3" }];
+  const to = [
+    { name: "Device 1" },
+    { name: "Device 2" },
+    { id: "dev3", name: "Device 3" },
+  ];
   const tab = { title: "Foo", url: "https://foo.bar/" };
   const report = await sendTab.send(to, tab);
   Assert.equal(report.succeeded.length, 1);
   Assert.equal(report.failed.length, 2);
   Assert.equal(report.succeeded[0].name, "Device 3");
   Assert.equal(report.failed[0].device.name, "Device 1");
   Assert.equal(report.failed[0].error.message, "Invoke error!");
   Assert.equal(report.failed[1].device.name, "Device 2");
   Assert.equal(report.failed[1].error.message, "Encrypt error!");
   Assert.ok(commands.invoke.calledTwice);
+  Assert.deepEqual(fxai.telemetry._events, [
+    {
+      object: "command-sent",
+      method: COMMAND_SENDTAB_TAIL,
+      value: "dev3-san",
+      extra: { flowID: "1" },
+    },
+  ]);
+});
+
+add_task(async function test_sendtab_receive() {
+  // We are testing 'receive' here, but might as well go through 'send'
+  // to package the data and for additional testing...
+  const commands = {
+    _invokes: [],
+    invoke(cmd, device, payload) {
+      this._invokes.push({ cmd, device, payload });
+    },
+  };
+
+  const fxai = FxaInternalMock();
+  const sendTab = new SendTab(commands, fxai);
+  sendTab._encrypt = (bytes, device) => {
+    return bytes;
+  };
+  sendTab._decrypt = bytes => {
+    return bytes;
+  };
+  const tab = { title: "tab title", url: "http://example.com" };
+  const to = [{ id: "devid", name: "The Device" }];
+
+  await sendTab.send(to, tab);
+  Assert.equal(commands._invokes.length, 1);
+
+  for (let { cmd, device, payload } of commands._invokes) {
+    Assert.equal(cmd, COMMAND_SENDTAB);
+    Assert.deepEqual(await sendTab.handle(device.id, payload), {
+      title: "tab title",
+      uri: "http://example.com",
+    });
+  }
+
+  Assert.deepEqual(fxai.telemetry._events, [
+    {
+      object: "command-sent",
+      method: COMMAND_SENDTAB_TAIL,
+      value: "devid-san",
+      extra: { flowID: "1" },
+    },
+    {
+      object: "command-received",
+      method: COMMAND_SENDTAB_TAIL,
+      value: "devid-san",
+      extra: { flowID: "1" },
+    },
+  ]);
 });
 
 add_task(async function test_commands_pollDeviceCommands_push() {
   // Server state.
   const remoteMessages = [
     {
       index: 11,
       data: {},
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -4,55 +4,37 @@
 
 "use strict";
 
 var EXPORTED_SYMBOLS = ["SyncTelemetry"];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
-const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
-const { Log } = ChromeUtils.import("resource://gre/modules/Log.jsm");
-const { AuthenticationError } = ChromeUtils.import(
-  "resource://services-sync/browserid_identity.js"
-);
-const { Weave } = ChromeUtils.import("resource://services-sync/main.js");
-const { Status } = ChromeUtils.import("resource://services-sync/status.js");
-const { Svc } = ChromeUtils.import("resource://services-sync/util.js");
-const { Resource } = ChromeUtils.import("resource://services-sync/resource.js");
-const { Observers } = ChromeUtils.import(
-  "resource://services-common/observers.js"
-);
-const { Async } = ChromeUtils.import("resource://services-common/async.js");
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  Async: "resource://services-common/async.js",
+  AuthenticationError: "resource://services-sync/browserid_identity.js",
+  Log: "resource://gre/modules/Log.jsm",
+  ObjectUtils: "resource://gre/modules/ObjectUtils.jsm",
+  Observers: "resource://services-common/observers.js",
+  OS: "resource://gre/modules/osfile.jsm",
+  Resource: "resource://services-sync/resource.js",
+  Services: "resource://gre/modules/Services.jsm",
+  Status: "resource://services-sync/status.js",
+  Svc: "resource://services-sync/util.js",
+  TelemetryController: "resource://gre/modules/TelemetryController.jsm",
+  TelemetryEnvironment: "resource://gre/modules/TelemetryEnvironment.jsm",
+  TelemetryUtils: "resource://gre/modules/TelemetryUtils.jsm",
+  Weave: "resource://services-sync/main.js",
+});
 
 let constants = {};
 ChromeUtils.import("resource://services-sync/constants.js", constants);
 
-ChromeUtils.defineModuleGetter(
-  this,
-  "TelemetryController",
-  "resource://gre/modules/TelemetryController.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "TelemetryUtils",
-  "resource://gre/modules/TelemetryUtils.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "TelemetryEnvironment",
-  "resource://gre/modules/TelemetryEnvironment.jsm"
-);
-ChromeUtils.defineModuleGetter(
-  this,
-  "ObjectUtils",
-  "resource://gre/modules/ObjectUtils.jsm"
-);
-ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
-
 XPCOMUtils.defineLazyServiceGetter(
   this,
   "Telemetry",
   "@mozilla.org/base/telemetry;1",
   "nsITelemetry"
 );
 
 const log = Log.repository.getLogger("Sync.Telemetry");
@@ -69,16 +51,18 @@ const TOPICS = [
   "weave:engine:sync:applied",
   "weave:engine:sync:step",
   "weave:engine:sync:uploaded",
   "weave:engine:validate:finish",
   "weave:engine:validate:error",
 
   "weave:telemetry:event",
   "weave:telemetry:histogram",
+  // and we are now used by FxA, so a custom event for that.
+  "fxa:telemetry:event",
 ];
 
 const PING_FORMAT_VERSION = 1;
 
 const EMPTY_UID = "0".repeat(32);
 
 // The set of engines we record telemetry for - any other engines are ignored.
 const ENGINES = new Set([
@@ -691,16 +675,30 @@ class SyncTelemetryImpl {
       return newDeviceID != this.lastDeviceID;
     }
     // We've gone from knowing one of the ids to not knowing it (which we
     // ignore) or we've gone from not knowing it to knowing it (which is fine),
     // so we shouldn't submit.
     return false;
   }
 
+  maybeSubmitForInterval() {
+    // We want to submit the ping every `this.submissionInterval` but only when
+    // there's no current sync in progress, otherwise we may end up submitting
+    // the sync and the events caused by it in different pings.
+    if (
+      this.current == null &&
+      Telemetry.msSinceProcessStart() - this.lastSubmissionTime >
+        this.submissionInterval
+    ) {
+      this.finish("schedule");
+      this.lastSubmissionTime = Telemetry.msSinceProcessStart();
+    }
+  }
+
   onSyncFinished(error) {
     if (!this.current) {
       log.warn("onSyncFinished but we aren't recording");
       return;
     }
     this.current.finished(error);
     if (this.payloads.length) {
       if (
@@ -719,23 +717,17 @@ class SyncTelemetryImpl {
       this.lastDeviceID = this.current.deviceID;
     }
     if (this.payloads.length < this.maxPayloadCount) {
       this.payloads.push(this.current.toJSON());
     } else {
       ++this.discarded;
     }
     this.current = null;
-    if (
-      Telemetry.msSinceProcessStart() - this.lastSubmissionTime >
-      this.submissionInterval
-    ) {
-      this.finish("schedule");
-      this.lastSubmissionTime = Telemetry.msSinceProcessStart();
-    }
+    this.maybeSubmitForInterval();
   }
 
   _addHistogram(hist) {
     let histogram = Telemetry.getHistogramById(hist);
     let s = histogram.snapshot();
     this.histograms[hist] = s;
   }
 
@@ -771,16 +763,17 @@ class SyncTelemetryImpl {
       if (extra) {
         event.push(extra);
       }
     } else if (extra) {
       event.push(null); // a null for the empty value.
       event.push(extra);
     }
     this.events.push(event);
+    this.maybeSubmitForInterval();
   }
 
   observe(subject, topic, data) {
     log.trace(`observed ${topic} ${data}`);
 
     switch (topic) {
       case "profile-before-change":
         this.shutdown();
@@ -848,16 +841,17 @@ class SyncTelemetryImpl {
 
       case "weave:engine:validate:error":
         if (this._checkCurrent(topic)) {
           this.current.onEngineValidateError(data, subject || "Unknown");
         }
         break;
 
       case "weave:telemetry:event":
+      case "fxa:telemetry:event":
         this._recordEvent(subject);
         break;
 
       case "weave:telemetry:histogram":
         this._addHistogram(data);
         break;
 
       default:
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -385,19 +385,50 @@ async function wait_for_ping(callback, a
   }
   if (getFullPing) {
     return record;
   }
   equal(record.syncs.length, 1);
   return record.syncs[0];
 }
 
-// Short helper for wait_for_ping
-function sync_and_validate_telem(allowErrorPings, getFullPing = false) {
-  return wait_for_ping(() => Service.sync(), allowErrorPings, getFullPing);
+// Perform a sync and validate all telemetry caused by the sync. If fnValidate
+// is null, we just check the ping records success. If fnValidate is specified,
+// then the sync must have recorded just a single sync, and that sync will be
+// passed to the function to be checked.
+async function sync_and_validate_telem(fnValidate = null) {
+  let numErrors = 0;
+  let telem = get_sync_test_telemetry();
+  let oldSubmit = telem.submit;
+  try {
+    telem.submit = function(record) {
+      // This is called via an observer, so failures here don't cause the test
+      // to fail :(
+      try {
+        // All pings must be valid.
+        assert_valid_ping(record);
+        if (fnValidate) {
+          // for historical reasons these callbacks expect a "sync" record, not
+          // the entire ping.
+          Assert.equal(record.syncs.length, 1);
+          fnValidate(record.syncs[0]);
+        } else {
+          // no validation function means it must be a "success" ping.
+          assert_success_ping(record);
+        }
+      } catch (ex) {
+        print("Failure in ping validation callback", ex, "\n", ex.stack);
+        numErrors += 1;
+      }
+    };
+    await Service.sync();
+    Assert.ok(numErrors == 0, "There were telemetry validation errors");
+  } finally {
+    telem.submit = oldSubmit;
+  }
 }
 
 // Used for the (many) cases where we do a 'partial' sync, where only a single
 // engine is actually synced, but we still want to ensure we're generating a
 // valid ping. Returns a promise that resolves to the ping, or rejects with the
 // thrown error after calling an optional callback.
 async function sync_engine_and_validate_telem(
   engine,
--- a/services/sync/tests/unit/test_corrupt_keys.js
+++ b/services/sync/tests/unit/test_corrupt_keys.js
@@ -128,18 +128,22 @@ add_task(async function test_locally_cha
     _(
       "Keys now: " + Service.collectionKeys.keyForCollection("history").keyPair
     );
 
     Assert.equal(hmacErrorCount, 0);
 
     _("HMAC error count: " + hmacErrorCount);
     // Now syncing should succeed, after one HMAC error.
-    let ping = await wait_for_ping(() => Service.sync(), true);
-    equal(ping.engines.find(e => e.name == "history").incoming.applied, 5);
+    await sync_and_validate_telem(ping => {
+      Assert.equal(
+        ping.engines.find(e => e.name == "history").incoming.applied,
+        5
+      );
+    });
 
     Assert.equal(hmacErrorCount, 1);
     _(
       "Keys now: " + Service.collectionKeys.keyForCollection("history").keyPair
     );
 
     // And look! We downloaded history!
     Assert.ok(
@@ -184,22 +188,23 @@ add_task(async function test_locally_cha
 
     _("Server key time hasn't changed.");
     Assert.equal(johndoe.modified("crypto"), old_key_time);
 
     _("Resetting HMAC error timer.");
     Service.lastHMACEvent = 0;
 
     _("Syncing...");
-    ping = await sync_and_validate_telem(true);
+    await sync_and_validate_telem(ping => {
+      Assert.equal(
+        ping.engines.find(e => e.name == "history").incoming.failed,
+        5
+      );
+    });
 
-    Assert.equal(
-      ping.engines.find(e => e.name == "history").incoming.failed,
-      5
-    );
     _(
       "Keys now: " + Service.collectionKeys.keyForCollection("history").keyPair
     );
     _(
       "Server keys have been updated, and we skipped over 5 more HMAC errors without adjusting history."
     );
     Assert.ok(johndoe.modified("crypto") > old_key_time);
     Assert.equal(hmacErrorCount, 6);
--- a/services/sync/tests/unit/test_errorhandler_1.js
+++ b/services/sync/tests/unit/test_errorhandler_1.js
@@ -58,18 +58,19 @@ add_task(async function test_401_logout(
     }
   });
 
   // Make sync fail due to login rejected.
   await configureIdentity({ username: "janedoe" }, server);
   Service._updateCachedURLs();
 
   _("Starting first sync.");
-  let ping = await sync_and_validate_telem(true);
-  deepEqual(ping.failureReason, { name: "httperror", code: 401 });
+  await sync_and_validate_telem(ping => {
+    deepEqual(ping.failureReason, { name: "httperror", code: 401 });
+  });
   _("First sync done.");
 
   await promiseErrors;
   Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR);
   Assert.ok(!Service.isLoggedIn);
 
   // Clean up.
   await Service.startOver();
@@ -84,21 +85,22 @@ add_task(async function test_credentials
 
   // By calling sync, we ensure we're logged in.
   await sync_and_validate_telem();
   Assert.equal(Status.sync, SYNC_SUCCEEDED);
   Assert.ok(Service.isLoggedIn);
 
   await EHTestsCommon.generateCredentialsChangedFailure();
 
-  let ping = await sync_and_validate_telem(true);
-  equal(ping.status.sync, CREDENTIALS_CHANGED);
-  deepEqual(ping.failureReason, {
-    name: "unexpectederror",
-    error: "Error: Aborting sync, remote setup failed",
+  await sync_and_validate_telem(ping => {
+    equal(ping.status.sync, CREDENTIALS_CHANGED);
+    deepEqual(ping.failureReason, {
+      name: "unexpectederror",
+      error: "Error: Aborting sync, remote setup failed",
+    });
   });
 
   Assert.equal(Status.sync, CREDENTIALS_CHANGED);
   Assert.ok(!Service.isLoggedIn);
 
   // Clean up.
   await Service.startOver();
   await promiseStopServer(server);
@@ -130,21 +132,22 @@ add_task(async function test_sync_non_ne
 
   // By calling sync, we ensure we're logged in.
   await Service.sync();
   Assert.equal(Status.sync, SYNC_SUCCEEDED);
   Assert.ok(Service.isLoggedIn);
 
   await EHTestsCommon.generateCredentialsChangedFailure();
 
-  let ping = await sync_and_validate_telem(true);
-  equal(ping.status.sync, CREDENTIALS_CHANGED);
-  deepEqual(ping.failureReason, {
-    name: "unexpectederror",
-    error: "Error: Aborting sync, remote setup failed",
+  await sync_and_validate_telem(ping => {
+    equal(ping.status.sync, CREDENTIALS_CHANGED);
+    deepEqual(ping.failureReason, {
+      name: "unexpectederror",
+      error: "Error: Aborting sync, remote setup failed",
+    });
   });
 
   Assert.equal(Status.sync, CREDENTIALS_CHANGED);
   // If we clean this tick, telemetry won't get the right error
   await Async.promiseYield();
   await clean();
   await promiseStopServer(server);
 });
@@ -247,21 +250,22 @@ add_task(async function test_sync_server
   await EHTestsCommon.setUp(server);
 
   const BACKOFF = 42;
   engine.enabled = true;
   engine.exception = { status: 503, headers: { "retry-after": BACKOFF } };
 
   Assert.equal(Status.service, STATUS_OK);
 
-  let ping = await sync_and_validate_telem(true);
-  equal(ping.status.sync, SERVER_MAINTENANCE);
-  deepEqual(ping.engines.find(e => e.failureReason).failureReason, {
-    name: "httperror",
-    code: 503,
+  await sync_and_validate_telem(ping => {
+    equal(ping.status.sync, SERVER_MAINTENANCE);
+    deepEqual(ping.engines.find(e => e.failureReason).failureReason, {
+      name: "httperror",
+      code: 503,
+    });
   });
 
   Assert.equal(Status.service, SYNC_FAILED_PARTIAL);
   Assert.equal(Status.sync, SERVER_MAINTENANCE);
 
   await clean();
   await promiseStopServer(server);
 });
--- a/services/sync/tests/unit/test_errorhandler_2.js
+++ b/services/sync/tests/unit/test_errorhandler_2.js
@@ -98,34 +98,34 @@ add_task(async function test_lastSync_no
   // Test info/collections prolonged server maintenance errors are reported.
   let server = await EHTestsCommon.sync_httpd_setup();
   await EHTestsCommon.setUp(server);
 
   await configureIdentity({ username: "johndoe" }, server);
 
   // Do an initial sync that we expect to be successful.
   let promiseObserved = promiseOneObserver("weave:service:reset-file-log");
-  await sync_and_validate_telem(false);
+  await sync_and_validate_telem();
   await promiseObserved;
 
   Assert.equal(Status.service, STATUS_OK);
   Assert.equal(Status.sync, SYNC_SUCCEEDED);
 
   let lastSync = Svc.Prefs.get("lastSync");
 
   Assert.ok(lastSync);
 
   // Report server maintenance on info/collections requests
   server.registerPathHandler(
     "/1.1/johndoe/info/collections",
     EHTestsCommon.service_unavailable
   );
 
   promiseObserved = promiseOneObserver("weave:service:reset-file-log");
-  await sync_and_validate_telem(true);
+  await sync_and_validate_telem(() => {});
   await promiseObserved;
 
   Assert.equal(Status.sync, SERVER_MAINTENANCE);
   Assert.equal(Status.service, SYNC_FAILED);
 
   // We shouldn't update lastSync on complete failure.
   Assert.equal(lastSync, Svc.Prefs.get("lastSync"));
 
@@ -414,19 +414,20 @@ add_task(async function test_sync_engine
       Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() {
         Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog);
         res();
       });
     });
   });
 
   Assert.ok(await EHTestsCommon.setUp(server));
-  let ping = await sync_and_validate_telem(true);
-  deepEqual(ping.status.service, SYNC_FAILED_PARTIAL);
-  deepEqual(ping.engines.find(e => e.status).status, ENGINE_UNKNOWN_FAIL);
+  await sync_and_validate_telem(ping => {
+    deepEqual(ping.status.service, SYNC_FAILED_PARTIAL);
+    deepEqual(ping.engines.find(e => e.status).status, ENGINE_UNKNOWN_FAIL);
+  });
 
   await promiseObserved;
 
   _("Status.engines: " + JSON.stringify(Status.engines));
   Assert.equal(Status.engines.catapult, ENGINE_UNKNOWN_FAIL);
   Assert.equal(Status.service, SYNC_FAILED_PARTIAL);
 
   // lastSync should update on partial failure.
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -127,17 +127,17 @@ add_task(async function test_basic() {
       coll,
       new ServerCollection({}, true).handler()
     );
   }
 
   let server = httpd_setup(handlers);
   await configureIdentity({ username: "johndoe" }, server);
 
-  let ping = await sync_and_validate_telem(true, true);
+  let ping = await wait_for_ping(() => Service.sync(), true, true);
 
   // Check the "os" block - we can't really check specific values, but can
   // check it smells sane.
   ok(ping.os, "there is an OS block");
   ok("name" in ping.os, "there is an OS name");
   ok("version" in ping.os, "there is an OS version");
   ok("locale" in ping.os, "there is an OS locale");
 
@@ -421,21 +421,22 @@ add_task(async function test_generic_eng
 
   try {
     const changes = await engine._tracker.getChangedIDs();
     _(
       `test_generic_engine_fail: Steam tracker contents: ${JSON.stringify(
         changes
       )}`
     );
-    let ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
-      name: "unexpectederror",
-      error: String(e),
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
+        name: "unexpectederror",
+        error: String(e),
+      });
     });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_engine_fail_weird_errors() {
@@ -443,28 +444,30 @@ add_task(async function test_engine_fail
   await Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
   engine.enabled = true;
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);
   try {
     let msg = "Bad things happened!";
     engine._errToThrow = { message: msg };
-    let ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
-      name: "unexpectederror",
-      error: "Bad things happened!",
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
+        name: "unexpectederror",
+        error: "Bad things happened!",
+      });
     });
     let e = { msg };
     engine._errToThrow = e;
-    ping = await sync_and_validate_telem(true);
-    deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
-      name: "unexpectederror",
-      error: JSON.stringify(e),
+    await sync_and_validate_telem(ping => {
+      deepEqual(ping.engines.find(err => err.name === "steam").failureReason, {
+        name: "unexpectederror",
+        error: JSON.stringify(e),
+      });
     });
   } finally {
     await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_overrideTelemetryName() {
@@ -480,41 +483,42 @@ add_task(async function test_overrideTel
   const problemsToReport = [
     { name: "someProblem", count: 123 },
     { name: "anotherProblem", count: 456 },
   ];
 
   try {
     info("Sync with validation problems");
     engine.problemsToReport = problemsToReport;
-    let ping = await sync_and_validate_telem(true);
-    let enginePing = ping.engines.find(e => e.name === "steam-but-better");
-    ok(enginePing);
-    ok(!ping.engines.find(e => e.name === "steam"));
-
-    deepEqual(
-      enginePing.validation,
-      {
-        version: 1,
-        checked: 0,
-        problems: problemsToReport,
-      },
-      "Should include validation report with overridden name"
-    );
+    await sync_and_validate_telem(ping => {
+      let enginePing = ping.engines.find(e => e.name === "steam-but-better");
+      ok(enginePing);
+      ok(!ping.engines.find(e => e.name === "steam"));
+      deepEqual(
+        enginePing.validation,
+        {
+          version: 1,
+          checked: 0,
+          problems: problemsToReport,
+        },
+        "Should include validation report with overridden name"
+      );
+    });
 
     info("Sync without validation problems");
     engine.problemsToReport = null;
-    ping = await sync_and_validate_telem(true);
-    enginePing = ping.engines.find(e => e.name === "steam-but-better");
-    ok(enginePing);
-    ok(!ping.engines.find(e => e.name === "steam"));
-    ok(
-      !enginePing.validation,
-      "Should not include validation report when there are no problems"
-    );
+    await sync_and_validate_telem(ping => {
+      let enginePing = ping.engines.find(e => e.name === "steam-but-better");
+      ok(enginePing);
+      ok(!ping.engines.find(e => e.name === "steam"));
+      ok(
+        !enginePing.validation,
+        "Should not include validation report when there are no problems"
+      );
+    });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_engine_fail_ioerror() {
   enableValidationPrefs();
@@ -537,27 +541,28 @@ add_task(async function test_engine_fail
 
   try {
     const changes = await engine._tracker.getChangedIDs();
     _(
       `test_engine_fail_ioerror: Steam tracker contents: ${JSON.stringify(
         changes
       )}`
     );
-    let ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    let failureReason = ping.engines.find(e => e.name === "steam")
-      .failureReason;
-    equal(failureReason.name, "unexpectederror");
-    // ensure the profile dir in the exception message has been stripped.
-    ok(
-      !failureReason.error.includes(OS.Constants.Path.profileDir),
-      failureReason.error
-    );
-    ok(failureReason.error.includes("[profileDir]"), failureReason.error);
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      let failureReason = ping.engines.find(e => e.name === "steam")
+        .failureReason;
+      equal(failureReason.name, "unexpectederror");
+      // ensure the profile dir in the exception message has been stripped.
+      ok(
+        !failureReason.error.includes(OS.Constants.Path.profileDir),
+        failureReason.error
+      );
+      ok(failureReason.error.includes("[profileDir]"), failureReason.error);
+    });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_clean_urls() {
   enableValidationPrefs();
@@ -569,33 +574,36 @@ add_task(async function test_clean_urls(
   await SyncTestingInfrastructure(server);
   engine._errToThrow = new TypeError(
     "http://www.google .com is not a valid URL."
   );
 
   try {
     const changes = await engine._tracker.getChangedIDs();
     _(`test_clean_urls: Steam tracker contents: ${JSON.stringify(changes)}`);
-    let ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    let failureReason = ping.engines.find(e => e.name === "steam")
-      .failureReason;
-    equal(failureReason.name, "unexpectederror");
-    equal(failureReason.error, "<URL> is not a valid URL.");
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      let failureReason = ping.engines.find(e => e.name === "steam")
+        .failureReason;
+      equal(failureReason.name, "unexpectederror");
+      equal(failureReason.error, "<URL> is not a valid URL.");
+    });
     // Handle other errors that include urls.
     engine._errToThrow =
       "Other error message that includes some:url/foo/bar/ in it.";
-    ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    failureReason = ping.engines.find(e => e.name === "steam").failureReason;
-    equal(failureReason.name, "unexpectederror");
-    equal(
-      failureReason.error,
-      "Other error message that includes <URL> in it."
-    );
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      let failureReason = ping.engines.find(e => e.name === "steam")
+        .failureReason;
+      equal(failureReason.name, "unexpectederror");
+      equal(
+        failureReason.error,
+        "Other error message that includes <URL> in it."
+      );
+    });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_initial_sync_engines() {
   enableValidationPrefs();
@@ -654,25 +662,26 @@ add_task(async function test_nserror() {
   await SyncTestingInfrastructure(server);
   engine._errToThrow = Components.Exception(
     "NS_ERROR_UNKNOWN_HOST",
     Cr.NS_ERROR_UNKNOWN_HOST
   );
   try {
     const changes = await engine._tracker.getChangedIDs();
     _(`test_nserror: Steam tracker contents: ${JSON.stringify(changes)}`);
-    let ping = await sync_and_validate_telem(true);
-    deepEqual(ping.status, {
-      service: SYNC_FAILED_PARTIAL,
-      sync: LOGIN_FAILED_NETWORK_ERROR,
-    });
-    let enginePing = ping.engines.find(e => e.name === "steam");
-    deepEqual(enginePing.failureReason, {
-      name: "nserror",
-      code: Cr.NS_ERROR_UNKNOWN_HOST,
+    await sync_and_validate_telem(ping => {
+      deepEqual(ping.status, {
+        service: SYNC_FAILED_PARTIAL,
+        sync: LOGIN_FAILED_NETWORK_ERROR,
+      });
+      let enginePing = ping.engines.find(e => e.name === "steam");
+      deepEqual(enginePing.failureReason, {
+        name: "nserror",
+        code: Cr.NS_ERROR_UNKNOWN_HOST,
+      });
     });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_sync_why() {
@@ -752,109 +761,173 @@ add_task(async function test_discarding(
     telem.submit = () =>
       ok(false, "Submitted telemetry ping when we should not have");
 
     for (let i = 0; i < 5; ++i) {
       await Service.sync();
     }
     telem.submit = oldSubmit;
     telem.submissionInterval = -1;
-    let ping = await sync_and_validate_telem(true, true); // with this we've synced 6 times
+    let ping = await wait_for_ping(() => Service.sync(), true, true); // with this we've synced 6 times
     equal(ping.syncs.length, 2);
     equal(ping.discarded, 4);
   } finally {
     telem.maxPayloadCount = 500;
     telem.submissionInterval = -1;
     telem.submit = oldSubmit;
     if (server) {
       await promiseStopServer(server);
     }
   }
 });
 
+add_task(async function test_submit_interval() {
+  let telem = get_sync_test_telemetry();
+  let oldSubmit = telem.submit;
+  let numSubmissions = 0;
+  telem.submit = function() {
+    numSubmissions += 1;
+  };
+
+  function notify(what, data = null) {
+    Svc.Obs.notify(what, JSON.stringify(data));
+  }
+
+  try {
+    // submissionInterval is set such that each sync should submit
+    notify("weave:service:sync:start", { why: "testing" });
+    notify("weave:service:sync:finish");
+    Assert.equal(numSubmissions, 1, "should submit this ping due to interval");
+
+    // As should each event outside of a sync.
+    Service.recordTelemetryEvent("object", "method");
+    Assert.equal(numSubmissions, 2);
+
+    // But events while we are syncing should not.
+    notify("weave:service:sync:start", { why: "testing" });
+    Service.recordTelemetryEvent("object", "method");
+    Assert.equal(numSubmissions, 2, "no submission for this event");
+    notify("weave:service:sync:finish");
+    Assert.equal(numSubmissions, 3, "was submitted after sync finish");
+  } finally {
+    telem.submit = oldSubmit;
+  }
+});
+
 add_task(async function test_no_foreign_engines_in_error_ping() {
   enableValidationPrefs();
 
   await Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = await serverForFoo(engine);
   engine._errToThrow = new Error("Oh no!");
   await SyncTestingInfrastructure(server);
   try {
-    let ping = await sync_and_validate_telem(true);
-    equal(ping.status.service, SYNC_FAILED_PARTIAL);
-    ok(ping.engines.every(e => e.name !== "bogus"));
+    await sync_and_validate_telem(ping => {
+      equal(ping.status.service, SYNC_FAILED_PARTIAL);
+      ok(ping.engines.every(e => e.name !== "bogus"));
+    });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_no_foreign_engines_in_success_ping() {
   enableValidationPrefs();
 
   await Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = await serverForFoo(engine);
 
   await SyncTestingInfrastructure(server);
   try {
-    let ping = await sync_and_validate_telem();
-    ok(ping.engines.every(e => e.name !== "bogus"));
+    await sync_and_validate_telem(ping => {
+      ok(ping.engines.every(e => e.name !== "bogus"));
+    });
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });
 
 add_task(async function test_events() {
   enableValidationPrefs();
 
   await Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = await serverForFoo(engine);
 
   await SyncTestingInfrastructure(server);
+
+  let telem = get_sync_test_telemetry();
+  telem.submissionInterval = Infinity;
+
   try {
     let serverTime = Resource.serverTime;
     Service.recordTelemetryEvent("object", "method", "value", { foo: "bar" });
     let ping = await wait_for_ping(() => Service.sync(), true, true);
     equal(ping.events.length, 1);
     let [timestamp, category, method, object, value, extra] = ping.events[0];
     ok(typeof timestamp == "number" && timestamp > 0); // timestamp.
     equal(category, "sync");
     equal(method, "method");
     equal(object, "object");
     equal(value, "value");
     deepEqual(extra, { foo: "bar", serverTime: String(serverTime) });
-    // Test with optional values.
-    Service.recordTelemetryEvent("object", "method");
-    ping = await wait_for_ping(() => Service.sync(), false, true);
+    ping = await wait_for_ping(
+      () => {
+        // Test with optional values.
+        Service.recordTelemetryEvent("object", "method");
+      },
+      false,
+      true
+    );
     equal(ping.events.length, 1);
     equal(ping.events[0].length, 4);
 
-    Service.recordTelemetryEvent("object", "method", "extra");
-    ping = await wait_for_ping(() => Service.sync(), false, true);
+    ping = await wait_for_ping(
+      () => {
+        Service.recordTelemetryEvent("object", "method", "extra");
+      },
+      false,
+      true
+    );
     equal(ping.events.length, 1);
     equal(ping.events[0].length, 5);
 
-    Service.recordTelemetryEvent("object", "method", undefined, { foo: "bar" });
-    ping = await wait_for_ping(() => Service.sync(), false, true);
+    ping = await wait_for_ping(
+      () => {
+        Service.recordTelemetryEvent("object", "method", undefined, {
+          foo: "bar",
+        });
+      },
+      false,
+      true
+    );
     equal(ping.events.length, 1);
     equal(ping.events[0].length, 6);
     [timestamp, category, method, object, value, extra] = ping.events[0];
     equal(value, null);
 
-    Service.recordTelemetryEvent("object", "method", undefined, { foo: "bar" });
-    let telem = get_sync_test_telemetry();
     // Fake a submission due to shutdown.
-    ping = await wait_for_ping(() => telem.finish("shutdown"), false, true);
+    ping = await wait_for_ping(
+      () => {
+        telem.submissionInterval = Infinity;
+        Service.recordTelemetryEvent("object", "method", undefined, {
+          foo: "bar",
+        });
+        telem.finish("shutdown");
+      },
+      false,
+      true
+    );
     equal(ping.syncs.length, 0);
     equal(ping.events.length, 1);
     equal(ping.events[0].length, 6);
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);
   }
 });