1193089 - [e10s][meta] Telemetry e10s experiment

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Vladan Djeric (:vladan)]

Basic idea: To understand how e10s affects performance with a minimum of e10s opt-in bias, force a subset of profiles from a pre-release channel into e10s and non-e10s configurations and compare Telemetry perf measures

Comment 1

[Vladan Djeric (:vladan)]

(In reply to :Felipe Gomes from comment #12)
> (In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #10)
> > (In reply to Jim Mathies [:jimm] from comment #9)
> > > If you want to a/b on aurora with random samples, now's is your chance with
> > > 42 merging! Just target 42 once it's on that channel.
> > 
> > Indeed. So I want to do something like "if clientID % 2 == 0, force e10s
> > pref on, otherwise force it off". Which e10s prefs do I need to flip?
> 
> Yeah a fresh bug for this would be nice. To do this you'll need to generate
> this clientID somehow (or is there already something that you can use?) and
> do this check in nsAppRunner.cpp mozilla::BrowserTabsRemoteAutostart().
> There you can muck with the value of the trialPref boolean to do this A/B
> testing.
> 
> I'd keep the prefs as they are now (i.e., keep
> browser.tabs.remote.autostart.2 = true), and when it's true, perform the A/B
> filtering.
> 
> It'd be nice to also add a new status entry that says disabledByAB in order
> to properly see that on telemetry.

Comment 2

[Georg Fritzsche [:gfritzsche]]

See bug 1191912 for an implementation of sampling on client id.
We could make this more re-usable for other use-cases.

If you have to do it in C++, see:
https://dxr.mozilla.org/mozilla-central/rev/d6ea652c579992daa9041cc9718bb7c6abefbc91/xpcom/threads/BackgroundHangMonitor.cpp#485

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

0. I have used our existing experiment infrastructure instead of following the suggestion in Comment 1. If the experiment infrastructure is missing something crucial for this particular experiment, then I would lean towards improving the current infrastructure rather than making this experiment a special case.
1. The experiment has effect only after a restart. Forcibly restarting Firefox is not an option so there isn't much we can do about that. That said, it’s ok to wait for the next restart as it’s easy to filter out the first submission of the experiment from the analysis.
2. The experiment is going to have a bias towards users with shorter lived session, due to 1. By letting the experiment run for a longer time period we would eventually get most of the population though.
3. The experiment does not restore the preference at each startup. I am interested to see how many users change back their preference. Maybe we could learn something about that sub-population (e.g. add-ons in common?).
4. The experiment doesn’t restore the preference to its original value once the experiment is concluded. Since the shutdown function is invoked when the application quits as well, we can’t revert the pref to its original value or its new value is not going to be picked up at startup. What would be the best way to deal with this?

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8667972 [details] [diff] [review]
1193089.patch

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

I haven't written any live experiments before, but this looks good from the Experiments implementation side.

One concern: i don't see any resetting of the pref after the experiment.
Can we .clearUserPref() it or something? I'm not really sure about the semantics of the e10s prefs though - felipe or mconley hopefully know better.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Can we assume that the target population all have the default remoteness pref set? If so, then clearUserPref is probably fine.

If not, we should probably make sure we put the user back to the setting that they were at before the experiment began.

Comment 6

[Georg Fritzsche [:gfritzsche]]

From previous experiments like bug 1144367 or bug 1127076 i see cleanup happening in uninstall(), but i'm sure felipe can expand on this.

https://developer.mozilla.org/en-US/Add-ons/Bootstrapped_extensions#uninstall

Comment 7

[Benjamin Smedberg]

My recommendation is that we don't ever set a user pref. Assuming that the experiment starts early enough, set the e10s pref on each startup in the default pref branch so that it persists for the life of the experiment but no farther.

Comment 8

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> My recommendation is that we don't ever set a user pref. Assuming that the
> experiment starts early enough, set the e10s pref on each startup in the
> default pref branch so that it persists for the life of the experiment but
> no farther.

That's what I tried initially. It seems though that if I set the "browser.tabs.remote.autostart.2" to false at startup in the default branch, the browser doesn't pick it up. Services.appinfo.browserTabsRemoteAutostart is still set to "true", as is the e10s checkbox in Preferences. That's the same behaviour I see if e10s is enabled and I manually flip "browser.tabs.remote.autostart.2"  to false. But in that case, the flag has an effect at the next startup.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #8)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> > My recommendation is that we don't ever set a user pref. Assuming that the
> > experiment starts early enough, set the e10s pref on each startup in the
> > default pref branch so that it persists for the life of the experiment but
> > no farther.
> 
> That's what I tried initially. It seems though that if I set the
> "browser.tabs.remote.autostart.2" to false at startup in the default branch,
> the browser doesn't pick it up. Services.appinfo.browserTabsRemoteAutostart
> is still set to "true", as is the e10s checkbox in Preferences. That's the
> same behaviour I see if e10s is enabled and I manually flip
> "browser.tabs.remote.autostart.2"  to false. But in that case, the flag has
> an effect at the next startup.

It's possible that by the time the experiment add-on attempts to overwrite the preference, Services.appinfo.browserTabsRemoteAutostart has already memo-ized the value, and it'll only take effect on the next start-up.

I guess this might be a deficiency of the experiment infrastructure - by the time the experiment runs, the variable we want to change may have already come into effect.

Is my hypothesis correct? If you set the browser.tabs.remote.autostart.2 to false in the experiment add-on and do a browser restart, is e10s then disabled?

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Yeah Services.appinfo.browserTabsRemoteAutostart is read very early on startup, by the gfx code. I don't think there's any chance for JS code to set it before it is read, so it will be hard to use the defaultPrefBranch as we used in some other experiments.
But using the uninstall() function in bootstrap.js should be fine for this, it works reliably when disabling the experiment, either manually or because it has expired. We just need to be careful not to reset the pref value for users who were already testing e10s before the experiment. (Granted, this will be a small number of users for beta, but it's always good to not lose any testers)

Comment 11

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8669691 [details] [diff] [review]
1193089.patch

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

::: experiments/e10s-aurora/code/bootstrap.js
@@ +3,5 @@
> +Cu.import("resource:///modules/experiments/Experiments.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +
> +const kSELF_ID = "[e-mail]";

I see that you named this here as "aurora", but it currently no longer makes sense to run this experiment on Aurora, as e10s is already enabled by default there. Unless we want to invert the experiment and disable half of the users, but I don't think we do. So I'm assuming that this experiment is intended to run for Beta.

@@ +19,5 @@
> +    this.branch.addObserver("", this, false);
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    // End experiment and respect user choice

Good idea to do this... I think it could work a bit differently so we can get better data out of the experiment

This prefs don't take any effect before restarting, so you don't need the pref observer. What you could do is to do this check on startup, and match the pref value + the actual value from Services.appinfo.browserTabsRemoteAutostart to the experiment branch, and possibly update the branch:

i.e., if in test branch, but:
  - prefs have been changed to false:
    - change branch to be "user-disabled"
  - prefs are still true but Services.appinfo.browserTabsRemoteAutostart is false:
    - change branch to be "force-disabled"

@@ +42,5 @@
> +  Task.spawn(function*() {
> +    let branch = yield Experiments.instance().getExperimentBranch(kSELF_ID);
> +
> +    // Respect user choice
> +    if (Preferences.isSet(OPTIN_PREF) || Preferences.isSet(PREF)) {

you could set this as branch here saying "already-opted-in"  (if branch was null)

@@ +48,5 @@
> +    }
> +
> +    switch (branch) {
> +      case null:
> +        if (Math.random() >= 0.5) {

I would overshoot this to 0.6 or perhaps 0.65 because some users will be force disabled due to the lack of hardware-accel or because of accessibility tools.

Another option would be to have this in the experiment parameters instead of manually here, and only deliver it to 60% of users in the target audience. With that you wouldn't need a control branch, as the control would be the users who didn't get it. This just depends on how you prefer to do the data analysis later, and both methods are fine with me.

Comment 13

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to :Felipe Gomes from comment #12)

> I see that you named this here as "aurora", but it currently no longer makes
> sense to run this experiment on Aurora, as e10s is already enabled by
> default there. Unless we want to invert the experiment and disable half of
> the users, but I don't think we do. So I'm assuming that this experiment is
> intended to run for Beta.

The experiment has to work for both Aurora and Beta. We need to run this first on Aurora to have an idea of how skewed are our earlier, non experimental based analyses were.


> I would overshoot this to 0.6 or perhaps 0.65 because some users will be
> force disabled due to the lack of hardware-accel or because of accessibility
> tools.

I would like to keep it the way it is and perform corrections in the analysis part so I can get a quantitive feeling for how common this issue is.

Comment 14

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8670490 [details] [diff] [review]
1193089.patch

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

Sorry for the delay, it was very tricky to make sure the code would do the right thing for all prefs combinations in aurora and beta.

Some comments:

* if you're gonna run the same .xpi in aurora/beta, you need to add it to the channel in manifest.json.

* if that's the case, it would be nice to do some renaming in the folder "e10s-aurora" and in the experiment id to match reality

* I haven't verified if the minVersion/maxVersion in install.rdf are the correct ones that you want.

* Take a look at the search-twhk3536 experiment code to see how we logged a lot of the actions to TelemetryLog. There's no need to go into as much detail as I went there, but it'd be great to have it here in the moments when the branch is being set, specially in the case where it's being changed because the expected value doesn't match reality.

* The name "user-disabled" is a bit misleading because it's not e10s that is disabled, but the user's participation in the experiment. (e.g., it's disabled because OPTIN_PREF is set). Just something to keep in mind


I would like to take a quick look at the final patch, but it will be quick.

::: experiments/e10s-aurora/code/bootstrap.js
@@ +32,5 @@
> +        }
> +
> +        if (Math.random() >= 0.5) {
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "control");
> +          Preferences.set(PREF, false);

we'll need to set "browser.displayedE10SPrompt.1" here to 5 to make sure users don't see a popup offer to try e10s on the next restart.  If you tested this against Aurora using the checkbox in about:preferences (instead of changing in about:config), you might not have seen this.

@@ +34,5 @@
> +        if (Math.random() >= 0.5) {
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "control");
> +          Preferences.set(PREF, false);
> +        } else {
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "experiment");

and here, set "browser.displayedE10SNotice" to 4 if you don't want the infobar to say "You're now helping to test e10s".  (maybe you do want?)

@@ +57,5 @@
> +        throw new Error("Unexpected experiment branch: " + branch);
> +    }
> +
> +    if (expected != Preferences.get(PREF)) {
> +      yield Experiments.instance().setExperimentBranch(kSELF_ID, "user-disabled");

TelemetryLog.log something like "setting branch to user-disabled because expected != PREF", and likewise for the cases

@@ +76,5 @@
> +
> +function uninstall() {
> +  Task.spawn(function*() {
> +    let branch = yield Experiments.instance().getExperimentBranch(kSELF_ID);
> +    if (branch == "control" || branch == "experiment") {

if branch == "experiment", please clear the "browser.displayedE10SNotice" pref to make sure we can show the infobar at a later time without messing the existing code too much.

Don't clear the "browser.displayedE10SPrompt.1" because it would

::: experiments/e10s-aurora/code/install.rdf
@@ +17,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>E10s A/B Test</em:name>
> +    <em:description>Measuring the effect of e10s</em:description>

These strings (name and description) will appear visible to users in the Experiments tab in about:addons, so let's use a more user-friendly version. E10s is an internal name. When building the string for the preferences checkbox we agreed on "Multi-process Firefox", so let's go with it too here.

It's possible to have localized strings in install.rdf, and it's usually better to do so, specially for something going out to the Beta population. But in reality there's not a good process or enough time to get these localized for experiments. I just wanted to raise the issue and to let you think if it's worth limiting the experiment to "en" only. (my opinion: not necessary)

::: experiments/e10s-aurora/manifest.json
@@ +6,5 @@
> +  "info"        : "<p><a href=\"https://bugzilla.mozilla.org/show_bug.cgi?id=1193089\">Related bug</a></p>",
> +  "manifest"    : {
> +    "id"               : "[e-mail]",
> +    "startTime"        : 1442707200,
> +    "endTime"          : 1446336000,

This is Oct 31. Is this enough time to get the data you want?  Also, IIRC you wanted to run aurora for a shorter period of time than beta? If that's the case then it would be good to ship this as two separate experiments, one for aurora and one for beta.

@@ +7,5 @@
> +  "manifest"    : {
> +    "id"               : "[e-mail]",
> +    "startTime"        : 1442707200,
> +    "endTime"          : 1446336000,
> +    "maxActiveSeconds" : 604800,

I'd go for at least 10 days, as not every user use their computer every day. 7 days will only give you 3 or 4 days worth of good data from each user.  If endTime is reached the experiment stops anyway.

Comment 16

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to :Felipe Gomes from comment #15)

> * if you're gonna run the same .xpi in aurora/beta, you need to add it to
> the channel in manifest.json.

I am planning to use different xpis.

> It's possible to have localized strings in install.rdf, and it's usually
> better to do so, specially for something going out to the Beta population.
> But in reality there's not a good process or enough time to get these
> localized for experiments. I just wanted to raise the issue and to let you
> think if it's worth limiting the experiment to "en" only. (my opinion: not
> necessary)

I am inclined to limit the experiment to "en" only.

Comment 17

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089

Comment 18

[Vladan Djeric (:vladan)]

Comment on attachment 8674235 [details] [diff] [review]
1193089

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

Do you know if experiment values & branches are reported to crash-stats?

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/TelemetryLog.jsm");
> +
> +const kSELF_ID = "[e-mail]";

why k-prefix only for this const?

@@ +8,5 @@
> +
> +const kSELF_ID = "[e-mail]";
> +const BRANCH = "browser.tabs.remote.";
> +const PREF = BRANCH + "autostart.2";
> +const OPTIN_PREF = BRANCH + "autostart";

b.t.r.autostart set to non-default means the user opted into of e10s at some point long ago? and b.t.r.autostart.2 means user opted out of e10s-enabled-by-default?

@@ +28,5 @@
> +    let expected = null;
> +
> +    switch (branch) {
> +      case null:
> +        if (Preferences.isSet(OPTIN_PREF) || Preferences.isSet(PREF)) {

are these prefs affected by the defaultness of e10s getting toggled before uplift to beta?

@@ +37,5 @@
> +
> +        if (Math.random() >= 0.5) {
> +          TelemetryLog.log(kSELF_ID, ["Setting branch to 'control' (disable e10s)."]);
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "control");
> +          Preferences.set(PREF, false);

same question about interactions with uplifts

@@ +38,5 @@
> +        if (Math.random() >= 0.5) {
> +          TelemetryLog.log(kSELF_ID, ["Setting branch to 'control' (disable e10s)."]);
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "control");
> +          Preferences.set(PREF, false);
> +          backupAndSetPref(POPUP_PREF, 5);

comment on the magic values or define them as consts

@@ +69,5 @@
> +      TelemetryLog.log(kSELF_ID, ["Setting branch to 'user-disabled' because PREF != expected."]);
> +      yield Experiments.instance().setExperimentBranch(kSELF_ID, "user-disabled");
> +      restorePref(POPUP_PREF);
> +      restorePref(DISPLAY_PREF);
> +    } else if(Preferences.isSet(OPTIN_PREF)) {

Can OPTIN_PREF still get set via UI?

@@ +83,5 @@
> +      restorePref(POPUP_PREF);
> +      restorePref(DISPLAY_PREF);
> +    }
> +  }).then(
> +    function() {

nit: you can use arrow functions

@@ +94,5 @@
> +function backupAndSetPref(pref, newValue) {
> +  let value = Preferences.get(pref);
> +  Preferences.set(pref, newValue);
> +  if (value !== undefined) {
> +    Preferences.set("experiments." + pref + ".backup", value);

is there a risk of collision with other experiments?

Comment 19

[Vladan Djeric (:vladan)]

Comment on attachment 8674235 [details] [diff] [review]
1193089

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

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +34,5 @@
> +          yield Experiments.instance().setExperimentBranch(kSELF_ID, "user-disabled");
> +          return;
> +        }
> +
> +        if (Math.random() >= 0.5) {

you'll have to randomize differently on Beta channel, because BHR is only enabled on a % of Beta profiles

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Answering some of Vladan's questions:


> > +const kSELF_ID = "[e-mail]";
> > +const BRANCH = "browser.tabs.remote.";
> > +const PREF = BRANCH + "autostart.2";
> > +const OPTIN_PREF = BRANCH + "autostart";
> 
> b.t.r.autostart set to non-default means the user opted into of e10s at some
> point long ago? and b.t.r.autostart.2 means user opted out of
> e10s-enabled-by-default?

correct and correct.  b.t.r.autostart might also be true when a user re-opts in after having opted out.

> @@ +28,5 @@
> > +    let expected = null;
> > +
> > +    switch (branch) {
> > +      case null:
> > +        if (Preferences.isSet(OPTIN_PREF) || Preferences.isSet(PREF)) {
> 
> are these prefs affected by the defaultness of e10s getting toggled before
> uplift to beta?

Yeah. Since during the last week of Aurora we usually disable e10s to get better non-10s coverage for beta, I'm hoping we don't run this experiment during that period to not cause problems for both the experiment and the beta coverage.

> 
> @@ +69,5 @@
> > +      TelemetryLog.log(kSELF_ID, ["Setting branch to 'user-disabled' because PREF != expected."]);
> > +      yield Experiments.instance().setExperimentBranch(kSELF_ID, "user-disabled");
> > +      restorePref(POPUP_PREF);
> > +      restorePref(DISPLAY_PREF);
> > +    } else if(Preferences.isSet(OPTIN_PREF)) {
> 
> Can OPTIN_PREF still get set via UI?

Yeah, if a user re-opts in after having opted out.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

I think we do have a good reason to do this with funnelcake, though -- because it would really be a/b testing rather than looking at the subset of people who have opted in to e10s, who may be self selected for being more willing to tolerate issues, which is what we would be getting from telemetry data.

Comment 22

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089

I added some logic to detect and deal properly with a defaultness change.

Comment 23

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089

Comment 24

[Vladan Djeric (:vladan)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> I think we do have a good reason to do this with funnelcake, though --
> because it would really be a/b testing rather than looking at the subset of
> people who have opted in to e10s, who may be self selected for being more
> willing to tolerate issues, which is what we would be getting from telemetry
> data.

The users won't be self-selecting, the patch above is for a TelemetryExperiment which will select a small number of profiles and then force half into an e10s or non-e10s configuration. We will then use their Telemetry reports to check for differences in performance metrics. Since this will happen on a pre-release channel, all users will have Telemetry enabled by default.

Comment 25

[Vladan Djeric (:vladan)]

(In reply to :Felipe Gomes from comment #20)
> Yeah. Since during the last week of Aurora we usually disable e10s to get
> better non-10s coverage for beta, I'm hoping we don't run this experiment
> during that period to not cause problems for both the experiment and the
> beta coverage.

We want to deploy this experiment as soon as possible on Aurora, this week even. The next uplift is in less than two weeks, so we'll have to deal with the default value getting toggled during the experiment time period.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #25)
> We want to deploy this experiment as soon as possible on Aurora, this week
> even. The next uplift is in less than two weeks, so we'll have to deal with
> the default value getting toggled during the experiment time period.

Ok. The new code up for review forces the prefs to the experiment values, even after a defaultness change. In that case we can't run 100% sampling on Aurora because that would be the same as not being able to toggle the default value off, and I think release mgmt really needs this one week of stabiliziation before the uplift.

Comment 27

[Vladan Djeric (:vladan)]

(In reply to :Felipe Gomes from comment #26)
> In that case we can't run 100% sampling on Aurora because that would be the
> same as not being able to toggle the> default value off, and I think release
> mgmt really needs this one week of stabiliziation before the uplift.

That's fine, we just need a few thousand profiles for this.

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8676309 [details] [diff] [review]
1193089

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

::: experiments/e10s-enabled-aurora/manifest.json
@@ +10,5 @@
> +    "endTime"          : 1446336000,
> +    "maxActiveSeconds" : 864000,
> +    "appName"          : ["Firefox"],
> +    "channel"          : ["aurora"],
> +    "sample"           : 1.0

Just re-stating here what I talked with Roberto on IRC:  If release management asks for a week of stability control prior to the uplift, I don't want to say "uh oh we can't do that because we're running an experiment that blocks it from happening". So please be mindful of the sampling value, and make sure that relmgmt is aware of this.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

I think we should go ahead with this experiment, and not worry about turning e10s off at the end of aurora. We did that for the 42 cycle because it is our "special release". 

Turning e10s off for the last week of the cycle might uncover problems we could catch before the beta release, but it doesn't give us a lot of certainty how beta 1's stability will be, since we're changing many other factors (pref flips and so on) between the last aurora update and beta 1.  For 43, let's leave e10s.  I'm thinking of that as giving us that much more time to improve stability with e10s enabled. 

Roberto can you request uplift to aurora? Thanks.

Comment 30

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #29)
> Roberto can you request uplift to aurora? Thanks.

Liz, maybe I misunderstood what you are asking me but since this is a Telemetry experiment it doesn't require an uplift to land, see http://hg.mozilla.org/webtools/telemetry-experiment-server/.

Comment 31

[Roberto Agostino Vitillo (:rvitillo)]

Err, https://wiki.mozilla.org/Telemetry/Experiments.

Comment 32

[Vladan Djeric (:vladan)]

Comment on attachment 8676309 [details] [diff] [review]
1193089

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

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +11,5 @@
> +const PREF = BRANCH + "autostart.2";
> +const OPTIN_PREF = BRANCH + "autostart";
> +const POPUP_PREF = "browser.displayedE10SPrompt.1";
> +const DISPLAY_PREF = "browser.displayedE10SNotice";
> +const ORIGINAL_PREF = "experiments." + PREF + ".backup." + SELF_ID;

ORIGINAL_PREF is the A/B group the user was assigned to? isn't that already reflected in experiment branch?

@@ +62,5 @@
> +      case "defaultness-changed":
> +        TelemetryLog.log(SELF_ID, ["Defaultness of PREF changed in the previous session: restoring branch."]);
> +        expected = !Preferences.get(ORIGINAL_PREF);
> +        if (expected) {
> +          yield Experiments.instance().setExperimentBranch(SELF_ID, "experiment");

so if random > 0.5 then the profile is assigned to the control branch and PREF = false and ORIGINAL_PREF = false

but if defaultness-changed, then ORIGINAL_PREF = false but expected = true and the user gets assigned to the experiment branch?

@@ +76,5 @@
> +      default:
> +        throw new Error("Unexpected experiment branch: " + branch);
> +    }
> +
> +    if (!Preferences.isSet(PREF) && (Preferences.get(PREF) != Preferences.get(ORIGINAL_PREF))) {  // defaultness of PREF has changed due to an imminent uplift

I would feel more comfortable if you checked for uplift window by explicitly checking the default value of PREF

defaultPrefs = Services.prefs.getDefaultBranch(null);
let defaultValue = defaultPrefs.get(PREF);

also, couldn't you just use 'expected' instead of ORIGINAL_PREF?

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Ah, I see. So you will land this, then QE will test it on staging, then we deploy it. Sounds good to me.

Comment 34

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #32)
> Comment on attachment 8676309 [details] [diff] [review]
> 1193089
> 
> Review of attachment 8676309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: experiments/e10s-enabled-aurora/code/bootstrap.js
> @@ +11,5 @@
> > +const PREF = BRANCH + "autostart.2";
> > +const OPTIN_PREF = BRANCH + "autostart";
> > +const POPUP_PREF = "browser.displayedE10SPrompt.1";
> > +const DISPLAY_PREF = "browser.displayedE10SNotice";
> > +const ORIGINAL_PREF = "experiments." + PREF + ".backup." + SELF_ID;
> 
> ORIGINAL_PREF is the A/B group the user was assigned to? isn't that already
> reflected in experiment branch?

ORIGINAL_PREF is the default value of PREF at the beginning of the experiment which may or may not reflect the experiment branch.

> @@ +62,5 @@
> > +      case "defaultness-changed":
> > +        TelemetryLog.log(SELF_ID, ["Defaultness of PREF changed in the previous session: restoring branch."]);
> > +        expected = !Preferences.get(ORIGINAL_PREF);
> > +        if (expected) {
> > +          yield Experiments.instance().setExperimentBranch(SELF_ID, "experiment");
> 
> so if random > 0.5 then the profile is assigned to the control branch and
> PREF = false and ORIGINAL_PREF = false
> 
> but if defaultness-changed, then ORIGINAL_PREF = false but expected = true
> and the user gets assigned to the experiment branch?
> 
> @@ +76,5 @@
> > +      default:
> > +        throw new Error("Unexpected experiment branch: " + branch);
> > +    }
> > +
> > +    if (!Preferences.isSet(PREF) && (Preferences.get(PREF) != Preferences.get(ORIGINAL_PREF))) {  // defaultness of PREF has changed due to an imminent uplift
> 
> I would feel more comfortable if you checked for uplift window by explicitly
> checking the default value of PREF
> 
> defaultPrefs = Services.prefs.getDefaultBranch(null);
> let defaultValue = defaultPrefs.get(PREF);

Ok.

> also, couldn't you just use 'expected' instead of ORIGINAL_PREF?

Yes, I could have.

Comment 35

[Vladan Djeric (:vladan)]

Comment on attachment 8676309 [details] [diff] [review]
1193089

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

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +47,5 @@
> +          Preferences.set(PREF, true);
> +          backupAndSetPref(DISPLAY_PREF, 4); // don't show the e10s infobar
> +        }
> +
> +        Preferences.set(ORIGINAL_PREF, Preferences.get(PREF));  // save default value at the beginning of the experiment

If ORIGINAL_PREF is the default value of PREF at the beginning of the experiment as you said, then why do you set PREF first and then back it up to ORIGINAL_PREF?

Comment 36

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

Comment 37

[Vladan Djeric (:vladan)]

Comment on attachment 8676434 [details] [diff] [review]
1193089.patch

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

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/TelemetryLog.jsm");
> +
> +const SELF_ID = "[e-mail]";

give it some time-based ID, we'll be doing a bunch of these, e.g. [e-mail]

::: experiments/e10s-enabled-aurora/manifest.json
@@ +10,5 @@
> +    "endTime"          : 1446336000,
> +    "maxActiveSeconds" : 864000,
> +    "appName"          : ["Firefox"],
> +    "channel"          : ["aurora"],
> +    "sample"           : 1.0

are we sampling at 100%?

Comment 38

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

Comment 39

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #37)
> Comment on attachment 8676434 [details] [diff] [review]
> 1193089.patch
> 
> Review of attachment 8676434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: experiments/e10s-enabled-aurora/code/bootstrap.js
> @@ +5,5 @@
> > +Cu.import("resource://gre/modules/Preferences.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/TelemetryLog.jsm");
> > +
> > +const SELF_ID = "[e-mail]";
> 
> give it some time-based ID, we'll be doing a bunch of these, e.g.
> [e-mail]

ok

> ::: experiments/e10s-enabled-aurora/manifest.json
> @@ +10,5 @@
> > +    "endTime"          : 1446336000,
> > +    "maxActiveSeconds" : 864000,
> > +    "appName"          : ["Firefox"],
> > +    "channel"          : ["aurora"],
> > +    "sample"           : 1.0
> 
> are we sampling at 100%?

Yes, we need all the data we can get on Aurora to investigate add-on performance.

Comment 40

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: 1193089.patch

Per e-mail/IRC discussions, I have limited the experiment to Aurora 43.

Comment 41

[Vladan Djeric (:vladan)]

Comment on attachment 8676821 [details] [diff] [review]
1193089.patch

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

::: experiments/e10s-enabled-aurora/code/bootstrap.js
@@ +79,5 @@
> +        throw new Error("Unexpected experiment branch: " + branch);
> +    }
> +
> +    let defaultPrefs = new Preferences({defaultBranch: true});
> +    if (defaultPrefs.get(PREF) != Preferences.get(ORIGINAL_PREF)) {  // defaultness of PREF has changed

You're checking if the default pref value has changed since the start of the experiment instead of checking if default pref value has changed since the last session. This is a bug if the experiment can see more than one defaultness change (theoretical problem?):

session 1: experiment begins, user is put into control group --> at end of session: e10s default = true, e10s pref = false (user set), branch = control
session 2: Firefox updates and e10s default changes to false --> at end of session: e10s default = false, e10s pref = false (not user set), branch = defaultness-changed-control
session 3: nothing changes --> at end of session: e10s default = false, e10s pref = false (not user set), branch = control
session 4: Firefox updates and the default pref value was changed again, this time to true --> at end of session: e10s default = true, e10s pref = ***TRUE***, branch = ***user-disabled!***

Comment 42

[Kamil Jozwiak [:kjozwiak]]

The only issue that I ran into while going through the different scenarios [1] is that the experiment is being installed even though telemetry was disabled via about:preferences#advanced & about:telemetry. This is more of a telemetry infrastructure problem rather than this specific experiment. If this is a real issue, it could have major privacy consequences. From what I remember from past experiments, if a user disables telemetry, the experiments shouldn't be installed.

I'm not sure if this is just because we're installing the experiment directly from a manifest and not through the official staging/release servers.. Felipe, could you take a look and see if this is a problem? STR below:

* install m-a
* about:preferences#advanced -> "Data Choices"
** Disable "Enable Firefox Developer Edition Health Report"
** Disable "Share additional data (i.e., Telemetry)"
** Disable "Enable Crash Reporter"
* ensure everything appears disabled via about:telemetry
* restart the browser
* go into about:config and change the following prefs
** xpinstall.signatures.required;false
** devtools.chrome.enabled;true
** experiments.logging.level;0
* Open the Browser Console
* experiments.manifest.uri;http://www.kamiljozwiak.io/firefox-manifest.json (this will trigger the experiment to be installed)

Please let me know if there's anything that I've missed while testing [2].

* [1] Test Cases Followed: https://public.etherpad-mozilla.org/p/e10s_experiment
* [2] Detailed QA Test Cases/Results: https://public.etherpad-mozilla.org/p/e10sExperimentTesting

Comment 43

[:Felipe Gomes (needinfo for replies!)]

Good catch Kamil, there is indeed a bug. I wonder how we never noticed it before. I filed bug 1217282 for that

Comment 44

[Roberto Agostino Vitillo (:rvitillo)]

Felipe suggested to use the filtering mechanism of the experiment infrastructure to not install the add-on when Telemetry and FHR are disabled. The filter function executes within a sandbox that can access only the current telemetry environment. The telemetry environment has a flag for the status of extended Telemetry but not for base Telemetry. Georg, is there a way to check the status of base Telemetry from within the telemetry environment? I am assuming here that if base Telemetry is disabled, then FHR is disabled as well.

That said, considering that the experiment is time sensitive and that when both FHR and Telemetry are disabled no data is sent, I think it's fine to deploy the experiment as is.

Comment 45

[Georg Fritzsche [:gfritzsche]]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #44)
> Felipe suggested to use the filtering mechanism of the experiment
> infrastructure to not install the add-on when Telemetry and FHR are
> disabled. The filter function executes within a sandbox that can access only
> the current telemetry environment. The telemetry environment has a flag for
> the status of extended Telemetry but not for base Telemetry. Georg, is there
> a way to check the status of base Telemetry from within the telemetry
> environment? I am assuming here that if base Telemetry is disabled, then FHR
> is disabled as well.
> 
> That said, considering that the experiment is time sensitive and that when
> both FHR and Telemetry are disabled no data is sent, I think it's fine to
> deploy the experiment as is.

We don't have access to the v2 status in the sandbox anymore after recent changes.
I think for the purposes here we should be fine to just use the JS filter and check for extended Telemetry.

Comment 46

[Vladan Djeric (:vladan)]

Jorge, the XPI is at the following URL, can you sign it and attach the signed version to this bug?

https://hg.mozilla.org/users/bsmedberg_mozilla.com/telemetry-experiment-server/file/5be3df64ec11/experiments/e10s-enabled-aurora

Comment 47

[Jorge Villalobos [:jorgev] (he/him)]

Handing over to Jason for signing.

Comment 48

[Jason Thomas [:jason]]

Attachment: experiment.xpi

Please see attached.

Comment 49

[:Felipe Gomes (needinfo for replies!)]

I implement the filter.js function from comment 44 to circumvent bug 1217282

http://hg.mozilla.org/webtools/telemetry-experiment-server/rev/2286e26b93cc

Comment 50

[Vladan Djeric (:vladan)]

This experiment was deployed (with Felipe's fix) this morning in bug 1217613

Comment 51

[Erin Lancaster [:elan]]

Are there any results to share at this point?

Comment 52

[Roberto Agostino Vitillo (:rvitillo)]

Erin, you can find a re-run of our general analysis at [1]. The perf team is going to discuss the results on Thursday and share the conclusions after that.

[1] http://nbviewer.ipython.org/github/vitillo/e10s_analyses/blob/master/aurora/e10s_experiment.ipynb