Bug 1348748 - Implement telemetry experiment annotations to TelemetryEnvironment.jsm. r=gfritzsche draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Mon, 20 Mar 2017 10:54:46 +0100
changeset 503012 ca51eb4341246e48732488db7b9e897cc594d793
parent 501295 1b9293be51637f841275541d8991314ca56561a5
child 550307 cc2c719badcbfc90ae3d6b827e11d8b4953f9d00
push id50450
push useralessio.placitelli@gmail.com
push dateWed, 22 Mar 2017 16:57:51 +0000
reviewersgfritzsche
bugs1348748
milestone55.0a1
Bug 1348748 - Implement telemetry experiment annotations to TelemetryEnvironment.jsm. r=gfritzsche MozReview-Commit-ID: KCb8MrWh4Rt
toolkit/components/telemetry/TelemetryEnvironment.jsm
toolkit/components/telemetry/docs/collection/index.rst
toolkit/components/telemetry/docs/data/environment.rst
toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryEnvironment.jsm
+++ b/toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -38,16 +38,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/UpdateUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "WindowsRegistry",
                                   "resource://gre/modules/WindowsRegistry.jsm");
 
 // 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;
 
 /**
  * This is a policy object used to override behavior for testing.
  */
 var Policy = {
   now: () => new Date(),
 };
 
@@ -75,16 +78,52 @@ this.TelemetryEnvironment = {
   registerChangeListener(name, listener) {
     return getGlobal().registerChangeListener(name, listener);
   },
 
   unregisterChangeListener(name) {
     return getGlobal().unregisterChangeListener(name);
   },
 
+  /**
+   * 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.
+   */
+  setExperimentActive(id, branch) {
+    return getGlobal().setExperimentActive(id, branch);
+  },
+
+  /**
+   * 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.
+   */
+  setExperimentInactive(id) {
+    return getGlobal().setExperimentInactive(id);
+  },
+
+  /**
+   * Returns an object containing the data for the active experiments.
+   *
+   * The returned object is of the format:
+   *
+   * {
+   *   "<experiment id>": { branch: "<branch>" },
+   *   // …
+   * }
+   */
+  getActiveExperiments() {
+    return getGlobal().getActiveExperiments();
+  },
+
   shutdown() {
     return getGlobal().shutdown();
   },
 
   // Policy to use when saving preferences. Exported for using them in tests.
   RECORD_PREF_STATE: 1, // Don't record the preference value
   RECORD_PREF_VALUE: 2, // We only record user-set prefs.
   RECORD_DEFAULTPREF_VALUE: 3, // We only record default pref if set
@@ -846,16 +885,57 @@ 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) {
+    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.");
+    }
+
+    let oldEnvironment = Cu.cloneInto(this._currentEnvironment, myScope);
+    // Add the experiment annotation.
+    let experiments = this._currentEnvironment.experiments || {};
+    experiments[saneId] = { "branch": saneBranch };
+    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 || {};
+    if (id in experiments) {
+      // Only attempt to notify if a previous annotation was found and removed.
+      let oldEnvironment = Cu.cloneInto(this._currentEnvironment, myScope);
+      // Remove the experiment annotation.
+      delete this._currentEnvironment.experiments[id];
+      // Notify of the change.
+      this._onEnvironmentChange("experiment-annotation-changed", oldEnvironment);
+    }
+  },
+
+  getActiveExperiments() {
+    return Cu.cloneInto(this._currentEnvironment.experiments || {}, myScope);
+  },
+
   shutdown() {
     this._log.trace("shutdown");
     this._shutdown = true;
   },
 
   /**
    * Only used in tests, set the preferences to watch.
    * @param aPreferences A map of preferences names and their recording policy.
--- a/toolkit/components/telemetry/docs/collection/index.rst
+++ b/toolkit/components/telemetry/docs/collection/index.rst
@@ -34,8 +34,16 @@ The current data collection possibilitie
    measuring-time
    custom-pings
    stack-capture
    *
 
 Browser Usage Telemetry
 ~~~~~~~~~~~~~~~~~~~~~~~
 For more information, see :ref:`browserusagetelemetry`.
+
+Experiment Annotation
+~~~~~~~~~~~~~~~~~~~~~
+Experiment annotations can be added through the API exposed in ``TelemetryEnvironment.jsm`` and are collected in the :doc:`environment <../data/environment>`:
+
+- ``TelemetryEnvironment.setExperimentActive(id, branch)``, adds an annotation to the environment for the provided ``id`` and ``branch``. This triggers a new subsession.
+- ``TelemetryEnvironment.setExperimentInactive(id)``, removes the annotation for the experiment with the provided ``id``. This triggers a new subsession.
+- ``TelemetryEnvironment.getActiveExperiments()``, returns a dictionary containing the informations for each active experiment.
--- a/toolkit/components/telemetry/docs/data/environment.rst
+++ b/toolkit/components/telemetry/docs/data/environment.rst
@@ -252,16 +252,20 @@ Structure:
             ...
         },
         activeExperiment: { // section is empty if there's no active experiment
             id: <string>, // id
             branch: <string>, // branch name
         },
         persona: <string>, // id of the current persona, null on GONK
       },
+      experiments: {
+        "<experiment id>": { branch: "<branch>" },
+        // ...
+      }
     }
 
 build
 -----
 
 buildId
 ~~~~~~~
 Firefox builds downloaded from mozilla.org use a 14-digit buildId. Builds included in other distributions may have a different format (e.g. only 10 digits).
@@ -369,8 +373,12 @@ This object contains operating system in
 
 addons
 ------
 
 activeAddons
 ~~~~~~~~~~~~
 
 Starting from Firefox 44, the length of the following string fields: ``name``, ``description`` and ``version`` is limited to 100 characters. The same limitation applies to the same fields in ``theme`` and ``activePlugins``.
+
+experiments
+-----------
+For each experiment we collect the ``id`` and the ``branch`` the client is enrolled in. Both fields are truncated to 100 characters and a warning is printed when that happens. This section will eventually supersede ``addons/activeExperiment``.
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -784,23 +784,41 @@ function checkAddonsSection(data, expect
     Assert.ok(checkString(experiment.id));
     Assert.ok(checkString(experiment.branch));
   }
 
   // Check persona
   Assert.ok(checkNullOrString(data.addons.persona));
 }
 
+function checkExperimentsSection(data) {
+  // We don't expect the experiments section to be always available.
+  let experiments = data.experiments || {};
+  if (Object.keys(experiments).length == 0) {
+    return;
+  }
+
+  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.");
+  }
+}
+
 function checkEnvironmentData(data, isInitial = false, expectBrokenAddons = false) {
   checkBuildSection(data);
   checkSettingsSection(data);
   checkProfileSection(data);
   checkPartnerSection(data, isInitial);
   checkSystemSection(data);
   checkAddonsSection(data, expectBrokenAddons);
+  checkExperimentsSection(data);
 }
 
 add_task(function* setup() {
   // Load a custom manifest to provide search engine loading from JAR files.
   do_load_manifest("chrome.manifest");
   registerFakeSysInfo();
   spoofGfxAdapter();
   do_get_profile();
@@ -1517,16 +1535,158 @@ add_task(function* test_osstrings() {
     Assert.equal(data.system.os.kernelVersion, null);
   }
 
   // Clean up.
   SysInfo.overrides = {};
   yield TelemetryEnvironment.testCleanRestart().onInitialized();
 });
 
+add_task(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) => {
+    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.");
+  };
+
+  // Clean the environment and check that it's reporting the correct info.
+  yield TelemetryEnvironment.testCleanRestart().onInitialized();
+  let data = TelemetryEnvironment.currentEnvironment;
+  checkEnvironmentData(data);
+
+  // We don't expect the experiments section to be there if no annotation
+  // happened.
+  Assert.ok(!("experiments" in data),
+            "No experiments section must be reported if nothing was annotated.");
+
+  // Add a change listener and add an experiment annotation.
+  let deferred = PromiseUtils.defer();
+  TelemetryEnvironment.registerChangeListener("test_experimentsAPI", (reason, env) => {
+    deferred.resolve(env);
+  });
+  TelemetryEnvironment.setExperimentActive(EXPERIMENT1, EXPERIMENT1_BRANCH);
+  let eventEnvironmentData = yield deferred.promise;
+
+  // 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);
+
+  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 = yield 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);
+
+  // The previous environment should only contain the first experiment.
+  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, eventEnvironmentData);
+  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", () => {
+    Assert.ok(false, "Removing an unknown experiment annotation must not trigger a change.");
+  });
+  TelemetryEnvironment.setExperimentInactive("unknown-experiment-id");
+  // Also make sure that passing non-string parameters arguments doesn't throw nor
+  // trigger a notification.
+  TelemetryEnvironment.setExperimentActive({}, "some-branch");
+  TelemetryEnvironment.setExperimentActive("some-id", {});
+  TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI3");
+
+  // Check that removing a known experiment leaves the other in place and triggers
+  // a change.
+  deferred = PromiseUtils.defer();
+  TelemetryEnvironment.registerChangeListener("test_experimentsAPI4", (reason, env) => {
+    deferred.resolve(env);
+  });
+  TelemetryEnvironment.setExperimentInactive(EXPERIMENT1);
+  eventEnvironmentData = yield 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);
+
+  // The previous environment should contain both annotations.
+  checkExperiment(EXPERIMENT1, EXPERIMENT1_BRANCH, eventEnvironmentData);
+  checkExperiment(EXPERIMENT2, EXPERIMENT2_BRANCH, eventEnvironmentData);
+
+  TelemetryEnvironment.unregisterChangeListener("test_experimentsAPI5");
+});
+
+add_task(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);
+  const EXPERIMENT_BRANCH_TRUNCATED = EXPERIMENT_BRANCH.substring(0, 100);
+
+  // Clean the environment and check that it's reporting the correct info.
+  yield TelemetryEnvironment.testCleanRestart().onInitialized();
+  let data = TelemetryEnvironment.currentEnvironment;
+  checkEnvironmentData(data);
+
+  // We don't expect the experiments section to be there if no annotation
+  // happened.
+  Assert.ok(!("experiments" in data),
+            "No experiments section must be reported if nothing was annotated.");
+
+  // Add a change listener and wait for the annotation to happen.
+  let deferred = PromiseUtils.defer();
+  TelemetryEnvironment.registerChangeListener("test_experimentsAPI",
+                                              () => deferred.resolve());
+  TelemetryEnvironment.setExperimentActive(EXPERIMENT, EXPERIMENT_BRANCH);
+  yield deferred.promise;
+
+  // Check that the current environment contains the truncated values
+  // for the experiment data.
+  data = TelemetryEnvironment.currentEnvironment;
+  checkEnvironmentData(data);
+  Assert.ok("experiments" in data,
+            "The environment must contain an experiments section.");
+  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");
+});
+
 add_task(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}],
   ]);
   Preferences.reset(PREF_TEST);
 
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -1262,19 +1262,93 @@ add_task(function* test_environmentChang
   Assert.equal(ping.type, PING_TYPE_MAIN);
   Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 1);
   Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), startDay.toISOString());
 
   Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
   Assert.ok(!(KEYED_ID in ping.payload.keyedHistograms));
+
+  yield TelemetryController.testShutdown();
+});
+
+add_task(function* test_experimentAnnotations_subsession() {
+  if (gIsAndroid) {
+    // We don't split subsessions on environment changes yet on Android.
+    return;
+  }
+
+  const EXPERIMENT1 = "experiment-1";
+  const EXPERIMENT1_BRANCH = "nice-branch";
+  const EXPERIMENT2 = "experiment-2";
+  const EXPERIMENT2_BRANCH = "other-branch";
+
+  yield TelemetryStorage.testClearPendingPings();
+  PingServer.clearRequests();
+
+  let now = fakeNow(2040, 1, 1, 12, 0, 0);
+  gMonotonicNow = fakeMonotonicNow(gMonotonicNow + 10 * MILLISECONDS_PER_MINUTE);
+
+  // Setup.
+  yield TelemetryController.testReset();
+  TelemetrySend.setServer("http://localhost:" + PingServer.port);
+  Assert.equal(TelemetrySession.getPayload().info.subsessionCounter, 1);
+
+  // Trigger a subsession split with a telemetry annotation.
+  gMonotonicNow = fakeMonotonicNow(gMonotonicNow + 10 * MILLISECONDS_PER_MINUTE);
+  let futureTestDate = futureDate(now, 10 * MILLISECONDS_PER_MINUTE);
+  now = fakeNow(futureTestDate);
+  TelemetryEnvironment.setExperimentActive(EXPERIMENT1, EXPERIMENT1_BRANCH);
+
+  let ping = yield PingServer.promiseNextPing();
+  Assert.ok(!!ping, "A ping must be received.");
+
+  Assert.equal(ping.type, PING_TYPE_MAIN, "The received ping must be a 'main' ping.");
+  Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE,
+               "The 'main' ping must be triggered by a change in the environment.");
+  // We expect the current experiments to be reported in the next ping, not this
+  // one.
+  Assert.ok(!("experiments" in ping.environment),
+            "The old environment must contain no active experiments.");
+  // Since this change wasn't throttled, the subsession counter must increase.
+  Assert.equal(TelemetrySession.getPayload().info.subsessionCounter, 2,
+               "The experiment annotation must trigger a new subsession.");
+
+  // Add another annotation to the environment. We're not advancing the fake
+  // timer, so no subsession split should happen due to throttling.
+  TelemetryEnvironment.setExperimentActive(EXPERIMENT2, EXPERIMENT2_BRANCH);
+  Assert.equal(TelemetrySession.getPayload().info.subsessionCounter, 2,
+               "The experiment annotation must not trigger a new subsession " +
+               "if throttling happens.");
+  let oldExperiments = TelemetryEnvironment.getActiveExperiments();
+
+  // Fake the timer and remove an annotation, we expect a new subsession split.
+  gMonotonicNow = fakeMonotonicNow(gMonotonicNow + 10 * MILLISECONDS_PER_MINUTE);
+  now = fakeNow(futureDate(now, 10 * MILLISECONDS_PER_MINUTE));
+  TelemetryEnvironment.setExperimentInactive(EXPERIMENT1, EXPERIMENT1_BRANCH);
+
+  ping = yield PingServer.promiseNextPing();
+  Assert.ok(!!ping, "A ping must be received.");
+
+  Assert.equal(ping.type, PING_TYPE_MAIN, "The received ping must be a 'main' ping.");
+  Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE,
+               "The 'main' ping must be triggered by a change in the environment.");
+  // We expect both experiments to be in this environment.
+  Assert.deepEqual(ping.environment.experiments, oldExperiments,
+                   "The environment must contain both the experiments.");
+  Assert.equal(TelemetrySession.getPayload().info.subsessionCounter, 3,
+               "The removing an experiment annotation must trigger a new subsession.");
+
+  yield TelemetryController.testShutdown();
 });
 
 add_task(function* test_savedPingsOnShutdown() {
+  yield TelemetryController.testReset();
+
   // On desktop, we expect both "saved-session" and "shutdown" pings. We only expect
   // the former on Android.
   const expectedPingCount = (gIsAndroid) ? 1 : 2;
   // Assure that we store the ping properly when saving sessions on shutdown.
   // We make the TelemetryController shutdown to trigger a session save.
   const dir = TelemetryStorage.pingDirectoryPath;
   yield OS.File.removeDir(dir, {ignoreAbsent: true});
   yield OS.File.makeDir(dir);