FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed, firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
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+

Comment 3

2 years ago
Pushed by jdolske@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f30fd1bbbe1
FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=MattN
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8785030 - Attachment is obsolete: true
Attachment #8785030 - Flags: review+
Attachment #8785030 - Flags: approval-mozilla-beta?
Attachment #8785030 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

2 years ago
Created attachment 8785368 [details] [diff] [review]
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.)
Attachment #8785368 - Flags: review+
(Assignee)

Comment 10

2 years ago
Created attachment 8785371 [details] [diff] [review]
Add metadata (Part 2, v.1)
Attachment #8785371 - Flags: review?(MattN+bmo)
Attachment #8785371 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 11

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8785368 - Attachment description: Fix probe (part 2, v.2) → Fix probe (part 1, v.2)
(Assignee)

Comment 12

2 years ago
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?
(Assignee)

Comment 14

2 years ago
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+

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65e93bfe39e5
https://hg.mozilla.org/mozilla-central/rev/f1c8f78cd6f6
https://hg.mozilla.org/mozilla-central/rev/e6121d319f60
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(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.