1340115 - Cap the amount of history we import from Chrome

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Dão Gottwald [:dao]]

This bug is about setting default values for the prefs introduced in bug 1339413 (browser.migrate.chrome.history.maxAgeInDays, browser.migrate.chrome.history.limit).

Comment 1

[Dão Gottwald [:dao]]

I'm gonna propose a limit of 1300 history entries. Roughly 20% of people importing from Chrome have more history than that according to: https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=1&end_date=2017-01-25&keys=chrome!chrome!edge!360se&max_channel_version=release%252F51&measure=FX_MIGRATION_HISTORY_QUANTITY&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-01-18&table=0&trim=1&use_submission_date=0

Comment 2

[Dão Gottwald [:dao]]

For the maximum age I'm going to propose half a year. I think this is generous in terms of user expectations. However I don't know how it will impact the amount history we'll import -- I have no idea when Chrome starts evicting history entries. Would be nice if we had telemetry for the age of the oldest entry.

Comment 3

[Dão Gottwald [:dao]]

Attachment: Bug 1340115 - Cap the amount of history we import from Chrome.

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

Comment 4

[:Gijs (out for now; he/him)]

Ideally I'd like to know more about the correlation between the time to import and the quantity we're importing and at what point we reach problematic timing (more than say 10s, assuming we don't hang the UI in which case the limit should be lower, I guess). But getting that information requires fiddling with sql.tmo. I wrote a start: 

SELECT FX_MIGRATION_HISTORY_IMPORT_MS, FX_MIGRATION_HISTORY_QUANTITY, client_id, LENGTH(REGEXP_EXTRACT(ARRAY_JOIN(
         -- Get the only non-0 migration source identifier:
         ARRAY_REMOVE(ARRAY_DISTINCT(fx_migration_source_browser), array[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0])[1], ''),
       '^0*[1-9]')) AS browserid from longitudinal
   WHERE settings IS NOT NULL
     AND fx_migration_source_browser IS NOT NULL
     AND FX_MIGRATION_HISTORY_IMPORT_MS IS NOT NULL
     AND FX_MIGRATION_HISTORY_QUANTITY IS NOT NULL
     AND CARDINALITY(ARRAY_REMOVE(ARRAY_DISTINCT(fx_migration_source_browser), array[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0])) = 1
     AND normalized_channel = 'release'
     AND LENGTH(REGEXP_EXTRACT(ARRAY_JOIN(
         -- Get the only non-0 migration source identifier:
         ARRAY_REMOVE(ARRAY_DISTINCT(fx_migration_source_browser), array[0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0])[1], ''),
       '^0*[1-9]')) = 6
     LIMIT 100


but keyed histograms are a pain to extract data from. You can look at the data like this, but you'd have to export it and then use a spreadsheet program to actually get something meaningful out in terms of the correlation.

I'm asking around in #telemetry to get some help to finish this query, but it might not happen until later today.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> Ideally I'd like to know more about the correlation between the time to
> import and the quantity we're importing and at what point we reach
> problematic timing

Ideally yes... we could also land this, wait for Telemetry results, and keep tweaking the prefs as needed, but there will still be some handwaving and best guesses involved since it likely won't be easy to tell apart the effects of both prefs.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to :Gijs from comment #4)
> > Ideally I'd like to know more about the correlation between the time to
> > import and the quantity we're importing and at what point we reach
> > problematic timing
> 
> Ideally yes... we could also land this, wait for Telemetry results, and keep
> tweaking the prefs as needed, but there will still be some handwaving and
> best guesses involved since it likely won't be easy to tell apart the
> effects of both prefs.

https://sql.telemetry.mozilla.org/queries/2979

If you sort by time (in ms), you can see that even for as few as 3500 items we take 38 seconds to import. Sure, we might not be hung all that time, but it seems to me that this likely means that 10,000 items is still too large a cut-off. I'd prefer being more conservative with something like 3000. Combined with the sorting and time cutoff, I would expect this to still provide the user with all the sites they can recall using (frequently), without spending ages actually doing the import.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to :Gijs from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > (In reply to :Gijs from comment #4)
> > > Ideally I'd like to know more about the correlation between the time to
> > > import and the quantity we're importing and at what point we reach
> > > problematic timing
> > 
> > Ideally yes... we could also land this, wait for Telemetry results, and keep
> > tweaking the prefs as needed, but there will still be some handwaving and
> > best guesses involved since it likely won't be easy to tell apart the
> > effects of both prefs.
> 
> https://sql.telemetry.mozilla.org/queries/2979
> 
> If you sort by time (in ms), you can see that even for as few as 3500 items
> we take 38 seconds to import. Sure, we might not be hung all that time, but
> it seems to me that this likely means that 10,000 items is still too large a
> cut-off. I'd prefer being more conservative with something like 3000.
> Combined with the sorting and time cutoff, I would expect this to still
> provide the user with all the sites they can recall using (frequently),
> without spending ages actually doing the import.

Uh, and I totally misremembered what the patch did, clearly - you're using a 1300 item limit. Did the earlier patch on the other bug have a higher limit and is that why I'm confused?

Regardless, what's odd is that the 1300 doesn't seem to correspond to 80% of the userbase per that query...

Comment 8

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #7)
> Uh, and I totally misremembered what the patch did, clearly - you're using a
> 1300 item limit. Did the earlier patch on the other bug have a higher limit
> and is that why I'm confused?

It wasn't part of any patch but I initially suggested using 10,000 as the limit in bug 1339413.

Comment 9

[:Gijs (out for now; he/him)]

So in my queries I'm seeing about 30-40% of people having more than 1300 items. Obviously we don't know how that number would be influenced if we also restricted the maximum age. It feels like 2000 items might be a reasonable compromise. What do you think, Dão?

Comment 10

[Dão Gottwald [:dao]]

Fine by me. We can reduce the number later if still needed.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8838020 [details]
Bug 1340115 - Cap the amount of history we import from Chrome.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113022/diff/1-2/

Comment 12

[:Gijs (out for now; he/him)]

Comment on attachment 8838020 [details]
Bug 1340115 - Cap the amount of history we import from Chrome.

https://reviewboard.mozilla.org/r/113022/#review115482

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2262b2682f73
Cap the amount of history we import from Chrome. r=Gijs

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2262b2682f73

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8838020 [details]
Bug 1340115 - Cap the amount of history we import from Chrome.

Approval Request Comment
[Feature/Bug causing the regression]: /

[User impact if declined]: performance issues with history import

[Is this code covered by automated tests?]: looking at browser/components/migration/tests/unit/, it seems that there are no unit tests for history imports :/

[Has the fix been verified in Nightly?]: was tested manually before landing

[Needs manual test from QE? If yes, steps to reproduce]: /

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: low risk

[Why is the change risky/not risky?]: the code behind these prefs was brand new, but now that it has baked for three weeks I think this is fairly safe

[String changes made/needed]: /

Comment 16

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #15)
> [Why is the change risky/not risky?]: the code behind these prefs was brand
> new, but now that it has baked for three weeks I think this is fairly safe

Closer to two weeks actually, but still.

Comment 17

[Gerry Chang [:gchang]]

Comment on attachment 8838020 [details]
Bug 1340115 - Cap the amount of history we import from Chrome.

Fix a performance issue when importing history. Beta53+.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/05659f9e9b7b

Comment 19

[Andrei Vaida [:avaida]]

Dão, should manual QA be looking at this? We're checking migration once per beta cycle, we could include this as well, if you think it would be of value.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #19)
> Dão, should manual QA be looking at this? We're checking migration once per
> beta cycle, we could include this as well, if you think it would be of value.

Yeah, probably a good idea.