Bug 1348748 - Implement telemetry experiment annotations to TelemetryEnvironment.jsm. r=gfritzsche
MozReview-Commit-ID: KCb8MrWh4Rt
--- 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);