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

Replace printStackTrace() with a proper logger #3739

Merged
merged 2 commits into from Oct 12, 2022

Conversation

Yeregorix
Copy link
Member

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.

Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

There's a fair few places we call e.PrintStackTrace(), mind fixing those up in one fell swoop?

@Yeregorix
Copy link
Member Author

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 Yeregorix changed the title Replace some printStackTrace() with a proper logger Replace printStackTrace() with a proper logger Aug 19, 2022
@Yeregorix
Copy link
Member Author

Yeregorix commented Aug 19, 2022

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

@Zidane
Copy link
Member

Zidane commented Aug 19, 2022

@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.

@Yeregorix
Copy link
Member Author

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)

@Zidane
Copy link
Member

Zidane commented Aug 19, 2022

@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.

@Yeregorix
Copy link
Member Author

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

@Yeregorix
Copy link
Member Author

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.

@Zidane Zidane merged commit aa7b9f9 into SpongePowered:api-8 Oct 12, 2022
@Yeregorix Yeregorix deleted the fix/log-errors branch October 12, 2022 20:09
@Yeregorix Yeregorix restored the fix/log-errors branch October 12, 2022 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants