I agree: the current LoggingFilter is pretty complicated. I looked at it in
the past but found it too complex to use it.
I even think we could just get rid of it and let our users write a
customized LoggingFilter when they need one.
But what Julien suggest also looks OK to me.
Regards,
Maarten
On Mon, Aug 25, 2008 at 9:39 AM, Julien Vermillard
<[EMAIL PROTECTED]>wrote:
> 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
>
