Hi devs,
SonarCloud raises java:S2629 on logging calls whose parameters need evaluation, e.g. this.logger.warn("Failed to resolve the entity [{}]. Cause: [{}]", reference, getRootCauseMessage(e)). The rule skips calls inside a catch block or inside a level guard, so what’s left is mostly warn()/error() calls outside a catch, plus debug() calls without a guard.
We have 4 possible solutions:
- No guard for the levels enabled by default in the logback config we ship (
info/warn/error), since the evaluation happens anyway; use@SuppressWarnings("java:S2629")in the code to record the decision. - Add an
if (this.logger.isXxxEnabled())guard whenever a parameter requires evaluation. - Use the SLF4J fluent API (
atWarn().setCause(e).addArgument(...)) so the arguments are supplied lazily, at least when there’s an evaluation. See PR #5798 from @mleduc, still open. - Disable
java:S2629for our projects.
My opinions: +1 for 2 (it’s really not a big deal and it’s useful if the admin changes the logging levels), +0 for 1 (it adds cruft in the code), +0 for 3 (I don’t like having several ways of doing the same thing — I’d be fine if we agreed to use only the fluent API, but that’s a lot of changes and I find it less readable), and -0 (close to -1) for 4.
Full disclosure: solution 1 is currently documented on JavaCodeStyle (“Do not add a level guard around a warn() or an error() call…”). I added it myself on 2026-08-03 and that was too hasty: we never really discussed it, hence this brainstorming. Some of it was discussed on the Matrix channel on 2026-07-09 following PR #5798, where @tmortagne, @mleduc and @surli all expressed reservations about applying the rule at all, but we never concluded. Whatever we decide here, I’ll update the page accordingly.
What triggered this is PR #6379, which applies solution 1 to 10 sites and which I’d rather not merge before we agree.
WDYT?
Thanks