1338812 - Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Dão Gottwald [:dao]]

Gijs says it shouldn't be a problem for the data we're looking at, but it's probably still a good idea to clean this up.

<dao> Gijs: what kind of things are migrationType and itemResources in https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/MigrationUtils.jsm#297 ?
<Gijs> dao: itemResources are objects that have a 'migrate()' function that gets called to migrate that particular resource
<Gijs> dao: migrationType is the type of resource migration (bookmarks, passwords, etc.)
<Gijs> dao: so for instance, for edge we import 2 resources of type bookmarks
<Gijs> (the normal bookmarks and the reading list)
<dao> and we use a separate TelemetryStopwatch for both?
<Gijs> dao: there's also an "other data" type that I think we use several of when doing a Firefox reset/refresh
<Gijs> dao: looks like it, yes.
<dao> Gijs: is that intentional? seems weird to me
<Gijs> dao: I don't think it was considered when implementing the stopwatches. Regardless, I don't think it makes a big difference here...
<Gijs> dao: there's only 1 item of each resource type for Chrome, so...
<Gijs> ditto for IE, though we have 2 separate ways of importing passwords (depending on the Windows/IE version), I'm fairly sure we only ever use one at a time.

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1338812 - Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource.

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

Comment 2

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

Comment on attachment 8836401 [details]
Bug 1338812 - Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource.

https://reviewboard.mozilla.org/r/111826/#review113098

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80d9bac9ec22
Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource. r=Gijs

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8836401 [details]
Bug 1338812 - Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource.

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

[User impact if declined]: the data collected for FX_MIGRATION_BOOKMARKS_IMPORT_MS, FX_MIGRATION_HISTORY_IMPORT_MS and FX_MIGRATION_LOGINS_IMPORT_MS could be wrong / misleading in some cases

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: no

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

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

[Is the change risky?]: not particularly risky

[Why is the change risky/not risky?]: it's fairly trivial

[String changes made/needed]: /

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/80d9bac9ec22

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 8836401 [details]
Bug 1338812 - Call TelemetryStopwatch only once per item (history / bookmarks / logins) rather than for each resource.

fix some telemetry probes, aurora53+, beta52+

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f039f3d3d073

Comment 8

[Carsten Book [:Tomcat]]

has problems to apply to beta:

merging browser/components/migration/MigrationUtils.jsm
warning: conflicts while merging browser/components/migration/MigrationUtils.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ad8573642c7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/4ad8573642c7