Consider enabling sync success logs by default on Nightly

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
P2
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: markh, Assigned: Bharat Raghunathan, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

7 months ago
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 ;)

Updated

7 months ago
Priority: -- → P2
(Reporter)

Updated

6 months ago
Mentor: markh@mozilla.com
Keywords: good-first-bug

Comment 1

6 months ago
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)
(Assignee)

Comment 3

6 months ago
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)
(Assignee)

Comment 4

6 months ago
Created attachment 8843560 [details] [diff] [review]
Enabled sync success logs by default only on Nightly
Attachment #8843560 - Flags: feedback?(markh)
(Reporter)

Comment 5

6 months ago
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+
(Reporter)

Updated

6 months ago
Assignee: nobody → bharatraghunthan9767
Keywords: checkin-needed
This needs to be in diff format to be landed.
Keywords: checkin-needed
(Assignee)

Comment 7

6 months ago
Created attachment 8843834 [details] [diff] [review]
Enabled sync success logs by default only on Nightly - submitting as diff/patch
Attachment #8843834 - Flags: review?(markh)
Attachment #8843834 - Flags: checkin?(ryanvm)
(Assignee)

Comment 8

6 months ago
Should I make obsolete the previous attachment #8843560 [details] [diff] [review] or leave it as it is?
Flags: needinfo?(markh)
(Reporter)

Comment 9

6 months ago
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)
(Reporter)

Updated

6 months ago
Flags: needinfo?(markh)
Keywords: checkin-needed

Comment 10

6 months ago
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
(Assignee)

Comment 11

6 months ago
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)

Comment 13

6 months ago
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
(Assignee)

Comment 14

6 months ago
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)
(Reporter)

Comment 15

6 months ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

5 months ago
Pushed to the reviewboard in order for the patch to be landed
(Reporter)

Comment 18

5 months ago
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!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

5 months ago
Please review the MozReview Request of Comment 20 not Comment 19 as it was a mistake.
Flags: needinfo?(markh)
(Reporter)

Comment 22

5 months ago
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

5 months ago
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)
(Reporter)

Comment 25

5 months ago
mozreview-review
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!
Comment hidden (mozreview-request)
(Assignee)

Comment 27

5 months ago
Changed the commit message accordingly and re-pushed it. Now please try landing it.
(Reporter)

Comment 28

5 months ago
mozreview-review
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+

Comment 29

5 months ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/b0abf85ea5e1
Enable sync success logs by default on Nightly. r=markh

Comment 30

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0abf85ea5e1
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
(Assignee)

Updated

5 months ago
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.