-
Notifications
You must be signed in to change notification settings - Fork 2
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
W-17280936: Use SLF4J API instead of JUL #71
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.
Approved, but let's revisit the performance considerations of log level checking when logging separation is enabled.
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.
Just a suggestion and a question
private static boolean useSlf4j() { | ||
if (isMuleLogSeparationEnabled) { | ||
// When log separation is enabled, the isLogEnabled() methods are expensive, and then we would be adding a | ||
// performance degradation. If the user still wants to use SLF4J, then they have to set the property | ||
// org.mule.grizzly.useSLF4J=true | ||
return useSlf4jProperty; | ||
} else { | ||
// If log separation is disabled, there is no evidence or known issues of a performance degradation caused | ||
// by this change, so it's safe to use SLF4J. | ||
return true; | ||
} | ||
} |
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.
Just a suggestion to give it a thought:
What if we make it so that isMuleLogSeparationEnabled
affects the default of useSlf4jProperty
? That way, it could be used as a kill switch with or without log separation, just in case. So, even if log separation is disabled, the user can still do org.mule.grizzly.useSLF4J=false
if there is a problem.
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.
Done. Also changed the property name to mule.grizzly.useSLF4J
@Override | ||
public void trace(String format, Object arg) { | ||
julDelegate.log(Level.FINEST, new MessageSupplier(MessageFormatter.format(format, arg))); | ||
} |
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.
Question here: the MessageFormatter.format
does not do any actual formatting until getMessage
is called?
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.
No, good catch!
See: https://www.slf4j.org/apidocs/org/slf4j/bridge/SLF4JBridgeHandler.html
FINEST -> TRACE
FINER -> DEBUG
FINE -> DEBUG
INFO -> INFO
WARNING -> WARN
SEVERE -> ERROR