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:38:41 GMT
Grrr, meant...

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



On Fri, Sep 7, 2018 at 11:37 AM, John Blum <jblum@pivotal.io> wrote:

> 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)
>



-- 
-John
john.blum10101 (skype)

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