Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: guidance on PMD guard log statement warning #5220

Merged
merged 5 commits into from
Mar 10, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions docs/Code-Conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Note that we try and stick to the standard Java conventions as much as possible.
- [Starred Imports](#starred-imports)
- [Others](#others)
- [Checkstyle](#checkstyle)
- [PMD](#pmd)
- [Testing](#testing)
- [JavaDoc](#javadoc)

Expand Down Expand Up @@ -139,6 +140,97 @@ The Checkstyle configuration file is located at `config/metrics/checkstyle/check
Checkstyle statistics are automatically calculated per build and turned into nifty metrics per build.
For instance, have a look at the [Checkstyle Report for the engine](http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/develop/checkstyle).

## PMD

We use the [PMD](https://pmd.github.io/) project to help keep the codebase free from common programming flaws like unused variables, empty catch blocks, unnecessary object creation, etc.
The Terasology engine's configuration is defined in [MovingBlocks/TeraConfig](https://github.com/movingblocks/teraconfig).
jdrueckert marked this conversation as resolved.
Show resolved Hide resolved
You can find the local copy of the configuration file at `config/metrics/pmd/pmd.xml`.
jdrueckert marked this conversation as resolved.
Show resolved Hide resolved

When working an area please keep an eye out for existing warnings to fix in files you're impacting anyway.
Make sure to run PMD on any new files you add since that's the best time to fix warnings.

### Check Execution

#### Gradle

You can easily run PMD via the Gradle wrapper `gradlew`.
See [PMD Gradle Plugin](https://docs.gradle.org/current/userguide/pmd_plugin.html) for more information.

- `gradlew check` to run all checks and tests (incl. PMD)
- `gradlew engine:pmdMain` to run PMD for just the engine's main source directory

#### IntelliJ Integration

We recommend to use the [PMD IDEA Plugin](https://plugins.jetbrains.com/plugin/15412-pmd-idea).
IntelliJ IDEA allows to easily run PMD checks for the whole project, specific folders, or just single files.

In case the IDE does not pick put the correct PMD rules automatically you may have to configure it manually.
The PMD configuration file is located at `config/metrics/checkstyle/pmd.xml`.

### Guard Log Statement Warnings

The [PMD GuardLogStatement rule](https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#guardlogstatement) identifies logs that are not guarded resulting in the String creation and manipulation as well as any associated calculations be performed even if the respective log level or the entire logging support are disabled.

See the following example for a non-guarded log statement:
```java
log.debug("log something" + method() + " and " + param.toString() + "concat strings");
jdrueckert marked this conversation as resolved.
Show resolved Hide resolved
```

#### Parameter Substitution

In general, parameter substitution is a sufficient log guard.
It also removes the need for explicit `toString()` calls.
Parameter substitution can be applied to the above-mentioned non-guarded log statement as follows:
```java
log.debug("log something {} and {}", method(), param);
```

Unfortunately, PMD is currently subject to a [bug](https://github.com/pmd/pmd/issues/4703) that can lead to warnings still being reported despite the parametrized logging approach.
If you utilized parameter substitution but are still facing the `GuardLogStatement` PMD warning, alternative approaches need to be found until the bug is resolved.
These include local variable creation, suppression, and the fluent log API.
For a decision about the appropriate approach, the context, the log level, and the individual performance impact of any relevant case should be taken into consideration.

#### Local Variable Creation

If the calculation performed was already performed before or will be performed after the log line (including in further log lines), introducing a local variable and referencing it in the log is a suitable approach.
In some cases a respective local variable may already exist and simply be referenced in your log statement.
```java
String localVar = method();
log.debug("log something {} and {}", localVar, param);
[...]
log.debug("log something else {}", localVar);
```

#### Suppression

For Terasology complete disablement of the logging support is very unlikely, as is the disablement of the `error` log level.
While `warn` and `info` log levels may be disabled in rare cases, they're usually not.
Accordingly, any reported cases on `error`, `warn`, and `debug` log levels should be considered for suppression.

Especially, if the performance impact is neglectable, e.g. for variable references (no method call) or simple getter or setter methods, or if the log frequency is very limited, e.g. only during initialization or cleanup, the PMD warning can be suppressed using an inline comment as follows:
```java
log.warn("log something {} and {}", method(), param); //NOPMD
```

If the logs are part of a logging-specific method, that is intentionally called (only) for logging specific aspects like machine specs or config values, the warning can be suppressed for the entire method like so:
```java
@SuppressWarnings("PMD.GuardLogStatement")
public logSpecs() {
log.info("log something {} and {}", method(), param)
}
```

Suppressing warnings allows for easier identification and reversion once the PMD bug is resolved.

#### Fluent Logging API

If parameter substitution is insufficient, local variable creation is not suitable, and suppression is not desired, the fluent logging API can be utilized as follows:
```java
log.atDebug().log("log something {} and {}", method(), param);
```

Please do not use the more verbose variant(s) of the fluent logging API to keep complexity low and your log statements readable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also provide an example here of a more "verbose variant"?

Something like this?

logger.atError().addArgument(() -> entry.getValue()).addArgument(e).log("log some exception: {}");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid this because I know people tend to skim through text and just copy anything 🙈
That's why I didn't want to provide an example for the version we don't want.
If you think we need it though, I can add it. What do you think @BenjaminAmos ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. That particular example was what inspired these guidelines, I believe?

Would something more vague work? For example:

Suggested change
Please do not use the more verbose variant(s) of the fluent logging API to keep complexity low and your log statements readable.
Please do not use the more verbose variant(s) of the fluent logging API (such as `LoggingEventBuilder#addArgument`) to keep complexity low and your log statements readable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think that's okay. I'll adjust it

Copy link
Member Author

@jdrueckert jdrueckert Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it. is it alright like that?


## Testing

Terasology's test suite can be found in the [engine-tests/src/test/java/org/terasology](https://github.com/MovingBlocks/Terasology/tree/develop/engine-tests/src/test/java/org/terasology) folder.
Expand Down
Loading