Bug 1309774 - Part 2. Add version number to validation data in sync ping. r=markh
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 20 Oct 2016 15:20:07 -0400
changeset 347438 637a0c2c060827b1093ca5ed08eda318a869c945
parent 347437 64001171c0c73e6779e8eb4904022ac4c402ac0e
child 347439 6e6ef4bbe35a0ab961dc5a52f277cccbc1ac6be8
push id10298
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:33:03 +0000
treeherdermozilla-aurora@7e29173b1641 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1309774
milestone52.0a1
Bug 1309774 - Part 2. Add version number to validation data in sync ping. r=markh MozReview-Commit-ID: FlDSEsT1ah9
services/sync/modules/bookmark_validator.js
services/sync/modules/collection_validator.js
services/sync/modules/telemetry.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/services/sync/modules/bookmark_validator.js
+++ b/services/sync/modules/bookmark_validator.js
@@ -17,16 +17,17 @@ this.EXPORTED_SYMBOLS = ["BookmarkValida
 const LEFT_PANE_ROOT_ANNO = "PlacesOrganizer/OrganizerFolder";
 const LEFT_PANE_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
 
 // Indicates if a local bookmark tree node should be excluded from syncing.
 function isNodeIgnored(treeNode) {
   return treeNode.annos && treeNode.annos.some(anno => anno.name == LEFT_PANE_ROOT_ANNO ||
                                                        anno.name == LEFT_PANE_QUERY_ANNO);
 }
+const BOOKMARK_VALIDATOR_VERSION = 1;
 
 /**
  * Result of bookmark validation. Contains the following fields which describe
  * server-side problems unless otherwise specified.
  *
  * - missingIDs (number): # of objects with missing ids
  * - duplicates (array of ids): ids seen more than once
  * - parentChildMismatches (array of {parent: parentid, child: childid}):
@@ -758,18 +759,19 @@ class BookmarkValidator {
       });
       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,
+        version: self.version,
         problems: result.problemData,
         recordCount: serverRecordCount
       };
     });
   }
 
 };
 
+BookmarkValidator.prototype.version = BOOKMARK_VALIDATOR_VERSION;
 
-
--- a/services/sync/modules/collection_validator.js
+++ b/services/sync/modules/collection_validator.js
@@ -194,8 +194,11 @@ class CollectionValidator {
     return {
       problemData: problems,
       clientRecords: clientItems,
       records: serverItems,
       deletedRecords: [...serverDeleted]
     };
   }
 }
+
+// Default to 0, some engines may override.
+CollectionValidator.prototype.version = 0;
--- a/services/sync/modules/telemetry.js
+++ b/services/sync/modules/telemetry.js
@@ -172,18 +172,19 @@ class EngineRecord {
     }
   }
 
   recordValidation(validationResult) {
     if (this.validation) {
       log.error(`Multiple validations occurred for engine ${this.name}!`);
       return;
     }
-    let { problems, duration, recordCount } = validationResult;
+    let { problems, version, duration, recordCount } = validationResult;
     let validation = {
+      version: version || 0,
       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;
--- a/services/sync/tests/unit/sync_ping_schema.json
+++ b/services/sync/tests/unit/sync_ping_schema.json
@@ -101,16 +101,17 @@
           "anyOf": [
             { "required": ["checked"] },
             { "required": ["failureReason"] }
           ],
           "properties": {
             "checked": { "type": "integer", "minimum": 0 },
             "failureReason": { "$ref": "#/definitions/error" },
             "took": { "type": "integer" },
+            "version": { "type": "integer" },
             "problems": {
               "type": "array",
               "minItems": 1,
               "$ref": "#/definitions/validationProblem"
             }
           }
         }
       }
--- a/services/sync/tests/unit/test_bookmark_validator.js
+++ b/services/sync/tests/unit/test_bookmark_validator.js
@@ -300,20 +300,23 @@ add_test(function test_cswc_serverUnexpe
 });
 
 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 validator = new BookmarkValidator();
     let data = {
-      duration, // We fake this just so that we can verify it's passed through.
+      // We fake duration and version just so that we can verify they're passed through.
+      duration,
+      version: validator.version,
       recordCount: server.length,
-      problems: new BookmarkValidator().compareServerWithClient(server, client).problemData,
+      problems: validator.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();
@@ -325,16 +328,17 @@ add_task(function *test_telemetry_integr
   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));
+  equal(bme.validation.version, new BookmarkValidator().version);
   deepEqual(bme.validation.problems, [
     { name: "badClientRoots", count: 3 },
     { name: "sdiff:childGUIDs", count: 1 },
     { name: "serverMissing", count: 1 },
     { name: "structuralDifferences", count: 1 },
   ]);
 });
 
--- a/toolkit/components/telemetry/docs/data/sync-ping.rst
+++ b/toolkit/components/telemetry/docs/data/sync-ping.rst
@@ -75,16 +75,18 @@ Structure:
                 }
               ],
               // 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.
               validation: {
+                // Optional validator version, default of 0.
+                version: <integer>,
                 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.
                   }