Sponge icon indicating copy to clipboard operation
Sponge copied to clipboard

Replace printStackTrace() with a proper logger

Open Yeregorix opened this issue 1 year ago • 7 comments

I originally had an issue where my plugin failed to construct but I had no traces in the log files so I wanted to replace the printStackTrace() with a proper logger. I took the opportunity to do this in several places.

Yeregorix avatar Aug 15 '22 10:08 Yeregorix

There are still other printStackTrace() indeed. Most of them are in the test plugins / unit tests, I thought it wasn't really important. Some others are in asm class visitors so wasn't sure if it would cause classloading issues or not.

Yeregorix avatar Aug 16 '22 14:08 Yeregorix

I have now replaced all printStackTrace except in test sourcesets, and except in SpongeWorldManager because it will be fixed by PR #3702.

Yeregorix avatar Aug 19 '22 10:08 Yeregorix

@Yeregorix @zml2008

I am aware at the amount of effort of such a change but zml you are 100% right, the "global logger" is not helpful, not to know where whatever is coming from. I think it is time to likely axe it so SpongeCommon.logger() goes away and each class that logs creates it's own logger. Thoughts? Beyond being a massive change, I see no issue. This doesn't change giving each plugin a logger. If a plugin wants to do class loggers or not that is up to them. This is just a stylistic choice for us to keep doing this for them.

Zidane avatar Aug 19 '22 16:08 Zidane

From a developer point of view, I prefer each class having its own logger. From a server owner pov, it's easier to identify which mod is involded when you see [Sponge] at the beginning of the logged lines.

  1. [Sponge] Something went wrong. (easy to identify that Sponge is involved) vs
  2. [SomeClass] Something went wrong. (most people have no idea what SomeClass is or to which mod it belongs)

Yeregorix avatar Aug 19 '22 16:08 Yeregorix

@Yeregorix I agree. However classes such as DataUpdaterDelegate fall right into this issue in this PR, no? I'd have to look at our typical log files but someone wouldn't know based on the logging lines.

Zidane avatar Aug 19 '22 16:08 Zidane

Yes, I'm just opposing the pros and cons of each convention.

Yeregorix avatar Aug 19 '22 17:08 Yeregorix

For the moment, for each class I used the common logger if I saw that it was already used in the class, otherwise I used a specific logger.

Yeregorix avatar Aug 19 '22 17:08 Yeregorix