1017806 - Telemetry Experiments: builtin support for multiple branches of an experiment

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Benjamin Smedberg]

A common request for experiments is to be able to divide a single experiment into multiple branches. Some recent use cases:

* install the translation experiment for everyone in certain locales in order to do additional data collection, but only show the translation UI to 50% of users. Translation is hacking this with a startup script and recording values in FHR, bug 1015525.

* test a range of 4 different pref values for a plugin pref, bug 1011136

* test multiple variations of social network sharing buttons in Firefox

Rather than having each experiment re-invent the wheel, we should make this part of the experiment system. For now, I think the experiment system should not do the actual partitioning, but should only record the experiment branch and make sure it ends up in the data payloads (FHR and telemetry). The experiment will still be responsible for selecting a branch on install/firstrun and informing the experiment manager about it.

API:
Experiments.instance().getExperimentBranch("experimentid"); // returns null if the branch hasn't been set
Experiments.instance().setExperimentBranch("experimentid", value-coerced-to-string);

Telemetry:
add "activeExperimentBranch" to the current "activeExperiment" data. null if not set.

FHR:
add "lastActiveBranch" to the current "lastActive" in FHR. null if not set.

bwinton/gregglind, please verify that this plan is reasonable.

mdeboer, cc'ing you on this in case this sounds easier than bug 1015525 and you'd like to just take it.

Comment 1

[Blake Winton (:bwinton) (:☕️)]

I think it sounds reasonable, but I have a couple of comments:

1) For the hypothetical newtab-tile-shuffler which puts the tiles in a random order, do you think the experimentid is a good place to store the order, or would 9-factorial be too many ids to track?

2) Currently, there's nothing Experiment-specific in the existing newtab-tile-shuffler add-on, and it seems to me that being able to use any add-on as an experiment is a useful feature.  Could we (perhaps optionally) put the experimentid calculation outside of the add-ons themselves?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Is the idea to do something like this?

on add-on install:
if (Math.random() > 0.5) {
  setExperimentBranch("group", "control");
  setPrefs(..);
} else {
  setExperimentBranch("group", "test");
  setPrefs(..);
}

Comment 3

[Benjamin Smedberg]

Well, I did in fact have to edit the newtab-tile-shuffler addon to make it an experiment, because it needs to be em:type=128. If we have this kind of thing as both an addon and an experiment, how would you record the variation in the non-experiment case?

FWIW, I don't think that 9-factorial would be a problem: record it using hex 4-bits-per-position so

"0x876543210" - normal order
"0x012345678" - reverse order
"0x346789012" - random order

Comment 4

[Benjamin Smedberg]

Felipe yes, although probably there would be an extra .getExperimentBranch call in there in case this is an upgrade of an existing experiment.

Comment 5

[Gregory Szorc [:gps]]

We may want to consider storing a set of string "tags" as opposed to a single scalar string. (I could easily foresee a requirement where a single experiment takes multiple branches.) Limiting ourselves to a single branch point seems... limiting.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Well, I did in fact have to edit the newtab-tile-shuffler addon to make it
> an experiment, because it needs to be em:type=128.

Ah, okay, then I don't mind adding a little more tie-ins to the experiment system.

> If we have this kind of
> thing as both an addon and an experiment, how would you record the variation
> in the non-experiment case?

I'ld set a pref.  (I'ld need to set a pref anyways, so that I'm re-using the same order between views.)

> FWIW, I don't think that 9-factorial would be a problem: record it using hex
> 4-bits-per-position so
> 
> "0x876543210" - normal order
> "0x012345678" - reverse order
> "0x346789012" - random order

Okay, so we're not going to auto-split among files based on experimentid or anything…

(And I'ld probably just record a string "012345678"…  ;)

Comment 7

[Benjamin Smedberg]

> Limiting ourselves to a single branch
> point seems... limiting.

That's true, but it makes automatic analysis easier in the simple cases. I can't think of a system right now where if we had multiple branches we still couldn't encode them in a string like "tabbar:blue" or something.

Simplicity wins most of the time.

Comment 8

[Mike de Boer [:mikedeboer]]

Well, hum, yeah...it does sound more reasonable to fix it like this than re-inventing the wheel there, like you said. Plus I'll get acquainted with the Experiments code!

Comment 9

[Georg Fritzsche [:gfritzsche]]

Do we have anything that really requires the branching to be happening in the experiments?
Could we just add branch-criteria to the manifest?

E.g.: "branches": { "control": 0.5, "test": 0.5 }

Comment 10

[Benjamin Smedberg]

I think that comment 9 (a static declaration of branch probabilities) would be a good followup bug to this one. As it stands, some of the social-sharing experiments may want to control their branches in custom ways, e.g. only test bookmark-toolbar buttons if the user sees the bookmark toolbar.

Comment 11

[Mike de Boer [:mikedeboer]]

I was able to fix bug 1015525 without the need for this feature, so I'm unassigning myself to move out of others' way who want to work on this!

Comment 12

[Benjamin Smedberg]

Attachment: experiment-branch

Checkpoint. I'm having trouble passing browser/experiments/test/xpcshell/test_healthreport.js because it's failing at this line:

http://hg.mozilla.org/mozilla-central/annotate/31de1a84b27f/browser/experiments/test/xpcshell/test_healthreport.js#l67

gps or gfritzsche do you have ideas as to the failure?

Comment 13

[Georg Fritzsche [:gfritzsche]]

Hm, i don't have an immediate good guess, but if it started failing with this patch this must have broken the data collection code path somehow.
Is ExperimentsProvider.recordLastActiveExperiment() still getting called as expected? Any errors there?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8444746 [details] [diff] [review]
experiment-branch

Review of attachment 8444746 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good overall, comments inline.

::: browser/experiments/Experiments.jsm
@@ +637,5 @@
> +  /**
> +   * Set the experiment branch for the specified experiment ID.
> +   * @returns Promise<>
> +   */
> +  setExperimentBranch: Task.async(function*(id, s) {

|s| should be renamed to something more descriptive.

@@ +1458,3 @@
>      for (let key of this.SERIALIZE_KEYS) {
>        if (!(key in data) && !this.DATE_KEYS.has(key)) {
>          this._log.error("initFromCacheData() - missing required key " + key);

As is implemented we won't be able to restore from a cache file with "_branch" data.
Either make us compatible or bump the CACHE_VERSION?

@@ +2072,5 @@
> +  version: 2,
> +
> +  fields: {
> +    lastActive: FIELD_DAILY_LAST_TEXT,
> +    lastActiveBranch: FIELD_DAILY_LAST_TEXT,

I can't find fitting examples in our tree and haven't really looked at the implementation of this, so... does it actually work to have two subsequent properties of type FIELD_DAILY_LAST_TEXT?

@@ +2150,5 @@
>          yield m.setDailyLastText("lastActive", todayActive.id,
>                                   this._experiments._policy.now());
> +        let branch = todayActive.branch;
> +        if (branch) {
> +          yield m.setDailyLastText("lastActiveBranch", b,

|b| should be |branch|?

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> @@ +2150,5 @@
> >          yield m.setDailyLastText("lastActive", todayActive.id,
> >                                   this._experiments._policy.now());
> > +        let branch = todayActive.branch;
> > +        if (branch) {
> > +          yield m.setDailyLastText("lastActiveBranch", b,
> 
> |b| should be |branch|?

I guess that's the issue you ran into here.

Comment 16

[Benjamin Smedberg]

Attachment: experiment-branch, rev. 1

It was actually a hardcoded version `1` in the test which tripped me up. This should be ready for review; I'm doing some additional manual testing now.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8445289 [details] [diff] [review]
experiment-branch, rev. 1

Review of attachment 8445289 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/experiments/Experiments.jsm
@@ +637,5 @@
> +  /**
> +   * Set the experiment branch for the specified experiment ID.
> +   * @returns Promise<>
> +   */
> +  setExperimentBranch: Task.async(function*(id, str) {

Nitpicking, but i'd still like a more descriptive identifier for the str parameter, e.g. branch or branchName?

@@ +2073,5 @@
> +  version: 2,
> +
> +  fields: {
> +    lastActive: FIELD_DAILY_LAST_TEXT,
> +    lastActiveBranch: FIELD_DAILY_LAST_TEXT,

I'm not sure on the double LAST_TEXT usage, but deferring this to your experience with FHR.

Comment 18

[Benjamin Smedberg]

I don't have significant FHR experience, but I'm not sure why this would be an issue: they are just separate fields both with a text type.

Comment 19

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/ea786f80e0d0

I prepared the following test experiment to see whether this worked and also be a little code sample for using the API: See http://hg.mozilla.org/users/bsmedberg_mozilla.com/telemetry-experiment-server/rev/branch-testing (the branch-testing branch). My own manual QA shows that the branch is recorded correctly and saved across restarts.

The branch shows up correctly in telemetry.

I don't see it in FHR, but I'm still investigating that. It may be an ordering issue.

Comment 20

[Benjamin Smedberg]

I figured out the FHR problem and will push a fix shortly (needed to add the branch data to the result of .getExperiments()).

Comment 21

[Benjamin Smedberg]

My intention is to try and uplift this into tomorrow's beta, by the way.

Comment 22

[Benjamin Smedberg]

Followup: https://hg.mozilla.org/integration/fx-team/rev/6929caed9dc4

Comment 23

[Benjamin Smedberg]

Comment on attachment 8445289 [details] [diff] [review]
experiment-branch, rev. 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Requirement for search experiment
Testing completed (on m-c, etc.): automated tests and my own manual testing
Risk to taking this patch (and alternatives if risky): risk is entirely to the experiment system, and I don't think this is especially risky.
String or IDL/UUID changes made by this patch: None

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea786f80e0d0
https://hg.mozilla.org/mozilla-central/rev/6929caed9dc4

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79deb31afe32
https://hg.mozilla.org/releases/mozilla-beta/rev/66f6820d8c0a

Comment 26

[Marco Mucci [:MarcoM]]

Hi Kamil, the desktop iteration ends on Monday.  Is it possible to have this bug verified by then.

Comment 27

[Kamil Jozwiak [:kjozwiak]]

Will be going through verification using the following test cases:

* went into about:healthreport -> Raw Data and checked and ensured that "lastActiveBranch" was being set
* enabled logging and observed the following in the browser console when installing the branch experiment:
** Setting experiment branch to a
** Setting experiment branch to c
* restarted the browser several times and ensured tha the correct branch is still being listed under about:healthreport -> Raw Data
* restarted the computer a few times and ensured that the correct branch is still being listed under about:healthreport -> Raw Data
* ensured that the experiment isn't being added into extensions.bootstrappedAddons
* branching was also tested in bug # 1029818 (there was 8 different branches and I ensured that the I landed in each group, double checked using about:healthreport -> Raw Data
* ensured that once uninstalled/disabked/expired, it appears disabled under about:addons and about:support
* ensured that once uninstalled/disabled/expired, all the preferences are set to the correct values

Comment 28

[Kamil Jozwiak [:kjozwiak]]

Also went through the following test cases:

- checking about:healthreport -> Raw Data and ensuring that "lastActiveBranch" is set correctly
- checking the telemetry payload and ensuring that "activeExperimentBranch" is set correctly
- ensured that once an experiment is removed/expired, "activeExperimentBranch" doesn't appear in the telemetry payload

Went through verification using the following builds:

* firefox-33.0a1.en-US.win32 [BuildID: 20140703030200]
** http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-03-03-02-00-mozilla-central/

* firefox-32.0a2.en-US.win32 [BuildID: 20140703004002]
** http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-03-00-40-02-mozilla-aurora/

* Firefox-31.0b6.en-US.win32 [BuildID: 20140630185627]
** http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0b6/win32/en-US/