Closed Bug 599928 Opened 9 years ago Closed 9 years ago

Need a single pref to toggle logging

Categories

(Firefox :: Sync, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
fennec 2.0b2+ ---

People

(Reporter: mconnor, Assigned: mfinkle)

References

Details

Attachments

(2 files)

There's no single, simple way to turn logging off and/or not touch disk at all.  We do a lot of I/O on the main thread, and we should probably be smarter about the defaults here.  I'm going to propose that we default to not logging anything, except for the add-on, which is our primary dev channel.  Users experiencing problems should be able to flip the pref easily and get details back.
this hurts alot on mobile.
Attached patch patchSplinter Review
This patch uses a pref to enable the file-based logging in Sync. The setting defaults to enabled, so no changes are needed for desktop Firefox. Fennec can add the pref to its mobile.js, or we can add a services pref file.

Using this patch, I timed the initial login durations on my N900, using the "weave:service:login:start" and "weave:service:login:finish" notifications as markers:

with file logging:    1987ms (avg of 5 runs)
without file logging: 1357ms (avg of 5 runs)

Looks like we could save ~630ms on N900 with this patch.

Note: subsequent logins show only very small, if any, improvements. The only real improvements can during initial login.

I could do some actual sync duration timing too, but I'm not sure how comparable syncs are to each other.
Assignee: mconnor → mark.finkle
Attachment #482159 - Flags: review?
Attachment #482159 - Flags: review? → review?(mconnor)
Comment on attachment 482159 [details] [diff] [review]
patch

I would like this defaulted to false in this code, and set to true in http://mxr.mozilla.org/services-central/source/fx-sync/addon/AddonGlue.js#62 so that it's only enabled for desktop users with the add-on, but not for users of built-in sync.

r=me with those changes
Attachment #482159 - Flags: review?(mconnor) → review+
Attached patch patch for add-onSplinter Review
Patch to enable logging in the add-on
Attachment #482249 - Flags: review?(mconnor)
Comment on attachment 482159 [details] [diff] [review]
patch

>+    let enabled = true;
>+    try {
>+      enabled = Svc.Prefs.get("log.appender.debugLog.enabled");
>+    } catch(e) {}

This can be written as

  let enabled = Svc.Prefs.get("log.appender.debugLog.enabled", true)

Svc.Prefs never throws but lets you provide fallback values if the pref doesn't exist. The fallback should probably be "false" here as per mconnor's review comments.
Attachment #482249 - Flags: review?(mconnor) → review+
tracking-fennec: --- → 2.0b2+
(In reply to comment #5)
> Comment on attachment 482159 [details] [diff] [review]
> patch
> 
> >+    let enabled = true;
> >+    try {
> >+      enabled = Svc.Prefs.get("log.appender.debugLog.enabled");
> >+    } catch(e) {}
> 
> This can be written as
> 
>   let enabled = Svc.Prefs.get("log.appender.debugLog.enabled", true)
> 
> Svc.Prefs never throws but lets you provide fallback values if the pref doesn't
> exist. The fallback should probably be "false" here as per mconnor's review
> comments.

Thanks! Changing for checkin:

let enabled = Svc.Prefs.get("log.appender.debugLog.enabled", false);
pushed both patches into http://hg.mozilla.og/services/fx-sync with nits fixed:
http://hg.mozilla.org/services/fx-sync/rev/92c6ee7d6618

The next fx-sync merge will move this into m-c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
verified with recent nightly builds of minefield
Status: RESOLVED → VERIFIED
Blocks: 607457
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.