-
-
Notifications
You must be signed in to change notification settings - Fork 973
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
Adapter for Platform Logging (JEP 264) #232
Changes from 6 commits
46c634a
4128670
ca3f942
019eec0
9066751
82b61d7
04bc268
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
|
||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<parent> | ||
<artifactId>slf4j-parent</artifactId> | ||
<groupId>org.slf4j</groupId> | ||
<version>2.0.0-alpha0</version> | ||
</parent> | ||
|
||
<artifactId>slf4j-jdk-platform-logging</artifactId> | ||
<packaging>jar</packaging> | ||
<name>SLF4J JDK Platform Logging Integration</name> | ||
<description>Integrated SLF4J with the Platform Logging API added by JEP 264 in Java 9</description> | ||
|
||
<url>http://www.slf4j.org</url> | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>slf4j-api</artifactId> | ||
</dependency> | ||
|
||
<dependency> | ||
<groupId>org.slf4j</groupId> | ||
<artifactId>slf4j-simple</artifactId> | ||
<version>2.0.0-alpha0</version> | ||
<scope>test</scope> | ||
</dependency> | ||
<dependency> | ||
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> | ||
<version>${junit.version}</version> | ||
<scope>test</scope> | ||
</dependency> | ||
</dependencies> | ||
|
||
<build> | ||
<plugins> | ||
<!-- target Java 9+ --> | ||
<plugin> | ||
<artifactId>maven-compiler-plugin</artifactId> | ||
<configuration> | ||
<release>11</release> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which Java version do you want to target? Java 9 is the most lenient, but I wouldn't encourage use of outdated versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say 9 as the slf4j project currently requires Java 8 to at runtime and 9 at build time. Requiring 9 for this module seems consistent. |
||
</configuration> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
module org.slf4j.jdk { | ||
requires org.slf4j; | ||
provides java.lang.System.LoggerFinder | ||
with org.slf4j.jdk.SLF4JSystemLoggerFinder; | ||
} | ||
Comment on lines
+1
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. java.lang.System.LoggerFinder was introduced in Java9. Thus there would be no point in a multi-release jar. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,136 @@ | ||||||||||||||||||||
package org.slf4j.jdk; | ||||||||||||||||||||
|
||||||||||||||||||||
import org.slf4j.Logger; | ||||||||||||||||||||
import org.slf4j.LoggerFactory; | ||||||||||||||||||||
|
||||||||||||||||||||
import java.util.MissingResourceException; | ||||||||||||||||||||
import java.util.ResourceBundle; | ||||||||||||||||||||
|
||||||||||||||||||||
import static java.util.Objects.requireNonNull; | ||||||||||||||||||||
|
||||||||||||||||||||
/** | ||||||||||||||||||||
* Adapts {@link Logger} to {@link System.Logger}. | ||||||||||||||||||||
*/ | ||||||||||||||||||||
class SLF4JSystemLogger implements System.Logger { | ||||||||||||||||||||
|
||||||||||||||||||||
private static final Logger INTERNAL_LOGGER = LoggerFactory.getLogger(SLF4JSystemLogger.class); | ||||||||||||||||||||
|
||||||||||||||||||||
private final Logger logger; | ||||||||||||||||||||
|
||||||||||||||||||||
public SLF4JSystemLogger(Logger logger) { | ||||||||||||||||||||
this.logger = requireNonNull(logger); | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
@Override | ||||||||||||||||||||
public String getName() { | ||||||||||||||||||||
return logger.getName(); | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
@Override | ||||||||||||||||||||
public boolean isLoggable(Level level) { | ||||||||||||||||||||
switch(level) { | ||||||||||||||||||||
case ALL: | ||||||||||||||||||||
// fall-through intended because `ALL` is loggable if the | ||||||||||||||||||||
// lowest level is enabled | ||||||||||||||||||||
case TRACE: | ||||||||||||||||||||
return logger.isTraceEnabled(); | ||||||||||||||||||||
case DEBUG: | ||||||||||||||||||||
return logger.isDebugEnabled(); | ||||||||||||||||||||
case INFO: | ||||||||||||||||||||
return logger.isInfoEnabled(); | ||||||||||||||||||||
case WARNING: | ||||||||||||||||||||
return logger.isWarnEnabled(); | ||||||||||||||||||||
case ERROR: | ||||||||||||||||||||
return logger.isErrorEnabled(); | ||||||||||||||||||||
case OFF: | ||||||||||||||||||||
// all logging is disabled if the highest level is disabled | ||||||||||||||||||||
return !logger.isErrorEnabled(); | ||||||||||||||||||||
default: | ||||||||||||||||||||
INTERNAL_LOGGER.error( | ||||||||||||||||||||
"SLF4J internal error: unknown log level {} passed to `isLoggable` (likely by the JDK).", level); | ||||||||||||||||||||
return true; | ||||||||||||||||||||
Comment on lines
+49
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was unsure how to such handle error cases. They're highly unlikely to occur (JDK API should be very stable), but if they do, IMHO SLF4J should not crash, misbehave as little as possible, but let the user know that there's a problem (hence the error logger). See below for two more such cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid error cases from future JDK changes, you could compare to |
||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+48
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would pull the
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The compiler doesn't do that check for switch statements (your IDE may, though). Whether the code is in the |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
@Override | ||||||||||||||||||||
public void log(Level level, ResourceBundle bundle, String msg, Throwable thrown) { | ||||||||||||||||||||
String message = getResourceStringOrMessage(bundle, msg); | ||||||||||||||||||||
switch(level) { | ||||||||||||||||||||
case ALL: | ||||||||||||||||||||
// fall-through intended because a message is visible on all log levels | ||||||||||||||||||||
// if it is logged on the lowest level | ||||||||||||||||||||
case TRACE: | ||||||||||||||||||||
logger.trace(message, thrown); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case DEBUG: | ||||||||||||||||||||
logger.debug(message, thrown); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case INFO: | ||||||||||||||||||||
logger.info(message, thrown); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case WARNING: | ||||||||||||||||||||
logger.warn(message, thrown); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case ERROR: | ||||||||||||||||||||
logger.error(message, thrown); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case OFF: | ||||||||||||||||||||
// don't do anything for a message on level `OFF` | ||||||||||||||||||||
break; | ||||||||||||||||||||
default: | ||||||||||||||||||||
INTERNAL_LOGGER.error( | ||||||||||||||||||||
"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level); | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+80
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above:
Suggested change
|
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
@Override | ||||||||||||||||||||
public void log(Level level, ResourceBundle bundle, String format, Object... params) { | ||||||||||||||||||||
String message = getResourceStringOrMessage(bundle, format); | ||||||||||||||||||||
switch(level) { | ||||||||||||||||||||
case ALL: | ||||||||||||||||||||
// fall-through intended because a message is visible on all log levels | ||||||||||||||||||||
// if it is logged on the lowest level | ||||||||||||||||||||
case TRACE: | ||||||||||||||||||||
logger.trace(message, params); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case DEBUG: | ||||||||||||||||||||
logger.debug(message, params); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case INFO: | ||||||||||||||||||||
logger.info(message, params); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case WARNING: | ||||||||||||||||||||
logger.warn(message, params); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case ERROR: | ||||||||||||||||||||
logger.error(message, params); | ||||||||||||||||||||
break; | ||||||||||||||||||||
case OFF: | ||||||||||||||||||||
// don't do anything for a message on level `OFF` | ||||||||||||||||||||
break; | ||||||||||||||||||||
default: | ||||||||||||||||||||
INTERNAL_LOGGER.error( | ||||||||||||||||||||
"SLF4J internal error: unknown log level {} passed to `log` (likely by the JDK).", level); | ||||||||||||||||||||
} | ||||||||||||||||||||
Comment on lines
+111
to
+114
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above:
Suggested change
|
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
private static String getResourceStringOrMessage(ResourceBundle bundle, String msg) { | ||||||||||||||||||||
if (bundle == null || msg == null) | ||||||||||||||||||||
return msg; | ||||||||||||||||||||
// ResourceBundle::getString throws: | ||||||||||||||||||||
// | ||||||||||||||||||||
// * NullPointerException for null keys | ||||||||||||||||||||
// * ClassCastException if the message is no string | ||||||||||||||||||||
// * MissingResourceException if there is no message for the key | ||||||||||||||||||||
// | ||||||||||||||||||||
// Handle all of these cases here to avoid log-related exceptions from crashing the JVM. | ||||||||||||||||||||
try { | ||||||||||||||||||||
return bundle.getString(msg); | ||||||||||||||||||||
} catch (MissingResourceException ex) { | ||||||||||||||||||||
return msg; | ||||||||||||||||||||
} catch (ClassCastException ex) { | ||||||||||||||||||||
return bundle.getObject(msg).toString(); | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package org.slf4j.jdk; | ||
|
||
import org.slf4j.LoggerFactory; | ||
|
||
/** | ||
* Uses SLF4J's {@link LoggerFactory#getLogger(String)} to get a logger | ||
* that is adapted for {@link System.Logger}. | ||
*/ | ||
public class SLF4JSystemLoggerFinder extends System.LoggerFinder { | ||
|
||
@Override | ||
public System.Logger getLogger(String name, Module module) { | ||
// JEP 264[1], which introduced the Platform Logging API, | ||
// contains the following note: | ||
// | ||
// > An implementation of the LoggerFinder service should make it | ||
// > possible to distinguish system loggers (used by system classes | ||
// > from the Bootstrap Class Loader (BCL)) and application loggers | ||
// > (created by an application for its own usage). This distinction | ||
// > is important for platform security. The creator of a logger can | ||
// > pass the class or module for which the logger is created to the | ||
// > LoggerFinder so that the LoggerFinder can figure out which kind | ||
// > of logger to return. | ||
// | ||
// If backends support the distinction support this distinction and | ||
// once `LoggerFactory`'s API is updated to forward a module, we | ||
// should do that here. | ||
// | ||
// [1] https://openjdk.java.net/jeps/264 | ||
Comment on lines
+13
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that I quite get the point: the The javadoc of
We don't know the logging backends and how they configure things. So we have to check with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It will be used by JDK classes, but it's implemented here and the quote addresses implementations, so I assume it applies to us. Didn't think to look at the JavaDoc, though, and it's not mentioned there, so maybe it's outdated? Re security manager, it is indeed weird that |
||
var slf4JLogger = LoggerFactory.getLogger(name); | ||
return new SLF4JSystemLogger(slf4JLogger); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
Implementation-Title: slf4j-ext | ||
Bundle-ManifestVersion: 2 | ||
Bundle-SymbolicName: slf4j.ext | ||
Bundle-Name: slf4j-jdk-platform-logging | ||
Bundle-Vendor: SLF4J.ORG | ||
Bundle-RequiredExecutionEnvironment: JavaSE-1.8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
org.slf4j.jdk.SLF4JSystemLoggerFinder |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,77 @@ | ||||||||||||||||||||||||||||||||||||||
package org.slf4j.jdk; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import org.junit.After; | ||||||||||||||||||||||||||||||||||||||
import org.junit.Before; | ||||||||||||||||||||||||||||||||||||||
import org.junit.Test; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import java.io.ByteArrayOutputStream; | ||||||||||||||||||||||||||||||||||||||
import java.io.PrintStream; | ||||||||||||||||||||||||||||||||||||||
import java.util.ArrayList; | ||||||||||||||||||||||||||||||||||||||
import java.util.ServiceLoader; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
import static org.junit.Assert.*; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
public class SLF4JSystemLoggerTest { | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
private static final PrintStream ERROR_OUT = System.err; | ||||||||||||||||||||||||||||||||||||||
private static final ByteArrayOutputStream OUTPUT = new ByteArrayOutputStream(); | ||||||||||||||||||||||||||||||||||||||
private static final PrintStream ERROR_OUT_REPLACEMENT = new PrintStream(OUTPUT); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@Before | ||||||||||||||||||||||||||||||||||||||
public void setUp() { | ||||||||||||||||||||||||||||||||||||||
System.setErr(ERROR_OUT_REPLACEMENT); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@After | ||||||||||||||||||||||||||||||||||||||
public void tearDown() { | ||||||||||||||||||||||||||||||||||||||
System.setErr(ERROR_OUT); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@Test | ||||||||||||||||||||||||||||||||||||||
public void loggerFinderLoadedAsOnlyService() { | ||||||||||||||||||||||||||||||||||||||
// this method asserts that there is exactly one `LoggerFinder` and its of the correct type | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this an actual requirement? It could be over-specified. We only would have to change the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that makes sense. |
||||||||||||||||||||||||||||||||||||||
getSLF4JSystemLoggerFinder(); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@Test | ||||||||||||||||||||||||||||||||||||||
public void loggerFinderReturnsLogger() { | ||||||||||||||||||||||||||||||||||||||
var logger = getSLF4JSystemLogger(); | ||||||||||||||||||||||||||||||||||||||
assertNotNull("LoggerFinder must return logger", logger); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
@Test | ||||||||||||||||||||||||||||||||||||||
public void loggerLogsMessage() { | ||||||||||||||||||||||||||||||||||||||
var logger = getSLF4JSystemLogger(); | ||||||||||||||||||||||||||||||||||||||
var message = "Test system logging!"; | ||||||||||||||||||||||||||||||||||||||
logger.log(System.Logger.Level.INFO, message); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
String output = getOutput(); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
assertTrue("Captured output should contain logged message.", output.contains(message)); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
private static System.LoggerFinder getSLF4JSystemLoggerFinder() { | ||||||||||||||||||||||||||||||||||||||
var loggerFinders = new ArrayList<System.LoggerFinder>(); | ||||||||||||||||||||||||||||||||||||||
// this fails when test is executed from the module path | ||||||||||||||||||||||||||||||||||||||
// because the module declaration does not declare | ||||||||||||||||||||||||||||||||||||||
// `uses System.LoggerFinder` | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the moment, only the class path integration is tested. Module path integration would fail here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I observe the following error: [ERROR] org.slf4j.jdk.SLF4JSystemLoggerTest.loggerLogsMessage Time elapsed: 0.003 s <<< ERROR! |
||||||||||||||||||||||||||||||||||||||
ServiceLoader.load(System.LoggerFinder.class).forEach(loggerFinders::add); | ||||||||||||||||||||||||||||||||||||||
assertEquals("There should be exactly one `LoggerFinder`.", 1, loggerFinders.size()); | ||||||||||||||||||||||||||||||||||||||
var loggerFinder = loggerFinders.get(0); | ||||||||||||||||||||||||||||||||||||||
assertEquals("The `LoggerFinder` should be of type `SLF4JSystemLoggerFinder`.", | ||||||||||||||||||||||||||||||||||||||
SLF4JSystemLoggerFinder.class, | ||||||||||||||||||||||||||||||||||||||
loggerFinder.getClass()); | ||||||||||||||||||||||||||||||||||||||
return loggerFinder; | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe:
Suggested change
Note: this |
||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
private static System.Logger getSLF4JSystemLogger() { | ||||||||||||||||||||||||||||||||||||||
var loggerFinder = getSLF4JSystemLoggerFinder(); | ||||||||||||||||||||||||||||||||||||||
return loggerFinder.getLogger("TestLogger", SLF4JSystemLoggerTest.class.getModule()); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
private static String getOutput() { | ||||||||||||||||||||||||||||||||||||||
ERROR_OUT_REPLACEMENT.flush(); | ||||||||||||||||||||||||||||||||||||||
return OUTPUT.toString(); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
} |
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'm not sure what the best name would be for the artifact, module, package, and classes. 😊 At the moment they're a little all over the place using the terms
jdk
,platform
, andsystem
. I think a single term in all places would be better.