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

W-17280936: Use SLF4J API instead of JUL #71

Merged
merged 11 commits into from
Nov 27, 2024
Merged

Conversation

eze210
Copy link

@eze210 eze210 commented Nov 15, 2024

See: https://www.slf4j.org/apidocs/org/slf4j/bridge/SLF4JBridgeHandler.html
FINEST -> TRACE
FINER -> DEBUG
FINE -> DEBUG
INFO -> INFO
WARNING -> WARN
SEVERE -> ERROR

@eze210 eze210 requested a review from a team as a code owner November 15, 2024 13:51
@eze210 eze210 changed the title Use SLF4J API instead of JUL W-17280936: Use SLF4J API instead of JUL Nov 20, 2024
asanguinetti
asanguinetti previously approved these changes Nov 26, 2024
Copy link

@asanguinetti asanguinetti left a 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.

Copy link

@asanguinetti asanguinetti left a 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

Comment on lines 27 to 38
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;
}
}

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.

Copy link
Author

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

Comment on lines 67 to 70
@Override
public void trace(String format, Object arg) {
julDelegate.log(Level.FINEST, new MessageSupplier(MessageFormatter.format(format, arg)));
}

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?

Copy link
Author

Choose a reason for hiding this comment

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

No, good catch!

@eze210 eze210 merged commit 34733ad into support/2.3.x Nov 27, 2024
6 checks passed
@eze210 eze210 deleted the chore/use-slf4j branch November 27, 2024 18:11
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.

2 participants