-
Notifications
You must be signed in to change notification settings - Fork 70
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
Use slf4j
+java.util.logging
for logging needs.
#383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I haven't reviewed the CustomLogger
changes in details, but the log level conversion looks good to me. Left a few nit comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remeh Left some comments but it's mostly nits. Great work on this!
/** contains returns if the given log level is contained. | ||
* If this level is `OFF`, `contains` returns `true` only if `other` is `OFF` | ||
* as well. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Javadoc-like comments in this file probably need a bit standardization/formatting
Co-authored-by: Srdjan Grubor <[email protected]>
Otherwise, the root logger remains at the INFO level and finer log levels can't be configured successfully
We need to match slf4j-jdk14's mapping. Also, add a test.
Old code had a file count set to 1 which would effectively cause the content of the file never to rotate. Setting this value to 2 ensures that we have 2 log files wherever possible (1 current, 1 rolled over).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments that I'll address right away
Co-authored-by: Olivier Vielpeau <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving to account for Remy's latest changes.
I've QA'ed the current state of the branch with an agent, LGTM.
Also bumps SLF4J from 1.7.26 to 1.7.32. Co-authored-by: truthbk <[email protected]> Co-authored-by: Srdjan Grubor <[email protected]> Co-authored-by: Olivier Vielpeau <[email protected]> Co-authored-by: Olivier Vielpeau <[email protected]>
Also bumps SLF4J from 1.7.26 to 1.7.32. Co-authored-by: truthbk <[email protected]> Co-authored-by: Srdjan Grubor <[email protected]> Co-authored-by: Olivier Vielpeau <[email protected]> Co-authored-by: Olivier Vielpeau <[email protected]>
JMXFetch needs for logging features are fairly small, this PR is switching all the logging to use
java.util.logging
SLF4J bindings.All features (file logging with rotation, stdout, stderr, log levels, rfc3339 date time format support) should still be present.
Some rough performances tests have been used to confirm that JMXFetch still performs well when it logs a lot, even if it is not really verbose in default scenarios.
This PR bumps SLF4J from
1.7.26
to1.7.32
.