Need a single pref to toggle logging

VERIFIED FIXED

Status

Cloud Services
Firefox Sync: Backend
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: mconnor, Assigned: mfinkle)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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.

Comment 1

7 years ago
this hurts alot on mobile.
Created attachment 482159 [details] [diff] [review]
patch

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?
(Assignee)

Updated

7 years ago
Attachment #482159 - Flags: review? → review?(mconnor)
(Reporter)

Comment 3

7 years ago
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+
Created attachment 482249 [details] [diff] [review]
patch for add-on

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.
(Reporter)

Updated

7 years ago
Attachment #482249 - Flags: review?(mconnor) → review+

Updated

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 603388
verified with recent nightly builds of minefield
Status: RESOLVED → VERIFIED

Updated

7 years ago
Blocks: 607457
You need to log in before you can comment on or make changes to this bug.