Refactor logging by SmylerMC · Pull Request #50 · BTE-Germany/TerraPlusMinus

SmylerMC · 2025-10-19T17:53:57Z

This PR aims to improve the overall logging situation within the plugin.

Most importantly, it makes sure all outputs coming from Terra+- go through its plugin logger. This makes sure it can be properly instrumented using Log4J and JUL configuration capabilities. For example, a server administrator might want to set the Terra+- logger's level to trace and mute all other plugins, all the while saving latest.log as JSON so it can be latter be grammatically analyzed. This means all calls to System#out or Exception#printStackTrace() have been removed or replaced with logger calls.

This implies changes in what exactly gets logged. Some messages have changed, some errors are redirected to end users (see changes for the /where command), and some things are simply no longer logged.

Two specific parts of the plugins were also rewritten more substantially with the goal to improve them, as substantial rewrites were necessary anyway to better handle and logging errors. This concerns automatic datapack installation and automatic configuration upgrade. Behavior should not change. Also, while I feel like the configuration part is somewhat better with those changes, I still think it has many issues.

Finally, I have removed the ascii-art that gets logged on startup, and replaced it with a simple line that prints the Terra+- and Terra-- versions, as it does clutter server logs with little benefits.

For reference, this is what Terra+- logs when starting and stopping a server (by default only INFO messages would be shown):

[10:37:55 INFO]: [T+-] Loading server plugin Terraplusminus v1.5.1
[10:37:55 INFO]: [T+-] Enabling Terraplusminus v1.5.1
[10:37:55 INFO]: [terra--] Starting cache pruning schedule
[10:37:55 DEBUG]: [T+-] Terraplusminus HTTP user agent: Terraplusminus/1.5.1 (terraminusminus/2.0.0-1.21.4; +https://bte-germany.de)
[10:37:55 DEBUG]: [T+-] Terra-- config file /home/smyler/BteFrance/DevServer/plugins/Terraplusminus/osm.json5 is already present in plugin directory
[10:37:55 DEBUG]: [T+-] Terra-- config file /home/smyler/BteFrance/DevServer/plugins/Terraplusminus/heights/README.md is already present in plugin directory
[10:37:55 DEBUG]: [T+-] Terra-- config file /home/smyler/BteFrance/DevServer/plugins/Terraplusminus/tree_cover/README.md is already present in plugin directory
[10:37:55 INFO]: [T+-] Terraplusminus successfully enabled (Terraplusminus v1.5.1, terraminusminus v2.0.0-1.21.4)
[10:38:00 DEBUG]: [T+-] Loading tree family birch with 3 size variants
[10:38:00 TRACE]: [T+-] Loading trees of family birch and size s with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size s and family birch
[10:38:00 TRACE]: [T+-] Loading trees of family birch and size m with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size m and family birch
[10:38:00 TRACE]: [T+-] Loading trees of family birch and size l with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size l and family birch
[10:38:00 DEBUG]: [T+-] Loading tree family snow with 3 size variants
[10:38:00 TRACE]: [T+-] Loading trees of family snow and size s with 0 variants
[10:38:00 TRACE]: [T+-] Loading trees of family snow and size m with 0 variants
[10:38:00 TRACE]: [T+-] Loading trees of family snow and size l with 0 variants
[10:38:00 DEBUG]: [T+-] Loading tree family savanna with 3 size variants
[10:38:00 TRACE]: [T+-] Loading trees of family savanna and size s with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size s and family savanna
[10:38:00 TRACE]: [T+-] Loading trees of family savanna and size m with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size m and family savanna
[10:38:00 TRACE]: [T+-] Loading trees of family savanna and size l with 0 variants
[10:38:00 DEBUG]: [T+-] Loading tree family spruce with 3 size variants
[10:38:00 TRACE]: [T+-] Loading trees of family spruce and size s with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree2 of size s and family spruce
[10:38:00 TRACE]: [T+-] Loading trees of family spruce and size l with 5 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size l and family spruce
[10:38:00 TRACE]: [T+-] Loading tree variant tree2 of size l and family spruce
[10:38:00 TRACE]: [T+-] Loading tree variant tree3 of size l and family spruce
[10:38:00 TRACE]: [T+-] Loading tree variant tree4 of size l and family spruce
[10:38:00 TRACE]: [T+-] Loading tree variant tree5 of size l and family spruce
[10:38:00 TRACE]: [T+-] Loading trees of family spruce and size m with 0 variants
[10:38:00 DEBUG]: [T+-] Loading tree family oak with 3 size variants
[10:38:00 TRACE]: [T+-] Loading trees of family oak and size l with 1 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size l and family oak
[10:38:00 TRACE]: [T+-] Loading trees of family oak and size m with 2 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size m and family oak
[10:38:00 TRACE]: [T+-] Loading tree variant tree2 of size m and family oak
[10:38:00 TRACE]: [T+-] Loading trees of family oak and size s with 2 variants
[10:38:00 TRACE]: [T+-] Loading tree variant tree1 of size s and family oak
[10:38:00 TRACE]: [T+-] Loading tree variant tree2 of size s and family oak
[10:38:00 INFO]: [T+-] Loaded 16 custom trees from 5 families
[10:38:49 INFO]: [T+-] Disabling Terraplusminus v1.5.1
[10:38:49 INFO]: [T+-] Plugin deactivated

SmylerMC · 2025-10-27T21:58:45Z

Fixes #30

Zoriot

Thanks for the PR! Generally looks good to, comment's regarding parameterized logging & component logger apply to all changes/loggings.

Zoriot · 2025-11-01T21:54:20Z


    private void updateConfig() {
-        FileBuilder fileBuilder = new FileBuilder(this);
+        PluginConfigManipulator manipulator = new PluginConfigManipulator(this);


Please move this in another Class

Not sure I understand what you want to see moved. The hole updateConfig() method?

Yea, that could be Split up a bit too, but not necessarily. Like having a Method per Version. But we shouldn't put too much time into that.

Imo that needs to be entirely reworked, so I did not address it in that PR. All I did was rewriting FileBuilder into PluginConfigManipulator where I was able to cleanup logging without the duplicated code and weird static side effects. I don't really want to touch updateConfig() in this PR beyond logging and trivial warnings like multi-line strings.

Imo that already broke the scope of the pr & should be mentioned in the title. Or moved to another pr.

Changing that to a whole new System (even when it's kinda working the Same under the hood) is a quite big change regarding the config system.
Still think this method is completely wrong in the main class and it's also stupid that you have to touch main class when just adding an option for xy.

Changing that to a whole new System (even when it's kinda working the Same under the hood) is a quite big change regarding the config system.

The system is exactly the same and all changes are syntactic:

Removed unused methods

Made a static field an instance field (because it had no business being static and that was very confusing)

Removed duplicated code by leveraging lambdas (this was necessary to avoid copy-pasting logging statements in 3 different places...)

Renamed the class because its name was very confusing.

Copilot is even complaining about problems that were carried over from the original code down below 😅 because I did not address anything else.

Yes, it should be moved elsewhere. I did some changes that I felt were necessary to improve logging, but I don't think going further would be in scope at all for this PR. As you said it yourself some of the changes are already reaching the limit.

Can you please make an issue for that?

Copilot

Pull Request Overview

This PR refactors logging and error handling across the plugin to modernize the codebase. The changes migrate from legacy logging APIs to Paper's ComponentLogger, improve error handling with more descriptive messages, and extract config manipulation functionality into a dedicated utility class.

Migrated all logging from Bukkit.getLogger() and java.util.logging.Level to Paper's ComponentLogger API
Refactored the FileBuilder class into a focused PluginConfigManipulator utility for config file manipulation
Improved exception handling with better error messages and appropriate logging levels
Fixed logic issues in WhereCommand where coordinates array could be uninitialized

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
PluginConfigManipulator.java	New utility class that extracts config file manipulation methods from FileBuilder with improved error handling
FileBuilder.java	Deleted legacy class that mixed YAML configuration access with file manipulation
TreePopulator.java	Migrated logging to ComponentLogger and improved exception messages
CustomBiomeProvider.java	Improved exception handling by silencing expected OutOfProjectionBoundsException and logging unexpected ones
PluginMessageEvent.java	Migrated logging to ComponentLogger
PlayerMoveEvent.java	Migrated logging to ComponentLogger and removed unused imports
WhereCommand.java	Fixed potential NPE by restructuring coordinate calculation and improved error handling
Terraplusminus.java	Comprehensive logging migration, refactored datapack installation logic, and updated config migration to use new PluginConfigManipulator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-02T09:14:41Z

-            e.printStackTrace();
-            Bukkit.getLogger().log(Level.SEVERE, "[T+-] Old config detected. Please delete and restart/reload.");
+        if (configVersion == 0.0) {  // That's the default value if the field was not set at all in the YAML
+            this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");


When an old config is detected (version 0.0), the method logs an error but continues execution. This will cause all subsequent if (configVersion == X.X) checks to fail, and the config will remain outdated. The method should return early after logging the error to prevent further processing.

Suggested change

this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");

this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");

return;

This problem was already there. I don't want to half address it as the whole logic flow for configuration upgrades is flawed (e.g. upgrading from 1.1 to 1.5 will only upgrade the configuration to 1.2...), and that's out of scope for this PR.

Document it by an issue so it can/will be fixed

Zoriot · 2025-11-06T15:32:25Z


    private void updateConfig() {
-        FileBuilder fileBuilder = new FileBuilder(this);
+        PluginConfigManipulator manipulator = new PluginConfigManipulator(this);


Can you please make an issue for that?

Zoriot · 2025-11-06T15:33:04Z

-            e.printStackTrace();
-            Bukkit.getLogger().log(Level.SEVERE, "[T+-] Old config detected. Please delete and restart/reload.");
+        if (configVersion == 0.0) {  // That's the default value if the field was not set at all in the YAML
+            this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");


Document it by an issue so it can/will be fixed

- Do not use this.config to access the configuration static field (ideally the configuration should not be in a static field, a complete rewrite of the config system is needed on that side). - Rename FileBuilder to PluginConfigManipulator and make it use instance fields and methods; remove all unused methods and rewrite the remaining ones to eliminate duplicated code and warnings, and use Java best practices like try-with blocks. - Use multiple strings I do believe this will have to be replaced at some point in the future and is still suboptimal, but at least it should be a lot less confusing and more manageable now.

…stead of silently failing and printing a useless stack trace

These kind of multiline logs add nothing of value besides "looking cool" and make logs harder to read, search and process for server admins.

SmylerMC · 2025-11-15T16:43:15Z

@Zoriot if this is approved, can we go ahead and merge it? Or is there anything else that needs improvement or clarification?

SmylerMC force-pushed the refactor/logging branch 3 times, most recently from 7e9df63 to 567c538 Compare October 25, 2025 08:05

SmylerMC marked this pull request as ready for review October 25, 2025 08:06

SmylerMC mentioned this pull request Oct 27, 2025

Unlock all osm.json5 configuration features #51

Merged

Zoriot requested changes Nov 1, 2025

View reviewed changes

Zoriot requested a review from Copilot November 2, 2025 09:11

Copilot AI reviewed Nov 2, 2025

View reviewed changes

SmylerMC force-pushed the refactor/logging branch 3 times, most recently from e315b7d to aec51e9 Compare November 2, 2025 10:28

Zoriot requested changes Nov 6, 2025

View reviewed changes

Zoriot added the enhancement label Nov 8, 2025

Zoriot assigned SmylerMC Nov 8, 2025

Zoriot added this to the v1.6.0 milestone Nov 8, 2025

This was linked to issues Nov 8, 2025

Paper Logging Warning #30

Closed

Height Datapack Missing Error on WorldInitEvent #33

Closed

SmylerMC mentioned this pull request Nov 9, 2025

Update to 1.21.8 #52

Merged

SmylerMC added 5 commits November 9, 2025 21:42

Switch logging to the plugin component SL4J logger

bea5931

Refactored datapack installation

a77906f

Get rid of trivial calls to Exception#printStackTrace()

6f81633

Send a message to players using /where from outside the projection in…

b2a9681

…stead of silently failing and printing a useless stack trace

SmylerMC force-pushed the refactor/logging branch from aec51e9 to ada5332 Compare November 11, 2025 07:46

SmylerMC added 3 commits November 11, 2025 08:51

Remove log ascii art when the plugin gets loaded

4f6468e

These kind of multiline logs add nothing of value besides "looking cool" and make logs harder to read, search and process for server admins.

Tweak tree-loading logging

8b12dea

Make config replacement atomic

9bb4b88

SmylerMC force-pushed the refactor/logging branch from ada5332 to 9bb4b88 Compare November 11, 2025 07:52

Zoriot approved these changes Nov 14, 2025

View reviewed changes

Zoriot merged commit cf276ea into BTE-Germany:master Nov 15, 2025
1 check passed

	this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");
	this.getComponentLogger().error("Old config detected. Please delete and restart/reload.");
	return;

Conversation

SmylerMC commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SmylerMC commented Oct 27, 2025

Uh oh!

Zoriot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SmylerMC commented Nov 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SmylerMC commented Oct 19, 2025 •

edited

Loading