1073027 - Control MozLoopService logging level with a preference

Status: RESOLVED FIXED

(Hello (Loop) :: Client, enhancement)

Description

[Matthew N. [:MattN]]

Attachment: v.1 Use maxLogLevel of ConsoleAPI

Being able to control the log level with a preference allows developers to keep more logging in the code without flooding the Browser Console for non-Loop developers.

From now on, logging in MozLoopService should use log.* instead of console.* methods.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1.1 Use maxLogLevel of ConsoleAPI.

Remove leftover unused import of Log.jsm.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

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

::: browser/components/loop/MozLoopService.jsm
@@ +70,5 @@
> +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> +XPCOMUtils.defineLazyGetter(this, "log", () => {
> +  let ConsoleAPI = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).ConsoleAPI;
> +  let level = Services.prefs.getPrefType(PREF_LOG_LEVEL) == Ci.nsIPrefBranch.PREF_STRING &&
> +                Services.prefs.getCharPref(PREF_LOG_LEVEL);

Why do we need to do a check here to make sure that this value is a string? There are plenty of places in our codebase that we call getCharPref before checking that the type of the preference is a string.

@@ +1244,5 @@
>    setLoopCharPref: function(prefName, value) {
>      try {
>        Services.prefs.setCharPref("loop." + prefName, value);
>      } catch (ex) {
> +      log.error("setLoopCharPref had trouble setting " + prefName +

This is a change from console.log (info) to log.error, which, given the duties of this code seems right. We shouldn't be failing to set a pref, and if we do we should log that prominently. But I just wanted to point this out in case you did this on accident.

Comment 3

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Comment on attachment 8495346 [details] [diff] [review]
> v.1.1 Use maxLogLevel of ConsoleAPI.
> 
> Review of attachment 8495346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/MozLoopService.jsm
> @@ +70,5 @@
> > +// Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
> > +XPCOMUtils.defineLazyGetter(this, "log", () => {
> > +  let ConsoleAPI = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).ConsoleAPI;
> > +  let level = Services.prefs.getPrefType(PREF_LOG_LEVEL) == Ci.nsIPrefBranch.PREF_STRING &&
> > +                Services.prefs.getCharPref(PREF_LOG_LEVEL);
> 
> Why do we need to do a check here to make sure that this value is a string?
> There are plenty of places in our codebase that we call getCharPref before
> checking that the type of the preference is a string.

That was because I was considering making it a hidden pref like some other modules did so I didn't want to get an exception if the pref wasn't defined. I decided at least while loop is in active development it makes sense to make the pref visible to make it easier for devs and QE to adjust it. I'll remove the getPrefType stuff.

> @@ +1244,5 @@
> >    setLoopCharPref: function(prefName, value) {
> >      try {
> >        Services.prefs.setCharPref("loop." + prefName, value);
> >      } catch (ex) {
> > +      log.error("setLoopCharPref had trouble setting " + prefName +
> 
> This is a change from console.log (info) to log.error, which, given the
> duties of this code seems right. We shouldn't be failing to set a pref, and
> if we do we should log that prominently. But I just wanted to point this out
> in case you did this on accident.

Yeah, that was intentional so that it shows up at the default log level and because it's an error. I think this can happen if the type of the value is not valid for that set*Pref function. e.g. only letters passed to setIntPref.

Comment 4

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/f5a053aaba52

Thanks

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5a053aaba52

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb0ec049ccd1

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495346 [details] [diff] [review]
v.1.1 Use maxLogLevel of ConsoleAPI.

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.