Is checking logger.isTraceEnabled() before logging a trace message useless?

Development Add-ons
, ,
1

Consider the following snippet:

if (logger.isTraceEnabled()) {
    logger.trace("{} {}", telegram.stateDetails(), telegram.toString());
    }

According to Slf4j FAQ when parameterized logging is used there’s no need to separately call isTraceEnabled(). As the title states: Is it useless?

While digging through logging I noticed that openHAB core depends on log4j-1.2.17 which came EOL almost 5 years ago. What might be the reason?

2

I agree. There is no need to check isTranceEnabled() or similar before logging something unless one of the logged parameters requires a costly calculation (e.g. if it is a method) or you exclude a block of logging statements. Our guidelines tells you to use parameterized logging statements which are good enough.

OHC uses slf4j for logging. log4j is used in one place and will be removed in OH3.

1 Like
3

Thanks for your answer.

My choice of words was bad. Didn’t mean to say that core depends on it or uses it. More like I was wondering why such an old package is listed as dependency.

4

I’m going to give you another perspective and why I disagree that it’s unnecessary. Let’s go with what both you and @cweitkamp say and eliminate the isTraceEnabled() part - so all you have is the logger.trace("{} {}", telegram.stateDetails(), telegram.toString()).

Now - give it a few years or maybe someone else decides to start working on some enhancements to the bundle and a modification to the stateDetails method makes it a very costly call. I can guarantee you that nobody (yourself included if done with sufficient time passing) will remember or give the call a second look at this. Now suddenly you are doing something costly (but probably not costly enough to really notice at the time) for something that isn’t enabled.

These are the types of (future) errors nobody thinks about and how things creep into code all the time.

If you’re calling a method on a parameter of a logging statement, you should be explicit with your code on whether that method should be called by having the isXXXEnabled plus your code is descriptive of your intent that way. Having the if is a good marker to future coders saying “i’m doing something different here - pay more attention to it” (which you don’t get if it’s a single logging statement - which we tend to overlook or ignore).

1 Like