Closed
Bug 1244187
Opened 9 years ago
Closed 9 years ago
Run an e10s A/B experiment on Beta 45 (second experiment)
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
e10s | m8+ | --- |
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
Attachments
(2 files, 11 obsolete files)
6.87 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
5.97 KB,
application/x-xpinstall
|
Details |
+++ 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•9 years ago
|
||
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 2•9 years ago
|
||
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•9 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•9 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•9 years ago
|
||
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•9 years ago
|
||
note: this is a patch for http://hg.mozilla.org/webtools/telemetry-experiment-server/
Assignee | ||
Comment 7•9 years ago
|
||
Hey Jason, could you sign this experiment for us?
Attachment #8717634 -
Flags: feedback?(jthomas)
Updated•9 years ago
|
Attachment #8717634 -
Flags: feedback?(jthomas)
Assignee | ||
Comment 9•9 years ago
|
||
Deployed for testing at https://people.mozilla.org/~fgomes/e10s-beta45-withoutaddons/firefox-manifest.json
Updated•9 years ago
|
Attachment #8717630 -
Flags: review?(dtownsend) → review+
Comment 10•9 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•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
Attachment #8718057 -
Flags: feedback?(jthomas) → feedback+
Comment 15•9 years ago
|
||
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•9 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%.
Comment 17•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8718049 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Hey Jason, here I am again.. Could you sign this one?
Attachment #8718535 -
Flags: feedback?(jthomas)
Comment 24•9 years ago
|
||
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•9 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•9 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!
Updated•9 years ago
|
Attachment #8718557 -
Flags: feedback?(jthomas) → feedback+
Assignee | ||
Comment 29•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 30•9 years ago
|
||
Added tag release_tag https://hg.mozilla.org/webtools/telemetry-experiment-server/rev/4b322837bf47 Deployed in bug 1247822
You need to log in
before you can comment on or make changes to this bug.
Description
•