Closed Bug 1289906 Opened 8 years ago Closed 8 years ago

Add telemetry indicating how "undo" of the automatic migration got disabled (by running it, because of inaction, because of changed data or because the user chose to keep data, or because of sync)

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [migration-needs-uplift])

Attachments

(3 files)

To evaluate how important undo is to users, besides getting numbers of how many people use it, we also need numbers of how people "don't" use it, and why it isn't used - because people actively choose to keep the automigrated data, because they don't make a decision, because they create bookmarks/passwords that remove the undo option, or because they sign into sync, or...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Whiteboard: [migration-needs-uplift]
Comment on attachment 8783491 [details]
Bug 1289906 - part 2: add more generic telemetry for undo reasons,

https://reviewboard.mozilla.org/r/73278/#review71062
Attachment #8783491 - Flags: review?(benjamin) → review+
Comment on attachment 8783490 [details]
Bug 1289906 - part 1: use an observer to know when the user signs into sync after an undo,

https://reviewboard.mozilla.org/r/73276/#review71336

The only thing I really care about is the obviously wrong removeObserver call - the rest are up to you.

::: browser/components/migration/AutoMigrate.jsm:27
(Diff revision 1)
>    "addLogin",
>    "modifyLogin",
>  ]);
>  
> +const kSyncTopics = new Set([
> +  "weave:service:setup-complete",

I'm not sure we need both of these? When migrating from a Firefox profile the Sync credentials are copied across, but that will still trigger the second. IOW, I think just the login:finish would be enough (although I can't see how having both would actually hurt)

A possible UX issue with this is that undo might be available for a while, then Sync kicks in causing undo to become unavailable even though the user didn't actually take any action. I've no idea if you consider that a problem, but if you do, you probably still need to check getSignedInUser() as that will return truthy immediately (ie, without waiting for Sync to kick in)

::: browser/components/migration/AutoMigrate.jsm:249
(Diff revision 1)
>  
>    removeUndoOption() {
>      // Remove observers, and ensure that exceptions doing so don't break
>      // removing the pref.
>      try {
> +      for (let topic of kSyncTopics) {

you could consider |of [... kSyncTopics, kPasswordManagerTopic]| here IIUC, and remove some of the duplicated "ignore exceptions" biolerplate (which IMO is a little misguided without logging a message somewhere, so the copy/paste error in the line below would have been noticed)

(Similarly above where you add the observers, but it's not saving much up there)

::: browser/components/migration/AutoMigrate.jsm:250
(Diff revision 1)
>    removeUndoOption() {
>      // Remove observers, and ensure that exceptions doing so don't break
>      // removing the pref.
>      try {
> +      for (let topic of kSyncTopics) {
> +        Services.obs.removeObserver(this, kSyncTopics);

s/kSyncTopics/topic/ here.
Attachment #8783490 - Flags: review?(markh) → review+
Comment on attachment 8783491 [details]
Bug 1289906 - part 2: add more generic telemetry for undo reasons,

https://reviewboard.mozilla.org/r/73278/#review71338
Attachment #8783491 - Flags: review?(markh) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e848a9e1ad6a
part 1: use an observer to know when the user signs into sync after an undo, r=markh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ba26444126
part 2: add more generic telemetry for undo reasons, r=bsmedberg,markh
https://hg.mozilla.org/mozilla-central/rev/e848a9e1ad6a
https://hg.mozilla.org/mozilla-central/rev/e1ba26444126
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8783491 [details]
Bug 1289906 - part 2: add more generic telemetry for undo reasons,

Approval Request Comment
[Feature/regressing bug #]: automigration (funnelcake for bug 1295873)
[User impact if declined]: we won't know what people do with the undo option - if they explicitly don't want the data, or everybody uses it, or if most people don't do anything and we stop offering it after a number of days (or because people create bookmarks, save passwords, or sign into sync, all of which make it impossible to figure out what to remove when we want to "undo" the initial migration).
[Describe test coverage new/current, TreeHerder]: there is a pre-existing automated test that covers this functionality, but we don't have a good way to test telemetry results themselves, so there is no automated test coverage for the telemetry itself.
[Risks and why]: low - all this code is behind the pref, and only applies when automigration is enabled as part of the funnelcake. The patches replace some of the earlier telemetry that is in bug 1283565.
[String/UUID change made/needed]: nope.
Attachment #8783491 - Flags: approval-mozilla-beta?
Attachment #8783491 - Flags: approval-mozilla-aurora?
Comment on attachment 8783490 [details]
Bug 1289906 - part 1: use an observer to know when the user signs into sync after an undo,

Approval Request Comment 8.
Attachment #8783490 - Flags: approval-mozilla-beta?
Attachment #8783490 - Flags: approval-mozilla-aurora?
Comment on attachment 8783491 [details]
Bug 1289906 - part 2: add more generic telemetry for undo reasons,

More telemetry for automigration, for funnelcake tests in 49.
Attachment #8783491 - Flags: approval-mozilla-beta?
Attachment #8783491 - Flags: approval-mozilla-beta+
Attachment #8783491 - Flags: approval-mozilla-aurora?
Attachment #8783491 - Flags: approval-mozilla-aurora+
Comment on attachment 8783490 [details]
Bug 1289906 - part 1: use an observer to know when the user signs into sync after an undo,

Automigration telmetry for uplift to aurora and beta.
Attachment #8783490 - Flags: approval-mozilla-beta?
Attachment #8783490 - Flags: approval-mozilla-beta+
Attachment #8783490 - Flags: approval-mozilla-aurora?
Attachment #8783490 - Flags: approval-mozilla-aurora+
This'll need rebasing for Aurora. Might as well check Beta too.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1289231
Flags: needinfo?(gijskruitbosch+bugs)
This includes the switch to using the locale file from content/ instead. It also includes the followup for the quotes. It also restricts the cases where all the automigration code will be enabled to en-US only (so that the hardcoded l10n is actually OK to use). The latter (2-line change) was reviewed by jaws over IRC, and I tested all of it locally against aurora.

This patch also includes the dep, because it had conflicts after the l10n change.

I expect this will apply on beta too, but that's hard to check because I don't have a tree with the rest of the patches applied to beta.
Mark, the initial telemetry data we're getting from b50 suggests that the sync case is literally never hit. That seems almost impossible.

Does the fxaccounts:onlogin thing for sure hit when people start with a clean new profile and start using sync (e.g. if they sign up within the browser) ? Any chance we've missed something here?
Flags: needinfo?(markh)
(In reply to :Gijs Kruitbosch from comment #16)
> Mark, the initial telemetry data we're getting from b50 suggests that the
> sync case is literally never hit. That seems almost impossible.

I agree that seems odd - however, it does seem that it would be rare that a Firefox user who doesn't currently use Sync does a profilereset, then chooses to sign in to Sync while "undo" is still available (4 days IIUC?)

> Does the fxaccounts:onlogin thing for sure hit when people start with a
> clean new profile and start using sync (e.g. if they sign up within the
> browser) ? Any chance we've missed something here?

Yeah, I'm confident that this observer fires in that case or Sync itself would be broken. This should be very easy to test though?
Flags: needinfo?(markh)
(In reply to Mark Hammond [:markh] from comment #17)
> (In reply to :Gijs Kruitbosch from comment #16)
> > Mark, the initial telemetry data we're getting from b50 suggests that the
> > sync case is literally never hit. That seems almost impossible.
> 
> I agree that seems odd - however, it does seem that it would be rare that a
> Firefox user who doesn't currently use Sync does a profilereset, then
> chooses to sign in to Sync while "undo" is still available (4 days IIUC?)

Well, the code here won't be running for a profile reset, but if the user installs Firefox beta on a machine with no existing Firefox data and a different default browser (so "new user" experience, as it were). We have telemetry indicating roughly 20,000 people made it through that flow in the first week or so of beta. It would be surprising if literally 0 of them signed up to fxa.

> > Does the fxaccounts:onlogin thing for sure hit when people start with a
> > clean new profile and start using sync (e.g. if they sign up within the
> > browser) ? Any chance we've missed something here?
> 
> Yeah, I'm confident that this observer fires in that case or Sync itself
> would be broken. This should be very easy to test though?

Yeah, I'll give this a poke later today. Thanks for checking.
Depends on: 1307382
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: