Closed Bug 1017806 Opened 5 years ago Closed 5 years ago

Telemetry Experiments: builtin support for multiple branches of an experiment

Categories

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

x86
All
defect
Not set

Tracking

(firefox31 verified, firefox32 verified, firefox33 verified)

VERIFIED FIXED
Firefox 33
Tracking Status
firefox31 --- verified
firefox32 --- verified
firefox33 --- verified

People

(Reporter: benjamin, Assigned: benjamin, Mentored)

References

Details

(Whiteboard: [lang=JS])

Attachments

(1 file, 1 obsolete file)

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.
Flags: firefox-backlog+
Flags: needinfo?(glind)
Flags: needinfo?(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?
Flags: needinfo?(bwinton)
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(..);
}
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
Felipe yes, although probably there would be an extra .getExperimentBranch call in there in case this is an upgrade of an existing experiment.
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.
(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"…  ;)
> 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.
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!
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Blocks: 1018200
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 }
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.
No longer blocks: 1015525
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!
Assignee: mdeboer → nobody
Status: ASSIGNED → NEW
Mentor: benjamin
Whiteboard: [p=5][mentor=benjamin@smedbergs.us][lang=JS][qa+] → [p=5][lang=JS][qa+]
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Points: --- → 5
Whiteboard: [p=5][lang=JS][qa+] → [lang=JS] [qa+]
Attached patch experiment-branch (obsolete) — Splinter Review
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?
Attachment #8444746 - Flags: feedback?(gps)
Attachment #8444746 - Flags: feedback?(georg.fritzsche)
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 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|?
Attachment #8444746 - Flags: feedback?(georg.fritzsche) → feedback+
(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.
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.
Attachment #8444746 - Attachment is obsolete: true
Attachment #8444746 - Flags: feedback?(gps)
Attachment #8445289 - Flags: review?(georg.fritzsche)
Flags: needinfo?(glind)
Iteration: --- → 33.2
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.
Attachment #8445289 - Flags: review?(georg.fritzsche) → review+
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.
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.
QA Contact: kamiljoz
I figured out the FHR problem and will push a fix shortly (needed to add the branch data to the result of .getExperiments()).
My intention is to try and uplift this into tomorrow's beta, by the way.
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
Attachment #8445289 - Flags: approval-mozilla-beta?
Attachment #8445289 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ea786f80e0d0
https://hg.mozilla.org/mozilla-central/rev/6929caed9dc4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Attachment #8445289 - Flags: approval-mozilla-beta?
Attachment #8445289 - Flags: approval-mozilla-beta+
Attachment #8445289 - Flags: approval-mozilla-aurora?
Attachment #8445289 - Flags: approval-mozilla-aurora+
Blocks: 1029271
Blocks: 1029270
Blocks: 1029269
Blocks: 1029267
Blocks: 1029266
Depends on: 1031531
Hi Kamil, the desktop iteration ends on Monday.  Is it possible to have this bug verified by then.
Flags: needinfo?(kamiljoz)
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
Flags: needinfo?(kamiljoz)
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/
Status: RESOLVED → VERIFIED
Whiteboard: [lang=JS] [qa+] → [lang=JS] [qa!]
QA Whiteboard: [qa!]
Whiteboard: [lang=JS] [qa!] → [lang=JS]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.