I was just randomly looking into this a couple of days ago as a tangent to
'observability' and came across this Stack Overflow:
https://stackoverflow.com/questions/6504407/is-there-a-need-to-do-a-iflog-isdebugenabled-check
where the first answer is the most succinct in my opinion.
In the geode code that I searched, we are not consistent with our use of
the if(statement) wrapper for debug, though most do have the wrapper.
Nick
On Fri, Sep 7, 2018 at 11:35 AM Jacob Barrett <jbarr...@pivotal.io> wrote:
> Checking with logger.isDebugEnabled() it still a good practice for hot
> paths to avoid the construction of intermediate object arrays to hold the
> var-args. Some logging libraries have fixed argument optimizations for
> var-args up to a specific limit. In very hot paths it is nice to
> check logger.isDebugEnabled() once into a temp boolean value and then check
> that in all the subsequent debug logging statements in the hot path.
>
> -Jake
>
>
> On Fri, Sep 7, 2018 at 11:18 AM Dan Smith <dsm...@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 <gosulli...@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
> > >
> >
>
