Bug 1309774 - Part 2. Add version number to validation data in sync ping. r?markh draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Thu, 20 Oct 2016 15:20:07 -0400
changeset 432303 ff0f3ee01bc41178dc1f91e3b1da5060f18b2ca7
parent 432302 a83c0a54edfcfdcbd2ab82ca7e739866bc4f20a2
child 535587 79aa59a16d500d057bf3b0797677e498b0fba199
push id34245
push userbmo:tchiovoloni@mozilla.com
push dateTue, 01 Nov 2016 15:46:57 +0000
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
@@ -168,18 +168,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.
                   }