Hi, Comments inline.
On Fri, 22 Aug 2008 22:47:32 +0200
Emmanuel Lecharny <[EMAIL PROTECTED]> wrote:
> Hi guys,
>
> I think there is something wrong with the current implementation of
> the LogFilter.
Sure it's a bit over complicated, specially that Map for IoEvent, due
to the fact we aren't adding an event every days.
>
> The idea is to be able to log something _if_ a specific event is set
> to a certain level. For instance, one may log as debug when a
> MessageReceived event is received, but only log errors when a
> MessageSent event is received.
>
> So when you initialize a logLevel, you inject two arguments :
> - the eventType
> - the desired log level
>
> public void setLogLevel(IoEventType eventType, LogLevel logLevel)
> {
>
> The problem with this approach is that the way it's implemented,
Really ? I start to think after chatting with you a little , that the
problem is trying to set log level with MINA. It's obviously log
framework job.
> there is no way to get some logs for two different event with two
> different log level, simply because there is only one Logger which is
> used globally, and set to a level globally, too.
>
> So we ned to fix that, and I'm wondering which is the best approach.
> - we can simply consider that this is totally useless to tie an event
> to a log level, and just log all events to the same level (pretty
> much what we currently have right now).
> - or we consider that the original idea is ok, but then we have to
> create a logger per event.
>
> In both case, I think that the utter complexity of the current
> implementation need to be simplified a lot. We don't need a map to
> store the relation between an event and a logLevel, as the log level
> will be implicit if we have one logger per event (or implicitly set
> to the global logger if we chose option #1), we don't need a LogLevel
> enum either, as the implementation is straightforward in both case :
>
> Option #1 :
>
> public void messageSent(NextFilter nextFilter, IoSession session,
> WriteRequest writeRequest) throws Exception {
> if (logger.isDebugEnabled()) {
> logger.debug("SENT: {}", writeRequest.getMessage());
> } else if (logger.isInfoEnabled()) {
> logger.info("SENT: {}", writeRequest.getMessage());
> } else if (logger.isTraceEnabled()) {
> logger.trace("SENT: {}", writeRequest.getMessage());
> } else if (logger.isWarnEnabled()) {
> logger.warn("SENT: {}", writeRequest.getMessage());
> } else if (logger.isErrorEnabled()) {
> logger.error("SENT: {}", writeRequest.getMessage());
> }
>
> nextFilter.messageSent(session, writeRequest);
> }
>
> Option #2 :
>
> public void messageSent(NextFilter nextFilter, IoSession session,
> WriteRequest writeRequest) throws Exception {
> if (logger.isDebugEnabled()) {
> loggerMessageSent.debug("SENT: {}",
> writeRequest.getMessage()); } else if (logger.isInfoEnabled()) {
> loggerMessageSent.info("SENT: {}",
> writeRequest.getMessage()); } else if (logger.isTraceEnabled()) {
> loggerMessageSent.trace("SENT: {}",
> writeRequest.getMessage()); } else if (logger.isWarnEnabled()) {
> loggerMessageSent.warn("SENT: {}",
> writeRequest.getMessage()); } else if (logger.isErrorEnabled()) {
> loggerMessageSent.error("SENT: {}",
> writeRequest.getMessage()); }
>
> nextFilter.messageSent(session, writeRequest);
> }
loggerSent.trace(writeRequest.getMessage());
no need SENT because it's already the sending logger.
>
> FYI, the current implemention is :
[..snip..]
Can you paste that somewhere else for my poor eyes ;)
LoggingFilter is a debug tool, in no production application you will
log all the sent/rcvd message.
The only usage could be exception & connec/disconnet
So I think we could :
hardcode created/sent/rcvd log level to trace/debug which is always
used for debug
hardcode close/connect to info
hardcode exception to error
use a logger per event type, so the final user use log4j/wathever
facilities to change the log level for hide/show messages, and not
configure the LoggingFilter for changing the loglevel to the hidden
one :)
> pretty complicated, IMHO :)
>
> Thoughts ?
>
and with that we provide a LogLevel class, sound pretty weirdo :)
Those changes are pretty radical, let's wait for some feedback on how
other users (ab)use the current implementation.
Julien
signature.asc
Description: PGP signature
