Bug 1267917 - Hook the sync bookmark validator into the new sync telemetry ping f?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Wed, 20 Jul 2016 13:37:02 -0400
changeset 390084 2267363878c86b27e15cafd537b96b8e5590a30a
parent 389642 8b59e729f75e91c15884574c46901169cdb9d8c2
child 525929 6f4c0f23fc84310e2ed85aac3b681c0e3f26c4e6
push id23595
push userbmo:tchiovoloni@mozilla.com
push dateWed, 20 Jul 2016 17:39:11 +0000
bugs1267917
milestone50.0a1
Bug 1267917 - Hook the sync bookmark validator into the new sync telemetry ping f?markh MozReview-Commit-ID: ECACktrOhRG
services/sync/modules/bookmark_validator.js
services/sync/modules/engines.js
services/sync/modules/engines/bookmarks.js
services/sync/modules/stages/enginesync.js
services/sync/modules/telemetry.js
services/sync/services-sync.js
services/sync/tests/unit/head_appinfo.js
services/sync/tests/unit/head_helpers.js
services/sync/tests/unit/sync_ping_schema.json
services/sync/tests/unit/test_bookmark_validator.js
toolkit/components/telemetry/docs/sync-ping.rst
--- a/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -2,16 +2,17 @@
  * 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";
 
 const Cu = Components.utils;
 
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
+Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/bookmark_utils.js");
 
 this.EXPORTED_SYMBOLS = ["BookmarkValidator", "BookmarkProblemData"];
 
 /**
  * Result of bookmark validation. Contains the following fields which describe
  * server-side problems unless otherwise specified.
@@ -71,24 +72,44 @@ class BookmarkProblemData {
     this.serverMissing = [];
     this.serverDeleted = [];
     this.serverUnexpected = [];
     this.differences = [];
     this.structuralDifferences = [];
   }
 
   /**
+   * Convert ("difference", [{ differences: ["tags", "name"] }, { differences: ["name"] }]) into
+   * [{ name: "difference:tags", count: 1}, { name: "difference:name", count: 2 }], etc.
+   */
+  _summarizeDifferences(prefix, diffs) {
+    let diffCounts = new Map();
+    for (let { differences } of diffs) {
+      for (let type of differences) {
+        let name = prefix + ":" + type;
+        let count = diffCounts.get(name) || 0;
+        diffCounts.set(name, count + 1);
+      }
+    }
+    return [...diffCounts].map(([name, count]) => ({ name, count }));
+  }
+
+  /**
    * Produce a list summarizing problems found. Each entry contains {name, count},
    * where name is the field name for the problem, and count is the number of times
    * the problem was encountered.
    *
    * Validation has failed if all counts are not 0.
+   *
+   * If the `full` argument is truthy, we also include information about which
+   * properties we saw structural differences in. Currently, this means either
+   * "sdiff:parentid" and "sdiff:childGUIDS" may be present.
    */
-  getSummary() {
-    return [
+  getSummary(full) {
+    let result = [
       { name: "clientMissing", count: this.clientMissing.length },
       { name: "serverMissing", count: this.serverMissing.length },
       { name: "serverDeleted", count: this.serverDeleted.length },
       { name: "serverUnexpected", count: this.serverUnexpected.length },
 
       { name: "structuralDifferences", count: this.structuralDifferences.length },
       { name: "differences", count: this.differences.length },
 
@@ -102,16 +123,21 @@ class BookmarkProblemData {
       { name: "missingChildren", count: this.missingChildren.length },
       { name: "multipleParents", count: this.multipleParents.length },
       { name: "deletedParents", count: this.deletedParents.length },
       { name: "childrenOnNonFolder", count: this.childrenOnNonFolder.length },
       { name: "duplicateChildren", count: this.duplicateChildren.length },
       { name: "parentNotFolder", count: this.parentNotFolder.length },
       { name: "wrongParentName", count: this.wrongParentName.length },
     ];
+    if (full) {
+      let structural = this._summarizeDifferences("sdiff", this.structuralDifferences);
+      result.push.apply(result, structural);
+    }
+    return result;
   }
 }
 
 class BookmarkValidator {
 
   createClientRecordsFromTree(clientTree) {
     // Iterate over the treeNode, converting it to something more similar to what
     // the server stores.
@@ -551,12 +577,46 @@ class BookmarkValidator {
         problemData.differences.push({id, differences});
       }
       if (structuralDifferences.length) {
         problemData.structuralDifferences.push({ id, differences: structuralDifferences });
       }
     }
     return inspectionInfo;
   }
+
+  _getServerState(engine) {
+    let collection = engine.itemSource();
+    let collectionKey = engine.service.collectionKeys.keyForCollection(engine.name);
+    collection.full = true;
+    let items = [];
+    collection.recordHandler = function(item) {
+      item.decrypt(collectionKey);
+      items.push(item.cleartext);
+    };
+    collection.get();
+    return items;
+  }
+
+  validate(engine) {
+    let self = this;
+    return Task.spawn(function*() {
+      let start = Date.now();
+      let clientTree = yield PlacesUtils.promiseBookmarksTree("", {
+        includeItemIds: true
+      });
+      let serverState = self._getServerState(engine);
+      let serverRecordCount = serverState.length;
+      let result = self.compareServerWithClient(serverState, clientTree);
+      let end = Date.now();
+      let duration = end-start;
+      return {
+        duration,
+        problems: result.problemData,
+        recordCount: serverRecordCount
+      };
+    });
+  }
+
 };
 
 
 
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -687,16 +687,25 @@ Engine.prototype = {
     this._tracker.ignoreAll = true;
     this._store.wipe();
     this._tracker.ignoreAll = false;
     this._tracker.clearChangedIDs();
   },
 
   wipeClient: function () {
     this._notify("wipe-client", this.name, this._wipeClient)();
+  },
+
+  /**
+   * If one exists, initialize and return a validator for this engine (which
+   * must have a `validate(engine)` method that returns a promise to an object
+   * with a getSummary method). Otherwise return null.
+   */
+  getValidator: function () {
+    return null;
   }
 };
 
 this.SyncEngine = function SyncEngine(name, service) {
   Engine.call(this, name || "SyncEngine", service);
 
   this.loadToFetch();
   this.loadPreviousFailed();
@@ -942,34 +951,34 @@ SyncEngine.prototype = {
     // Keep track of what to delete at the end of sync
     this._delete = {};
   },
 
   /**
    * A tiny abstraction to make it easier to test incoming record
    * application.
    */
-  _itemSource: function () {
+  itemSource: function () {
     return new Collection(this.engineURL, this._recordObj, this.service);
   },
 
   /**
    * Process incoming records.
    * In the most awful and untestable way possible.
    * This now accepts something that makes testing vaguely less impossible.
    */
   _processIncoming: function (newitems) {
     this._log.trace("Downloading & applying server changes");
 
     // Figure out how many total items to fetch this sync; do less on mobile.
     let batchSize = this.downloadLimit || Infinity;
     let isMobile = (Svc.Prefs.get("client.type") == "mobile");
 
     if (!newitems) {
-      newitems = this._itemSource();
+      newitems = this.itemSource();
     }
 
     if (this._defaultSort) {
       newitems.sort = this._defaultSort;
     }
 
     if (isMobile) {
       batchSize = MOBILE_BATCH_SIZE;
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -13,16 +13,17 @@ var Cu = Components.utils;
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://services-common/async.js");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/util.js");
 Cu.import("resource://services-sync/bookmark_utils.js");
+Cu.import("resource://services-sync/bookmark_validator.js");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/PlacesBackups.jsm");
 
 const ANNOS_TO_TRACK = [BookmarkAnnos.DESCRIPTION_ANNO, BookmarkAnnos.SIDEBAR_ANNO,
                         PlacesUtils.LMANNO_FEEDURI, PlacesUtils.LMANNO_SITEURI];
 
 const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
 const FOLDER_SORTINDEX = 1000000;
@@ -418,16 +419,20 @@ BookmarksEngine.prototype = {
     // Don't bother finding a dupe if the incoming item has duplicates.
     if (item.hasDupe) {
       this._log.trace(item.id + " already a dupe: not finding one.");
       return;
     }
     let mapped = this._mapDupe(item);
     this._log.debug(item.id + " mapped to " + mapped);
     return mapped;
+  },
+
+  getValidator() {
+    return new BookmarkValidator();
   }
 };
 
 function BookmarksStore(name, engine) {
   Store.call(this, name, engine);
 
   // Explicitly nullify our references to our cached services so we don't leak
   Svc.Obs.add("places-shutdown", function() {
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -10,16 +10,18 @@ this.EXPORTED_SYMBOLS = ["EngineSynchron
 
 var {utils: Cu} = Components;
 
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/policies.js");
 Cu.import("resource://services-sync/util.js");
+Cu.import("resource://services-common/observers.js");
+Cu.import("resource://services-common/async.js");
 
 /**
  * Perform synchronization of engines.
  *
  * This was originally split out of service.js. The API needs lots of love.
  */
 this.EngineSynchronizer = function EngineSynchronizer(service) {
   this._log = Log.repository.getLogger("Sync.Synchronizer");
@@ -184,16 +186,18 @@ EngineSynchronizer.prototype = {
           this.service.uploadMetaGlobal(meta);
           delete meta.isNew;
           delete meta.changed;
         } catch (error) {
           this._log.error("Unable to upload meta/global. Leaving marked as new.");
         }
       }
 
+      this._tryValidateEngines(enginesToSync)
+
       // If there were no sync engine failures
       if (this.service.status.service != SYNC_FAILED_PARTIAL) {
         Svc.Prefs.set("lastSync", new Date().toString());
         this.service.status.sync = SYNC_SUCCEEDED;
       }
     } finally {
       Svc.Prefs.reset("firstSync");
 
@@ -201,16 +205,101 @@ EngineSynchronizer.prototype = {
       let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
       this._log.info("Sync completed at " + dateStr
                      + " after " + syncTime + " secs.");
     }
 
     this.onComplete(null);
   },
 
+  _tryValidateEngines(recentlySyncedEngines) {
+    if (!Services.telemetry.canRecordBase || !Svc.Prefs.get("validation.enabled", false)) {
+      this._log.info("Skipping validation: validation or telemetry reporting is disabled");
+      return;
+    }
+
+    let lastValidation = Svc.Prefs.get("validation.lastTime", 0);
+    let validationInterval = Svc.Prefs.get("validation.interval", 60 * 60 * 24);
+    let nowSeconds = Math.floor(Date.now() / 1000);
+
+    if (nowSeconds - lastValidation < validationInterval) {
+      this._log.info("Skipping validation: too recent since last validation attempt");
+      return;
+    }
+    // Update the time now, even if we may return false still. We don't want to
+    // check the rest of these more frequently than once a day.
+    Svc.Prefs.set("validation.lastTime", nowSeconds);
+
+    // Validation only occurs a certain percentage of the time.
+    let validationProbability = Svc.Prefs.get("validation.percentageChance", 0) / 100.0;
+    if (validationProbability < Math.random()) {
+      this._log.info("Skipping validation: Probability threshold not met");
+      return;
+    }
+    let maxRecords = Svc.Prefs.get("validation.maxRecords", 0);
+    if (!maxRecords) {
+      // Don't bother asking the server for the counts if we know validation
+      // won't happen anyway.
+      return;
+    }
+
+    // maxRecords of -1 means "any number", so we can skip asking the server.
+    // Used for tests.
+    let info;
+    if (maxRecords < 0) {
+      info = {};
+      for (let e of recentlySyncedEngines) {
+        info[e.name] = 1; // needs to be < maxRecords
+      }
+      maxRecords = 2;
+    } else {
+
+      let collectionCountsURL = this.service.userBaseURL + "info/collection_counts";
+      try {
+        let infoResp = this.service._fetchInfo(collectionCountsURL);
+        info = infoResp.obj; // might throw because obj is a getter which parses json.
+      } catch (e) {
+        // Not running validation is totally fine, so we just write an error log and return.
+        this._log.error("Can't run validation: Caught error when fetching counts {}", e);
+        return;
+      }
+    }
+
+    if (!info) {
+      return;
+    }
+
+    let engineLookup = new Map(recentlySyncedEngines.map(e => [e.name, e]));
+    let toRun = [];
+    for (let [engineName, recordCount] of Object.entries(info)) {
+      let engine = engineLookup.get(engineName);
+      if (recordCount > maxRecords || !engine) {
+        continue;
+      }
+      let validator = engine.getValidator();
+      if (!validator) {
+        continue;
+      }
+      // Put this in an array so that we know how many we're going to do, so we
+      // don't tell users we're going to run some validators when we aren't.
+      toRun.push({ engine, validator });
+    }
+
+    if (!toRun.length) {
+      return;
+    }
+    Services.console.logStringMessage(
+      "Sync is about to run a consistency check. This may be slow, and " +
+      "can be controlled using the pref \"services.sync.validation.enabled\".");
+    for (let { validator, engine } of toRun) {
+      let result = Async.promiseSpinningly(validator.validate(engine));
+      Observers.notify("weave:engine:sync:validated", result, engine.name);
+    }
+  },
+
   // Returns true if sync should proceed.
   // false / no return value means sync should be aborted.
   _syncEngine: function _syncEngine(engine) {
     try {
       engine.sync();
     }
     catch(e) {
       if (e.status == 401) {
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -37,16 +37,17 @@ const TOPICS = [
   "weave:service:sync:finish",
   "weave:service:sync:error",
 
   "weave:engine:sync:start",
   "weave:engine:sync:finish",
   "weave:engine:sync:error",
   "weave:engine:sync:applied",
   "weave:engine:sync:uploaded",
+  "weave:engine:sync:validated",
 ];
 
 const PING_FORMAT_VERSION = 1;
 
 // The set of engines we record telemetry for - any other engines are ignored.
 const ENGINES = new Set(["addons", "bookmarks", "clients", "forms", "history",
                          "passwords", "prefs", "tabs"]);
 
@@ -162,16 +163,34 @@ class EngineRecord {
     for (let property of properties) {
       if (counts[property]) {
         incomingData[property] = counts[property];
         this.incoming = incomingData;
       }
     }
   }
 
+  recordValidation(validationResult) {
+    if (this.validation) {
+      log.error(`Multiple validations occurred for engine ${this.name}!`);
+      return;
+    }
+    let { problems, duration, recordCount } = validationResult;
+    let summarized = problems.getSummary(true).filter(({count}) => count > 0);
+    if (summarized.length) {
+      this.validation = summarized;
+    }
+    if (duration > 0) {
+      this.validationTook = duration;
+    }
+    if (recordCount) {
+      this.validationChecked = recordCount;
+    }
+  }
+
   recordUploaded(counts) {
     if (counts.sent || counts.failed) {
       if (!this.outgoing) {
         this.outgoing = [];
       }
       this.outgoing.push({
         sent: counts.sent || undefined,
         failed: counts.failed || undefined,
@@ -287,16 +306,31 @@ class TelemetryRecord {
 
   onEngineApplied(engineName, counts) {
     if (this._shouldIgnoreEngine(engineName)) {
       return;
     }
     this.currentEngine.recordApplied(counts);
   }
 
+  onEngineValidated(engineName, validationData) {
+    if (this._shouldIgnoreEngine(engineName, false)) {
+      return;
+    }
+    let engine = this.engines.find(e => e.name === engineName);
+    if (!engine && this.currentEngine && engineName === this.currentEngine.name) {
+      engine = this.currentEngine;
+    }
+    if (engine) {
+      engine.recordValidation(validationData);
+    } else {
+      log.warn(`Validation event triggered for engine ${engineName}, which hasn't been synced!`);
+    }
+  }
+
   onEngineUploaded(engineName, counts) {
     if (this._shouldIgnoreEngine(engineName)) {
       return;
     }
     this.currentEngine.recordUploaded(counts);
   }
 
   _shouldIgnoreEngine(engineName, shouldBeCurrent = true) {
@@ -417,16 +451,22 @@ class SyncTelemetryImpl {
         break;
 
       case "weave:engine:sync:uploaded":
         if (this._checkCurrent(topic)) {
           this.current.onEngineUploaded(data, subject);
         }
         break;
 
+      case "weave:engine:sync:validated":
+        if (this._checkCurrent(topic)) {
+          this.current.onEngineValidated(data, subject);
+        }
+        break;
+
       default:
         log.warn(`unexpected observer topic ${topic}`);
         break;
     }
   }
 }
 
 this.SyncTelemetry = new SyncTelemetryImpl(ENGINES);
--- a/services/sync/services-sync.js
+++ b/services/sync/services-sync.js
@@ -73,8 +73,14 @@ pref("services.sync.log.logger.engine.ta
 pref("services.sync.log.logger.engine.addons", "Debug");
 pref("services.sync.log.logger.engine.apps", "Debug");
 pref("services.sync.log.logger.identity", "Debug");
 pref("services.sync.log.logger.userapi", "Debug");
 pref("services.sync.log.cryptoDebug", false);
 
 pref("services.sync.fxa.termsURL", "https://accounts.firefox.com/legal/terms");
 pref("services.sync.fxa.privacyURL", "https://accounts.firefox.com/legal/privacy");
+
+// services.sync.validation.enabled is in browser/app/profile/firefox.js
+pref("services.sync.validation.interval", 86400);
+pref("services.sync.validation.percentageChance", 10);
+pref("services.sync.validation.maxRecords", 100);
+
--- a/services/sync/tests/unit/head_appinfo.js
+++ b/services/sync/tests/unit/head_appinfo.js
@@ -12,16 +12,22 @@ gSyncProfile = do_get_profile();
 // Init FormHistoryStartup and pretend we opened a profile.
 var fhs = Cc["@mozilla.org/satchel/form-history-startup;1"]
             .getService(Ci.nsIObserver);
 fhs.observe(null, "profile-after-change", null);
 
 // An app is going to have some prefs set which xpcshell tests don't.
 Services.prefs.setCharPref("identity.sync.tokenserver.uri", "http://token-server");
 
+// Set the validation prefs to attempt validation every time to avoid non-determinism.
+Services.prefs.setIntPref("services.sync.validation.interval", 0);
+Services.prefs.setIntPref("services.sync.validation.percentageChance", 100);
+Services.prefs.setIntPref("services.sync.validation.maxRecords", -1);
+Services.prefs.setBoolPref("services.sync.validation.enabled", true);
+
 // Make sure to provide the right OS so crypto loads the right binaries
 function getOS() {
   switch (mozinfo.os) {
     case "win":
       return "WINNT";
     case "mac":
       return "Darwin";
     default:
--- a/services/sync/tests/unit/head_helpers.js
+++ b/services/sync/tests/unit/head_helpers.js
@@ -251,29 +251,37 @@ function get_sync_test_telemetry() {
 
 function assert_valid_ping(record) {
   if (record) {
     if (!SyncPingValidator(record)) {
       deepEqual([], SyncPingValidator.errors, "Sync telemetry ping validation failed");
     }
     equal(record.version, 1);
     lessOrEqual(record.when, Date.now());
+    if (record.engines) {
+      for (let e of record.engines) {
+        if (e.validation) {
+          greater(e.validationChecked, 0);
+        }
+      }
+    }
   }
 }
 
 // Asserts that `ping` is a ping that doesn't contain any failure information
 function assert_success_ping(ping) {
   ok(!!ping);
   assert_valid_ping(ping);
   ok(!ping.failureReason);
   equal(undefined, ping.status);
   greater(ping.engines.length, 0);
   for (let e of ping.engines) {
     ok(!e.failureReason);
     equal(undefined, e.status);
+    equal(undefined, e.validation);
     if (e.outgoing) {
       for (let o of e.outgoing) {
         equal(undefined, o.failed);
         notEqual(undefined, o.sent);
       }
     }
     if (e.incoming) {
       equal(undefined, e.incoming.failed);
--- a/services/sync/tests/unit/sync_ping_schema.json
+++ b/services/sync/tests/unit/sync_ping_schema.json
@@ -39,16 +39,18 @@
   "definitions": {
     "engine": {
       "required": ["name"],
       "additionalProperties": false,
       "properties": {
         "failureReason": { "$ref": "#/definitions/error" },
         "name": { "enum": ["addons", "bookmarks", "clients", "forms", "history", "passwords", "prefs", "tabs"] },
         "took": { "type": "integer", "minimum": 1 },
+        "validationTook": { "type": "integer", "minimum": 1 },
+        "validationChecked": { "type": "integer", "minimum": 1 },
         "status": { "type": "string" },
         "incoming": {
           "type": "object",
           "additionalProperties": false,
           "anyOf": [
             {"required": ["applied"]},
             {"required": ["failed"]},
             {"required": ["newFailed"]},
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -1,12 +1,13 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 Components.utils.import("resource://services-sync/bookmark_validator.js");
+Components.utils.import("resource://services-sync/util.js");
 
 function inspectServerRecords(data) {
   return new BookmarkValidator().inspectServerRecords(data);
 }
 
 add_test(function test_isr_rootOnServer() {
   let c = inspectServerRecords([{
     id: 'places',
@@ -232,11 +233,48 @@ add_test(function test_cswc_differences(
     let c = new BookmarkValidator().compareServerWithClient(server, client).problemData;
     equal(c.clientMissing.length, 0);
     equal(c.serverMissing.length, 0);
     deepEqual(c.differences, [{id: 'cccccccccccc', differences: ['type']}]);
   }
   run_next_test();
 });
 
+function validationPing(server, client, duration) {
+  return wait_for_ping(function() {
+    // fake this entirely
+    Svc.Obs.notify("weave:service:sync:start");
+    Svc.Obs.notify("weave:engine:sync:start", null, "bookmarks");
+    Svc.Obs.notify("weave:engine:sync:finish", null, "bookmarks");
+    let data = {
+      duration, // We fake this just so that we can verify it's passed through.
+      recordCount: server.length,
+      problems: new BookmarkValidator().compareServerWithClient(server, client).problemData,
+    };
+    Svc.Obs.notify("weave:engine:sync:validated", data, "bookmarks");
+    Svc.Obs.notify("weave:service:sync:finish");
+  }, true); // Allow "failing" pings, since having validation info indicates failure.
+}
+
+add_task(function *test_telemetry_integration() {
+  let {server, client} = getDummyServerAndClient();
+  // remove c
+  server.pop();
+  server[0].children.pop();
+  const duration = 50;
+  let ping = yield validationPing(server, client, duration);
+  ok(!!ping.engines);
+  let bme = ping.engines.find(e => e.name === "bookmarks");
+  ok(!!bme);
+  ok(!!bme.validation);
+  equal(bme.validationChecked, server.length);
+  equal(bme.validationTook, duration);
+  bme.validation.sort((a, b) => String.localeCompare(a.name, b.name));
+  deepEqual(bme.validation, [
+    { name: "sdiff:childGUIDs", count: 1 },
+    { name: "serverMissing", count: 1 },
+    { name: "structuralDifferences", count: 1 },
+  ]);
+});
+
 function run_test() {
   run_next_test();
 }
--- a/toolkit/components/telemetry/docs/sync-ping.rst
+++ b/toolkit/components/telemetry/docs/sync-ping.rst
@@ -34,16 +34,23 @@ Structure::
           service: <string>, // The value of the Status.service property, unless it indicates success.
         },
         // Information about each engine's sync.
         engines: [
           {
             name: <string>, // "bookmarks", "tabs", etc.
             took: <integer duration in milliseconds>, // Optional, values of 0 are omitted.
 
+            // Optional, the number of server records processed while doing validation.
+            // Present if validation was run for this engine.
+            validationChecked: <integer>,
+
+            // Optional, Amount of time spent running validation for this engine.
+            validationTook: <non-monotonic integer duration in milliseconds>,
+
             status: <string>, // The value of Status.engines, if it holds a non-success value.
 
             // Optional, excluded if all items would be 0. A missing item indicates a value of 0.
             incoming: {
               applied: <integer>, // Number of records applied
               succeeded: <integer>, // Number of records that applied without error
               failed: <integer>, // Number of records that failed to apply
               newFailed: <integer>, // Number of records that failed for the first time this sync
@@ -135,8 +142,10 @@ engine.name
 ~~~~~~~~~~~
 
 Third-party engines are not reported, so only the following values are allowed: ``addons``, ``bookmarks``, ``clients``, ``forms``, ``history``, ``passwords``, ``prefs``, and ``tabs``.
 
 engine.validation
 ~~~~~~~~~~~~~~~~~
 
 For engines that can run validation on themselves, an array of objects describing validation errors that have occurred. Items that would have a count of 0 are excluded. Each engine will have its own set of items that it might put in the ``name`` field, but there are a finite number. See ``BookmarkProblemData.getSummary`` in `services/sync/modules/bookmark\_validator.js <https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/bookmark_validator.js>`_ for an example.
+
+If this is present, ``engine.validationChecked`` should be greater than zero.