Bug 1723987 - Cleanup Nimbus experiments older than 6 months r=k88hudson
authorAndrei Oprea <andrei.br92@gmail.com>
Fri, 20 Aug 2021 16:19:38 +0000
changeset 589446 317ef734e9ab3f06b3eb5d1150929cc78f87c792
parent 589445 a533746e243528e46e857e1714cd2088cdecdf00
child 589447 6ba2b54d02f691c42ae372b5c40da72c880e7311
push id148357
push useraoprea@mozilla.com
push dateFri, 20 Aug 2021 16:21:58 +0000
treeherderautoland@317ef734e9ab [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson
bugs1723987
milestone93.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1723987 - Cleanup Nimbus experiments older than 6 months r=k88hudson Differential Revision: https://phabricator.services.mozilla.com/D122878
toolkit/components/nimbus/lib/ExperimentStore.jsm
toolkit/components/nimbus/lib/SharedDataMap.jsm
toolkit/components/nimbus/test/unit/test_ExperimentStore.js
toolkit/components/nimbus/test/unit/test_SharedDataMap.js
--- a/toolkit/components/nimbus/lib/ExperimentStore.jsm
+++ b/toolkit/components/nimbus/lib/ExperimentStore.jsm
@@ -219,16 +219,18 @@ class ExperimentStore extends SharedData
     this.getAllActive().forEach(({ branch }) => {
       if (branch?.feature?.featureId) {
         this._emitFeatureUpdate(
           branch.feature.featureId,
           "feature-experiment-loaded"
         );
       }
     });
+
+    Services.tm.idleDispatchToMainThread(() => this._cleanupOldRecipes());
   }
 
   /**
    * Given a feature identifier, find an active experiment that matches that feature identifier.
    * This assumes, for now, that there is only one active experiment per feature per browser.
    * Does not activate the experiment (send an exposure event)
    *
    * @param {string} featureId
@@ -278,16 +280,34 @@ class ExperimentStore extends SharedData
 
   /**
    * @returns {Enrollment[]}
    */
   getAllActive() {
     return this.getAll().filter(experiment => experiment.active);
   }
 
+  /**
+   * Remove inactive enrollments older than 6 months
+   */
+  _cleanupOldRecipes() {
+    // Roughly six months
+    const threshold = 15552000000;
+    const nowTimestamp = new Date().getTime();
+    const recipesToRemove = this.getAll().filter(
+      experiment =>
+        !experiment.active &&
+        // Flip the comparison here to catch scenarios in which lastSeen is
+        // invalid or undefined. The result with be a comparison with NaN
+        // which is always false
+        !(nowTimestamp - new Date(experiment.lastSeen).getTime() < threshold)
+    );
+    this._removeEntriesByKeys(recipesToRemove.map(r => r.slug));
+  }
+
   _emitExperimentUpdates(experiment) {
     this.emit(`update:${experiment.slug}`, experiment);
     if (experiment.branch.feature) {
       this.emit(`update:${experiment.branch.feature.featureId}`, experiment);
       this._emitFeatureUpdate(
         experiment.branch.feature.featureId,
         "experiment-updated"
       );
--- a/toolkit/components/nimbus/lib/SharedDataMap.jsm
+++ b/toolkit/components/nimbus/lib/SharedDataMap.jsm
@@ -115,16 +115,30 @@ class SharedDataMap extends EventEmitter
       );
     }
     this._store.data[key] = value;
     this._store.saveSoon();
     this._syncToChildren();
     this._notifyUpdate();
   }
 
+  /**
+   * Replace the stored data with an updated filtered dataset for cleanup
+   * purposes. We don't notify of update because we're only filtering out
+   * old unused entries.
+   *
+   * @param {string[]} keysToRemove - list of keys to remove from the persistent store
+   */
+  _removeEntriesByKeys(keysToRemove) {
+    for (let key of keysToRemove) {
+      delete this._store.data[key];
+    }
+    this._store.saveSoon();
+  }
+
   setNonPersistent(key, value) {
     if (!this.isParent) {
       throw new Error(
         "Setting values from within a content process is not allowed"
       );
     }
 
     this._nonPersistentStore[key] = value;
--- a/toolkit/components/nimbus/test/unit/test_ExperimentStore.js
+++ b/toolkit/components/nimbus/test/unit/test_ExperimentStore.js
@@ -677,8 +677,71 @@ add_task(async function test_storeValueP
   store._updateSyncStore({ ...experiment, active: false });
   Assert.ok(
     !Services.prefs.getStringPref(`${SYNC_DATA_PREF_BRANCH}purple`, ""),
     "Experiment cleanup"
   );
   Assert.deepEqual(branch.getChildList(""), [], "Variables are also removed");
   delete FeatureManifest.purple;
 });
+
+add_task(async function test_cleanupOldRecipes() {
+  let store = ExperimentFakes.store();
+  let sandbox = sinon.createSandbox();
+  let stub = sandbox.stub(store, "_removeEntriesByKeys");
+  const experiment1 = ExperimentFakes.experiment("foo", {
+    branch: {
+      slug: "variant",
+      feature: { featureId: "purple", enabled: true },
+    },
+  });
+  const experiment2 = ExperimentFakes.experiment("bar", {
+    branch: {
+      slug: "variant",
+      feature: { featureId: "purple", enabled: true },
+    },
+  });
+  const experiment3 = ExperimentFakes.experiment("baz", {
+    branch: {
+      slug: "variant",
+      feature: { featureId: "purple", enabled: true },
+    },
+  });
+  const experiment4 = ExperimentFakes.experiment("faz", {
+    branch: {
+      slug: "variant",
+      feature: { featureId: "purple", enabled: true },
+    },
+  });
+  // Exp 2 is kept because it's recent (even though it's not active)
+  // Exp 4 is kept because it's active
+  experiment2.lastSeen = new Date().toISOString();
+  experiment2.active = false;
+  experiment1.lastSeen = new Date("2020-01-01").toISOString();
+  experiment1.active = false;
+  experiment3.active = false;
+  delete experiment3.lastSeen;
+  store._data = {
+    foo: experiment1,
+    bar: experiment2,
+    baz: experiment3,
+    faz: experiment4,
+  };
+
+  store._cleanupOldRecipes();
+
+  Assert.ok(stub.calledOnce, "Recipe cleanup called");
+  Assert.equal(
+    stub.firstCall.args[0].length,
+    2,
+    "We call to remove enrollments"
+  );
+  Assert.equal(
+    stub.firstCall.args[0][0],
+    experiment1.slug,
+    "Should remove expired enrollment"
+  );
+  Assert.equal(
+    stub.firstCall.args[0][1],
+    experiment3.slug,
+    "Should remove invalid enrollment"
+  );
+});
--- a/toolkit/components/nimbus/test/unit/test_SharedDataMap.js
+++ b/toolkit/components/nimbus/test/unit/test_SharedDataMap.js
@@ -240,8 +240,21 @@ with_sharedDataMap(async function test_s
   await instance.init();
 
   Assert.ok(!instance.hasRemoteDefaultsReady(), "False on init");
 
   instance.setNonPersistent("__REMOTE_DEFAULTS", { foo: 1 });
 
   Assert.ok(instance.hasRemoteDefaultsReady(), "Has 1 entry");
 });
+
+with_sharedDataMap(async function test_updateStoreData({ instance, sandbox }) {
+  await instance.init();
+
+  Assert.ok(!instance.get("foo"), "No value initially");
+
+  instance.set("foo", "foo");
+  instance.set("bar", "bar");
+  instance._removeEntriesByKeys(["bar"]);
+
+  Assert.ok(instance.get("foo"), "We keep one of the values");
+  Assert.ok(!instance.get("bar"), "The other value is removed");
+});