Hi, Some comments:
1) I understand your worries about performance impact but I think we should to a little benchmark before jumping to conclusions. Maybe you already did ? 2) I was planning to create an Enum of all the keys that MdcInjectionFilter supports (remoteAddress, remotePort,...) and add a constructor that takes an EnumSet, that way users can minimize the size of the map. 3) Currently, when using the MdcInjectionFilter, all logging events generated by code down the call-stack (or is it up ?) will also have the MDC set, even when that code is completely unwaware of MINA. That's a big advantage IMO. And AFAICS this can only be done using a ThreadLocal ? I think the solution we discussed previously, is reasonable: 1) IoSessionLogger constructor takes an org.slf4j.Logger and an IoSession 2) use a session attribute " IoSessionLogger.usePrefix" that determines if a prefix should be prepended 3) IoSessionLogger forwards all messages to the org.slf4j.Logger and optionally prepends a prefix * people using the MdcInjectionFilter will see the remoteAddress for ALL log messages * people not using MdcInjectionFilter will only see the remoteAddress for log messages generated via IoSessionLogger On the other hand, when performance impact of MdcInjectionFilter is really small, and when MDC support is added for JUL I think we could just drop IoSessionLogger, as you wrote on the slf4j mailing list. What do the other MINA users think ? Anybody wants to do a little benchmark ? Maarten On 10/23/07, Trustin Lee <[EMAIL PROTECTED]> wrote: > > Hi, > > As you know MDC (Map backed Diagnostic Context IIRC) for all modern > logging frameworks use ThreadLocal. I think this is a big headache > for MINA because MINA doesn't work very well with ThreadLocal. > Looking at MdcInjectionFilter, we have to copy all map data for each > event emission because a session spans over multiple threads. This is > inefficient and can lead to performance issues. > > I thought about fixing this 'impedance mismatch' between ThreadLocal > and MINA, but didn't success to come up with a good answer yet. > > At least for logging frameworks, we could resolve this problem if all > logging frameworks provides SPI for MDC, so we can directly get > attributes from session attribute map, which is much more efficient > than using the current MDC implementation. However, the reality is > that no logging framework provides such facility. > > Do we need SDC (Session backed Diagnostic Context) here? I think yes. > > As for Log4J, you might be able to achieve that by extending > PatternLayout and PatternParser and let users use our PatternLayout > class in their log4j.properties (or .xml). > > Logback looks like it provides some extension mechanism somehow, but I > didn't look into it much in detail. java.util.logging needs somewhat > more work to do IMO. > > One problem is how the Logger instance is created. Because we have to > associate a session with a logger, we have to provide a wrapper class > for an existing SLF4J Logger instance: > > Logger logger = new > IoSessionLogger(LoggerFactory.getLogger(ABC.class), session); > > The next problem is to access the associated session from each logging > framework's implementation. It's impossible AFAIK unfortunately > because SLF4J just forwards the logging calls other than providing a > mean to pass additional information to the actual logging framework > being used. > > Possible workarounds are: > > 1) Change existing logging frameworks. (impossible probably) > 2) Provide additional pattern layout configuration in IoSessionLogger, > which is configurable via a constructor parameter of IoSessionLogger. > And of course with the default global layout. (somewhat ugly) > > WDYT? Any better ideas? > > Trustin > -- > what we call human nature is actually human habit > -- > http://gleamynode.net/ > -- > PGP Key ID: 0x0255ECA6 >
