1244187 - Run an e10s A/B experiment on Beta 45 (second experiment)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

+++ This bug was initially created as a clone of Bug #1241336 +++

Same as with the previous experiment, but now this time excluding users with add-ons installed (by activating the code from bug 1234675)

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: e10s-beta45-withoutaddons

Same code as last experiment, just includes the toggling of the pref to block users with add-ons from running e10s.

Comment 2

[Vladan Djeric (:vladan)]

Comment on attachment 8713705 [details] [diff] [review]
e10s-beta45-withoutaddons

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

::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
@@ +45,5 @@
>          } else {
>            TelemetryLog.log(SELF_ID, ["Setting branch to 'first-session-experiment' (enable e10s next session)."]);
>            yield Experiments.instance().setExperimentBranch(SELF_ID, "first-session-experiment");
>            Preferences.set(PREF, true);
> +          Preferences.set(BLOCK_ADDONS_PREF, true);

We don't need Telemetry from single-process users with addons in this experiment. So I guess we'll filter out such Telemetry submissions server-side?

::: experiments/e10s-beta45-withoutaddons/manifest.json
@@ +9,2 @@
>      "startTime"        : 1453680001,
>      "endTime"          : 1455148801,

the start and end dates need to be updated

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to Vladan Djeric (:vladan) -- last day at Moz Friday Jan 29 from comment #2)
> Comment on attachment 8713705 [details] [diff] [review]
> e10s-beta45-withoutaddons
> 
> Review of attachment 8713705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
> @@ +45,5 @@
> >          } else {
> >            TelemetryLog.log(SELF_ID, ["Setting branch to 'first-session-experiment' (enable e10s next session)."]);
> >            yield Experiments.instance().setExperimentBranch(SELF_ID, "first-session-experiment");
> >            Preferences.set(PREF, true);
> > +          Preferences.set(BLOCK_ADDONS_PREF, true);
> 
> We don't need Telemetry from single-process users with addons in this
> experiment. So I guess we'll filter out such Telemetry submissions
> server-side?

Yeah. I could filter out these users from receiving the experiment, or adding code to the experiment to exclude them too. But one of the goals is to check that the in-tree code to block add-ons users works well, so I want to be able to validate on telemetry that users with add-ons got into the "force-disabled" branch.

> 
> ::: experiments/e10s-beta45-withoutaddons/manifest.json
> @@ +9,2 @@
> >      "startTime"        : 1453680001,
> >      "endTime"          : 1455148801,
> 
> the start and end dates need to be updated

yeah, I'll figure those dates later

Comment 4

[Chris H-C :chutten]

Approximately 46.2% of Beta 45 users who submitted pings on Feb 1 have no addons (excepting Hello, Pocket, and non-extension addons like "service"s): https://gist.github.com/chutten/37a869da702aad5502cb 

If we only catch 50% of these clients in the experiment, that gives us a little over 11% of the Beta population in control and experiment branches at the beginning of the experiment (compared to around 25% of the Beta population in each branch of the with-addons experiment previously)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: e10s-beta45-withoutaddons

Dave, after talking with Chutten we concluded that the experiment needs to have the following behavior:

=====

if (non-default prefs) {
  branch = "user-disabled";
} else if (user has addons installed) {
  branch = "addon-user";
} else {
  branch = Math.random() < 0.5 ? "experiment" : "control";
}

If an experiment or control user installs an add-on, we will move them to addon-user

===

This will allow us to directly compare experiment and control groups data, and also test the add-on disabling code.

The previous experiment already did everything except the addon-user part, so that's what is included here.

The startTime/endTime in the manifest represents Feb 12 - Feb 29

Comment 6

[:Felipe Gomes (needinfo for replies!)]

note: this is a patch for http://hg.mozilla.org/webtools/telemetry-experiment-server/

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: experiment.xpi unsigned

Hey Jason, could you sign this experiment for us?

Comment 8

[Jason Thomas [:jason]]

Attachment: experiment.xpi signed

Please see attached.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Deployed for testing at https://people.mozilla.org/~fgomes/e10s-beta45-withoutaddons/firefox-manifest.json

Comment 10

[Jonathan Howard]

More will have upgraded to beta 45 this time.

Measurement gets removed in 46. No easy evolution graph after that I know of.
https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=bucket-1&cumulative=1&end_date=2016-01-29&keys=!__none__!__none__&max_channel_version=beta%252F45&measure=E10S_AUTOSTART&min_channel_version=beta%252F44&processType=false&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-01-29&trim=1&use_submission_date=1

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Blassey, how much of the beta population would you like to include in this second experiment? From whatever sample we include, ~40% of the users will have no add-ons, and we will split them into two groups (test and control). So approximately 20% of the sample will get e10s running.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: new bootstrap.js diff

Dave, the QA folks mentioned an interesting edge case not handled before. While there was code to move a user part of the test group ("experiment") into "addon-user" when they install an add-on, the same wouldn't happen for the "control" group.  This patch moves the code around to do it for both, and ends up simplifying things a little bit.

We could do this filtering on data analysis, but since it's easy to do it here, might as well do it..

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: new experiment.xpi, unsigned

Hello Jason, there was a small change in the code.. could you re-sign the xpi for us?

Comment 14

[Jason Thomas [:jason]]

Attachment: new experiment.xpi, signed

Please see attached.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

if we're going to get 20% of the experiment population running e10s, I'm inclined to say we should sample 100% of the beta population. Any reason not to?

Comment 16

[Chris H-C :chutten]

Timidity/Prudence?

#1 It exposes the entire user base to experiment code. We would have to be absolutely certain that it is rock solid.

#2 It also exposes the 53.8% of the Beta user base with addons to the "block e10s when addons are installed" code. I don't know what confidence levels there are in that code. I thought that was something we were testing with this experiment.

#3 The proportion of the Beta population left alone is 20% of the experiment population (50% of the non-addon population, 0% of the addon population) plus 100% of the non-experiment population. If there is no non-experiment population, this drastically reduces the amount and changes the composition of Beta users we leave alone. This can limit long-term study of Beta, and may have an impact on churn if the experiment results in negative user experience. I know of no current long-term studies on Beta population, so maybe the former is a non-issue. The latter is covered under #1, above.

--

As I see it, #1 is a fairly low issue. The experiment code fits on a single screen and is very readable. Get a couple of extra feedback+ for due diligence and I think we can account for it.

#2 and #3 we could mitigate by changing the extension code to send only some proportion of addon-havers into the "addon-user" case (maybe 50%? That ought to keep the "left-alone" population's distribution congruent to the Beta population as a whole). Since they are the majority of any sample population of Beta users, it might be worthwhile to consider even if we don't sample at 100%.

Comment 17

[Benjamin Smedberg]

Our general rule from discussions with release management is that we want at least 50% of the release population to be running the shipping code. I think that we will have plenty of data from an e10s experiment that runs with just a 50% sample.

We also have several other experiments running or running soon, so 100% would interfere with the plugin pixel experiment and there is the ongoing search unification experiment.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Attachment: New experiment design, unsigned

So here's the new experiment design. It should work as follows:

at the first run of the experiment, simply split them into two groups:
  - control:     don't change anything
  - experiment:  toggle the e10s and addons-blocking prefs


Then, on later runs, we will track whether the user has add-ons installed or not, ending up with four groups:

  1 - control-addon-user
  2 - control-no-addons
  3 - experiment-addon-user
  4 - experiment-no-addons


That way we can do comparisons between groups 1 and 3 (to test addon-blocking code), and 2 and 4 (to get performance data on e10s)

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8718454 [details] [diff] [review]
New experiment design, unsigned

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

I'm a little nervous because we have so many controls over when e10s runs. One consequence of this:

1. User gets put into the experiment-no-addons group and has e10s turned on.
2. User uses some accessibility tool which forces e10s off
3. Experiment moves user into the force-disabled group
4. Experiment ends and uninstalls.
5. User stops using the accessibility too and now has e10s turned on.

Can we use the getE10SBlocked notification to help solve this?

::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
@@ +21,5 @@
>  function startup() {
>    // Seems startup() function is launched twice after install, we're
>    // unsure why so far. We only want it to run once.
>    if (gStarted) {
>      return;

Is there a bug filed on this?

@@ +37,4 @@
>      switch (branch) {
>        case null:
>          // Profile is ineligible if either of the two e10s prefs is non-default
>          if (Preferences.isSet(OPTIN_PREF) || Preferences.isSet(PREF)) {

Should we also check "browser.tabs.remote.force-enable"?

@@ +75,1 @@
>          expected = false;

Should we move them to the no-addons group if they've removed their add-ons or is this a terminal state?

Comment 20

[:Felipe Gomes (needinfo for replies!)]

(In reply to Dave Townsend [:mossop] from comment #19)
> Comment on attachment 8718454 [details] [diff] [review]
> New experiment design, unsigned
> 
> Review of attachment 8718454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little nervous because we have so many controls over when e10s runs.
> One consequence of this:
> 
> 1. User gets put into the experiment-no-addons group and has e10s turned on.
> 2. User uses some accessibility tool which forces e10s off
> 3. Experiment moves user into the force-disabled group
> 4. Experiment ends and uninstalls.
> 5. User stops using the accessibility too and now has e10s turned on.

That's a good point. It has been like that for the previous add-ons too, but we can fix it here. I think the simple solution would be to also reset the pref for these users when the experiment ends. Essentially skipping the reset only for the "user-disabled" group which is the only group who had custom prefs at the beginning.


> 
> Can we use the getE10SBlocked notification to help solve this?
> 
> ::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
> @@ +21,5 @@
> >  function startup() {
> >    // Seems startup() function is launched twice after install, we're
> >    // unsure why so far. We only want it to run once.
> >    if (gStarted) {
> >      return;
> 
> Is there a bug filed on this?

This has been boilerplate for a long time, maybe it has even been fixed by now. After this one is done I'll recheck if this is still the case and file a bug if it is.

> 
> @@ +37,4 @@
> >      switch (branch) {
> >        case null:
> >          // Profile is ineligible if either of the two e10s prefs is non-default
> >          if (Preferences.isSet(OPTIN_PREF) || Preferences.isSet(PREF)) {
> 
> Should we also check "browser.tabs.remote.force-enable"?

Yeah

> 
> @@ +75,1 @@
> >          expected = false;
> 
> Should we move them to the no-addons group if they've removed their add-ons
> or is this a terminal state?

I had it like this.. I removed for simplicity's sake, but I guess it's not that complicated.

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: interdiff for latest changes

With latest changes.  I'm unsure about leaving the "experiment-addon-user" as a final state or not, but I added the transition here.. Thoughts?

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: final-version.diff

Ok, I made some tweaks, included the transition to "-no-addons" and removed the part at the bottom that rechecks all the prefs, since that wasn't necessary and was actually wrong for some cases. What really matters is if `expected != Services.appinfo.browserTabsRemoteAutostart`, which I left.

I tested all of the possible transitions, and everything appears to work as expected.

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: yet another xpi, unsigned

Hey Jason, here I am again.. Could you sign this one?

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8718532 [details] [diff] [review]
final-version.diff

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

::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
@@ +104,5 @@
>        default:
>          throw new Error("Unexpected experiment branch: " + branch);
>      }
>  
> +    if (expected != Services.appinfo.browserTabsRemoteAutostart) {

Rather than manually updating expected throughout the code paths above you can just initialize it directly here as branch == "experiment-no-addons". That is the only branch we expect to be in e10s in.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8718535 [details]
yet another xpi, unsigned

(I'll update the xpi with Dave's latest comment)

Comment 26

[:Felipe Gomes (needinfo for replies!)]

(In reply to Dave Townsend [:mossop] from comment #24)
> Comment on attachment 8718532 [details] [diff] [review]
> final-version.diff
> 
> Review of attachment 8718532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: experiments/e10s-beta45-withoutaddons/code/bootstrap.js
> @@ +104,5 @@
> >        default:
> >          throw new Error("Unexpected experiment branch: " + branch);
> >      }
> >  
> > +    if (expected != Services.appinfo.browserTabsRemoteAutostart) {
> 
> Rather than manually updating expected throughout the code paths above you
> can just initialize it directly here as branch == "experiment-no-addons".
> That is the only branch we expect to be in e10s in.

Great point, it simplifies a lot. Thanks for that and for all the reviews here!

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Attachment: final xpi for signing, r=Mossop

Comment 28

[Jason Thomas [:jason]]

Attachment: final xpi signed

Please see attached.

Comment 29

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/3ac53e844122
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/80c7c3ba66d5

I'll file a bug for deployment in a moment, and wait for QA to be finished before adding the release_tag

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Added tag release_tag
https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/4b322837bf47

Deployed in bug 1247822