Telemetry Experiments: builtin support for multiple branches of an experiment

VERIFIED FIXED in Firefox 31

Status

Firefox Health Report
Client: Desktop
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg, Mentored)

Tracking

unspecified
Firefox 33
x86
All
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox31 verified, firefox32 verified, firefox33 verified)

Details

(Whiteboard: [lang=JS])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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+
(Assignee)

Updated

3 years ago
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(..);
}
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
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

3 years ago
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"…  ;)
(Assignee)

Comment 7

3 years ago
> 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
(Assignee)

Updated

3 years ago
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 }
(Assignee)

Comment 10

3 years ago
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.
Blocks: 1015525
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@smedbergs.us
Whiteboard: [p=5][mentor=benjamin@smedbergs.us][lang=JS][qa+] → [p=5][lang=JS][qa+]
(Assignee)

Updated

3 years ago
Assignee: nobody → benjamin
Status: NEW → ASSIGNED

Updated

3 years ago
Points: --- → 5
Whiteboard: [p=5][lang=JS][qa+] → [lang=JS] [qa+]
(Assignee)

Comment 12

3 years ago
Created attachment 8444746 [details] [diff] [review]
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?
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.
(Assignee)

Comment 16

3 years ago
Created attachment 8445289 [details] [diff] [review]
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.
Attachment #8444746 - Attachment is obsolete: true
Attachment #8444746 - Flags: feedback?(gps)
Attachment #8445289 - Flags: review?(georg.fritzsche)
(Assignee)

Updated

3 years ago
Flags: needinfo?(glind)

Updated

3 years ago
Iteration: --- → 33.2
Blocks: 1029746
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+
(Assignee)

Comment 18

3 years ago
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.
(Assignee)

Comment 19

3 years ago
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
(Assignee)

Comment 20

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

Comment 21

3 years ago
My intention is to try and uplift this into tomorrow's beta, by the way.
(Assignee)

Comment 22

3 years ago
Followup: https://hg.mozilla.org/integration/fx-team/rev/6929caed9dc4
(Assignee)

Comment 23

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox33: --- → fixed
Attachment #8445289 - Flags: approval-mozilla-beta?
Attachment #8445289 - Flags: approval-mozilla-beta+
Attachment #8445289 - Flags: approval-mozilla-aurora?
Attachment #8445289 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

3 years ago
Blocks: 1029271
(Assignee)

Updated

3 years ago
Blocks: 1029270
(Assignee)

Updated

3 years ago
Blocks: 1029269
(Assignee)

Updated

3 years ago
Blocks: 1029267
(Assignee)

Updated

3 years ago
Blocks: 1029266
https://hg.mozilla.org/releases/mozilla-aurora/rev/79deb31afe32
https://hg.mozilla.org/releases/mozilla-beta/rev/66f6820d8c0a
status-firefox31: affected → fixed
status-firefox32: affected → fixed

Updated

3 years ago
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
status-firefox31: fixed → verified
status-firefox32: fixed → verified
status-firefox33: fixed → verified
Whiteboard: [lang=JS] [qa+] → [lang=JS] [qa!]

Updated

3 years ago
QA Whiteboard: [qa!]
Whiteboard: [lang=JS] [qa!] → [lang=JS]
You need to log in before you can comment on or make changes to this bug.