I think that logging in hot paths/loops is probably something that should be avoided. And if it is necessary, I suspect that the JIT will short-circuit that call since debug levels don't generally change.
There probably are a few hot paths that need to optimize logging, but that's not the majority. Can we agree to avoid this pattern in new code, since it adversely affects readability? Galen On Fri, Sep 7, 2018 at 1:46 PM, Nicholas Vallely <nvall...@pivotal.io> wrote: > 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 > > > > > > > > > >
