1298208 - FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Justin Dolske [:Dolske]]

Looking at the FX_MIGRATION_HOMEPAGE_IMPORTED telemetry probe on Beta-48 (https://mzl.la/2bJYfoH), it seems this always report "true".

The code was added as:

  Services.telemetry.getKeyedHistogramById("FX_MIGRATION_HOMEPAGE_IMPORTED")
                     .add(this._source, !!this._newHomePage);

Unfortunately, it looks like this._newHomePage has a value of "DEFAULT" to indicate that the user doesn't want to import their homepage:

http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/browser/components/migration/content/migration.js#315,329


So, need a patch to s/!!this._newHomePage/this._newHomePage != "DEFAULT"/

Should we rename the probe to make sure we're not looking at bogus data?

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Bug 1298208 - FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

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

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8785030 [details]
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

https://reviewboard.mozilla.org/r/74334/#review72188

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f30fd1bbbe1
FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=MattN

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8785030 [details]
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

Approval Request Comment
[Feature/regressing bug #]: bug 731025
[User impact if declined]: No user impact, correctness fix for telemetry data
[Describe test coverage new/current, TreeHerder]: none, but QA tests feature for releases
[Risks and why]: Trivial fix, ultra low risk.
[String/UUID change made/needed]: N/A

Comment 5

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

This managed to break all builds like https://treeherder.mozilla.org/logviewer.html#?job_id=2577571&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/20988ef014a1

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Imo, we should just completely remove homepage importing.
Most of the browsers today offer a default homepage with search capability and local contents, or an homepage that points to their corporate page (msn, apple...). Very few users change it.
Plus we are moving towards better interaction like ActivityStream.
This import code gives very small benefits to the user, and it's also (IIRC) one of the few remaining pieces that is synchronous. The cost largely overweight the benefit.

Comment 7

[Justin Dolske [:Dolske]]

Bah. That's what I get for walking away at the end of the day before checking a local build. In my defense, histogram-whitelists.json was added after the original patch.

I fixed the original patch by updating histogram-whitelists.json (this was just a build-time check), and as penance added an extra patch to update the other migration telemetry probes with a bug number and email, allowing them to be removed from the whitelist. [Telemetry probes are now checked for mandatory fields, older probes missing these fields are grandfathered in via this whitelist.]

Build is fine locally now. :)

Comment 8

[Justin Dolske [:Dolske]]

(In reply to Marco Bonardo [::mak] from comment #6)
> Imo, we should just completely remove homepage importing.

I tend to agree, I think we were curious to first see if people were already opting-out / how often it had a page to import.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Fix probe (part 1, v.2)

(I can't figure out how to make mozreview actually attach patches again to this bug, so we're back to normal patches. Grr.)

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Add metadata (Part 2, v.1)

Comment 11

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/65e93bfe39e501e15ab1d044d35b92c3ca7ada4e
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=mattn

https://hg.mozilla.org/integration/fx-team/rev/f1c8f78cd6f6e49a5f67b37567888473b898cb90
Bug 1298208 - add bug number and notification emails for migrator probes, removing them from exception whitelist. r=mattn

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8785368 [details] [diff] [review]
Fix probe (part 1, v.2)

Approval Request Comment
[Feature/regressing bug #]: bug 731025
[User impact if declined]: No user impact, correctness fix for telemetry data
[Describe test coverage new/current, TreeHerder]: none, but QA tests feature for releases
[Risks and why]: Trivial fix, ultra low risk.
[String/UUID change made/needed]: N/A

Comment 13

[Justin Dolske [:Dolske]]

https://hg.mozilla.org/integration/fx-team/rev/e6121d319f60b0e800d4b446ce8a0fc9cd48c21e
Bug 1298208 - followup fix for correct JS type. CLOSED TREE

Comment 14

[Justin Dolske [:Dolske]]

I hate this bug. :(

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785368 [details] [diff] [review]
Fix probe (part 1, v.2)

Sounds like this is fairly crucial for us to know from telemetry. 
Let's take it for aurora and for beta 8. 
Andrei does your team test migration between beta 8 and release? A little bit of smoketest on migration might be good.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65e93bfe39e5
https://hg.mozilla.org/mozilla-central/rev/f1c8f78cd6f6
https://hg.mozilla.org/mozilla-central/rev/e6121d319f60

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/47d0465d6f57
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ed3f227fe0b

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/280ba53d17e1
https://hg.mozilla.org/releases/mozilla-beta/rev/c8f9000e88f2

Comment 19

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Comment on attachment 8785368 [details] [diff] [review]
> Fix probe (part 1, v.2)
> 
> Sounds like this is fairly crucial for us to know from telemetry. 
> Let's take it for aurora and for beta 8. 
> Andrei does your team test migration between beta 8 and release? A little
> bit of smoketest on migration might be good.

We've spot-checked migration again using the latest beta tinderbox build (containing this fix)
on Windows 10 x64 [1], Ubuntu 16.04 x64 [2] and Mac OS X 10.11.6 [3]. 

Apart from a few already known bugs [4], there were no new issues identified during testing.

Please let me know if there's anything else we can help with here.


[1] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/1472680282/
[2] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-linux64/1472680282/
[3] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1472680282/
[4] Known bugs - Windows 10 x64: https://mzl.la/2cbWqOq
               - Ubuntu 16.04 x64: https://mzl.la/2cbUvK6
               - Mac OS X 10.11.6: https://mzl.la/2cbUB47