Bug 1376599 - Allow annotating experiments with a type. r=Dexter a=jcristau
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Wed, 28 Jun 2017 17:18:00 -0400
changeset 411822 e2dc26e7f164b205b8bef15da7891ea5b712d7da
parent 411821 1a6afc516f27243665c1c411d5c4143d16770415
child 411823 95bd58aaf67878795cdb002b418d3a1f20e6d1b8
push id7476
push usercbook@mozilla.com
push dateWed, 05 Jul 2017 13:11:22 +0000
treeherdermozilla-beta@9856974e03e2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter, jcristau
bugs1376599
milestone55.0
Bug 1376599 - Allow annotating experiments with a type. r=Dexter a=jcristau
toolkit/components/telemetry/TelemetryEnvironment.jsm
toolkit/components/telemetry/docs/collection/experiments.rst
toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
--- a/toolkit/components/telemetry/TelemetryEnvironment.jsm
+++ b/toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -38,16 +38,17 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 
 // The maximum length of a string (e.g. description) in the addons section.
 const MAX_ADDON_STRING_LENGTH = 100;
 // The maximum length of a string value in the settings.attribution object.
 const MAX_ATTRIBUTION_STRING_LENGTH = 100;
 // The maximum lengths for the experiment id and branch in the experiments section.
 const MAX_EXPERIMENT_ID_LENGTH = 100;
 const MAX_EXPERIMENT_BRANCH_LENGTH = 100;
+const MAX_EXPERIMENT_TYPE_LENGTH = 20;
 
 /**
  * This is a policy object used to override behavior for testing.
  */
 var Policy = {
   now: () => new Date(),
 };
 
@@ -82,19 +83,21 @@ this.TelemetryEnvironment = {
 
   /**
    * Add an experiment annotation to the environment.
    * If an annotation with the same id already exists, it will be overwritten.
    * This triggers a new subsession, subject to throttling.
    *
    * @param {String} id The id of the active experiment.
    * @param {String} branch The experiment branch.
+   * @param {Object} [options] Optional object with options.
+   * @param {String} [options.type=false] The specific experiment type.
    */
-  setExperimentActive(id, branch) {
-    return getGlobal().setExperimentActive(id, branch);
+  setExperimentActive(id, branch, options={}) {
+    return getGlobal().setExperimentActive(id, branch, options);
   },
 
   /**
    * Remove an experiment annotation from the environment.
    * If the annotation exists, a new subsession will triggered.
    *
    * @param {String} id The id of the active experiment.
    */
@@ -882,35 +885,47 @@ EnvironmentCache.prototype = {
     this._log.trace("unregisterChangeListener for " + name);
     if (this._shutdown) {
       this._log.warn("registerChangeListener - already shutdown");
       return;
     }
     this._changeListeners.delete(name);
   },
 
-  setExperimentActive(id, branch) {
+  setExperimentActive(id, branch, options) {
     this._log.trace("setExperimentActive");
     // Make sure both the id and the branch have sane lengths.
     const saneId = limitStringToLength(id, MAX_EXPERIMENT_ID_LENGTH);
     const saneBranch = limitStringToLength(branch, MAX_EXPERIMENT_BRANCH_LENGTH);
     if (!saneId || !saneBranch) {
       this._log.error("setExperimentActive - the provided arguments are not strings.");
       return;
     }
 
     // Warn the user about any content truncation.
     if (saneId.length != id.length || saneBranch.length != branch.length) {
       this._log.warn("setExperimentActive - the experiment id or branch were truncated.");
     }
 
+    // Truncate the experiment type if present.
+    if (options.hasOwnProperty("type")) {
+      let type = limitStringToLength(options.type, MAX_EXPERIMENT_TYPE_LENGTH);
+      if (type.length != options.type.length) {
+        options.type = type;
+        this._log.warn("setExperimentActive - the experiment type was truncated.");
+      }
+    }
+
     let oldEnvironment = Cu.cloneInto(this._currentEnvironment, myScope);
     // Add the experiment annotation.
     let experiments = this._currentEnvironment.experiments || {};
     experiments[saneId] = { "branch": saneBranch };
+    if (options.hasOwnProperty("type")) {
+      experiments[saneId].type = options.type;
+    }
     this._currentEnvironment.experiments = experiments;
     // Notify of the change.
     this._onEnvironmentChange("experiment-annotation-changed", oldEnvironment);
   },
 
   setExperimentInactive(id) {
     this._log.trace("setExperimentInactive");
     let experiments = this._currentEnvironment.experiments || {};
--- a/toolkit/components/telemetry/docs/collection/experiments.rst
+++ b/toolkit/components/telemetry/docs/collection/experiments.rst
@@ -4,21 +4,22 @@ Experiment Annotation
 This API allows privileged JavaScript to annotate the :doc:`../data/environment` with any experiments a client is participating in.
 
 The experiment annotations are sent with any ping that includes the :doc:`../data/environment` data.
 
 The JS API
 ==========
 Privileged JavaScript code can annotate experiments using the functions exposed by ``TelemetryEnvironment.jsm``.
 
-The following function adds an annotation to the environment for the provided ``id`` and ``branch``. Calling this function repeatedly with the same ``id`` will overwrite the state and trigger new subsessions (subject to throttling).
+The following function adds an annotation to the environment for the provided ``id``, ``branch`` and ``options``. Calling this function repeatedly with the same ``id`` will overwrite the state and trigger new subsessions (subject to throttling).
+``options`` is an object that currently may contain ``type``, to tag the experiment with a specific type.
 
 .. code-block:: js
 
-    TelemetryEnvironment.setExperimentActive(id, branch)
+    TelemetryEnvironment.setExperimentActive(id, branch, [options={}}])
 
 This removes the annotation for the experiment with the provided ``id``.
 
 .. code-block:: js
 
     TelemetryEnvironment.setExperimentInactive(id)
 
 This synchronously returns a dictionary containing the information for each active experiment.
@@ -31,8 +32,9 @@ This synchronously returns a dictionary 
 
     Both ``setExperimentActive`` and ``setExperimentInactive`` trigger a new subsession. However
     the latter only does so if there was an active experiment with the provided ``id``.
 
 Limits and restrictions
 -----------------------
 To prevent abuses, the content of both the experiment ``id`` and ``branch`` is limited to
 100 characters in length.
+``type`` is limited to a lenght of 20 characters.
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -802,16 +802,19 @@ function checkExperimentsSection(data) {
 
   for (let id in experiments) {
     Assert.ok(checkString(id), id + " must be a valid string.");
 
     // Check that we have valid experiment info.
     let experimentData = experiments[id];
     Assert.ok("branch" in experimentData, "The experiment must have branch data.")
     Assert.ok(checkString(experimentData.branch), "The experiment data must be valid.");
+    if ("type" in experimentData) {
+      Assert.ok(checkString(experimentData.type));
+    }
   }
 }
 
 function checkEnvironmentData(data, isInitial = false, expectBrokenAddons = false) {
   checkBuildSection(data);
   checkSettingsSection(data);
   checkProfileSection(data);
   checkPartnerSection(data, isInitial);
@@ -1608,17 +1611,17 @@ add_task(async function test_osstrings()
 });
 
 add_task(async function test_experimentsAPI() {
   const EXPERIMENT1 = "experiment-1";
   const EXPERIMENT1_BRANCH = "nice-branch";
   const EXPERIMENT2 = "experiment-2";
   const EXPERIMENT2_BRANCH = "other-branch";
 
-  let checkExperiment = (id, branch, environmentData) => {
+  let checkExperiment = (environmentData, id, branch, type=null) => {
     Assert.ok("experiments" in environmentData,
               "The current environment must report the experiment annotations.");
     Assert.ok(id in environmentData.experiments,
               "The experiments section must contain the expected experiment id.");
     Assert.equal(environmentData.experiments[id].branch, branch,
                  "The experiment branch must be correct.");
   };
 
@@ -1643,36 +1646,36 @@ add_task(async function test_experiments
   // Check that the old environment does not contain the experiments.
   checkEnvironmentData(eventEnvironmentData);
   Assert.ok(!("experiments" in eventEnvironmentData),
             "No experiments section must be reported in the old environment.");
 
   // Check that the current environment contains the right experiment.
   data = TelemetryEnvironment.currentEnvironment;
   checkEnvironmentData(data);
-  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, data);
+  checkExperiment(data, EXPERIMENT1, EXPERIMENT1_BRANCH);
 
   TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI");
 
   // Add a second annotation and check that both experiments are there.
   deferred = PromiseUtils.defer();
   TelemetryEnvironment.registerChangeListener("test_experimentsAPI2", (reason, env) => {
     deferred.resolve(env);
   });
   TelemetryEnvironment.setExperimentActive(EXPERIMENT2, EXPERIMENT2_BRANCH);
   eventEnvironmentData = await deferred.promise;
 
   // Check that the current environment contains both the experiment.
   data = TelemetryEnvironment.currentEnvironment;
   checkEnvironmentData(data);
-  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, data);
-  checkExperiment(EXPERIMENT2, EXPERIMENT2_BRANCH, data);
+  checkExperiment(data, EXPERIMENT1, EXPERIMENT1_BRANCH);
+  checkExperiment(data, EXPERIMENT2, EXPERIMENT2_BRANCH);
 
   // The previous environment should only contain the first experiment.
-  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, eventEnvironmentData);
+  checkExperiment(eventEnvironmentData, EXPERIMENT1, EXPERIMENT1_BRANCH);
   Assert.ok(!(EXPERIMENT2 in eventEnvironmentData),
             "The old environment must not contain the new experiment annotation.");
 
   TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI2");
 
   // Check that removing an unknown experiment annotation does not trigger
   // a notification.
   TelemetryEnvironment.registerChangeListener("test_experimentsAPI3", () => {
@@ -1694,23 +1697,26 @@ add_task(async function test_experiments
   TelemetryEnvironment.setExperimentInactive(EXPERIMENT1);
   eventEnvironmentData = await deferred.promise;
 
   // Check that the current environment contains just the second experiment.
   data = TelemetryEnvironment.currentEnvironment;
   checkEnvironmentData(data);
   Assert.ok(!(EXPERIMENT1 in data),
             "The current environment must not contain the removed experiment annotation.");
-  checkExperiment(EXPERIMENT2, EXPERIMENT2_BRANCH, data);
+  checkExperiment(data, EXPERIMENT2, EXPERIMENT2_BRANCH);
 
   // The previous environment should contain both annotations.
-  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, eventEnvironmentData);
-  checkExperiment(EXPERIMENT2, EXPERIMENT2_BRANCH, eventEnvironmentData);
+  checkExperiment(eventEnvironmentData, EXPERIMENT1, EXPERIMENT1_BRANCH);
+  checkExperiment(eventEnvironmentData, EXPERIMENT2, EXPERIMENT2_BRANCH);
 
-  TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI5");
+  // Set an experiment with a type and check that it correctly shows up.
+  TelemetryEnvironment.setExperimentActive("typed-experiment", "random-branch", {type: "ab-test"});
+  data = TelemetryEnvironment.currentEnvironment;
+  checkExperiment(data, "typed-experiment", "random-branch", "ab-test");
 });
 
 add_task(async function test_experimentsAPI_limits() {
   const EXPERIMENT = "experiment-2-experiment-2-experiment-2-experiment-2-experiment-2" +
                      "-experiment-2-experiment-2-experiment-2-experiment-2";
   const EXPERIMENT_BRANCH = "other-branch-other-branch-other-branch-other-branch-other" +
                             "-branch-other-branch-other-branch-other-branch-other-branch";
   const EXPERIMENT_TRUNCATED = EXPERIMENT.substring(0, 100);
@@ -1742,16 +1748,22 @@ add_task(async function test_experiments
   Assert.ok(EXPERIMENT_TRUNCATED in data.experiments,
             "The experiments must be reporting the truncated id.");
   Assert.ok(!(EXPERIMENT in data.experiments),
             "The experiments must not be reporting the full id.");
   Assert.equal(EXPERIMENT_BRANCH_TRUNCATED, data.experiments[EXPERIMENT_TRUNCATED].branch,
             "The experiments must be reporting the truncated branch.");
 
   TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI");
+
+  // Check that an overly long type is truncated.
+  const longType = "a0123456678901234567890123456789";
+  TelemetryEnvironment.setExperimentActive("exp", "some-branch", {type: longType});
+  data = TelemetryEnvironment.currentEnvironment;
+  Assert.equal(data.experiments["exp"].type, longType.substring(0, 20));
 });
 
 add_task(async function test_environmentShutdown() {
   // Define and reset the test preference.
   const PREF_TEST = "toolkit.telemetry.test.pref1";
   const PREFS_TO_WATCH = new Map([
     [PREF_TEST, {what: TelemetryEnvironment.RECORD_PREF_STATE}],
   ]);