Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

45 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+)

Details

Attachments

(2 attachments, 11 obsolete attachments)

6.87 KB, patch
mossop
: review+
Details | Diff | Splinter Review
5.97 KB, application/x-xpinstall
Details
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8713705 [details] [diff] [review]
e10s-beta45-withoutaddons

Same code as last experiment, just includes the toggling of the pref to block users with add-ons from running e10s.
Attachment #8713705 - Flags: review?(vladan.bugzilla)
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
Attachment #8713705 - Flags: review?(vladan.bugzilla)
(Assignee)

Comment 3

2 years ago
(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

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

Comment 5

2 years ago
Created attachment 8717630 [details] [diff] [review]
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
Attachment #8713705 - Attachment is obsolete: true
Attachment #8717630 - Flags: review?(dtownsend)
(Assignee)

Comment 6

2 years ago
note: this is a patch for http://hg.mozilla.org/webtools/telemetry-experiment-server/
(Assignee)

Comment 7

2 years ago
Created attachment 8717634 [details]
experiment.xpi unsigned

Hey Jason, could you sign this experiment for us?
Attachment #8717634 - Flags: feedback?(jthomas)

Updated

2 years ago
Attachment #8717634 - Flags: feedback?(jthomas)
Created attachment 8717638 [details]
experiment.xpi signed

Please see attached.
(Assignee)

Comment 9

2 years ago
Deployed for testing at https://people.mozilla.org/~fgomes/e10s-beta45-withoutaddons/firefox-manifest.json
Attachment #8717630 - Flags: review?(dtownsend) → review+

Comment 10

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

Comment 11

2 years ago
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.
Flags: needinfo?(blassey.bugs)
(Assignee)

Comment 12

2 years ago
Created attachment 8718049 [details] [diff] [review]
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..
Attachment #8718049 - Flags: review?(dtownsend)
(Assignee)

Comment 13

2 years ago
Created attachment 8718057 [details]
new experiment.xpi, unsigned

Hello Jason, there was a small change in the code.. could you re-sign the xpi for us?
Attachment #8717634 - Attachment is obsolete: true
Attachment #8717638 - Attachment is obsolete: true
Attachment #8718057 - Flags: feedback?(jthomas)

Updated

2 years ago
Attachment #8718057 - Flags: feedback?(jthomas) → feedback+
Created attachment 8718067 [details]
new experiment.xpi, signed

Please see attached.
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?
Flags: needinfo?(blassey.bugs)

Comment 16

2 years ago
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%.
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.
Attachment #8718049 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 18

2 years ago
Created attachment 8718454 [details] [diff] [review]
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)
Attachment #8717630 - Attachment is obsolete: true
Attachment #8718049 - Attachment is obsolete: true
Attachment #8718057 - Attachment is obsolete: true
Attachment #8718067 - Attachment is obsolete: true
Attachment #8718454 - Flags: review?(dtownsend)
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?
(Assignee)

Comment 20

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

Comment 21

2 years ago
Created attachment 8718500 [details] [diff] [review]
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?
Attachment #8718500 - Flags: review?(dtownsend)
(Assignee)

Comment 22

2 years ago
Created attachment 8718532 [details] [diff] [review]
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.
Attachment #8718454 - Attachment is obsolete: true
Attachment #8718500 - Attachment is obsolete: true
Attachment #8718454 - Flags: review?(dtownsend)
Attachment #8718500 - Flags: review?(dtownsend)
Attachment #8718532 - Flags: review?(dtownsend)
(Assignee)

Comment 23

2 years ago
Created attachment 8718535 [details]
yet another xpi, unsigned

Hey Jason, here I am again.. Could you sign this one?
Attachment #8718535 - Flags: feedback?(jthomas)
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.
Attachment #8718532 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 25

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

(I'll update the xpi with Dave's latest comment)
Attachment #8718535 - Attachment is obsolete: true
Attachment #8718535 - Flags: feedback?(jthomas)
(Assignee)

Comment 26

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

Comment 27

2 years ago
Created attachment 8718557 [details]
final xpi for signing, r=Mossop
Attachment #8718557 - Flags: feedback?(jthomas)

Updated

2 years ago
Attachment #8718557 - Flags: feedback?(jthomas) → feedback+
Created attachment 8718566 [details]
final xpi signed

Please see attached.
Attachment #8718557 - Attachment is obsolete: true
(Assignee)

Comment 29

2 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1247822
(Assignee)

Comment 30

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

Deployed in bug 1247822

Updated

a year ago
Depends on: 1248130
(Assignee)

Updated

a year ago
Depends on: 1250338
You need to log in before you can comment on or make changes to this bug.