Closed Bug 1298208 Opened 5 years ago Closed 5 years ago

FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

Tracking

()

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

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(2 files, 1 obsolete file)

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?
Assignee: nobody → dolske
Comment on attachment 8785030 [details]
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

https://reviewboard.mozilla.org/r/74334/#review72188
Attachment #8785030 - Flags: review?(MattN+bmo) → review+
Pushed by jdolske@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f30fd1bbbe1
FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=MattN
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
Attachment #8785030 - Flags: approval-mozilla-beta?
Attachment #8785030 - Flags: approval-mozilla-aurora?
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.
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. :)
Flags: needinfo?(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.
Attachment #8785030 - Attachment is obsolete: true
Attachment #8785030 - Flags: review+
Attachment #8785030 - Flags: approval-mozilla-beta?
Attachment #8785030 - Flags: approval-mozilla-aurora?
(I can't figure out how to make mozreview actually attach patches again to this bug, so we're back to normal patches. Grr.)
Attachment #8785368 - Flags: review+
Attachment #8785371 - Flags: review?(MattN+bmo)
Attachment #8785371 - Flags: review?(MattN+bmo) → review+
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
Attachment #8785368 - Attachment description: Fix probe (part 2, v.2) → Fix probe (part 1, v.2)
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
Attachment #8785368 - Flags: approval-mozilla-beta?
Attachment #8785368 - Flags: approval-mozilla-aurora?
I hate this bug. :(
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.
Flags: needinfo?(andrei.vaida)
Attachment #8785368 - Flags: approval-mozilla-beta?
Attachment #8785368 - Flags: approval-mozilla-beta+
Attachment #8785368 - Flags: approval-mozilla-aurora?
Attachment #8785368 - Flags: approval-mozilla-aurora+
(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
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.