Closed Bug 1332557 Opened 6 years ago Closed 6 years ago

Consider enabling sync success logs by default on Nightly

Categories

(Firefox :: Sync, defect, P2)

Product:
Firefox
Component:
Sync
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: markh, Assigned: bharatraghunthan9767, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(3 files)

Enabled sync success logs by default only on Nightly
4.03 KB, patch
markh
: review+
Details | Diff | Splinter Review
Enabled sync success logs by default only on Nightly - submitting as diff/patch
1.03 KB, patch
markh
: review+
Details | Diff | Splinter Review
Bug 1332557 - Enable sync success logs by default on Nightly.
59 bytes, text/x-review-board-request
markh
: review+
Details 
We regularly get bug reports from Nightly users. It would be helpful, and within the scope of what Nightly is for, to have services.sync.log.appender.file.logOnSuccess be true on Nightly by default - that way we could ask such users for their logs and be more likely to have something useful.

(We could also argue this for Aurora, but one yak at a time ;)
Priority: -- → P2
Mentor: markh
Keywords: good-first-bug
Can I still take this?
Flags: needinfo?(markh)
Of course! Do you already have your Firefox build environment set up? If not, check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction.

You'll want to add the pref to http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/services/sync/services-sync.js, and change the `moz.build` file in that directory so that it preprocesses the prefs file. Have a look at the DevTools prefs file (http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/devtools/client/preferences/devtools.js) and its build manifest for an example of how to set a Nightly-only pref.

Please feel free to ask questions here, or on IRC in the #sync channel, if you get stuck. Thanks for taking this on!
Flags: needinfo?(markh)
I have taken on this bug as it was unassigned and have submitted on https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc however, it is not appearing as a MozReview Request here, so I'm attaching it separately.
Flags: needinfo?(markh)
Comment on attachment 8843560 [details] [diff] [review]
Enabled sync success logs by default only on Nightly

That looks exactly correct, thanks.
Attachment #8843560 - Attachment is patch: true
Attachment #8843560 - Attachment mime type: application/javascript → text/plain
Flags: needinfo?(markh)
Attachment #8843560 - Flags: feedback?(markh) → review+
Assignee: nobody → bharatraghunthan9767
Keywords: checkin-needed
This needs to be in diff format to be landed.
Keywords: checkin-needed
Should I make obsolete the previous attachment #8843560 [details] [diff] [review] or leave it as it is?
Flags: needinfo?(markh)
Comment on attachment 8843834 [details] [diff] [review]
Enabled sync success logs by default only on Nightly - submitting as diff/patch

Review of attachment 8843834 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks - I previously looked at the reviewboard commit - you should probably work out what went wrong there as using reviewboard is much simpler for reviewers.
Attachment #8843834 - Flags: review?(markh)
Attachment #8843834 - Flags: review+
Attachment #8843834 - Flags: checkin?(ryanvm)
Flags: needinfo?(markh)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c651974bb5ce
Enable sync success logs by default only on Nightly r=markh
Keywords: checkin-needed
Comment to attachment #8843834 [details] [diff] [review]
https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc
Is there anything more to be done or is the bug fixed?
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=81812063&repo=mozilla-inbound
Flags: needinfo?(bharatraghunthan9767)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af18a377c790
Backed out changeset c651974bb5ce for test failures in test_service_migratePrefs.js
Should I modify test_service_migratePrefs.js to test for Nightly or not, or change the code in services-sync.js (like changing order of the prefs etc.)?
Flags: needinfo?(bharatraghunthan9767) → needinfo?(markh)
Thanks for your contribution, and my apologies for not noticing the change would cause test failures.

The test should be changed (even though it's now out of date) - if you add near the top of the file:

Cu.import("resource://gre/modules/AppConstants.jsm");
const LOG_ON_SUCCESS_DEFAULT = AppConstants.NIGHTLY_BUILD;

then change:
do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), false);

to:
do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), LOG_ON_SUCCESS_DEFAULT);

The test will pass. Seeing as it has been backed out once, we really need to run a try build for this, so you will need to work out how to push to mozreview, and we can trigger a test build from that. Please catch us in IRC in the #sync channel if you still have trouble pushing to mozreview.

I also opened bug 1345009 to remove the code that caused the failure, so I guess if you are keen you could take that other bug first, after which no further changes would be needed here :)
Flags: needinfo?(markh)
Pushed to the reviewboard in order for the patch to be landed
Try is failing with an "E" error that indicates your local tree is a week or so old and needs updating - could you please rebase your patches against a current mozilla-central, modify the commit message so it has ". r?markh" at the end, then re-push to reviewboard? You could then push you own try build too (or I'm happy to do that) - I expect try will then pass and I can push it for you. https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F has more info.

Thanks for your patches and your patience!
Please review the MozReview Request of Comment 20 not Comment 19 as it was a mistake.
Flags: needinfo?(markh)
Something strange has gone on with that patch - https://reviewboard.mozilla.org/r/118104/diff/3/ shows only one .patch file. It's not possible to review an earlier iteration of the patch, so I think you need to push a new patch with only the changes and no .patch files.
Flags: needinfo?(markh)
I'm sorry, it was because my local copy of the mozilla-central repo was in a mess with multiple branches suddenly created that may have caused the problem.
I hope it's fine now?
Flags: needinfo?(markh)
Comment on attachment 8844774 [details]
Bug 1332557 - Enable sync success logs by default on Nightly.

https://reviewboard.mozilla.org/r/118104/#review120802

The try run looks good! :)

Can you please change the commit message to "Bug 1332557 - Enable sync success logs by default on Nightly. r?markh" and re-push it, then I'll land it.

Thanks!
Changed the commit message accordingly and re-pushed it. Now please try landing it.
Comment on attachment 8844774 [details]
Bug 1332557 - Enable sync success logs by default on Nightly.

https://reviewboard.mozilla.org/r/118104/#review120964

Thanks!
Attachment #8844774 - Flags: review?(markh) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/b0abf85ea5e1
Enable sync success logs by default on Nightly. r=markh
https://hg.mozilla.org/mozilla-central/rev/b0abf85ea5e1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.