Bug 1267917 - Hook the sync bookmark validator into the new sync telemetry ping r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Mon, 12 Sep 2016 14:59:25 -0400
changeset 416323 a72af25f3ff973cf0e231b1c1bd93ac2ddcfaec9
parent 416316 8ffdd738e7490e562063602a8256f6351a507367
child 531815 7026751434eb41b5adf87a6c0ae3908920516186
push id30100
push userbmo:tchiovoloni@mozilla.com
push dateWed, 21 Sep 2016 21:28:28 +0000
reviewersmarkh
bugs1267917
milestone52.0a1
Bug 1267917 - Hook the sync bookmark validator into the new sync telemetry ping r?markh MozReview-Commit-ID: ECACktrOhRG
browser/app/profile/firefox.js
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/data/sync-ping.rst
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1521,8 +1521,14 @@ pref("browser.crashReports.unsubmittedCh
 #endif
 
 // chancesUntilSuppress is how many times we'll show the unsubmitted
 // crash report notification across different days and shutdown
 // without a user choice before we suppress the notification for
 // some number of days.
 pref("browser.crashReports.unsubmittedCheck.chancesUntilSuppress", 4);
 pref("browser.crashReports.unsubmittedCheck.autoSubmit", false);
+
+#ifdef NIGHTLY_BUILD
+// Enable the (fairly costly) client/server validation on nightly only. The other prefs
+// controlling validation are located in /services/sync/services-sync.js
+pref("services.sync.validation.enabled", true);
+#endif
--- 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.
@@ -77,24 +78,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 },
 
@@ -110,16 +131,21 @@ class BookmarkProblemData {
       { name: "deletedChildren", count: this.deletedChildren.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 {
 
   _followQueries(recordMap) {
     for (let [guid, entry] of recordMap) {
       if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith("place:"))) {
@@ -688,12 +714,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
@@ -706,16 +706,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();
@@ -956,34 +965,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
@@ -16,16 +16,18 @@ Cu.import("resource://gre/modules/XPCOMU
 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://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/PlacesBackups.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "BookmarkValidator",
+                                  "resource://services-sync/bookmark_validator.js");
 
 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;
 const {
   SOURCE_SYNC,
@@ -555,16 +557,19 @@ BookmarksEngine.prototype = {
       }
     }
 
     // The local, duplicate ID is always deleted on the server - but for
     // bookmarks it is a logical delete.
     // Simply adding this (now non-existing) ID to the tracker is enough.
     this._modified.set(localDupeGUID, { modified: now, deleted: true });
   },
+  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() {
     for (let query in this._stmts) {
--- a/services/sync/modules/stages/enginesync.js
+++ b/services/sync/modules/stages/enginesync.js
@@ -10,16 +10,19 @@ 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");
+Cu.import("resource://gre/modules/Task.jsm");
 
 /**
  * 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");
@@ -153,22 +156,25 @@ EngineSynchronizer.prototype = {
     if (allowEnginesHint && engineNamesToSync) {
       this._log.info("Syncing specified engines", engineNamesToSync);
       enginesToSync = engineManager.get(engineNamesToSync).filter(e => e.enabled);
     } else {
       this._log.info("Syncing all enabled engines.");
       enginesToSync = engineManager.getEnabled();
     }
     try {
+      // We don't bother validating engines that failed to sync.
+      let enginesToValidate = [];
       for (let engine of enginesToSync) {
         // If there's any problems with syncing the engine, report the failure
         if (!(this._syncEngine(engine)) || this.service.status.enforceBackoff) {
           this._log.info("Aborting sync for failure in " + engine.name);
           break;
         }
+        enginesToValidate.push(engine);
       }
 
       // If _syncEngine fails for a 401, we might not have a cluster URL here.
       // If that's the case, break out of this immediately, rather than
       // throwing an exception when trying to fetch metaURL.
       if (!this.service.clusterURL) {
         this._log.debug("Aborting sync, no cluster URL: " +
                         "not uploading new meta/global.");
@@ -184,16 +190,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.");
         }
       }
 
+      Async.promiseSpinningly(this._tryValidateEngines(enginesToValidate));
+
       // 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 +209,116 @@ EngineSynchronizer.prototype = {
       let dateStr = new Date().toLocaleFormat(LOG_DATE_FORMAT);
       this._log.info("Sync completed at " + dateStr
                      + " after " + syncTime + " secs.");
     }
 
     this.onComplete(null);
   },
 
+  _tryValidateEngines: Task.async(function* (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");
+    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");
+    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);
+        if (!infoResp.success) {
+          this._log.error("Can't run validation: request to info/collection_counts responded with "
+                          + resp.status);
+          return;
+        }
+        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) {
+        this._log.debug(`Skipping validation for ${engineName} because it's not an engine or ` +
+                        `the number of records (${recordCount}) is greater than the maximum allowed (${maxRecords}).`);
+        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\".\n" +
+      "If you encounter any problems because of this, please file a bug.");
+    for (let { validator, engine } of toRun) {
+      try {
+        let result = yield validator.validate(engine);
+        Observers.notify("weave:engine:validate:finish", result, engine.name);
+      } catch (e) {
+        this._log.error(`Failed to run validation on ${engine.name}!`, e);
+        Observers.notify("weave:engine:validate:error", e, engine.name)
+        // Keep validating -- there's no reason to think that a failure for one
+        // validator would mean the others will fail.
+      }
+    }
+  }),
+
   // 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
@@ -38,16 +38,18 @@ 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:validate:finish",
+  "weave:engine:validate:error",
 ];
 
 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"]);
 
@@ -161,16 +163,46 @@ 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 validation = {
+      checked: recordCount || 0,
+    };
+    if (duration > 0) {
+      validation.took = Math.round(duration);
+    }
+    let summarized = problems.getSummary(true).filter(({count}) => count > 0);
+    if (summarized.length) {
+      validation.problems = summarized;
+    }
+    this.validation = validation;
+  }
+
+  recordValidationError(e) {
+    if (this.validation) {
+      log.error(`Multiple validations occurred for engine ${this.name}!`);
+      return;
+    }
+
+    this.validation = {
+      failureReason: transformError(e)
+    };
+  }
+
   recordUploaded(counts) {
     if (counts.sent || counts.failed) {
       if (!this.outgoing) {
         this.outgoing = [];
       }
       this.outgoing.push({
         sent: counts.sent || undefined,
         failed: counts.failed || undefined,
@@ -293,16 +325,46 @@ 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!`);
+    }
+  }
+
+  onEngineValidateError(engineName, error) {
+    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.recordValidationError(error);
+    } else {
+      log.warn(`Validation failure 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) {
@@ -462,16 +524,28 @@ class SyncTelemetryImpl {
         break;
 
       case "weave:engine:sync:uploaded":
         if (this._checkCurrent(topic)) {
           this.current.onEngineUploaded(data, subject);
         }
         break;
 
+      case "weave:engine:validate:finish":
+        if (this._checkCurrent(topic)) {
+          this.current.onEngineValidated(data, subject);
+        }
+        break;
+
+      case "weave:engine:validate:error":
+        if (this._checkCurrent(topic)) {
+          this.current.onEngineValidateError(data, subject || "Unknown");
+        }
+        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,22 @@ pref("services.sync.log.logger.identity"
 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");
 
 pref("services.sync.telemetry.submissionInterval", 43200); // 12 hours in seconds
 pref("services.sync.telemetry.maxPayloadCount", 500);
+
+// Note that services.sync.validation.enabled is located in browser/app/profile/firefox.js
+
+// We consider validation this frequently. After considering validation, even
+// if we don't end up validating, we won't try again unless this much time has passed.
+pref("services.sync.validation.interval", 86400); // 24 hours in seconds
+
+// We only run validation `services.sync.validation.percentageChance` percent of
+// the time, even if it's been the right amount of time since the last validation,
+// and you meet the maxRecord checks.
+pref("services.sync.validation.percentageChance", 10);
+
+// We won't validate an engine if it has more than this many records on the server.
+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
@@ -273,16 +273,20 @@ function assert_success_ping(ping) {
   assert_valid_ping(ping);
   ping.syncs.forEach(record => {
     ok(!record.failureReason);
     equal(undefined, record.status);
     greater(record.engines.length, 0);
     for (let e of record.engines) {
       ok(!e.failureReason);
       equal(undefined, e.status);
+      if (e.validation) {
+        equal(undefined, e.validation.problems);
+        equal(undefined, e.validation.failureReason);
+      }
       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
@@ -77,19 +77,32 @@
           }
         },
         "outgoing": {
           "type": "array",
           "minItems": 1,
           "items": { "$ref": "#/definitions/outgoingBatch" }
         },
         "validation": {
-          "type": "array",
-          "minItems": 1,
-          "$ref": "#/definitions/validationProblem"
+          "type": "object",
+          "additionalProperties": false,
+          "anyOf": [
+            { "required": ["checked"] },
+            { "required": ["failureReason"] }
+          ],
+          "properties": {
+            "checked": { "type": "integer", "minimum": 0 },
+            "failureReason": { "$ref": "#/definitions/error" },
+            "took": { "type": "integer" },
+            "problems": {
+              "type": "array",
+              "minItems": 1,
+              "$ref": "#/definitions/validationProblem"
+            }
+          }
         }
       }
     },
     "outgoingBatch": {
       "type": "object",
       "additionalProperties": false,
       "anyOf": [
         {"required": ["sent"]},
--- 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',
@@ -242,11 +243,49 @@ 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:validate:finish", 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);
+  ok(bme.validation.problems)
+  equal(bme.validation.checked, server.length);
+  equal(bme.validation.took, duration);
+  bme.validation.problems.sort((a, b) => String.localeCompare(a.name, b.name));
+  deepEqual(bme.validation.problems, [
+    { name: "sdiff:childGUIDs", count: 1 },
+    { name: "serverMissing", count: 1 },
+    { name: "structuralDifferences", count: 1 },
+  ]);
+});
+
 function run_test() {
   run_next_test();
 }
--- a/toolkit/components/telemetry/docs/data/sync-ping.rst
+++ b/toolkit/components/telemetry/docs/data/sync-ping.rst
@@ -65,24 +65,31 @@ Structure:
                   sent: <integer>, // Number of outgoing records sent. Zero values are omitted.
                   failed: <integer>, // Number that failed to send. Zero values are omitted.
                 }
               ],
               // Optional, excluded if there were no errors
               failureReason: { ... }, // Same as above.
 
               // Optional, excluded if it would be empty or if the engine cannot
-              // or did not run validation on itself. Entries with a count of 0
-              // are excluded.
-              validation: [
-                {
-                  name: <string>, // The problem identified.
-                  count: <integer>, // Number of times it occurred.
-                }
-              ]
+              // or did not run validation on itself.
+              validation: {
+                checked: <integer>,
+                took: <non-monotonic integer duration in milliseconds>,
+                // Entries with a count of 0 are excluded, the array is excluded if no problems are found.
+                problems: [
+                  {
+                    name: <string>, // The problem identified.
+                    count: <integer>, // Number of times it occurred.
+                  }
+                ],
+                // Format is same as above, this is only included if we tried and failed
+                // to run validation, and if it's present, all other fields in this object are optional.
+                failureReason: { ... },
+              }
             }
           ]
         }]
       }
     }
 
 info
 ----
@@ -145,12 +152,12 @@ Stores error information, if any is pres
 
    - ``error``: The message provided by the error.
 
 syncs.engine.name
 ~~~~~~~~~~~~~~~~~
 
 Third-party engines are not reported, so only the following values are allowed: ``addons``, ``bookmarks``, ``clients``, ``forms``, ``history``, ``passwords``, ``prefs``, and ``tabs``.
 
-syncs.engine.validation
-~~~~~~~~~~~~~~~~~~~~~~~
+syncs.engine.validation.problems
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 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.