Bug 1588018 - Remove remaining event telemetry from the bookmarks mirror. r=markh

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

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

services/sync/modules/engines/bookmarks.js

@@ -1319,9 +1319,6 @@ BufferedBookmarksStore.prototype = {
 
     return SyncedBookmarksMirror.open({
       path: mirrorPath,
-      recordTelemetryEvent: (object, method, value, extra) => {
-        this.engine.service.recordTelemetryEvent(object, method, value, extra);
-      },
       recordStepTelemetry: (name, took, counts) => {
         Observers.notify(
           "weave:engine:sync:step",

toolkit/components/places/SyncedBookmarksMirror.jsm

@@ -254,15 +254,15 @@ ProgressTracker.STEPS = {
 class SyncedBookmarksMirror {
   constructor(
     db,
+    wasCorrupt = false,
     {
-      recordTelemetryEvent,
       recordStepTelemetry,
       recordValidationTelemetry,
       finalizeAt = PlacesUtils.history.shutdownClient.jsclient,
     } = {}
   ) {
     this.db = db;
-    this.recordTelemetryEvent = recordTelemetryEvent;
+    this.wasCorrupt = wasCorrupt;
     this.recordValidationTelemetry = recordValidationTelemetry;
 
     this.merger = new SyncedBookmarksMerger();
@@ -292,9 +292,6 @@ class SyncedBookmarksMirror {
    * @param  {String} options.path
    *         The path to the mirror database file, either absolute or relative
    *         to the profile path.
-   * @param  {Function} options.recordTelemetryEvent
-   *         A function with the signature `(object: String, method: String,
-   *         value: String?, extra: Object?)`, used to emit telemetry events.
    * @param  {Function} options.recordStepTelemetry
    *         A function with the signature `(name: String, took: Number,
    *         counts: Array?)`, where `name` is the name of the merge step,
@@ -315,46 +312,29 @@ class SyncedBookmarksMirror {
    */
   static async open(options) {
     let db = await PlacesUtils.promiseUnsafeWritableDBConnection();
-    let path = OS.Path.join(OS.Constants.Path.profileDir, options.path);
-    let whyFailed = "initialize";
-    try {
-      try {
-        await attachAndInitMirrorDatabase(db, path);
-      } catch (ex) {
-        if (isDatabaseCorrupt(ex)) {
-          MirrorLog.warn(
-            "Error attaching mirror to Places; removing and " +
-              "recreating mirror",
-            ex
-          );
-          options.recordTelemetryEvent("mirror", "open", "retry", {
-            why: "corrupt",
-          });
-
-          whyFailed = "remove";
-          await OS.File.remove(path);
-
-          whyFailed = "replace";
-          await attachAndInitMirrorDatabase(db, path);
-        } else {
-          MirrorLog.error("Unrecoverable error attaching mirror to Places", ex);
-          throw ex;
-        }
-      }
-      try {
-        let info = await OS.File.stat(path);
-        let size = Math.floor(info.size / 1024);
-        options.recordTelemetryEvent("mirror", "open", "success", { size });
-      } catch (ex) {
-        MirrorLog.warn("Error recording stats for mirror database size", ex);
-      }
-    } catch (ex) {
-      options.recordTelemetryEvent("mirror", "open", "error", {
-        why: whyFailed,
-      });
-      throw ex;
+    if (!db) {
+      throw new TypeError("Can't open mirror without Places connection");
     }
-    return new SyncedBookmarksMirror(db, options);
+    let path = OS.Path.join(OS.Constants.Path.profileDir, options.path);
+    let wasCorrupt = false;
+    try {
+      await attachAndInitMirrorDatabase(db, path);
+    } catch (ex) {
+      if (isDatabaseCorrupt(ex)) {
+        MirrorLog.warn(
+          "Error attaching mirror to Places; removing and " +
+            "recreating mirror",
+          ex
+        );
+        wasCorrupt = true;
+        await OS.File.remove(path);
+        await attachAndInitMirrorDatabase(db, path);
+      } else {
+        MirrorLog.error("Unrecoverable error attaching mirror to Places", ex);
+        throw ex;
+      }
+    }
+    return new SyncedBookmarksMirror(db, wasCorrupt, options);
   }
 
   /**

toolkit/components/places/tests/sync/head_sync.js

@@ -272,11 +272,6 @@ async function fetchAllKeywords(info) {
 async function openMirror(name, options = {}) {
   let buf = await SyncedBookmarksMirror.open({
     path: `${name}_buf.sqlite`,
-    recordTelemetryEvent(...args) {
-      if (options.recordTelemetryEvent) {
-        options.recordTelemetryEvent.call(this, ...args);
-      }
-    },
     recordStepTelemetry(...args) {
       if (options.recordStepTelemetry) {
         options.recordStepTelemetry.call(this, ...args);

toolkit/components/places/tests/sync/test_bookmark_abort_merging.js

@@ -55,7 +55,6 @@ add_task(async function test_blocker_state() {
   let buf = await SyncedBookmarksMirror.open({
     path: "blocker_state_buf.sqlite",
     finalizeAt: barrier.client,
-    recordTelemetryEvent(...args) {},
     recordStepTelemetry(...args) {},
     recordValidationTelemetry(...args) {},
   });

toolkit/components/places/tests/sync/test_bookmark_corruption.js

@@ -2893,15 +2893,7 @@ add_task(async function test_invalid_guid() {
 add_task(async function test_sync_status_mismatches() {
   let dateAdded = new Date();
 
-  let mergeTelemetryEvents = [];
-  let buf = await openMirror("sync_status_mismatches", {
-    recordTelemetryEvent(object, method, value, extra) {
-      equal(object, "mirror", "Wrong object for telemetry event");
-      if (method == "merge") {
-        mergeTelemetryEvents.push({ value, extra });
-      }
-    },
-  });
+  let buf = await openMirror("sync_status_mismatches");
 
   info("Ensure mirror is up-to-date with Places");
   let initialChangesToUpload = await buf.apply();

toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js

@@ -29,7 +29,6 @@ add_task(async function test_migrate_after_downgrade() {
   let dbFile = await setupFixtureFile("mirror_v5.sqlite");
   let oldBuf = await SyncedBookmarksMirror.open({
     path: dbFile.path,
-    recordTelemetryEvent(object, method, value, extra) {},
     recordStepTelemetry() {},
     recordValidationTelemetry() {},
   });
@@ -43,7 +42,6 @@ add_task(async function test_migrate_after_downgrade() {
 
   let buf = await SyncedBookmarksMirror.open({
     path: dbFile.path,
-    recordTelemetryEvent(object, method, value, extra) {},
     recordStepTelemetry() {},
     recordValidationTelemetry() {},
   });
@@ -71,7 +69,6 @@ add_task(async function test_migrate_from_5_to_current() {
   let dbFile = await setupFixtureFile("mirror_v5.sqlite");
   let buf = await SyncedBookmarksMirror.open({
     path: dbFile.path,
-    recordTelemetryEvent(object, method, value, extra) {},
     recordStepTelemetry() {},
     recordValidationTelemetry() {},
   });
@@ -143,56 +140,21 @@ add_task(async function test_migrate_from_5_to_current() {
 // Migrations between 1 and 2 discard the entire database.
 add_task(async function test_migrate_from_1_to_2() {
   let dbFile = await setupFixtureFile("mirror_v1.sqlite");
-  let telemetryEvents = [];
   let buf = await SyncedBookmarksMirror.open({
     path: dbFile.path,
-    recordTelemetryEvent(object, method, value, extra) {
-      telemetryEvents.push({ object, method, value, extra });
-    },
   });
-  let dbFileSize = Math.floor((await OS.File.stat(dbFile.path)).size / 1024);
-  deepEqual(telemetryEvents, [
-    {
-      object: "mirror",
-      method: "open",
-      value: "retry",
-      extra: { why: "corrupt" },
-    },
-    {
-      object: "mirror",
-      method: "open",
-      value: "success",
-      extra: { size: dbFileSize.toString(10) },
-    },
-  ]);
+  ok(
+    buf.wasCorrupt,
+    "Migrating from unsupported version should mark database as corrupt"
+  );
   await buf.finalize();
 });
 
 add_task(async function test_database_corrupt() {
   let corruptFile = await setupFixtureFile("mirror_corrupt.sqlite");
-  let telemetryEvents = [];
   let buf = await SyncedBookmarksMirror.open({
     path: corruptFile.path,
-    recordTelemetryEvent(object, method, value, extra) {
-      telemetryEvents.push({ object, method, value, extra });
-    },
   });
-  let dbFileSize = Math.floor(
-    (await OS.File.stat(corruptFile.path)).size / 1024
-  );
-  deepEqual(telemetryEvents, [
-    {
-      object: "mirror",
-      method: "open",
-      value: "retry",
-      extra: { why: "corrupt" },
-    },
-    {
-      object: "mirror",
-      method: "open",
-      value: "success",
-      extra: { size: dbFileSize.toString(10) },
-    },
-  ]);
+  ok(buf.wasCorrupt, "Opening corrupt database should mark it as such");
   await buf.finalize();
 });