943998 - Need debug and uri pref for FirefoxAccounts

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Edwin Wong [:edwong]]

We need a way to set the server uri and verbose debugging on device.  For Persona we have:
user_pref("toolkit.identity.uri", "https://firefoxos.anosrep.org");
user_pref("toolkit.identity.debug", true);

Maybe we could have
user_pref("toolkit.fxaccounts.uri", "https://stage.fxa.org");
user_pref("toolkit.fxaccounts.debug", true);

Comment 1

[Zachary Carter [:zaach]]

Bug 935232 adds this pref:

+// The URL of the Firefox Accounts auth server backend
+pref("identity.fxaccounts.auth.uri", "https://api-accounts.dev.lcip.org/v1");

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

The attached patch adds the "identity.fxaccounts.debug" preference to enable/disable debug messages.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8341053 [details] [diff] [review]
v1

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

Is there any reason we can't use Log.jsm instead, similar to how sync does?  Then a pref can control the log level and we will not be restricted to dump() output.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #4)
> Is there any reason we can't use Log.jsm instead, similar to how sync does? 
> Then a pref can control the log level and we will not be restricted to
> dump() output.

See also bug 909967 - this introduces use of Log.jsm, but it currently hard-code a log-level of debug.  Maybe this bug could extend its scope a little to provide a consistent logging interface - eg, using prefs to set each log's level?

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Using Log.jsm sounds good to me. I'll update the patch soon. Thanks Mark!

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

With this patch, we add an "identity.fxaccounts.loglevel" preference that takes one of the string values at [1]. We will be logging in the console and if no preference is provided we will only log error messages.

Mark, is this what you were referring to?

I still need to rebase on top of bug 909967, check all the log messages and probably add three different levels (error, debug, info), but I would love to hear your feedback about the general approach (basically the changes in FxAccountsCommon.js).

Thanks!

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm#45

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8342480 [details] [diff] [review]
v2

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

::: services/fxaccounts/FxAccounts.jsm
@@ +101,5 @@
>      return d.promise;
>    },
>  
>    getCertificate: function(data, keyPair, mustBeValidUntil) {
> +    log.debug("getCertificate " + internal.signedInUserStorage);

also note the logger takes a second param, and object that will be JSON.stringified and appended to the end of the message - so something like:

  log.debug("getCertificate:", {signedInUserStorage: internal.signedInUserStorage});

might be appropriate in some cases.

::: services/fxaccounts/FxAccountsCommon.js
@@ +9,5 @@
> +
> +// loglevel preference should be one of: "FATAL", "ERROR", "WARN", "INFO",
> +// "CONFIG", "DEBUG", "TRACE" or "ALL". We will be logging error messages by
> +// default.
> +const PREF_LOG_LEVEL = "identity.fxaccounts.loglevel";

you probably shouldn't use LOG_LEVEL - allcaps implies a const, which this isn't.

Also, I'm a little surprised this works - I thought the level needed to be an integer.  Sync does:

this._log.level = Log.Level[pref_value];

which relies on the Log module's 'Level' object which maps strings to values.

But in general, yeah, this is what I meant.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8342480 [details] [diff] [review]
v2

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

::: services/fxaccounts/FxAccountsCommon.js
@@ +11,5 @@
> +// "CONFIG", "DEBUG", "TRACE" or "ALL". We will be logging error messages by
> +// default.
> +const PREF_LOG_LEVEL = "identity.fxaccounts.loglevel";
> +try {
> +  this.LOG_LEVEL =

actually, another thought is that maybe this could go into a:

XPCOMUtils.defineLazyModuleGetter(this, 'log', function() {
 // read the pref, create the log, set the level and create the formatter
});

which should mean we don't pay any penalty for the preference read etc at import time, but only the first time we try and log something.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Sorry for the late update on this. I've been traveling and busy with other issues.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Thanks again for the feedback Mark :)

(In reply to Mark Hammond [:markh] from comment #8)
> Comment on attachment 8342480 [details] [diff] [review]
> v2
> 
> Review of attachment 8342480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +101,5 @@
> >      return d.promise;
> >    },
> >  
> >    getCertificate: function(data, keyPair, mustBeValidUntil) {
> > +    log.debug("getCertificate " + internal.signedInUserStorage);
> 
> also note the logger takes a second param, and object that will be
> JSON.stringified and appended to the end of the message - so something like:
> 
>   log.debug("getCertificate:", {signedInUserStorage:
> internal.signedInUserStorage});
> 
> might be appropriate in some cases.
> 

For some reason, I couldn't make this work, so I had to manually do the JSON.stringify.

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

FWIW, I had the same problem with log and the second json arg.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8346093 [details] [diff] [review]
v2

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

Looks fine to me.  I suspect the logs might end up too noisy and might later need finer granularity, but we can deal with that as needed.

[Turns out that second param to the log functions is only used when a StructuredFormatter is used, which seems a little dumb, but there you have it]

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Thanks Mark!

https://hg.mozilla.org/integration/b2g-inbound/rev/b3d70eda988d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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