Add System.out.println catcher #6278
Conversation
|
For finding the plugin class that called it, would using log4j's StackLocaterUtil be better than writing your own? I believe it does the same thing and is used by log4j in its LogManager#getLogger() for getting a logger for the caller class. |
|
If you're referring to this over
To me, that implies that they treat it as internal code and thus won't worry about API breakage. If you're referring to the |
|
Ignore, apparently my shit internet made it comment twice. |
|
https://docs.oracle.com/javase/9/docs/api/java/lang/StackWalker.html#getCallerClass-- No idea how this works with our setup though, but, worth looking at |
underscore11code
force-pushed
the
feature/sysoutcatcher
branch
from
7b08201
to
f306d54
Compare
|
Yeah looks like StackWalker works, and is much tidier. I was expecting classloader issues, but looks like it's fine. Refactored to use it. |
|
Perhaps you should also move the calls to setOut and setErr to the same place where this happens: System.setOut(IoBuilder.forLogger(logger).setLevel(Level.INFO).buildPrintStream());
System.setErr(IoBuilder.forLogger(logger).setLevel(Level.WARN).buildPrintStream());That would make it clearer which calls happen first |
underscore11code
force-pushed
the
feature/sysoutcatcher
branch
2 times, most recently
from
806b54e
to
a935a42
Compare
underscore11code
force-pushed
the
feature/sysoutcatcher
branch
from
a935a42
to
8209530
Compare
| + } else blacklistedPlugins = ImmutableSet.of(); | ||
| + | ||
| + // Listen to System.out.println | ||
| + PrintStream wrappedOut = new PrintStream(System.out) { |
There was a problem hiding this comment.
(Blocker) I'd prefer if these anonymous classes were replaced with a private class and use variables for the different constants used.
There was a problem hiding this comment.
All right, I've done what I assume you meant, lmk how this looks.
underscore11code
force-pushed
the
feature/sysoutcatcher
branch
2 times, most recently
from
1a880bc
to
ab31b14
Compare
|
Seeing as how there's already a plugin that does this, when the admin wants it, I don't see this as a necessary thing. Just my two cents: some plugin creators like to use sysout's for cleaner looking logs, but the logs themselves end up being different based on what server software is being used. |
|
not using your own logger is a pretty common anti-practice which often creates a huge number of headaches for server admins due to plugin devs failing to use their own loggers, it makes it hard to work out where something is coming from and there is generally very little sane reason to do so long term intent is that all plugin logger usage should be directed to the correct place to limit the side-effects of this anti-practice which just poses many headaches for server owners and us |
From the original PR note:
|
Yeh this needs a global opt out for even adding the prefixes. Networks built on entirely custom plugins often know what they are doing and don't want extra things like |
This only effects people using sysout, if you use a logger to begin with you won't have any issue. Logger instances are simple to obtain and there are no shortage of ways to get them. Edit: |
|
I don't see a reason to deny the request for an on/off system property to toggle this behavior. Personally I'm indifferent, since I will always have this behavior enabled.
No, don't do that. That would be swapping a bad practice for a slightly less bad one, at which point you've accomplished little. |
|
Yeah I'll swap it over to a boolean on/off when I get home. Don't actually remember my rationale for per-plugin whitelist vs blanket disable of nag. One thing I'm noticing is due to me overriding the |
|
No, don't mess with that. Would you want to see a stacktrace interspersed with nag messages? In this ecosystem, be glad for a plugin to use |
|
Oops sorry, should have clarified. For exceptions, I'd disable the nags. Because yeah I agree, Thinking about it though, it'd probably be too hard to find the calling class of that though, so I'd need to also loose the prefix for exception methods, but that should be fine, should be able to figure out the cause of an exception from the stacktrace. |
|
The global switch to disable nags is a good idea. Personally, I wouldn’t mind There should not be an option to disable the extra prefix, however. It’s a tiny bit of noise, and if these custom networks don’t want it, I suggest just moving to local constants for per-class loggers, which is very clean, very easy, and a small price to pay for cleaner logs. |
Proximyst
force-pushed
the
feature/sysoutcatcher
branch
from
ab31b14
to
407f9d0
Compare
|
So, when a plugin embeds an external library, and that library outputs to Because, yknow, the output telling server owners to nag me about something I don't control isn't particularly helpful. I strongly imagine I'm not going to be the only plugin author to ever embed an external library, and thus not the only person that needs a way to disable the spam. |
|
Just out of curiosity, what libraries are you using that aren't using even the most basic of logging frameworks? |
|
The fact that the nag message shows more than once is defo something which should be changed, but, these libraries really should be using a logger, not doing so especially in these types of environments is generally deemed as something we got past over using a proper logging framework I don't think that there is an ultra great solution here otherwise but using Sout has been a long standing issue which causes many issues for server owners, which, er... |
|
Re: a solution: At the very least, something that can be set or called somewhere within a plugin to indicate it should be excluded from the nag method. A sort of "I understand it's wrong but can't change it" checkbox (note: specifically set by the plugin, not by each and every server owner). Also re: |
|
Can you tell us what library is causing this issue? Logging frameworks are far more than just sysout wrappers and I find it highly surprising that a library is using a logging framework that doesn't support one of the standard APIs supported by Paper et al. |
|
Again, as I said: I can hackaround for my own single case (and in fact already have), I'm posting here because I genuinely believe this is not going to be just my one case. |
|
people will nag the author, who can see that this isn't actually caused by the plugins code but a 3rd party, and thus can forward the issue to the author of said library. no production code should use system out. |
|
HikariCP uses slf4j and still triggers this warning so just using a logging framework definitely doesn't solve this for all libraries... |
|
Well you should bring this up with them https://github.com/brettwooldridge/HikariCP/search?q=out.println |
These are not the log messages I am talking about, any of their slf4j logging usage will go to System out as Paper does not seem to properly support that framework. ( |
|
@Phoenix616 Can't reproduce that. implementation("com.zaxxer:HikariCP:5.0.0")
implementation("com.h2database:h2:1.4.200") try {
org.h2.Driver.load();
final var hikariConfig = new HikariConfig();
hikariConfig.setJdbcUrl("jdbc:h2:mem:testplugin");
this.hikariDataSource = new HikariDataSource(hikariConfig);
} catch (final Exception ex) {
LOGGER.warn("Could not initialise Hikari with H2", ex);
} |
|
Interesting. Maybe h2 includes bindings? That's the only difference to my setup (and me using maven). Manually including the slf4j-log4j-impl bindings artifact solved this for me though. Maybe it might be worth considering directly including that in Paper although I guess that could result in incompatibilities with different log4j implementation versions. |
|
Same result with: implementation("com.h2database:h2:1.4.200") {
isTransitive = false
}HikariCP is the only one which brings in |
|
Turns out my issue stemmed from the fact that I relocated the |
Inspired by @Draycia's SysoutCatcher (Used with permission). Redirects any plugin calls to System.out/err.print(ln) to the plugin's logger, along with a nag message (per-plugin disableable with a system property).
There's probably a ton of problems y'all gonna have with this, among which I anticipate being the class finder, nag message, etc etc. If you want, I'm on the Discord,
@_11#0011, feel free to ping me to discuss there instead.