1289172 - Enable and test automigration on beta 50

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

Per discussion between Michael, Stephen, Justin and me, we're looking for a way to enable the auto-migration code on pre-release channels. This will involve some compromises in comparison with the 'ideal' state, but we'll get feedback quicker this way. In no particular order, I'll try to do patches that:

- show a notification bar on about:home offering the user to undo the import
- remove that notification and undo options once it's been shown on 3 separate days
- remove the notification and undo options if the user adds/moves/changes bookmarks or passwords
- does some telemetry on when those things happen
- does some telemetry on how much time the migration takes so that can inform when/how we switch to using about:newtab instead of about:home for these cases.
- disable the 'firstrun' pages on pre-release channels (after discussing that with stakeholders)

Comment 1

[:Gijs (he/him)]

I originally thought I'd just file a series of patches on this bug but then decided to use deps instead. If this is a meta bug, keeping it in my assigned-to list doesn't accomplish anything, so unassigning.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1289172 - enable automigration on beta 50 for a few cycles,

Review commit: https://reviewboard.mozilla.org/r/78360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78360/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8790654 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

Approval Request Comment
[Feature/regressing bug #]: test for automatic migration of data from the default browser when a user first uses Firefox

[User impact if declined]: we won't know if/how effective the automatic migration and the UI for it are, meaning we would fly blind about making decisions on whether this is worth pursuing, how much time to invest in particular parts of it, etc.

We can't really do the same kind of testing on nightly/aurora because few (if any) users will run those builds as the first copy of Firefox that they ever run on their machine. We already have data that shows this (ie the volume of data for the "manual" migration telemetry we have on those builds is so low that by default it doesn't show up on telemetry.m.o). The data we do have for those trains is likely extremely skewed because of both volume and the 'type' of users that run aurora/nightly.

[Describe test coverage new/current, TreeHerder]: there is automated test coverage for most of the code that's enabled by this change.

[Risks and why]: low. We hope to land this on aurora, so that the first beta has this turned on, and then turn it off again on the beta train after several betas (we're thinking 2-3 weeks) have gathered data about how this works out in practice. So the risk to release is 0 - we're just changing some prefs, and we'll revert those changes well before release.

[String/UUID change made/needed]: nope.

NB: do not land before r+ from cmore & co on disabling the "first run" landing pages for these beta builds (part of this patch).

Chris: asking you for review not to get you to read the code, but so you can approve as/when you + Jen are happy with this, and so that nobody ends up landing the patch before that's the case. To clarify some of our correspondence from yesterday: I'm now sure (see patch) that this *only* affects the "first run" pages, not the "whatsnew"/upgrade pages (ie those will stay).

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Gijs, this one gives me some heartburn. 50 is less than a week away from entering Beta cycle which means this feature is jumping from Nightly to Beta and skipping an entire ~6-week Aurora cycle. Is there a strong justification to do that? As such (to me) it seems like this is a hairy piece of code and should be baked in each channel and perhaps a few cycles on Beta before we go-to-release on it. 

Also, the funnelcake experiment which contains this code hasn't even hit the controlled set yet so we have no feedback on possible issues with auto-migrating from different browsers. I am assuming that is one of the (direct/indirect) objectives from the "onboarding" experiment. Is that correct?

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 8790654 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

https://reviewboard.mozilla.org/r/78360/#review77140

r+ assuming pending approvals for disabling the first-run pages.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Ritu Kothari (:ritu) from comment #4)
> Hi Gijs, this one gives me some heartburn. 50 is less than a week away from
> entering Beta cycle which means this feature is jumping from Nightly to Beta
> and skipping an entire ~6-week Aurora cycle. Is there a strong justification
> to do that? As such (to me) it seems like this is a hairy piece of code and
> should be baked in each channel and perhaps a few cycles on Beta before we
> go-to-release on it. 
> 
> Also, the funnelcake experiment which contains this code hasn't even hit the
> controlled set yet so we have no feedback on possible issues with
> auto-migrating from different browsers.

No, this code already shipped with the special one-off funnelcakes based on Firefox 47 (bug 1272776). You're thinking of the recent uplifts for the second funnelcake to use this (bug 1295873).

As from the discussion around those uplifts and Gijs' comment 3, we feel this is a reasonable approach. Migration code basically only runs for brand new installs, so gets very little usage on Nightly/Aurora Thus, bake time on those channels has little added value. [In fact usage is so low that I mistakenly filed bug 1298244, thinking our probes were broken, but the Telemetry dashboards actually filter out such low-volume reports!] Also, this is a limited-time test to get the data we need to decide on next-actions in finishing and eventually shipping the feature.

And to be clear, we're not "going to release" with this right now. The intent it to have this enabled for a couple of betas, and then disable it again. (While we look at the data, make any changes needed, and decide what to do next.)

Comment 7

[Sylvestre Ledru [:Sylvestre]]

> The intent it to have this enabled for a couple of betas
Why don't you use EARLY_BETA_OR_EARLIER ? 
https://dxr.mozilla.org/mozilla-beta/source/build/defines.sh
it will take care of that for you?

Comment 8

[:Gijs (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> > The intent it to have this enabled for a couple of betas
> Why don't you use EARLY_BETA_OR_EARLIER ? 
> https://dxr.mozilla.org/mozilla-beta/source/build/defines.sh
> it will take care of that for you?

I can find no documentation about when that flag is turned off (defines.sh certainly doesn't say, which would have been useful...). I tried to ask you on IRC but you didn't respond. The best circumstantial evidence I can find is "after b4" which is after 2 weeks.

I'm concerned that that would mean our sampling period would be too small, and I'm additionally concerned that if this patch doesn't land before 50 goes to beta, that would eat into that period even more. It seems simpler to just have a patch and do a straight backout of that patch when we're done, especially when it's only 3 lines.

I would also expect that if we used EARLY_BETA, people would then expect/encourage us to land it on all branches meaning it would run every early beta cycle, which I don't think is what we want at this point.


Ritu, I think dolske has addressed your concerns so I'm clearing needinfo. Feel free to ping me on here or IRC if you have other questions.


Basically, see this as "enable feature X on nightly for some weeks with a pref flip to gather data", but on beta. All things being equal we would have been happy to do this on Nightly but as noted we would not get any "real" data at all, so that wouldn't actually help anybody. In terms of stability, I am confident this won't affect existing users, and that there will be no serious concerns for new users, based on feedback from the funnelcake we did in the 47 cycle. The base of the code has already gone out in the wild without issue, as it were, and also has automated tests.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8790654 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

Per IRC + email I think we're all good here - Chris and Jen said it was fine, and Matt Grimes' stuff will run against whatsnew (not firstrun) so should be unaffected by these changes. Jen, can you confirm that this is good to go from a www.m.o perspective ?

Comment 10

[:Gijs (he/him)]

Eh, I just meant to ask for review...

Comment 11

[Jennifer Bertsch [:jbertsch]]

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 8790654 [details]
> Bug 1289172 - enable automigration on beta 50 for a few cycles,
> 
> Per IRC + email I think we're all good here - Chris and Jen said it was
> fine, and Matt Grimes' stuff will run against whatsnew (not firstrun) so
> should be unaffected by these changes. Jen, can you confirm that this is
> good to go from a www.m.o perspective ?

As far as I know, the Marketing team isn't actively using Beta whatsnew and firstrun pages.  Adding Kaply and Winston in case they need these pages for Yahoo in this time frame

Comment 12

[Mike Kaply [:mkaply]]

FYI:

1. You can't show whatsnew pages between betas (6 to 7 for instance).
2. Showing whatsnew pages via the build infrastructure is broken.

So any use of whatsnew has to be hardcoded.

That being said, as far as I know, we only have plans to use firstrun.

Comment 13

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #12)
> FYI:
> 
> 1. You can't show whatsnew pages between betas (6 to 7 for instance).
> 2. Showing whatsnew pages via the build infrastructure is broken.
> 
> So any use of whatsnew has to be hardcoded.
> 
> That being said, as far as I know, we only have plans to use firstrun.

Yes, just so we're clear: we do not intend to affect whatsnew at all, only firstrun. Most of this communication was originally via email, so I'm sorry that that wasn't clear when you were needinfo'd.

What's not clear to me is if you have plans to use firstrun during the first few weeks of the 50 beta cycle specifically on beta, and if it would be showstopping for you if we stopped showing people who install 50 beta fresh the firstrun page (see patch). Can you clarify?

Comment 14

[Mike Kaply [:mkaply]]

As far as I know, all our use of firstrun will be on release, not on beta. Winston can give further details.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Gijs, (as I said on IRC), with the way this patch is written, it is turning this pref on without any flags guarding it from going to release. Is there a way to incorporate a beta_only safeguard on this one? I understand early_beta might not work as it is only on until Beta4.

Comment 16

[:Gijs (he/him)]

Attachment: Bug 1289172 - enable automigration on beta 50 for a few cycles,

Review commit: https://reviewboard.mozilla.org/r/79194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79194/

Comment 17

[:Gijs (he/him)]

(In reply to Ritu Kothari (:ritu) from comment #15)
> Hi Gijs, (as I said on IRC),

Sorry, I missed you on IRC. :-(

> with the way this patch is written, it is
> turning this pref on without any flags guarding it from going to release. Is
> there a way to incorporate a beta_only safeguard on this one? I understand
> early_beta might not work as it is only on until Beta4.

I've tried, see the second patch. But because browser/branding/ pref files aren't currently preprocessed, so it was messier than I would have liked. Is this better from a relman perspective?

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Ritu Kothari (:ritu) from comment #15)
> > Hi Gijs, (as I said on IRC),
> 
> Sorry, I missed you on IRC. :-(
> 
> > with the way this patch is written, it is
> > turning this pref on without any flags guarding it from going to release. Is
> > there a way to incorporate a beta_only safeguard on this one? I understand
> > early_beta might not work as it is only on until Beta4.
> 
> I've tried, see the second patch. But because browser/branding/ pref files
> aren't currently preprocessed, so it was messier than I would have liked. Is
> this better from a relman perspective?

This looks great! Thank you. I will approve uplift once the patch is r+'d.

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 8791924 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

https://reviewboard.mozilla.org/r/79194/#review77840

Comment 20

[:Gijs (he/him)]

Comment on attachment 8791924 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

Approval Request Comment: see comment #3.

Ritu, I take it this is what you'd like us to go with? If so, I intend to land when I get an OK from Winston, or if I hear nothing, sometime during the weekend, based on the fact that Chris, Jen, Matt Grimes, and Mike all think we should be OK. (I pinged Winston via email as well, to be sure he sees this.)

Comment 21

[:Gijs (he/him)]

Winston confirmed via email that there are no specific plans for firstrun for 50 beta, and that we're good to go.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8791924 [details]
Bug 1289172 - enable automigration on beta 50 for a few cycles,

Assuming all other stakeholders are ok, let's do this.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This has ESLint failures that I'm guessing are a sign of more trouble to come.
https://treeherder.mozilla.org/logviewer.html#?job_id=3571204&repo=mozilla-aurora

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for those failures https://hg.mozilla.org/releases/mozilla-aurora/rev/05c3dc5f50c0

Comment 25

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> This has ESLint failures that I'm guessing are a sign of more trouble to
> come.
> https://treeherder.mozilla.org/logviewer.html#?job_id=3571204&repo=mozilla-
> aurora

Not really, eslint is dumb and shouldn't be validating preprocessed files (really, validating pref files generally is pointless because we don't use an actual JS parser for them (so eslint might be fine with things that the pref parser won't be OK with), but that's a different story). I changed one of the pref files to start using preprocessing at the explicit request of relman, and didn't realize this would be picked up by eslint. I've added a minimal ignore clause and relanded. I had already verified locally that the preprocessing works correctly and produces a valid pref file. I'll file a bug on ignoring all of browser/branding tomorrow because we ignore all of browser/app for the same reasons, but I'll leave that for tomorrow, err, I mean, after I wake up. Leaving ni for that.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b9499912b323

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #25)
> I'll file a
> bug on ignoring all of browser/branding tomorrow because we ignore all of
> browser/app for the same reasons, but I'll leave that for tomorrow, err, I
> mean, after I wake up. Leaving ni for that.

Filed bug 1303525.

Comment 27

[:Gijs (he/him)]

Backed out so we get one 'normal' beta before 50 goes to release and rc.

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/42362b3185baaa3c0d82c481f5facd031c7ee993


Resolving this because there's nothing else to be done for this bug.