Bug 1492010 - Ensure Sync engines with overridden telemetry names correctly report their validation results. r=tcsc

Differential Revision: https://phabricator.services.mozilla.com/D6097

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

services/sync/modules/telemetry.js

@@ -154,11 +154,22 @@ class EngineRecord {
     // so we need to keep both it and when.
     this.startTime = tryGetMonotonicTimestamp();
     this.name = name;
+
+    // This allows cases like bookmarks-buffered to have a separate name from
+    // the bookmarks engine.
+    let engineImpl = Weave.Service.engineManager.get(name);
+    if (engineImpl && engineImpl.overrideTelemetryName) {
+      this.overrideTelemetryName = engineImpl.overrideTelemetryName;
+    }
   }
 
   toJSON() {
-    let result = Object.assign({}, this);
-    delete result.startTime;
+    let result = { name: this.overrideTelemetryName || this.name };
+    let properties = ["took", "status", "failureReason", "incoming", "outgoing",
+      "validation"];
+    for (let property of properties) {
+      result[property] = this[property];
+    }
     return result;
   }
 
@@ -170,12 +181,6 @@ class EngineRecord {
     if (error) {
       this.failureReason = SyncTelemetry.transformError(error);
     }
-    // This allows cases like bookmarks-buffered to have a separate name from
-    // the bookmarks engine.
-    let engineImpl = Weave.Service.engineManager.get(this.name);
-    if (engineImpl && engineImpl.overrideTelemetryName) {
-      this.name = engineImpl.overrideTelemetryName;
-    }
   }
 
   recordApplied(counts) {

services/sync/tests/unit/head_helpers.js

@@ -486,11 +486,13 @@ async function registerRotaryEngine() {
 }
 
 // Set the validation prefs to attempt validation every time to avoid non-determinism.
-function enableValidationPrefs() {
-  Svc.Prefs.set("engine.bookmarks.validation.interval", 0);
-  Svc.Prefs.set("engine.bookmarks.validation.percentageChance", 100);
-  Svc.Prefs.set("engine.bookmarks.validation.maxRecords", -1);
-  Svc.Prefs.set("engine.bookmarks.validation.enabled", true);
+function enableValidationPrefs(engines = ["bookmarks"]) {
+  for (let engine of engines) {
+    Svc.Prefs.set(`engine.${engine}.validation.interval`, 0);
+    Svc.Prefs.set(`engine.${engine}.validation.percentageChance`, 100);
+    Svc.Prefs.set(`engine.${engine}.validation.maxRecords`, -1);
+    Svc.Prefs.set(`engine.${engine}.validation.enabled`, true);
+  }
 }
 
 async function serverForEnginesWithKeys(users, engines, callback) {

services/sync/tests/unit/test_telemetry.js

@@ -44,11 +44,15 @@ SteamEngine.prototype = {
   _storeObj: SteamStore,
   _trackerObj: SteamTracker,
   _errToThrow: null,
+  problemsToReport: null,
   async _sync() {
     if (this._errToThrow) {
       throw this._errToThrow;
     }
   },
+  getValidator() {
+    return new SteamValidator();
+  },
 };
 
 function BogusEngine(service) {
@@ -57,6 +61,31 @@ function BogusEngine(service) {
 
 BogusEngine.prototype = Object.create(SteamEngine.prototype);
 
+class SteamValidator {
+  async canValidate() {
+    return true;
+  }
+
+  async validate(engine) {
+    return {
+      problems: new SteamValidationProblemData(engine.problemsToReport),
+      version: 1,
+      duration: 0,
+      recordCount: 0,
+    };
+  }
+}
+
+class SteamValidationProblemData {
+  constructor(problemsToReport = []) {
+    this.problemsToReport = problemsToReport;
+  }
+
+  getSummary() {
+    return this.problemsToReport;
+  }
+}
+
 async function cleanAndGo(engine, server) {
   await engine._tracker.clearChangedIDs();
   Svc.Prefs.resetBranch("");
@@ -394,7 +423,7 @@ add_task(async function test_engine_fail_weird_errors() {
 
 
 add_task(async function test_overrideTelemetryName() {
-  enableValidationPrefs();
+  enableValidationPrefs(["steam"]);
 
   await Service.engineManager.register(SteamEngine);
   let engine = Service.engineManager.get("steam");
@@ -403,10 +432,33 @@ add_task(async function test_overrideTelemetryName() {
   let server = await serverForFoo(engine);
   await SyncTestingInfrastructure(server);
 
+  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);
-    ok(ping.engines.find(e => e.name === "steam-but-better"));
+    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");
   } finally {
     await cleanAndGo(engine, server);
     await Service.engineManager.unregister(engine);