Last Comment Bug 661587 - Don't use the root logger
: Don't use the root logger
Status: VERIFIED FIXED
[verified in services]
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on:
Blocks: Log.jsm
  Show dependency treegraph
 
Reported: 2011-06-02 11:06 PDT by Philipp von Weitershausen [:philikon]
Modified: 2011-08-03 10:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (27.50 KB, patch)
2011-06-09 11:19 PDT, Philipp von Weitershausen [:philikon]
rnewman: review+
Details | Diff | Splinter Review
alternate patch: use a distinct repository (48.11 KB, patch)
2011-06-10 09:07 PDT, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-06-02 11:06:57 PDT
We'd like to uplift log4moz to toolkit at some point (bug 451283). One prerequisite is to get Sync to stop taking control of the root logger.
Comment 1 Philipp von Weitershausen [:philikon] 2011-06-09 11:19:21 PDT
Created attachment 538295 [details] [diff] [review]
v1
Comment 2 Richard Newman [:rnewman] 2011-06-09 12:15:09 PDT
Comment on attachment 538295 [details] [diff] [review]
v1

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

What's this, 23 files? :D

Pleasantly formulaic!
Comment 3 Philipp von Weitershausen [:philikon] 2011-06-10 07:14:01 PDT
Actually, the more I think about it, the more I wonder whether we should perhaps just get rid of the default LoggerRepository (Log4Moz.repository) and make every component that wants to use log4moz create its own logger hierarchy in its own repository. Then we don't need the redundant "Sync." prefix everywhere.
Comment 4 Dave Townsend [:mossop] 2011-06-10 07:16:14 PDT
(In reply to comment #3)
> Actually, the more I think about it, the more I wonder whether we should
> perhaps just get rid of the default LoggerRepository (Log4Moz.repository)
> and make every component that wants to use log4moz create its own logger
> hierarchy in its own repository. Then we don't need the redundant "Sync."
> prefix everywhere.

Would this make it impossible to simply turn on all logging? If so I would be against that.
Comment 5 Philipp von Weitershausen [:philikon] 2011-06-10 07:19:52 PDT
(In reply to comment #4)
> Would this make it impossible to simply turn on all logging? If so I would
> be against that.

What does "all logging" mean? What does "turn on" mean?

Having each component use its own repository would mean there's no single root logger which means you can't just attach a, say, DumpAppender to the one and only root logger to get all logging output from all components. Is that what you want? It doesn't sound very useful to me...
Comment 6 Dave Townsend [:mossop] 2011-06-10 09:04:42 PDT
Being able to turn on logging for everything is often pretty useful if you have no idea what is causing some disk thrashing at a particular time etc. As more things start to use log4moz I'd expect to see something like the error console start to just log things and having custom repositories for everything seems more complex than just having everything use a sensible namespace
Comment 7 Philipp von Weitershausen [:philikon] 2011-06-10 09:07:45 PDT
Created attachment 538532 [details] [diff] [review]
alternate patch: use a distinct repository
Comment 8 Philipp von Weitershausen [:philikon] 2011-06-10 09:08:44 PDT
(In reply to comment #6)
> Being able to turn on logging for everything is often pretty useful if you
> have no idea what is causing some disk thrashing at a particular time etc.
> As more things start to use log4moz I'd expect to see something like the
> error console start to just log things and having custom repositories for
> everything seems more complex than just having everything use a sensible
> namespace

Fair enough. I wonder if we should disallow separate repositories then.
Comment 9 Dave Townsend [:mossop] 2011-06-10 09:10:30 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > Being able to turn on logging for everything is often pretty useful if you
> > have no idea what is causing some disk thrashing at a particular time etc.
> > As more things start to use log4moz I'd expect to see something like the
> > error console start to just log things and having custom repositories for
> > everything seems more complex than just having everything use a sensible
> > namespace
> 
> Fair enough. I wonder if we should disallow separate repositories then.

The repository never seemed like a worthwhile abstraction to me, I commented as such in bug 451283 comment 16
Comment 10 Philipp von Weitershausen [:philikon] 2011-06-10 09:21:28 PDT
(In reply to comment #9)
> The repository never seemed like a worthwhile abstraction to me, I commented
> as such in bug 451283 comment 16

Yup. I was just curious since you were critiquing dissimilarities with Log4J similarities (Formatter v. Layout), and Log4J *does* have repositories.
Comment 11 Dave Townsend [:mossop] 2011-06-10 09:37:47 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > The repository never seemed like a worthwhile abstraction to me, I commented
> > as such in bug 451283 comment 16
> 
> Yup. I was just curious since you were critiquing dissimilarities with Log4J
> similarities (Formatter v. Layout), and Log4J *does* have repositories.

I think that must have been a newer addition since I last used log4J and seems to be an optional feature (in that you can use log4J quite happily by ignoring it).
Comment 12 Philipp von Weitershausen [:philikon] 2011-06-13 11:56:30 PDT
http://hg.mozilla.org/services/services-central/rev/0122402eba05
Comment 13 Philipp von Weitershausen [:philikon] 2011-06-13 12:06:45 PDT
STRs for QA:

The Sync log (or logs, since bug 610832) should all look like this:

1307991868256	Sync.Service	INFO	Loading Weave 1.10.0
1307991868309	Sync.Engine.Bookmarks	DEBUG	Engine initialized
1307991868313	Sync.Engine.Forms	DEBUG	Engine initialized
1307991868316	Sync.Engine.History	DEBUG	Engine initialized
1307991868320	Sync.Engine.Passwords	DEBUG	Engine initialized
1307991868323	Sync.Engine.Prefs	DEBUG	Engine initialized
1307991868326	Sync.Engine.Tabs	DEBUG	Engine initialized
...

Notice the "Sync." prefix before each component.
Comment 14 Tracy Walker [:tracy] 2011-06-14 08:34:49 PDT
verified with latest s-c builds
Comment 15 Philipp von Weitershausen [:philikon] 2011-06-15 02:33:46 PDT
http://hg.mozilla.org/mozilla-central/rev/0122402eba05

Note You need to log in before you can comment on or make changes to this bug.