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 > 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 ? > > * Users can display meaningful information without using MdcInjectionFilter. > * Advanced users can disable the default prefix very easily and use > MdcInjectionFilter. > * 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 :-) Maarten > > Cheers, > Trustin > -- > what we call human nature is actually human habit > -- > http://gleamynode.net/ > -- > PGP Key ID: 0x0255ECA6 >
