Closed Bug 1279501 Opened 8 years ago Closed 8 years ago

Add migrator telemetry for number of imported items

Categories

(Firefox :: Migration, defect, P3)

Product:
Firefox
Component:
Migration
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: Dolske, Assigned: Gijs)

References

Details

Attachments

(1 file)

Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,
58 bytes, text/x-review-board-request
benjamin
: review+
mak
: review+
gchang
: approval-mozilla-aurora+
Details 
One of the reasons we support (and are improving) migrators is the theory that users who try a new browser will be more likely to keep using it if it's got all the stuff from their old browser.

It would be useful to record how many items are migrated, so that we can understand the impact of this. Is there a correlation between the number of items imported and the retention of that user? One would expect this matters a lot with users who were heavily invested in their bookmarks/passwords, whereas migration wouldn't really matter for users who barely used that.

This might also provide some indirect signal for the "how much is available" probe that we couldn't really implement in bug 1276694 (where we went for recency instead)... While we wouldn't know the number of items from browsers that were available but *not* selected for migration, it would give us a better understanding of how much is being imported when users make an explicit choice. We can then compare that with automigration: to see if it's similar, and to see if it correlates with the odds of a user selecting "undo". (I'm specifically thinking of the case of "you only used IE to download Firefox, and we unhelpfully migrated the default IR bookmarks".)

[I was thinking it would be nice answer things like "how often did the user undo an automigration, but the redo an import from another browser that had more data", but I'm not sure that's possible with a single probe.]


Despite my long comment, I think this is a simple FX_MIGRATION_NUM_ITEMS_IMPORTED probe, keyed by data type. :-)
Blocks: 1309613
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8803624 [details]
Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,

https://reviewboard.mozilla.org/r/87810/#review87054

::: browser/components/migration/ChromeProfileMigrator.js:342
(Diff revision 1)
>            }
>          }
>  
>          if (places.length > 0) {
>            yield new Promise((resolve, reject) => {
> -            PlacesUtils.asyncHistory.updatePlaces(places, {
> +            MigrationUtils.insertPlacesWrapper(places, {

we should stop using updatePlaces in most code (bug 1282770) to rather move to the newest and more web-ext friendly PlacesUtils.history.insertMany API.

In preparation for that, I'd rather name this insertVisitsWrapper. the API will change so it's not critical to do that now, but I'd prefer to start giving this a nicer name.

::: browser/components/migration/MigrationUtils.jsm:288
(Diff revision 1)
>        let notify = function(aMsg, aItemType) {
>          Services.obs.notifyObservers(null, aMsg, aItemType);
>        };
>  
> +      for (let resourceType of ["bookmarks", "history", "logins"]) {
> +        MigrationUtils._importQuantities[resourceType] = 0;

nit: off-hand looks like here you may just do this._importQuantities = new Map([["bookmarks", 0], ["history", 0]...]); and don't define it by default. The Map api is just slightly more verbose than a plain object.
Later you can just iterate over it rather than hardcoding multiple times the list of keys.

::: toolkit/components/telemetry/Histograms.json:4840
(Diff revision 1)
>    },
> +  "FX_MIGRATION_BOOKMARKS_QUANTITY": {
> +    "bug_numbers": [1279501],
> +    "alert_emails": ["gijs@mozilla.com"],
> +    "expires_in_version": "56",
> +    "kind": "exponential",

I'm not a telemetry peer, but now we have new type of histograms, and looks like this one could maybe be a "count" histogram or a keyed histogram (and then you'd only have one histogram with categories, that would better scale in the future)

Eventually you may ask gfritzche, but off-hand what you want here may be a keyed histogram
Attachment #8803624 - Flags: review?(mak77)
Before I forget, I know we don't have a lot of tests around migration, but we have a few tests checking bookmarks/history for some browsers, and it may be nice to add a small subtest to one of those to check telemetry is properly collected.
I was just discussing with the telemetry team about the fact too ofte we land new probes without a test, and we don't notice regressions early enough.
Comment on attachment 8803624 [details]
Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,

https://reviewboard.mozilla.org/r/87810/#review88436

data-review=me
Attachment #8803624 - Flags: review?(benjamin) → review+
Comment on attachment 8803624 [details]
Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,

https://reviewboard.mozilla.org/r/87810/#review88486

::: browser/components/migration/ChromeProfileMigrator.js:468
(Diff revision 2)
>                break;
>              case AUTH_TYPE.SCHEME_BASIC:
>              case AUTH_TYPE.SCHEME_DIGEST:
>                // signon_realm format is URIrealm, so we need remove URI
>                loginInfo.httpRealm = row.getResultByName("signon_realm")
> -                                    .substring(loginInfo.hostName.length + 1);
> +                                    .substring(loginInfo.hostname.length + 1);

nit: indentation (align dots)

::: browser/components/migration/MigrationUtils.jsm:931
(Diff revision 2)
>  
> +  _importQuantities: {
> +    bookmarks: 0,
> +    logins: 0,
> +    history: 0,
> +  },

nit: add newline
Attachment #8803624 - Flags: review?(mak77) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77012ce07e57
add telemetry for the number of bookmarks, history visits and logins are imported from another browser, r=bsmedberg,mak
Backed out and relanded because I forgot to update the telemetry buckets per discussion with Marco on IRC (that didn't get reflected on the bug, which is why I forgot...). Should be all good now.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b99f3a05db46
add telemetry for the number of bookmarks, history visits and logins are imported from another browser, r=bsmedberg,mak
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/b99f3a05db46
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Comment on attachment 8803624 [details]
Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,

Approval Request Comment
[Feature/regressing bug #]: want to test automigration on 51 beta and get data about which users elect to "undo" automigration (and what might motivate them to choose that option).
[User impact if declined]: can't do the above. :-)
[Describe test coverage new/current, TreeHerder]: there is test coverage for migration already, and the landed csets add automated test coverage that verify the telemetry collection mechanism's counts match those of the importers.
[Risks and why]: low, telemetry change that has automated tests
[String/UUID change made/needed]: nope
Attachment #8803624 - Flags: approval-mozilla-aurora?
Comment on attachment 8803624 [details]
Bug 1279501 - add telemetry for the number of bookmarks, history visits and logins are imported from another browser,

Add telemetry to help collect about "undo" automigration. Take it in 51 aurora.
Attachment #8803624 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is hitting conflicts on the uplift to aurora.
Flags: needinfo?(gijskruitbosch+bugs)
This is reporting nothing on any of the channels that I can see. Going to look tomorrow to figure out why.
Flags: needinfo?(gijskruitbosch+bugs)
I am an idiot.

>           let histogram = Services.telemetry.getKeyedHistogram(histogramId);

should be getKeyedHistogramById

Also bug 1311100's fixes can't come soon enough.

I'll file a dep to get this fixed and uplifted ASAP.
Flags: needinfo?(gijskruitbosch+bugs)
Depends on: 1319788
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: