qa: address PMD GuardLogStatement warnings

[soloturn]

using fluent logger avoids evaluating methods, and fixes the PMD warning: log not surrounded by "if". this is in case the argument to a parameterized logger is a method call, like here:

- logger.error("onAudioEnd() notification failed for {}", entry.getValue(), e);
+ logger.atError().addArgument(() -> entry.getValue()).addArgument(e).log("onAudioEnd() notification failed for {}");

Edit by niruandaleth/jdrueckert:
PMD warnings were fixed in different ways, among others the following variant of the fluent API which is less verbose than the one originally proposed (see above):

- logger.error("onAudioEnd() notification failed for {}", entry.getValue(), e);
+ logger.atError().log("onAudioEnd() notification failed for {}", entry.getValue(), e);

[BenjaminAmos]

This might take a while to review (92 files changed) but at a cursory glance there's a few questions that occurred to me about this:

• For the PMD warning (log not surrounded by "if"), is this about still evaluating methods even when logging is disabled? Is there a significant impact already from not doing this?
• The "fluent" logger style appears more verbose and makes logging harder to distinguish from existing code. Is this to be used everywhere or just where a method is called for a log parameter?
• How would you suggest we enforce this change? I suspect that new code will bring new log statements and they are highly likely to use the original logger methods unless enforced.

[soloturn]

• For the PMD warning (log not surrounded by "if"), is this about still evaluating methods even when logging is disabled? Is there a significant impact already from not doing this?

true. methods still would be called. depending which method it is the impact varies. not calling the method when not logging has, contrary, a clear and predictable impact: no code executed. which is good.

• The "fluent" logger style appears more verbose and makes logging harder to distinguish from existing code. Is this to be used everywhere or just where a method is called for a log parameter?

i changed it only where a method is called as log parameter and pmd warned.

• How would you suggest we enforce this change? I suspect that new code will bring new log statements and they are highly likely to use the original logger methods unless enforced.

we could turn on pmd again, it warns or blocks depending how we set it. there is no problem to use original logger methods, at least imo. there are other opinionated approaches, like using google flogger, which would convert everything, but that might be too much for terasology. personally i would not enforce it, but rely on peoples working brains on code review. as well as local gradle clean build warning about it. but that is not my call, i am happy with any approach you guys take.

[soloturn]

This might take a while to review (92 files changed)

you find it easier to chunk it?

[jdrueckert]

Note: only reviewed roughly half the files so far. Need to get back to the rest.
Feel free to already apply the requested changes meanwhile.

General assumption: cases in which we want to disable logging completely are extremely rare, so we mainly want to avoid method evaluation in debug logs considering that the typical default log level will be info resulting in error, warning, and info logs being written.

Cases where we want to ignore the PMD warnings instead of using Fluent API to avoid additional verbosity leading to code being harder to read:

• logs in catch clauses -> limited method evaluation
• logs in dedicated warning/error/log methods -> intention of these methods is to write logs; if at all, calling these methods should be guarded
• logs with simple getter methods

[jdrueckert]

more review comments along the same lines as the previous

[jdrueckert]

Finally done with all files. Please re-request a review once you implemented all requested changes 🙂

addressed most of the stuff. a couple were requesting a functional code change which is not part of a pull request writing log statements different.

[jdrueckert]

@soloturn I went through all files again, applied the requested changes that you resolved without actually applying them. I also refactored your changes to use the less verbose variant of the fluent API that was discussed a while ago in our group sync.

From my pov this is good to go now, so I'm approving it.

For future reference two comments:

1. Please try avoid force pushes as they can remove review comments. Force pushes are rarely necessary, usually merging upstream changes or the most recent main branch state in should be sufficient.
2. Code refactorings (please don't confuse them with functional changes: introducing a local variable and referencing it does not modify the code's functionality) are fine when addressing code scanner findings.

While writing this second point and remembering your last comment on this PR, I also want to share an observation:
Based on the discussions we had during the work on our last milestone, I expected this PR to be about addressing code scanner findings (in particular PMD GuardLogStatement warnings).
According to your comment, in which you state that this PR is about "writing log statements different", you seem to have a different intent here, though. An intent I didn't suspect, especially in the light of the group discussions where everybody except yourself voiced concerns about the verbosity of the fluent API variant you wanted to switch to...

Maybe this was our main misunderstanding here.

Anyway, this PR is approved now, so feel free to merge it.