Bug 1547034: PreferenceExperimentAction supports multiple prefs r=mythmon
authorEthan Glasser-Camp <ethan@betacantrips.com>
Thu, 16 May 2019 15:04:22 +0000
changeset 536130 0de43d5275d15b83e80fa021bac4175f85a5d3a4
parent 536129 454e86015872654981d491d0867f91e4c6685e6f
child 536131 31cf332c9b21698336c5e6c18939a3a8b58a7902
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmythmon
bugs1547034
milestone68.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 1547034: PreferenceExperimentAction supports multiple prefs r=mythmon The existing, single-preference action format is supported by a new SinglePreferenceExperimentAction, which converts single-preference actions into multiple-preference actions. We keep the wire format name "preference-experiment" for SinglePreferenceExperimentAction for now, but perhaps one day we can move that to "single-preference-experiment". Depends on D29872 Differential Revision: https://phabricator.services.mozilla.com/D29873
toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm
toolkit/components/normandy/actions/schemas/index.js
toolkit/components/normandy/lib/ActionsManager.jsm
toolkit/components/normandy/test/browser/browser.ini
toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js
--- a/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
+++ b/toolkit/components/normandy/actions/PreferenceExperimentAction.jsm
@@ -19,17 +19,17 @@ var EXPORTED_SYMBOLS = ["PreferenceExper
 
 /**
  * Enrolls a user in a preference experiment, in which we assign the
  * user to an experiment branch and modify a preference temporarily to
  * measure how it affects Firefox via Telemetry.
  */
 class PreferenceExperimentAction extends BaseAction {
   get schema() {
-    return ActionSchemas["preference-experiment"];
+    return ActionSchemas["multiple-preference-experiment"];
   }
 
   constructor() {
     super();
     this.seenExperimentNames = [];
   }
 
   _preExecution() {
@@ -39,56 +39,52 @@ class PreferenceExperimentAction extends
     }
   }
 
   async _run(recipe) {
     const {
       branches,
       isHighPopulation,
       isEnrollmentPaused,
-      preferenceBranchType,
-      preferenceName,
-      preferenceType,
       slug,
     } = recipe.arguments;
 
     this.seenExperimentNames.push(slug);
 
     // If we're not in the experiment, enroll!
     const hasSlug = await PreferenceExperiments.has(slug);
     if (!hasSlug) {
-      // If there's already an active experiment using this preference, abort.
+      // Check all preferences that could be used by this experiment.
+      // If there's already an active experiment that has set that preference, abort.
       const activeExperiments = await PreferenceExperiments.getAllActive();
-      const hasConflicts = activeExperiments.some(exp => exp.preferences.hasOwnProperty(preferenceName));
-      if (hasConflicts) {
-        throw new Error(
-          `Experiment ${slug} ignored; another active experiment is already using the
-          ${preferenceName} preference.`
-        );
+      for (const branch of branches) {
+        const conflictingPrefs = Object.keys(branch.preferences).filter(preferenceName => {
+          return activeExperiments.some(exp => exp.preferences.hasOwnProperty(preferenceName));
+        });
+        if (conflictingPrefs.length > 0) {
+          throw new Error(
+            `Experiment ${slug} ignored; another active experiment is already using the
+            ${conflictingPrefs[0]} preference.`
+          );
+        }
       }
 
       // Determine if enrollment is currently paused for this experiment.
       if (isEnrollmentPaused) {
         this.log.debug(`Enrollment is paused for experiment "${slug}"`);
         return;
       }
 
       // Otherwise, enroll!
       const branch = await this.chooseBranch(slug, branches);
       const experimentType = isHighPopulation ? "exp-highpop" : "exp";
       await PreferenceExperiments.start({
         name: slug,
         branch: branch.slug,
-        preferences: {
-          [preferenceName]: {
-            preferenceValue: branch.value,
-            preferenceBranchType,
-            preferenceType,
-          },
-        },
+        preferences: branch.preferences,
         experimentType,
       });
     } else {
       // If the experiment exists, and isn't expired, bump the lastSeen date.
       const experiment = await PreferenceExperiments.get(slug);
       if (experiment.expired) {
         this.log.debug(`Experiment ${slug} has expired, aborting.`);
       } else {
new file mode 100644
--- /dev/null
+++ b/toolkit/components/normandy/actions/SinglePreferenceExperimentAction.jsm
@@ -0,0 +1,64 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const {PreferenceExperimentAction} = ChromeUtils.import("resource://normandy/actions/PreferenceExperimentAction.jsm");
+ChromeUtils.defineModuleGetter(this, "ActionSchemas", "resource://normandy/actions/schemas/index.js");
+ChromeUtils.defineModuleGetter(this, "JsonSchemaValidator", "resource://gre/modules/components-utils/JsonSchemaValidator.jsm");
+
+var EXPORTED_SYMBOLS = ["SinglePreferenceExperimentAction"];
+
+/**
+ * The backwards-compatible version of the preference experiment that
+ * can only accept a single preference.
+ */
+class SinglePreferenceExperimentAction extends PreferenceExperimentAction {
+  get schema() {
+    return ActionSchemas["single-preference-experiment"];
+  }
+
+  async _run(recipe) {
+    const {
+      preferenceBranchType,
+      preferenceName,
+      preferenceType,
+      branches,
+      ...remainingArguments
+    } = recipe.arguments;
+
+    const newArguments = {
+      ...remainingArguments,
+      branches: branches.map(branch => {
+        const { value, ...branchProps } = branch;
+        return {
+          ...branchProps,
+          preferences: {
+            [preferenceName]: {
+              preferenceBranchType,
+              preferenceType,
+              preferenceValue: value,
+            },
+          },
+        };
+      }),
+    };
+
+    const multiprefSchema = ActionSchemas["multiple-preference-experiment"];
+
+    let [valid, validatedArguments] = JsonSchemaValidator.validateAndParseParameters(newArguments, multiprefSchema);
+    if (!valid) {
+      throw new Error(`Transformed arguments do not match schema. Original arguments: ${JSON.stringify(recipe.arguments)}, new arguments: ${JSON.stringify(newArguments)}, schema: ${JSON.stringify(multiprefSchema)}`);
+    }
+
+    recipe.arguments = validatedArguments;
+
+    const newRecipe = {
+      ...recipe,
+      arguments: validatedArguments,
+    };
+
+    return super._run(newRecipe);
+  }
+}
--- a/toolkit/components/normandy/actions/schemas/index.js
+++ b/toolkit/components/normandy/actions/schemas/index.js
@@ -162,17 +162,105 @@ const ActionSchemas = {
       "learnMoreUrl": {
         "description": "URL to show to the user when they click Learn More",
         "default": null,
         "type": ["string", "null"],
       },
     },
   },
 
-  "preference-experiment": {
+  "multiple-preference-experiment": {
+    $schema: "http://json-schema.org/draft-04/schema#",
+    title: "Run a feature experiment activated by a set of preferences.",
+    type: "object",
+    required: [
+      "slug",
+      "branches",
+    ],
+    properties: {
+      slug: {
+        description: "Unique identifier for this experiment",
+        type: "string",
+        pattern: "^[A-Za-z0-9\\-_]+$",
+      },
+      experimentDocumentUrl: {
+        description: "URL of a document describing the experiment",
+        type: "string",
+        format: "uri",
+        default: "",
+      },
+      isHighPopulation: {
+        description: "Marks the preference experiment as a high population experiment, that should be excluded from certain types of telemetry",
+        type: "boolean",
+        default: "false",
+      },
+      isEnrollmentPaused: {
+        description: "If true, new users will not be enrolled in the study.",
+        type: "boolean",
+        default: false,
+      },
+      branches: {
+        description: "List of experimental branches",
+        type: "array",
+        minItems: 1,
+        items: {
+          type: "object",
+          required: [
+            "slug",
+            "ratio",
+            "preferences",
+          ],
+          properties: {
+            slug: {
+              description: "Unique identifier for this branch of the experiment",
+              type: "string",
+              pattern: "^[A-Za-z0-9\\-_]+$",
+            },
+            ratio: {
+              description: "Ratio of users who should be grouped into this branch",
+              type: "integer",
+              minimum: 1,
+            },
+            preferences: {
+              description: "The set of preferences to be set if this branch is chosen",
+              type: "object",
+              patternProperties: {
+                ".*": {
+                  type: "object",
+                  properties: {
+                    preferenceType: {
+                      description: "Data type of the preference that controls this experiment",
+                      type: "string",
+                      enum: ["string", "integer", "boolean"],
+                    },
+                    preferenceBranchType: {
+                      description: "Controls whether the default or user value of the preference is modified",
+                      type: "string",
+                      enum: ["user", "default"],
+                      default: "default",
+                    },
+                    preferenceValue: {
+                      description: "Value for this preference when this branch is chosen",
+                      type: ["string", "number", "boolean"],
+                    },
+                  },
+                  required: [
+                    "preferenceType",
+                    "preferenceValue",
+                  ],
+                },
+              },
+            },
+          },
+        },
+      },
+    },
+  },
+
+  "single-preference-experiment": {
     $schema: "http://json-schema.org/draft-04/schema#",
     title: "Run a feature experiment activated by a preference.",
     type: "object",
     required: [
       "slug",
       "preferenceName",
       "preferenceType",
       "branches",
--- a/toolkit/components/normandy/lib/ActionsManager.jsm
+++ b/toolkit/components/normandy/lib/ActionsManager.jsm
@@ -3,39 +3,44 @@ const {LogManager} = ChromeUtils.import(
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AddonStudyAction: "resource://normandy/actions/AddonStudyAction.jsm",
   ConsoleLogAction: "resource://normandy/actions/ConsoleLogAction.jsm",
   PreferenceExperimentAction: "resource://normandy/actions/PreferenceExperimentAction.jsm",
   PreferenceRollbackAction: "resource://normandy/actions/PreferenceRollbackAction.jsm",
   PreferenceRolloutAction: "resource://normandy/actions/PreferenceRolloutAction.jsm",
   ShowHeartbeatAction: "resource://normandy/actions/ShowHeartbeatAction.jsm",
+  SinglePreferenceExperimentAction: "resource://normandy/actions/SinglePreferenceExperimentAction.jsm",
   Uptake: "resource://normandy/lib/Uptake.jsm",
 });
 
 var EXPORTED_SYMBOLS = ["ActionsManager"];
 
 const log = LogManager.getLogger("recipe-runner");
 
 /**
  * A class to manage the actions that recipes can use in Normandy.
  */
 class ActionsManager {
   constructor() {
     this.finalized = false;
 
     const addonStudyAction = new AddonStudyAction();
+    const singlePreferenceExperimentAction = new SinglePreferenceExperimentAction();
 
     this.localActions = {
       "addon-study": addonStudyAction,
       "console-log": new ConsoleLogAction(),
       "opt-out-study": addonStudyAction, // Legacy name used for addon-study on Normandy server
-      "preference-experiment": new PreferenceExperimentAction(),
+      "multi-preference-experiment": new PreferenceExperimentAction(),
+      // Historically, this name meant SinglePreferenceExperimentAction.
+      "preference-experiment": singlePreferenceExperimentAction,
       "preference-rollback": new PreferenceRollbackAction(),
       "preference-rollout": new PreferenceRolloutAction(),
+      "single-preference-experiment": singlePreferenceExperimentAction,
       "show-heartbeat": new ShowHeartbeatAction(),
     };
   }
 
   async runRecipe(recipe) {
     let actionName = recipe.action;
 
     if (actionName in this.localActions) {
--- a/toolkit/components/normandy/test/browser/browser.ini
+++ b/toolkit/components/normandy/test/browser/browser.ini
@@ -13,16 +13,17 @@ head = head.js
 skip-if = !healthreport || !telemetry
 [browser_about_studies.js]
 [browser_actions_AddonStudyAction.js]
 [browser_actions_ConsoleLogAction.js]
 [browser_actions_PreferenceExperimentAction.js]
 [browser_actions_PreferenceRolloutAction.js]
 [browser_actions_PreferenceRollbackAction.js]
 [browser_actions_ShowHeartbeatAction.js]
+[browser_actions_SinglePreferenceExperimentAction.js]
 [browser_ActionsManager.js]
 [browser_AddonStudies.js]
 skip-if = (verify && (os == 'linux'))
 [browser_BaseAction.js]
 [browser_CleanupManager.js]
 [browser_ClientEnvironment.js]
 [browser_EventEmitter.js]
 [browser_Heartbeat.js]
--- a/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
+++ b/toolkit/components/normandy/test/browser/browser_actions_PreferenceExperimentAction.js
@@ -7,27 +7,45 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/Preferences.jsm", this);
 ChromeUtils.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
 ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this);
 ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
 ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
 ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
 ChromeUtils.import("resource://normandy/actions/PreferenceExperimentAction.jsm", this);
 
-function argumentsFactory(args) {
-  return {
-    slug: "test",
-    preferenceName: "fake.preference",
+function branchFactory(opts = {}) {
+  const defaultPreferences = {
+    "fake.preference": {},
+  };
+  const defaultPrefInfo = {
     preferenceType: "string",
     preferenceBranchType: "default",
-    branches: [
-      { slug: "test", value: "foo", ratio: 1 },
-    ],
+    preferenceValue: "foo",
+  };
+  const preferences = {};
+  for (const [prefName, prefInfo] of Object.entries(opts.preferences || defaultPreferences)) {
+    preferences[prefName] = { ...defaultPrefInfo, ...prefInfo };
+  }
+  return {
+    slug: "test",
+    ratio: 1,
+    ...opts,
+    preferences,
+  };
+}
+
+function argumentsFactory(args) {
+  const defaultBranches = (args && args.branches) || [{}];
+  const branches = defaultBranches.map(branchFactory);
+  return {
+    slug: "test",
     isHighPopulation: false,
     ...args,
+    branches,
   };
 }
 
 function preferenceExperimentFactory(args) {
   return recipeFactory({
     name: "preference-experiment",
     arguments: argumentsFactory(args),
   });
@@ -76,21 +94,37 @@ decorate_task(
 
 decorate_task(
   withStub(PreferenceExperiments, "start"),
   PreferenceExperiments.withMockExperiments([]),
   async function enroll_user_if_never_been_in_experiment(startStub) {
     const action = new PreferenceExperimentAction();
     const recipe = preferenceExperimentFactory({
       slug: "test",
-      preferenceName: "fake.preference",
-      preferenceBranchType: "user",
       branches: [
-        { slug: "branch1", value: "branch1", ratio: 1 },
-        { slug: "branch2", value: "branch2", ratio: 1 },
+        {
+          slug: "branch1",
+          preferences: {
+            "fake.preference": {
+              preferenceBranchType: "user",
+              preferenceValue: "branch1",
+            },
+          },
+          ratio: 1,
+        },
+        {
+          slug: "branch2",
+          preferences: {
+            "fake.preference": {
+              preferenceBranchType: "user",
+              preferenceValue: "branch2",
+            },
+          },
+          ratio: 1,
+        },
       ],
     });
     sinon.stub(action, "chooseBranch").callsFake(async function(slug, branches) {
       return branches[0];
     });
 
     await action.runRecipe(recipe);
     await action.finalize();
@@ -188,17 +222,21 @@ decorate_task(
       },
       expired: false,
     },
   ]),
   async function do_nothing_if_preference_is_already_being_tested(startStub, reportRecipeStub) {
     const action = new PreferenceExperimentAction();
     const recipe = preferenceExperimentFactory({
       slug: "new",
-      preferenceName: "conflict.pref",
+      branches: [
+        {
+          preferences: {"conflict.pref": {}},
+        },
+      ],
     });
     action.chooseBranch = sinon.stub().callsFake(async function(slug, branches) {
       return branches[0];
     });
 
     await action.runRecipe(recipe);
     await action.finalize();
 
@@ -242,18 +280,32 @@ decorate_task(
 );
 
 decorate_task(
   withStub(Sampling, "ratioSample"),
   async function chooseBranch_uses_ratioSample(ratioSampleStub) {
     ratioSampleStub.returns(Promise.resolve(1));
     const action = new PreferenceExperimentAction();
     const branches = [
-      { value: "branch0", ratio: 1 },
-      { value: "branch1", ratio: 2 },
+      {
+        preferences: {
+          "fake.preference": {
+            preferenceValue: "branch0",
+          },
+        },
+        ratio: 1,
+      },
+      {
+        preferences: {
+          "fake.preference": {
+            preferenceValue: "branch1",
+          },
+        },
+        ratio: 2,
+      },
     ];
     const sandbox = sinon.createSandbox();
     let result;
     try {
       sandbox.stub(ClientEnvironment, "userId").get(() => "fake-id");
       result = await action.chooseBranch("exp-slug", branches);
     } finally {
       sandbox.restore();
new file mode 100644
--- /dev/null
+++ b/toolkit/components/normandy/test/browser/browser_actions_SinglePreferenceExperimentAction.js
@@ -0,0 +1,96 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+"use strict";
+
+ChromeUtils.import("resource://gre/modules/components-utils/Sampling.jsm", this);
+ChromeUtils.import("resource://gre/modules/Services.jsm", this);
+ChromeUtils.import("resource://gre/modules/Preferences.jsm", this);
+ChromeUtils.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
+ChromeUtils.import("resource://normandy/lib/ClientEnvironment.jsm", this);
+ChromeUtils.import("resource://normandy/lib/PreferenceExperiments.jsm", this);
+ChromeUtils.import("resource://normandy/lib/TelemetryEvents.jsm", this);
+ChromeUtils.import("resource://normandy/lib/Uptake.jsm", this);
+ChromeUtils.import("resource://normandy/actions/PreferenceExperimentAction.jsm", this);
+ChromeUtils.import("resource://normandy/actions/SinglePreferenceExperimentAction.jsm", this);
+
+function argumentsFactory(args) {
+  return {
+    slug: "test",
+    preferenceName: "fake.preference",
+    preferenceType: "string",
+    preferenceBranchType: "default",
+    branches: [
+      { slug: "test", value: "foo", ratio: 1 },
+    ],
+    isHighPopulation: false,
+    ...args,
+  };
+}
+
+function preferenceExperimentFactory(args) {
+  return recipeFactory({
+    name: "preference-experiment",
+    arguments: argumentsFactory(args),
+  });
+}
+
+decorate_task(
+  withStub(PreferenceExperimentAction.prototype, "_run"),
+  PreferenceExperiments.withMockExperiments([]),
+  async function enroll_user_if_never_been_in_experiment(runStub) {
+    const action = new SinglePreferenceExperimentAction();
+    const recipe = preferenceExperimentFactory({
+      slug: "test",
+      preferenceName: "fake.preference",
+      preferenceBranchType: "user",
+      branches: [
+        {
+          slug: "branch1",
+          value: "branch1",
+          ratio: 1,
+        },
+        {
+          slug: "branch2",
+          value: "branch2",
+          ratio: 1,
+        },
+      ],
+    });
+    sinon.stub(action, "chooseBranch").callsFake(async function(slug, branches) {
+      return branches[0];
+    });
+
+    await action.runRecipe(recipe);
+    await action.finalize();
+
+    Assert.deepEqual(runStub.args, [[{
+      id: recipe.id,
+      name: "preference-experiment",
+      arguments: {
+        slug: "test",
+        isHighPopulation: false,
+        branches: [{
+          slug: "branch1",
+          ratio: 1,
+          preferences: {
+            "fake.preference": {
+              preferenceValue: "branch1",
+              preferenceType: "string",
+              preferenceBranchType: "user",
+            },
+          },
+        }, {
+          slug: "branch2",
+          ratio: 1,
+          preferences: {
+            "fake.preference": {
+              preferenceValue: "branch2",
+              preferenceType: "string",
+              preferenceBranchType: "user",
+            },
+          },
+        }],
+      },
+    }]]);
+  }
+);