geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Blum <jb...@pivotal.io>
Subject Re: [DISCUSS] Wrapping log calls in Conditionals like isDebugEnabled, isTraceEnabled, etc.
Date Fri, 07 Sep 2018 18:37:06 GMT
Technically, I think that is SLF4J syntax (but maybe Log4J2 supports this
format now as well; anyway).

Still, you should be careful with log statements like...

 logger.debug("Logging in user {}" + user);

Assuming the User class itself and an "informative" and properly
constructed toString() method.  So use it judiciously and wisely.


On Fri, Sep 7, 2018 at 11:18 AM, Dan Smith <dsmith@pivotal.io> wrote:

> I think this pattern is a holdover from using log4j 1. In that version, you
> added an if check to avoid unnecessary string concatenation if debug was
> enabled.
>
>
>    1. if (logger.isDebugEnabled()) {
>    2.     logger.debug("Logging in user " + user.getName() + " with
> birthday " + user.getBirthdayCalendar());
>    3. }
>
>
> Log4j2 lets you pass a format string, so you can just do this:
>
> logger.debug("Logging in user {} with birthday {}", user.getName(),
> user.getBirthdayCalendar());
>
>
> These examples come from the log4j2 docs:
>
> https://logging.apache.org/log4j/2.0/manual/api.html
>
> -Dan
>
> On Fri, Sep 7, 2018 at 10:50 AM, Galen O'Sullivan <gosullivan@apache.org>
> wrote:
>
> > Hi all,
> >
> > I've noticed a pattern in Geode where we wrap a log call in a check at
> the
> > same level, such as:
> >
> >     if (logger.isDebugEnabled()) {
> >           logger.debug("cleaning up incompletely started
> > DistributionManager due to exception", r);
> >         }
> >
> > Is there any reason to do this? To my mind, it's an extra conditional
> that
> > should essentially be the first check inside `logger.debug(...)` anyways,
> > and it complicates the code and makes it less readable. I've even seen
> > places in the code which have `if (logger.isDebugEnabled())
> > logger.trace(...))` and such.
> >
> > I would like to propose that unless there is a compelling reason to use
> > this pattern, we remove all extra checks entirely.
> >
> > Galen
> >
>



-- 
-John
john.blum10101 (skype)

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message