On 10/15/07, Maarten Bosteels <[EMAIL PROTECTED]> wrote: > Hi Trustin, > > On 10/15/07, Trustin Lee <[EMAIL PROTECTED]> wrote: > > Hi Maarten, > > > > On 10/10/07, Maarten Bosteels <[EMAIL PROTECTED]> wrote: > > > > > Trustin, maybe we should reopen the discussion about removing > > > > > IoSessionLogger > > > > > in favour of MdcInjectionFilter :-) > > > > > > > > Well, there's at least one person who's using java.util.logging in the > > > > mailing list, and there should be more people who's not in here. If > > > > we are going to drop good support for JUL, what is the use of SLF4J? > > > > > > Well, SLF4J gives people choice and it still would, people could still > > > choose > > > any of the supported logging systems, BUT when the logging framework > > > of their choice doesn't support MDC, they wouldn't see the > > > remoteAddress in their logs. > > > But ok, I can understand that you find that unacceptable. > > > > > > On the other hand, I would prefer if logging events generated by > > > ProtocolCodecFilter > > > would have org.apache.mina.filter.codec.ProtocolCodecFilter as their > > > logger. > > > I know this is not required but it is a very widely used convention. > > > > Very true. We need to provide an easy easy way to customize the behavior. > > > > > I thought of another solution: > > > > > > * the MdcInjectionFilter stores the remoteAddress in a ThreadLocal > > > * we create a decorator for org.slf4j.Logger that retrieves the > > > remoteAddress from the ThreadLocal and prepends it to the log-message > > > (optionally of course, only needed when the logging framework doesn't > > > support MDC) > > > * all mina-classes use this decorator instead of IoSessionLogger > > > > > > I haven't tried this idea, but I think it would work ok. > > > Normally, I am wary of using ThreadLocal's but for logging I consider > > > it acceptable. > > > > Wouldn't ThreadLocal cause some kind of bottleneck in a high performance > > server? > > I don't think so, but a benchmark with/without mdcInjectionFilter > could be interesting of course. > > Since jdk 1.3 there is no synchronization needed for ThreadLocal, > see this article by Brian Goetz: > > http://www.ibm.com/developerworks/java/library/j-threads3.html > > see also: > http://www.ibm.com/developerworks/forums/dw_thread.jsp?message=13929885&cat=10&thread=155193&treeDisplayType=threadmode1&forum=181#13929885
Uh, i didn't know that. Thanks for the education! :D > > I'm in favor of doing the following: > > > > * Make IoSessionLoggger implement SLF4J Logger > > * Add IoSessionLogger.enableDefaultPrefix (true by default) static property > > * A user will create a logger by calling 'new > > IoSessionLogger(org.slf4j.Logger)'. > > * IoSessionLogger forwards all calls to the underlying SLF4J logger if > > enableDefaultPrefix is false. Otherwise, IoSessionLogger instance > > prefixes the default prefix for all messages. > > > > WDYT? By doing this, we can incorporate all the suggested changes we > > discussed: > > Looks good ! > I assume you mean that IoSessionLogger would *always* delegate to the > underlying SLF4J logger, but only prepend the prefix when > enableDefaultPrefix is true ? Yeah exactly. > > * IoSessionLogger will implement SLF4J Logger, so it's much more flexible. > > > > > In fact, I think this functionality could be added to SLF4J to make > > > all SLF4J implementations MDC-capable. I'll ask Ceki what he thinks > > > about it. > > > > Exactly. I saw your message in the SLF4J mailing list, though I felt > > like Ceki is not that interested in your suggestion. :( > > Well, that wasn't my impression. I was happy that he responded so quickly and > was willing to add MDC support to SLF4J. > I had the impression he liked the idea. We'll see when I provide the patches > :-) Yeah he liked the idea but he didn't look like he want to implement it by himself. :) Cheers, Trustin -- what we call human nature is actually human habit -- http://gleamynode.net/ -- PGP Key ID: 0x0255ECA6
