Closed Bug 943998 Opened 6 years ago Closed 6 years ago

Need debug and uri pref for FirefoxAccounts

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)

People

(Reporter: edwong, Assigned: ferjm)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 2 obsolete files)

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);
Depends on: 935245
No longer depends on: 935245
Depends on: 929386
Whiteboard: [qa+]
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");
Assignee: nobody → ferjmoreno
Attached patch v1 (obsolete) — Splinter Review
Attachment #8341053 - Flags: review?(mhammond)
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.
Attachment #8341053 - Flags: review?(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?
Using Log.jsm sounds good to me. I'll update the patch soon. Thanks Mark!
Attached patch v2 (obsolete) — Splinter Review
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
Attachment #8341053 - Attachment is obsolete: true
Attachment #8342480 - Flags: feedback?(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.
Attachment #8342480 - Flags: feedback?(mhammond) → feedback+
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.
Attached patch v2Splinter Review
Sorry for the late update on this. I've been traveling and busy with other issues.
Attachment #8342480 - Attachment is obsolete: true
Attachment #8346093 - Flags: review?(mhammond)
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]
Attachment #8346093 - Flags: review?(mhammond) → review+
Depends on: 949526
No longer depends on: 929386
https://hg.mozilla.org/mozilla-central/rev/b3d70eda988d
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
You need to log in before you can comment on or make changes to this bug.