661587 - Don't use the root logger

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

Attachment: v1

Comment 2

[Richard Newman [:rnewman]]

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

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

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

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

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

Attachment: alternate patch: use a distinct repository

Comment 8

[Philipp von Weitershausen [:philikon]]

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

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

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

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

http://hg.mozilla.org/services/services-central/rev/0122402eba05

Comment 13

[Philipp von Weitershausen [:philikon]]

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

verified with latest s-c builds

Comment 15

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/0122402eba05