Remove FHR client id migration code in Firefox 50

RESOLVED FIXED in Firefox 60

Status

()

defect
P4
normal
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: gfritzsche, Assigned: o0ignition0o, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
We used to store the client id in <profile>/healthreport/state.json.
This moved to the more generic <profile>/datareporting/state.json and we have migration code in place for a while now.

More recently, FHR was removed. We should remove the migration code from ClientID.jsm in a future release (suggested: Firefox 50).
Also, we apparently never removed the old state file etc., so we should remove any healthreport/ directories that still exist.

Android added client id handling code on the Java side, that code should also drop the healthreport/state.json migration.
(Assignee)

Comment 1

a year ago
Hey Georg, could you please assign this one to me ? :)
(Reporter)

Comment 2

a year ago
Hey Jeremy, i assigned it to you.
Sorry, this mail dropped under my radar.
Assignee: nobody → jeremy.lempereur
(Reporter)

Updated

a year ago
Mentor: gfritzsche, alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
(Assignee)

Comment 3

a year ago
Hi Georg :)

I'm trying to figure out If I should remove any js content related to fhr or not ( https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/healthreport-prefs.js ) as well as things which re related to "session-state.json" ( https://searchfox.org/mozilla-central/source/browser/components/migration/tests/unit/test_fx_telemetry.js#119 ) 

Am I supposed to submit separate patches and get rid of anything related to healthreport ?
(Reporter)

Comment 4

a year ago
Hey Jeremy,

sorry, i should have been more specific here.
The specific thing we want to remove here is a fallback to import from a legacy file from old versions:
https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/toolkit/modules/ClientID.jsm#115

You can run the test that covers this using:
> ./mach test toolkit/modules/tests/xpcshell/test_client_id.js

To be safe, we can run the Telemetry tests too:
> ./mach test toolkit/components/telemetry/tests/unit/

The profile migration is a good point, there is handling for the old state file here that we can remove:
https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/browser/components/migration/FirefoxProfileMigrator.js#270

The tests to run here should be:
> ./mach test browser/components/migration/tests/unit/


The handling for the other state files we need to leave in place, they're still being used by the current code.
Specifically we use
- "state.json" (in the new location) for saving the Telemetry client id
- "session-state.json" for storing Telemetry state related to sessions (identifier for the current session & subsession, tracking the count of sessions so far, ...).

For cleaning up healthreport-prefs.js, that's best to think about in a separate bug, feel free to file one!
We'll need to keep at least the "uploadEnabled" pref in some place for now.
(Assignee)

Comment 5

a year ago
Hi, sorry for the delay :'( I somehow ran hg pull and I cannot run ./mach bootstrap and ./mach build anymore.
I need to fix this before I submit a patch.
I'm still working on the issue, please be patient ^^'
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
Comment on attachment 8943785 [details]
Bug 1249596 - Part 2: Remove FHR client id migration code.

Thanks Jeremy!
I'm a bit short on time in the next days - Alessio, can you take a look?
Attachment #8943785 - Flags: review?(gfritzsche) → review?(alessio.placitelli)

Comment 8

a year ago
mozreview-review
Comment on attachment 8943785 [details]
Bug 1249596 - Part 2: Remove FHR client id migration code.

https://reviewboard.mozilla.org/r/214162/#review220144

This looks good, thanks! Would you mind filing a bug for for cleaning up healthreport-prefs.js, as discussed in comment 4?

::: commit-message-abbe0:1
(Diff revision 1)
> +Bug 1249596 - Remove FHR client id migration code. r=gfritzsche

Please note (for future patches) that the first line of the commit message should end with "r?gfritzsche" not "r=gfritzsche". The question mark indicates that this is a review request, the equal sign means that the review was done.
Attachment #8943785 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 9

a year ago
Hi Alessio,

Thanks a lot for the review, as well as for the hint ! 

I m going to file a followup bug on this, and I'd love to handle the cleanup if someone wishes to mentor me :)
(In reply to Jeremy Lempereur from comment #9)
> Hi Alessio,
> 
> Thanks a lot for the review, as well as for the hint ! 
> 
> I m going to file a followup bug on this, and I'd love to handle the cleanup
> if someone wishes to mentor me :)

You're welcome! I'm happy to help there as well ;)
Comment hidden (mozreview-request)
I think I've figured out a way to amend the commit, so I marked you as r? :)
(In reply to Jeremy Lempereur from comment #12)
> I think I've figured out a way to amend the commit, so I marked you as r? :)

Great, thanks!
(Assignee)

Comment 14

a year ago
mozreview-review-reply
Comment on attachment 8943785 [details]
Bug 1249596 - Part 2: Remove FHR client id migration code.

https://reviewboard.mozilla.org/r/214162/#review220144

Hi :) I'm sorry it's the first time i try to ship something :x

I got a permission denied message :/

I think it's normal since I'm not supposed to be able to land patches, should i just edit the commit message to `r=dexter` and let you land it ?
(In reply to Jeremy Lempereur from comment #14)
> I got a permission denied message :/

That's expected as I don't think you have commit-level privileges :)

> I think it's normal since I'm not supposed to be able to land patches,
> should i just edit the commit message to `r=dexter` and let you land it ?

I just landed the patch: no need to change the commit message as mozreview takes care of that automatically!

Comment 16

a year ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5040af317e67
Remove FHR client id migration code. r=Dexter
Awesome, thanks !

Comment 18

a year ago
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88e01feba34c
Backed out changeset 5040af317e67 for Mochitest Android 4.3 RC4 failures at testUnifiedTelemetryClientId on a CLOSED TREE
Outch :'( this looks like it's linked to https://bugzilla.mozilla.org/show_bug.cgi?id=1431544 , should I merge both patches?
(In reply to Jeremy Lempereur from comment #19)
> Outch :'( this looks like it's linked to
> https://bugzilla.mozilla.org/show_bug.cgi?id=1431544 , should I merge both
> patches?

Mh, we have two options:

- Add that patch to this bug (as a second commit so that they can land together and gkruglov@mozilla.com can review)
- Keep the bugs separate, land the other bug first and than this one.

I personally think that the first option would be cleaner, but I don't mind to much if you go for the second :)
I'll take the cleaner approach then, that should help me ramp up my hg jutsu by the way :)
(In reply to Jeremy Lempereur from comment #21)
> I'll take the cleaner approach then, that should help me ramp up my hg jutsu
> by the way :)

Sure! Feel free to ping me here or over IRC if the way to the hg enlightenment gets troublesome :D
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Not really sure whether I really worked or I broke everything ^^' 
grisha I'll mark 1431544 as resolved once both patchs are valid and get landed I guess :)

Comment 26

a year ago
mozreview-review
Comment on attachment 8944690 [details]
Bug 1249596 - Part 1: Remove FHR client id migration code on Android.

https://reviewboard.mozilla.org/r/214840/#review220508

::: commit-message-ee0b1:1
(Diff revision 1)
> +Bug 1249596 - Part 1: Remove FHR client id migration code. r?grisha

nit: let's change this first line to mention that this patch is about the Android code, something like "Bug 1249596 - Part 1: Remove the FHR client id migration code for Android. r?grisha"
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

a year ago
mozreview-review
Comment on attachment 8944690 [details]
Bug 1249596 - Part 1: Remove FHR client id migration code on Android.

https://reviewboard.mozilla.org/r/214840/#review222748

This looks good!  One tiny nit.

I triggered the Robocop and android-{test,lint,findbugs,checkstyle} jobs on your try push.  If those are green, we're good to go.  Sorry for the delays, and thanks for helping Firefox for Android!

::: commit-message-ee0b1:3
(Diff revision 2)
> +Bug 1249596 - Part 1: Remove FHR client id migration code on Android. r?grisha
> +
> +Removed FHR client id migration code on the android side.

super-nit: capital "Android".
Attachment #8944690 - Flags: review+
Thanks a lot for your review !
I'll edit the commit message and mark you as reviewer asap :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

a year ago
Duplicate of this bug: 1431544
Ok I'm about to submit a patch for two treeherder tests I seem to have missed. 

I don't really know how to handle the last one though : 

https://treeherder.mozilla.org/testview.html?repo=try&revision=43bf3710b8f20e86683649a316845f2527b9cfcb => testSettingsPages 

Any hints ?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Ok I've ran the 3 tests on my computer, and they all pass. 
nalexander could you please trigger the CI again ? *crossing fingers*
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Ok I've made a huge mess (had to learn about hg histedit the hard way ^^') but it should be all right now.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 7 changes to 7 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 13e93b36ecb3 needs "Bug N" or "No bug" in the commit message.
remote: Jeremy Lempereur <jeremy.lempereur@gmail.com>
remote: Bugg 1249596 - Part 1: Remove FHR client id migration code on Android. r=nalexander
remote: 
remote: Removed FHR client id migration code on the Android side.
remote: 
remote: MozReview-Commit-ID: X9QKtbh6r3
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Ok yet an other rookie mistake :'( 
dexter This should be fine now ! Ready for autoland !

Comment 49

a year ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/565351d7da66
Part 1: Remove FHR client id migration code on Android. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/a44ddbed30dc
Part 2: Remove FHR client id migration code. r=Dexter

Comment 50

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/565351d7da66
https://hg.mozilla.org/mozilla-central/rev/a44ddbed30dc
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.