Closed Bug 760906 Opened 9 years ago Closed 9 years ago

Enable logging with flag

Categories

(Web Apps Graveyard :: AppsInTheCloud, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: anant, Assigned: anant)

Details

(Whiteboard: [blocking-aitc+], [qa!])

Attachments

(1 file)

We have a bunch of _log.info's in the code. Evaluate if we should log to disk only on error and verify that our logging levels are sane and don't hamper performance.
OS: Linux → All
Hardware: x86 → All
(In reply to Anant Narayanan [:anant] from comment #0)
> We have a bunch of _log.info's in the code. Evaluate if we should log to
> disk only on error and verify that our logging levels are sane and don't
> hamper performance.

Might be better to generally to go with an approach that disables logging on a production build by default, but allow it to be turned on through a pref. The average typical user does not care about the logs that are being produced by the AITC code, so having it enabled by default probably only brings drawbacks than benefits.

If we go with a pref-based approach, then I'll be a little less concerned about the perf impacts the logging may cause, given that only testers will be impacted by this, not typical users. The only cases where it would be a concern is in cases where hangs occur (i.e. huge perf slowdown).

Thoughts?
I haven't noticed production level logs getting logged on Nightly with the default prefs right now. Am I looking in the right spot? If not, then if the logs don't appear by default, then this bug should be resolved as invalid, as there is no end user impact to this unless a tester explicitly sets the logging parameters.
Summary: Verify that the disk I/O levels for AITC logging are sane → Enable logging with flag
Whiteboard: [blocking-aitc]
Whiteboard: [blocking-aitc] → [blocking-aitc+]
I went through a few different iterations of this patch, tried a FileAppender (discouraged because of blocking I/O), and a StorageAppender (more work because we would have to write to file on a timer or some other checkpoints).

Settled on using a DumpAppender, the main intention at this stage is simply to enable logging for QA purposes and advanced users. Currently, there is no logging at all.
Assignee: nobody → anant
Status: NEW → ASSIGNED
Attachment #632203 - Flags: review?(gps)
Comment on attachment 632203 [details] [diff] [review]
Log to DumpAppender - v1

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

r+ with pref name change.

::: services/aitc/services-aitc.js
@@ +1,2 @@
> +// Root logger
> +pref("services.aitc.log", false);

I'd make it services.aitc.log.appender.dump since "log" is ambiguous.

@@ +20,5 @@
>  pref("services.aitc.marketplace.url", "https://marketplace.mozilla.org");
>  
>  pref("services.aitc.service.log.level", "Debug");
>  
> +// Temporary value. Change to the production server (bug 760903)

Can you add a "TODO" here so this shows up in grep?
Attachment #632203 - Flags: review?(gps) → review+
https://hg.mozilla.org/services/services-central/rev/7d0a38909321
Whiteboard: [blocking-aitc+] → [blocking-aitc+], [fixed in services]
https://hg.mozilla.org/mozilla-central/rev/7d0a38909321
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [blocking-aitc+], [fixed in services] → [blocking-aitc+]
Whiteboard: [blocking-aitc+] → [blocking-aitc+], [qa+]
Verified on Mac and Windows.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-aitc+], [qa+] → [blocking-aitc+], [qa!]
Product: Web Apps → Web Apps Graveyard
You need to log in before you can comment on or make changes to this bug.