760906 - Enable logging with flag

Status: VERIFIED FIXED

(Web Apps Graveyard :: AppsInTheCloud, defect)

Description

[Anant Narayanan [:anant]]

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.

Comment 1

[Jason Smith [:jsmith]]

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

Comment 2

[Jason Smith [:jsmith]]

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.

Comment 3

[Anant Narayanan [:anant]]

Attachment: Log to DumpAppender - v1

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.

Comment 4

[Gregory Szorc [: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?

Comment 5

[Anant Narayanan [:anant]]

https://hg.mozilla.org/services/services-central/rev/7d0a38909321

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/7d0a38909321

Comment 7

[Jason Smith [:jsmith]]

Verified on Mac and Windows.