Closed Bug 1249596 Opened 9 years ago Closed 7 years ago

Remove FHR client id migration code in Firefox 50

Categories

(Toolkit :: Telemetry, defect, P4)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: gfritzsche, Assigned: o0ignition0o, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

Bug 1249596 - Part 2: Remove FHR client id migration code.
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1249596 - Part 1: Remove FHR client id migration code on Android.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
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.
Hey Georg, could you please assign this one to me ? :)
Hey Jeremy, i assigned it to you. Sorry, this mail dropped under my radar.
Assignee: nobody → jeremy.lempereur
Mentor: gfritzsche, alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
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 ?
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.
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 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 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+
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 ;)
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!
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!
Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5040af317e67 Remove FHR client id migration code. r=Dexter
Awesome, thanks !
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
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 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 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 :)
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 ?
Ok I've ran the 3 tests on my computer, and they all pass. nalexander could you please trigger the CI again ? *crossing fingers*
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
Ok yet an other rookie mistake :'( dexter This should be fine now ! Ready for autoland !
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
https://hg.mozilla.org/mozilla-central/rev/565351d7da66 https://hg.mozilla.org/mozilla-central/rev/a44ddbed30dc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: