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);

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");

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

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.

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

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

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

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

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.

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

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]

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