From ec3416d756a979eeee0614662372f47af6ceec4e Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Wed, 20 Oct 2021 17:09:45 +0000 Subject: [PATCH] Bug 1732914 - Add new unenrollment reason when targeting is no longer satisfied r=k88hudson Differential Revision: https://phabricator.services.mozilla.com/D128716 --- .../nimbus/lib/ExperimentManager.jsm | 8 +++- .../lib/RemoteSettingsExperimentLoader.jsm | 4 +- .../unit/test_ExperimentManager_lifecycle.js | 48 +++++++++++++++++++ .../test_RemoteSettingsExperimentLoader.js | 35 ++++++++++++++ 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/toolkit/components/nimbus/lib/ExperimentManager.jsm b/toolkit/components/nimbus/lib/ExperimentManager.jsm index e46cefbac087..0be7e696f881 100644 --- a/toolkit/components/nimbus/lib/ExperimentManager.jsm +++ b/toolkit/components/nimbus/lib/ExperimentManager.jsm @@ -126,8 +126,9 @@ class _ExperimentManager { /** * Runs when the all recipes been processed during an update, including at first run. * @param {string} sourceToCheck + * @param {object} options Extra context used in telemetry reporting */ - onFinalize(sourceToCheck) { + onFinalize(sourceToCheck, { recipeMismatches } = { recipeMismatches: [] }) { if (!sourceToCheck) { throw new Error("When calling onFinalize, you must specify a source."); } @@ -141,7 +142,10 @@ class _ExperimentManager { if (!this.sessions.get(source)?.has(slug)) { log.debug(`Stopping study for recipe ${slug}`); try { - this.unenroll(slug, "recipe-not-seen"); + let reason = recipeMismatches.includes(slug) + ? "targeting-mismatch" + : "recipe-not-seen"; + this.unenroll(slug, reason); } catch (err) { Cu.reportError(err); } diff --git a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm index 877d815141ae..ee53784058f9 100644 --- a/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm +++ b/toolkit/components/nimbus/lib/RemoteSettingsExperimentLoader.jsm @@ -321,6 +321,7 @@ class _RemoteSettingsExperimentLoader { } let matches = 0; + let recipeMismatches = []; if (recipes && !loadingError) { for (const r of recipes) { if (await this.checkTargeting(r)) { @@ -329,11 +330,12 @@ class _RemoteSettingsExperimentLoader { await this.manager.onRecipe(r, "rs-loader"); } else { log.debug(`${r.id} did not match due to targeting`); + recipeMismatches.push(r.slug); } } log.debug(`${matches} recipes matched. Finalizing ExperimentManager.`); - this.manager.onFinalize("rs-loader"); + this.manager.onFinalize("rs-loader", { recipeMismatches }); } if (trigger !== "timer") { diff --git a/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js b/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js index a20861f8626b..5e40d1096818 100644 --- a/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js +++ b/toolkit/components/nimbus/test/unit/test_ExperimentManager_lifecycle.js @@ -225,3 +225,51 @@ add_task(async function test_onFinalize_unenroll() { "should clear sessions[test]" ); }); + +add_task(async function test_onFinalize_unenroll_mismatch() { + const manager = ExperimentFakes.manager(); + const sandbox = sinon.createSandbox(); + sandbox.spy(manager, "unenroll"); + + await manager.onStartup(); + + // Add an experiment to the store without calling .onRecipe + // This simulates an enrollment having happened in the past. + let recipe0 = ExperimentFakes.experiment("foo", { + experimentType: "unittest", + userFacingName: "foo", + userFacingDescription: "foo", + lastSeen: Date.now().toLocaleString(), + source: "test", + }); + await manager.store.addExperiment(recipe0); + + const recipe1 = ExperimentFakes.recipe("bar"); + // Unique features to prevent overlap + recipe1.branches[0].features[0].featureId = "red"; + recipe1.branches[1].features[0].featureId = "red"; + await manager.onRecipe(recipe1, "test"); + const recipe2 = ExperimentFakes.recipe("baz"); + recipe2.branches[0].features[0].featureId = "green"; + recipe2.branches[1].features[0].featureId = "green"; + await manager.onRecipe(recipe2, "test"); + + // Finalize + manager.onFinalize("test", { recipeMismatches: [recipe0.slug] }); + + Assert.equal( + manager.unenroll.callCount, + 1, + "should only call unenroll for the unseen recipe" + ); + Assert.equal( + manager.unenroll.calledWith("foo", "targeting-mismatch"), + true, + "should unenroll a experiment whose recipe wasn't seen in the current session" + ); + Assert.equal( + manager.sessions.has("test"), + false, + "should clear sessions[test]" + ); +}); diff --git a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js index 16b5380c6b31..0fb866739f26 100644 --- a/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js +++ b/toolkit/components/nimbus/test/unit/test_RemoteSettingsExperimentLoader.js @@ -134,6 +134,41 @@ add_task(async function test_updateRecipes() { ); }); +add_task(async function test_updateRecipes_someMismatch() { + const loader = ExperimentFakes.rsLoader(); + + const PASS_FILTER_RECIPE = ExperimentFakes.recipe("foo", { + targeting: "true", + }); + const FAIL_FILTER_RECIPE = ExperimentFakes.recipe("foo", { + targeting: "false", + }); + sinon.stub(loader, "setTimer"); + sinon.spy(loader, "updateRecipes"); + + sinon + .stub(loader.remoteSettingsClient, "get") + .resolves([PASS_FILTER_RECIPE, FAIL_FILTER_RECIPE]); + sinon.stub(loader.manager, "onRecipe").resolves(); + sinon.stub(loader.manager, "onFinalize"); + + Services.prefs.setBoolPref(ENABLED_PREF, true); + await loader.init(); + ok(loader.updateRecipes.calledOnce, "should call .updateRecipes"); + equal( + loader.manager.onRecipe.callCount, + 1, + "should call .onRecipe only for recipes that pass" + ); + ok(loader.manager.onFinalize.calledOnce, "Should call onFinalize."); + ok( + loader.manager.onFinalize.calledWith("rs-loader", { + recipeMismatches: [FAIL_FILTER_RECIPE.slug], + }), + "should call .onFinalize with the recipes that failed targeting" + ); +}); + add_task(async function test_updateRecipes_forFirstStartup() { const loader = ExperimentFakes.rsLoader(); const PASS_FILTER_RECIPE = ExperimentFakes.recipe("foo", {