You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/12/24 01:34:25 UTC
[GitHub] [logging-log4j2] vorburger opened a new pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
vorburger opened a new pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653
https://issues.apache.org/jira/browse/LOG4J2-3282
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r778467489
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
> > I'm a bit worried about these
>
> Sure, let's improve this!
:smile:
>
> > mutating system properties is a surprising side-effect in a constructor,
> > (...) Also no need to re-set the value on each object instantiation.
>
> Right. Let me move that to... where would this fit better - say right in the `JULProvider`? Or, IMHO probably better & more appropriate actually (to me), in the `JULLoggerContext`. Would you like me to do that?
The context would make more sense to me, but it may not always work (more on this as I answer the next question!)
>
> > and this will replace any value that's already set.
>
> This particular point could of course also be very easily addressed by making it just a tiny bit smarter - read the property, and add our (two) packages (unless they are already set, while we are at it). Would you like me to do that?
>
> > What if somebody else modifies it later?
>
> They COULD break it yes - unless they are kind/smart enough to also do as above?
>
> But apparently _"[The `jdk.logger.packages` system property is consulted only once](https://hg.openjdk.java.net/jdk9/jdk9/jdk/file/ddf8af0e536a/src/java.logging/share/classes/java/util/logging/LogRecord.java#l675)."_ anyway.
This is a great point, I think it means that we cannot rely on setting the property anywhere in the bridge! It's probably safe to assume that any application using the log4j-to-jul bridge is using jul in other ways, so the log4j loggers may not be the first to be initialized or used. In such an environment the set of packages may already be initialized:
https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/java.base/share/classes/jdk/internal/logger/SimpleConsoleLogger.java#L439-L445
>
> FYI I tried to search how common use of `jdk.logger.packages` across GitHub, but without the JDK, is; but seem to hit some GitHub index search internal issue [with this query which says 318 results](https://github.com/search?q=%22jdk.logger.packages%22+-%22The+system+property+%7B%40code+jdk.logger.packages%7D+can+define+a+comma+separated%22+-path%3Asrc%2Fjava.base%2Fshare+-filename%3ALoggerFinderLoaderTest.java+-filename%3A%2FSimpleConsoleLogger.java&type=Code) - but does not show them (to me).
>
> > The other case we should consider is what happens if the log4j2 `log4j-slf4j-impl` module (or any other api bridge into log4j-api) is combined with `log4j-to-jul`. In the case of `log4j-slf4j-impl`, users will call `LoggerFactory.getLogger(clazz);` which returns a `org.apache.logging.slf4j.Log4jLogger` wrapping an ExtendedLogger (JULLogger). The fqcn field on the logevent will be `org.apache.logging.slf4j.Log4jLogger` which isn't included in this list of packages.
>
> Nice catch! Would it suffice to simply add `org.apache.logging.slf4j` as a (hard-coded?) 3rd package name to this `jdk.logger.packages` list? Are you concerned about other API bridges? Are there any in `logging-log4j2`? Or are you thinking elsewhere?
I'm not terribly keen on this idea because it forces the jul adapter to have full knowledge of all logger implementations, and I'm confident they're not all public. The overlap between consumers of the jul bridge and custom logger bridges into log4j-api should be very small, but I'd like to avoid creating new idiosyncrasies if there's any chance we can avoid them.
I'm less worried about the concrete examples within this repo than I am about third-party custom bridge implementations that we're not aware of.
>
> But perhaps more importantly: Why would someone combine `log4j-slf4j-impl` with `log4j-to-jul` anyway? If one wanted to build an application and depended on 3rd-party libraries which use both Log4j and Slf4j, and wanted everything to end up in JUL, wouldn't one simply use `log4j-to-jul` + [`slf4j-jdk14`](https://www.slf4j.org/manual.html#swapping)?
I completely agree that it's not ideal, and well-understood systems won't do this, but it should still work correctly. I've encountered more systems than I'd like which log via a complex system of adapters rather than bridging directly to the desired framework. We can't control which bridges are used, only how the components we publish behave.
> (BTW Is there any advantage of [using `log4j-slf4j-impl`](https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/) over `slf4j-log4j12`? But either of these only seem to provide value, as far as I understand, if the end-goal of the logging redirection fun is `log4j-core`... with a `log4j-to-jul`, I'm not sure why anyone would want to combine it like that.)
`slf4j-log4j12` actually targets log4j-1.2 (log4j 1.x was last released in 2012 and EOL was announced in 2015)
>
> > (and most computationally expensive).
>
> I assumed that there actually should not be a performance problem in JUL about this (I think & hope), because `java.util.logging.LogRecord` only calls its private `inferCaller()` lazily if anyone actually invokes `getSourceClassName()` and `getSourceMethodName()`? (With that `needToInferCaller` thing.) Or am I missing something?
Right, when the relevant methods aren't called, it's cheap (no work is done!) however when they are called, it's relatively expensive compared to the other operations the logger is responsible for. I haven't benchmarked JUL location-aware methods in a long time, but I'd expect an output format which uses them to be substantially slower than one which does not.
---
I haven't had a chance to address the remaining pieces, but I need to run for dinner half-way through! Sorry for the delay, I'll try to get back to this either tonight or tomorrow :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r777230229
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
Review comment:
Done. Resolving this thread, and undrafting - this PR is now ready for review, from my side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775607939
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
Review comment:
Of course :-)
The final class means we don't have to design or review for all the ways an external consumer may decide to extend this class. This way we don't need to review as many edge cases, and we have much greater flexibility to change the class in the future without causing odd semantic breaks.
I don't believe there's any measurable performance impact (C2 should be able to optimize non-final and final methods similarly if the receiver type is always the same. Also, bytecode manipulation allows final classes to be subclassed, e.g. Mockito + inline mock maker)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1007091463
> I'll have to wait for that to be fixed, and then merge that in here and it should be green.
@carterkozak build for this now running (I expect it to be green).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1007756608
I think this is ready to merge once level agreement is fixed :-) Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1007291907
> The `JULLogger` class could be package private, as the type itself is never exposed directly, only used as a cast target in tests. This would give us leniency for future refactors. I think the same could apply to all the new classes except `JULProvider` which must be instantiable via ServiceLoader.
Done. But both the `JULProvider` AND the `JULLoggerContextFactory` must be `public`, otherwise we get a:
ERROR StatusLogger Unable to create class org.apache.logging.log4j.tojul.JULLoggerContextFactory specified in provider URL null
java.lang.IllegalAccessException: class org.apache.logging.log4j.LogManager cannot access a member of class org.apache.logging.log4j.tojul.JULLoggerContextFactory with modifiers "public"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775574350
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
+ record.setThrown(message.getThrowable());
Review comment:
Oh sorry I misinterpreted - this is about using message's getThrowable() iff t is null! Please ignore my comment above. Either way, this is now applied. Thank You!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775084582
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
I agree, it gives us a touch more flexibility. OTOH, if a new class does not declare protected methods, does it matter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r780546050
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
@carterkozak Better now? Resolve if OK, our shout if I'm somehow mixing up which way we're mapping what here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775604539
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
Review comment:
1: That's exactly right, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775604217
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
I think Gary meant to reply to https://github.com/apache/logging-log4j2/pull/653#discussion_r775083886
I haven't used the Truth library outside of perhaps a couple bug-fixes in guava/error-prone. I will be able to review much more quickly and easily if you use frameworks I'm familiar with. Truth appears to bring in a few large transitive dependencies as well. Unless we're consolidating all the assertj/junit asserts into truth, we're probably better off avoiding [the lava layer](http://mikehadlow.blogspot.com/2014/12/the-lava-layer-anti-pattern.html).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775607662
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
ACK, then I'll rework this to use AssertJ instead of Truth, no worries. (FYI I've added additional tests just now.) Expected timeframe: Coming couple of days, hopefully some time this week (but not today anymore and unlikely tomorrow).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775609043
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Huh - AssertJ and Truth are 99% (`isSameInstanceAs` => `isSameAs`) API compatible! COOL. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1005138990
Thanks, @vorburger :-)
It looks like our pom.xml has exclusions for `META-INF/services` in some modules `<exclude>src/main/resources/META-INF/services/**/*</exclude>`, but I think your solution is cleaner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1006005856
> make the Maven RAT plugin actually print out the offending files, instead of pointing to a file where these are listed
As far as I can tell this isn't something that's easily "just" possible - but I've created https://issues.apache.org/jira/browse/RAT-294 to suggest it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1007756608
I think this is ready to merge once level agreement is fixed :-) Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1006040324
> @carterkozak the RAT failure is back again, and I'm not sure I understand why, given I've only added 3 commits without
Please ignore, I'm tired and somehow mixed up tabs with older and current build logs.
This PR fails to build not because of RAT but because of a failing test, see #685 and #684.
I'll have to wait for that to be fixed, and then merge that in here and it should be green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775528603
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
Ah, right. So I personally really like https://truth.dev and assumed it was a "more modern" alternative to https://assertj.github.io/doc/... but it seems like AssertJ may have "caught up" since I last had a closer look at it (years ago) and also has that "fluent style" that I like in Truth (only now, or always had?). There is an internal process at work that is a tiny bit easier for me if I can use Truth instead of AssertJ, so I have to ask 😸 if this feedback is a "suggestion" or a "requirement", for merge? I totally do understand if you make it a hard requirement, for consistency across Log4j, and am happy to switch and adapt, but I thought I would ask just to double check once before doing so.
> I agree, it gives us a touch more flexibility.
Which one, AssertJ or Truth?
> OTOH, if a new class does not declare protected methods, does it matter?
Sorry I did not understand this (my choice of assertion framework here wasn't related to testing declared protected methods).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775575152
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
Review comment:
Sure, done. May I ask why, just for learning? Does this make a (real, noticeable, measurable) difference for performance? Or is it not for performance but "style" - you don't want classes to be "open" and extensible by users? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775609433
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
Review comment:
2. Is now all that's really left to do here (from my side); I'll look more into on one of the next few days, and then un-draft this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r778441809
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
> I'm a bit worried about these
Sure, let's improve this!
> mutating system properties is a surprising side-effect in a constructor,
> (...) Also no need to re-set the value on each object instantiation.
Right. Let me move that to... where would this fit better - say right in the `JULProvider`? Or, IMHO probably better & more appropriate actually (to me), in the `JULLoggerContext`. Would you like me to do that?
> and this will replace any value that's already set.
This particular point could of course also be very easily addressed by making it just a tiny bit smarter - read the property, and add our (two) packages (unless they are already set, while we are at it). Would you like me to do that?
> What if somebody else modifies it later?
They COULD break it yes - unless they are kind/smart enough to also do as above?
But apparently _"[The `jdk.logger.packages` system property is consulted only once](https://hg.openjdk.java.net/jdk9/jdk9/jdk/file/ddf8af0e536a/src/java.logging/share/classes/java/util/logging/LogRecord.java#l675)."_ anyway.
FYI I tried to search how common use of `jdk.logger.packages` across GitHub, but without the JDK, is; but seem to hit some GitHub index search internal issue [with this query which says 318 results](https://github.com/search?q=%22jdk.logger.packages%22+-%22The+system+property+%7B%40code+jdk.logger.packages%7D+can+define+a+comma+separated%22+-path%3Asrc%2Fjava.base%2Fshare+-filename%3ALoggerFinderLoaderTest.java+-filename%3A%2FSimpleConsoleLogger.java&type=Code) - but does not show them (to me).
> The other case we should consider is what happens if the log4j2 `log4j-slf4j-impl` module (or any other api bridge into log4j-api) is combined with `log4j-to-jul`. In the case of `log4j-slf4j-impl`, users will call `LoggerFactory.getLogger(clazz);` which returns a `org.apache.logging.slf4j.Log4jLogger` wrapping an ExtendedLogger (JULLogger). The fqcn field on the logevent will be `org.apache.logging.slf4j.Log4jLogger` which isn't included in this list of packages.
Nice catch! Would it suffice to simply add `org.apache.logging.slf4j` as a (hard-coded?) 3rd package name to this `jdk.logger.packages` list? Are you concerned about other API bridges? Are there any in `logging-log4j2`? Or are you thinking elsewhere?
But perhaps more importantly: Why would someone combine `log4j-slf4j-impl` with `log4j-to-jul` anyway? If one wanted to build an application and depended on 3rd-party libraries which use both Log4j and Slf4j, and wanted everything to end up in JUL, wouldn't one simply use `log4j-to-jul` + [`slf4j-jdk14`](https://www.slf4j.org/manual.html#swapping)? (BTW Is there any advantage of [using `log4j-slf4j-impl`](https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/) over `slf4j-log4j12`? But either of these only seem to provide value, as far as I understand, if the end-goal of the logging redirection fun is `log4j-core`... with a `log4j-to-jul`, I'm not sure why anyone would want to combine it like that.)
> (and most computationally expensive).
I assumed that there actually should not be a performance problem in JUL about this (I think & hope), because `java.util.logging.LogRecord` only calls its private `inferCaller()` lazily if anyone actually invokes `getSourceClassName()` and `getSourceMethodName()`? (With that `needToInferCaller` thing.) Or am I missing something?
> It may be reasonable to set the location values to `null` rather than allowing potentially incorrect values to be used, as this is likely the most complex part of the implementation
I'm happy to do that if that allows getting this merged sooner without this. Someone (myself or someone else) could then raise a follow-up PR about that, perhaps with a JIRA dedicated to it. Would you like me to do that and remove what's related to that? I'm asking to reach agreement instead of just going ahead to make changes, because it feels like we may be close to resolve this, so let's be explicit about what road we want to go.
> Otherwise using something like `StackLocatorUtil.calcLocation(fqcn)` in a subclass should do the trick.
Thanks for pointing out that utility; I ignored its existence. FYI [I actually initially explored a `LazyLogRecord`](https://github.com/apache/logging-log4j2/compare/release-2.x...vorburger:release-2.x_LOG4J2-3282_log4j-to-jul_LazyLogRecord#diff-cfaa47cd734fd2002c8729469acfa4c8c9a70f4b91d6ce9e0deff1106ddf0340), but then gave up on that because relying on that `jdk.logger.packages` system property seemed to work great and so much simpler. I'm still not convinved what's wrong with that, and I would personally prefer exploring this in a follow-up PR instead of adding this here, and set the location values to `null` for now, if you don't want to go ahead with the `jdk.logger.packages` system property approach (after minor improvement, as discussed above).
As for using `calcLocation(fqcn)`, would you be willing to educate me on the need for 340 lines of extra code (in `org.apache.logging.log4j.util.StackLocatorUtil` and `StackLocator` of `log4j-api`) for something that JUL seems to already be doing just fine on its own? That somehow... doesn't sit quite right, with me. But I certainly respect that YMMV, and background I may be missing.
PS: There is a bit of background here that may be useful to share in this context: Something I'd like to further investigate, after this PR is accepted, at first just internally at work and possible later as a public contribution of some form (unclear, really more of a thought experiment at this very early stage), is [if it may somehow be possible to have some sort of more "minimal" `log4j-api`](https://github.com/apache/logging-log4j2/compare/release-2.x...vorburger:release-2.x_split-API), for use only with `log4j-to-jul`... if I can avoid using `StackLocatorUtil` and `StackLocator` in `log4j-to-jul`, that's already 2 classes less from `log4j-api`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1005988157
> It looks like our pom.xml has exclusions for `META-INF/services` in some modules `<exclude>src/main/resources/META-INF/services/**/*</exclude>`, but I think your solution is cleaner.
@carterkozak the RAT failure is back again, and I'm not sure I understand why, given I've only added 3 commits without changes to license headers, or new files. (Or is this perhaps unrelated to this PR, like in #684?) Perhaps we could make the Maven RAT plugin actually print out the offending files, instead of pointing to a file where these are listed, which is always hard to debug on a headless build server.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak merged pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak merged pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1007291907
> The `JULLogger` class could be package private, as the type itself is never exposed directly, only used as a cast target in tests. This would give us leniency for future refactors. I think the same could apply to all the new classes except `JULProvider` which must be instantiable via ServiceLoader.
Done. But both the `JULProvider` AND the `JULLoggerContextFactory` must be `public`, otherwise we get a:
ERROR StatusLogger Unable to create class org.apache.logging.log4j.tojul.JULLoggerContextFactory specified in provider URL null
java.lang.IllegalAccessException: class org.apache.logging.log4j.LogManager cannot access a member of class org.apache.logging.log4j.tojul.JULLoggerContextFactory with modifiers "public"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775572900
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
+ record.setThrown(message.getThrowable());
Review comment:
`java.util.logging.LogRecord.setThrown(Throwable)` actually only does `this.thrown = thrown;` anyway, so this doesn't actually matter, but given that this has jumped out at you, let me commit your suggestion anyway, as it's clearer to read (to you).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775081486
##########
File path: pom.xml
##########
@@ -964,6 +964,19 @@
<version>0.8.1</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <!-- https://javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/TestLogHandler.html used in log4j-to-jul tests -->
+ <artifactId>guava-testlib</artifactId>
+ <version>31.0.1-jre</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.truth</groupId>
+ <artifactId>truth</artifactId>
+ <version>1.1.3</version>
+ <scope>test</scope>
+ </dependency>
Review comment:
How would you feel about [assertj](https://joel-costigliola.github.io/assertj/)? We already use it several places in this project, best to consolidate on a single library than use a different on per module :-)
I've written some automation to migrate junit asserts to assertj, on my backlog to apply to log4j. Time constraints are tough, though.
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
Review comment:
Good questions!
### 1. getFormattedMessage/getMessage/getParameters
JUL `LogRecord` and the log4j-api `Message` are similar in that they both provide a raw format string, and a parameter array. However, the log4j-api encodes how the format and parameters are combined in the `Message` implementation (`getFormattedMessage` + `formatTo(StringBuilder)` handle formatting) where JUL (default formatters anyhow) use [java.text.MessageFormat.format](https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/java.logging/share/classes/java/util/logging/Formatter.java#L92-L150).
Given that, we don't want to use `getFormat()` and definitely don't want to set the parameters here, otherwise there's JUL may do substitution on the user-provided message...and we don't want to go there again ;-)
### 2. Logger name/fqcn
We definitely want to set the logger name to `getName` which is the name of the logger as requested from the LogManager.
The `fqcn` is the fully qualified name of the logger implementation, e.g. `org.apache.logging.log4j.tojul.JULLogger`. This is useful to find the call-site, where we can traverse just above that fqcn. JUL supports this sort of thing using `getSourceClassName ` and `getSourceMethodName`, so it may be a good idea to support the functionality here, otherwise the `LogRecord` will always report `JULLogger.logMessage` as the caller. I'd subclass LogRecord to lazily evaluate the call-site when it's requested given the compute cost of StackWalker/etc.
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
+ record.setThrown(message.getThrowable());
Review comment:
I think we need to check the `Throwable t` parameter
```suggestion
record.setThrown(t == null ? message.getThrowable() : t);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r778374993
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
I'm a bit worried about these, mutating system properties is a surprising side-effect in a constructor, and this will replace any value that's already set. What if somebody else modifies it later? Also no need to re-set the value on each object instantiation.
The other case we should consider is what happens if the log4j2 `log4j-slf4j-impl` module (or any other api bridge into log4j-api) is combined with `log4j-to-jul`. In the case of `log4j-slf4j-impl`, users will call `LoggerFactory.getLogger(clazz);` which returns a `org.apache.logging.slf4j.Log4jLogger` wrapping an ExtendedLogger (JULLogger). The fqcn field on the logevent will be `org.apache.logging.slf4j.Log4jLogger` which isn't included in this list of packages.
It may be reasonable to set the location values to `null` rather than allowing potentially incorrect values to be used, as this is likely the most complex part of the implementation (and most computationally expensive). Otherwise using something like `StackLocatorUtil.calcLocation(fqcn)` in a subclass should do the trick.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r778441809
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
> I'm a bit worried about these
Sure, let's improve this!
> mutating system properties is a surprising side-effect in a constructor,
> (...) Also no need to re-set the value on each object instantiation.
Right. Let me move that to... where would this fit better - say right in the `JULProvider`? Or, IMHO probably better & more appropriate actually (to me), in the `JULLoggerContext`. Would you like me to do that?
> and this will replace any value that's already set.
This particular point could of course also be very easily addressed by making it just a tiny bit smarter - read the property, and add our (two) packages (unless they are already set, while we are at it). Would you like me to do that?
> What if somebody else modifies it later?
They COULD break it yes - unless they are kind/smart enough to also do as above?
But apparently _"[The `jdk.logger.packages` system property is consulted only once](https://hg.openjdk.java.net/jdk9/jdk9/jdk/file/ddf8af0e536a/src/java.logging/share/classes/java/util/logging/LogRecord.java#l675)."_ anyway.
FYI I tried to search how common use of `jdk.logger.packages` across GitHub, but without the JDK, is; but seem to hit some GitHub index search internal issue [with this query which says 318 results](https://github.com/search?q=%22jdk.logger.packages%22+-%22The+system+property+%7B%40code+jdk.logger.packages%7D+can+define+a+comma+separated%22+-path%3Asrc%2Fjava.base%2Fshare+-filename%3ALoggerFinderLoaderTest.java+-filename%3A%2FSimpleConsoleLogger.java&type=Code) - but does not show them (to me).
> The other case we should consider is what happens if the log4j2 `log4j-slf4j-impl` module (or any other api bridge into log4j-api) is combined with `log4j-to-jul`. In the case of `log4j-slf4j-impl`, users will call `LoggerFactory.getLogger(clazz);` which returns a `org.apache.logging.slf4j.Log4jLogger` wrapping an ExtendedLogger (JULLogger). The fqcn field on the logevent will be `org.apache.logging.slf4j.Log4jLogger` which isn't included in this list of packages.
Nice catch! Would it suffice to simply add `org.apache.logging.slf4j` as a (hard-coded?) 3rd package name to this `jdk.logger.packages` list? Are you concerned about other API bridges? Are there any in `logging-log4j2`? Or are you thinking elsewhere?
But perhaps more importantly: Why would someone combine `log4j-slf4j-impl` with `log4j-to-jul` anyway? If one wanted to build an application and depended on 3rd-party libraries which use both Log4j and Slf4j, and wanted everything to end up in JUL, wouldn't one simply use `log4j-to-jul` + [`slf4j-jdk14`](https://www.slf4j.org/manual.html#swapping)? (BTW Is there any advantage of [using `log4j-slf4j-impl`](https://logging.apache.org/log4j/2.x/log4j-slf4j-impl/) over `slf4j-log4j12`? But either of these only seem to provide value, as far as I understand, if the end-goal of the logging redirection fun is `log4j-core`... with a `log4j-to-jul`, I'm not sure why anyone would want to combine it like that.)
> (and most computationally expensive).
I assumed that there actually should not be a performance problem in JUL about this (I think & hope), because `java.util.logging.LogRecord` only calls its private `inferCaller()` lazily if anyone actually invokes `getSourceClassName()` and `getSourceMethodName()`? (With that `needToInferCaller` thing.) Or am I missing something?
> It may be reasonable to set the location values to `null` rather than allowing potentially incorrect values to be used, as this is likely the most complex part of the implementation
I'm happy to do that if that allows getting this merged sooner without this. Someone (myself or someone else) could then raise a follow-up PR about that, perhaps with a JIRA dedicated to it. Would you like me to do that and remove what's related to that? I'm asking to reach agreement instead of just going ahead to make changes, because it feels like we may be close to resolve this, so let's be explicit about what road we want to go.
> Otherwise using something like `StackLocatorUtil.calcLocation(fqcn)` in a subclass should do the trick.
Thanks for pointing out that utility; I ignored its existence. FYI [I actually initially explored a `LazyLogRecord`](https://github.com/apache/logging-log4j2/compare/release-2.x...vorburger:release-2.x_LOG4J2-3282_log4j-to-jul_LazyLogRecord#diff-cfaa47cd734fd2002c8729469acfa4c8c9a70f4b91d6ce9e0deff1106ddf0340), but then gave up on that because relying on that `jdk.logger.packages` system property seemed to work great and so much simpler. I'm still not convinved what's wrong with that, and I would personally prefer exploring this in a follow-up PR instead of adding this here, and set the location values to `null` for now, if you don't want to go ahead with the `jdk.logger.packages` system property approach (after minor improvement, as discussed above).
As for using `calcLocation(fqcn)`, would you be willing to educate me on the need for 340 lines of extra code (in `org.apache.logging.log4j.util.StackLocatorUtil` and `StackLocator` of `log4j-api`) for something that JUL seems to already be doing just fine on its own? That somehow... doesn't sit quite right, with me. But I certainly respect that YMMV, and background I may be missing.
PS: There is a bit of background here that may be useful to share in this context: Something I'd like to further investigate, after this PR is accepted, at first just internally at work and possible later as a public contribution of some form (unclear, really more of a thought experiment at this very early stage), is [if it may somehow be possible to have some sort of more "minimal" `log4j-api`](https://github.com/apache/logging-log4j2/compare/release-2.x...vorburger:release-2.x_split-API), for use only with `log4j-to-jul`... if I can avoid using `StackLocatorUtil` and `StackLocator` in `log4j-to-jul`, that's already 2 classes classes from `log4j-api`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r780541889
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
Oh! Right. I'll change this (right now).
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
@carterkozak Better now? Resolve if OK, our shout if I'm somehow mixing up which way we're mapping what here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r780541031
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
This doesn't agree with `convertLevel` above:
https://github.com/apache/logging-log4j2/blob/a58a06bf2365165ac5abdde931bb4ecd1adf0b3c/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java#L83-L84
Let's be consistent with the `log4j-jul` module https://github.com/apache/logging-log4j2/blob/a58a06bf2365165ac5abdde931bb4ecd1adf0b3c/log4j-jul/src/main/java/org/apache/logging/log4j/jul/DefaultLevelConverter.java#L55-L75
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775083886
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
Review comment:
My preference is final-by-default for new types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#issuecomment-1003744225
@carterkozak This is "code complete" and ready for your review, from my side.
PS: I was trying to figure out why Apache RAT initially choked on my new `log4j-to-jul/src/main/resources/META-INF/services/org.apache.logging.log4j.spi.Provider` without ASL header in `mvn verify` - even though it apparently doesn't seem to mind the exact same file in `log4j-core` and `log4j-to-slf4j` (and `log4j-api` test) to not have the license... I could not see why/where, and so opted to simply add the usual header there as well, to make RAT happy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r779049362
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
> This is a great point, I think it means that we cannot rely on setting the property anywhere in the bridge!
ACK, that now does make perfect sense to me as well (in retrospect).
>> (...) if that allows getting this merged sooner without this. (...) and remove what's related to that?
> This is entirely up to you, I'm happy either way.
All right, I've gone gone ahead, and (for now) simply removed support for `getSourceClassName()` & `getSourceMethodName()` - they now return `null`; this can be again revisited later in a follow-up PR.
Please re-re-review (sorry, and Thank You!) and lemme know how if any other feedback, or if this LGTY as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r778937770
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,266 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ System.setProperty("jdk.logger.packages", getClass().getPackageName() + "," + getClass().getSuperclass().getPackageName());
Review comment:
> I'm happy to do that if that allows getting this merged sooner without this. Someone (myself or someone else) could then raise a follow-up PR about that, perhaps with a JIRA dedicated to it. Would you like me to do that and remove what's related to that? I'm asking to reach agreement instead of just going ahead to make changes, because it feels like we may be close to resolve this, so let's be explicit about what road we want to go.
This is entirely up to you, I'm happy either way.
> As for using calcLocation(fqcn), would you be willing to educate me on the need for 340 lines of extra code (in org.apache.logging.log4j.util.StackLocatorUtil and StackLocator of log4j-api) for something that JUL seems to already be doing just fine on its own? That somehow... doesn't sit quite right, with me. But I certainly respect that YMMV, and background I may be missing.
The issue here is that JUL isn't doing this well enough for wrapped loggers -- the configuration point that JUL provides is statically initialized and cannot be (reasonably) mutated after it is configured. I believe this is based on the assumption that an application will use exactly one logging facade, however modern java services tend to pull together many libraries in a way that invalidates such a model. The JUL model can certainly work if you're able to add all the paths to jvm startup args, but setting them later doesn't give me confidence that the values will always be respected. It's unlikely that all users will correctly set values in the jvm startup args.
> PS: There is a bit of background here that may be useful to share in this context: Something I'd like to further investigate, after this PR is accepted, at first just internally at work and possible later as a public contribution of some form (unclear, really more of a thought experiment at this very early stage), is if it may somehow be possible to have some sort of more "minimal" log4j-api, for use only with log4j-to-jul... if I can avoid using StackLocatorUtil and StackLocator in log4j-to-jul, that's already 2 classes less from log4j-api.
I appreciate the context. Reducing coupling and reducing api size are reasonable goals, however I have a hard time justifying incomplete or incorrect behavior based on a thought experiment. What would prevent a future contributor from making an update which relies on an unexpected component?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r780541031
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
This doesn't agree with `convertLevel` above:
https://github.com/apache/logging-log4j2/blob/a58a06bf2365165ac5abdde931bb4ecd1adf0b3c/log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java#L83-L84
Let's be consistent with the `log4j-jul` module https://github.com/apache/logging-log4j2/blob/a58a06bf2365165ac5abdde931bb4ecd1adf0b3c/log4j-jul/src/main/java/org/apache/logging/log4j/jul/DefaultLevelConverter.java#L55-L75
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r780541889
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,267 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+final class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ private final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public Logger getWrappedLogger() {
+ return logger;
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // NOT getFormat()
+ // NOT record.setParameters(message.getParameters()); BECAUSE getFormattedMessage() NOT getFormat()
+ record.setLoggerName(getName());
+ record.setThrown(t == null ? message.getThrowable() : t);
+ // Source class/method is not supported (yet)
+ record.setSourceClassName(null);
+ record.setSourceMethodName(null);
+ logger.log(record);
+ // fqcn is un-used
+ }
+
+ // Convert Level in Log4j scale to JUL scale.
+ // See getLevel() for the mapping. Note that JUL's FINEST & CONFIG are never returned because Log4j has no such levels, and
+ // that Log4j's FATAL is simply mapped to JUL's SEVERE as is Log4j's ERROR because JUL does not distinguish between ERROR and FATAL.
+ private java.util.logging.Level convertLevel(final Level level) {
+ switch (level.getStandardLevel()) {
+ // Test in logical order of likely frequency of use
+ // Must be kept in sync with #getLevel()
+ case ALL:
+ return java.util.logging.Level.ALL;
+ case TRACE:
+ return java.util.logging.Level.FINER;
+ case DEBUG:
+ return java.util.logging.Level.FINE;
+ case INFO:
+ return java.util.logging.Level.INFO;
+ case WARN:
+ return java.util.logging.Level.WARNING;
+ case ERROR:
+ return java.util.logging.Level.SEVERE;
+ case FATAL:
+ return java.util.logging.Level.SEVERE;
+ case OFF:
+ return java.util.logging.Level.OFF;
+ default:
+ // This is tempting: throw new IllegalStateException("Impossible Log4j Level encountered: " + level.intLevel());
+ // But it's not a great idea, security wise. If an attacker *SOMEHOW* managed to create a Log4j Level instance
+ // with an unexpected level (through JVM de-serialization, despite readResolve() { return Level.valueOf(this.name); },
+ // or whatever other means), then we would blow up in a very unexpected place and way. Let us therefore instead just
+ // return SEVERE for unexpected values, because that's more likely to be noticed than a FINER.
+ // Greetings, Michael Vorburger.ch <http://www.vorburger.ch>, for Google, on 2021.12.24.
+ return java.util.logging.Level.SEVERE;
+ }
+ }
+
+ /**
+ * Level in Log4j scale.
+ * JUL Levels are mapped as follows:
+ * <ul>
+ * <li>OFF => OFF
+ * <li>SEVERE => ERROR
+ * <li>WARNING => WARN
+ * <li>INFO => INFO
+ * <li>CONFIG => INFO
+ * <li>FINE => DEBUG
+ * <li>FINER => DEBUG (inspired by https://github.com/qos-ch/slf4j/blob/6e784e4ca3a5ae9c5dc421fcd01a417af5bf5ace/jul-to-slf4j/src/main/java/org/slf4j/bridge/SLF4JBridgeHandler.java)
Review comment:
Oh! Right. I'll change this (right now).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak merged pull request #653: Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
carterkozak merged pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775577767
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
+ private static final long serialVersionUID = 1L;
+
+ // @VisibleForTesting
+ final Logger logger;
+
+ // This implementation is inspired by org.apache.logging.slf4j.SLF4JLogger
+
+ public JULLogger(final String name, final MessageFactory messageFactory, final Logger logger) {
+ super(name, messageFactory);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ public JULLogger(final String name, final Logger logger) {
+ super(name);
+ this.logger = requireNonNull(logger, "logger");
+ }
+
+ @Override
+ public void logMessage(final String fqcn, final Level level, final Marker marker, final Message message, final Throwable t) {
+ java.util.logging.Level julLevel = convertLevel(level);
+ if (!logger.isLoggable(julLevel)) {
+ return;
+ }
+ LogRecord record = new LogRecord(julLevel, message.getFormattedMessage()); // TODO getFormat() or getFormattedMessage() ?
+ record.setLoggerName(getName()); // TODO getName() or fqcn parameter? What's that?
+ record.setParameters(message.getParameters());
Review comment:
1. So KEEP `new LogRecord(julLevel, message.getFormattedMessage())` as-is, and REMOVE the `// TODO getFormat() or getFormattedMessage() ?` AND also remove the `record.setParameters(message.getParameters());` - am I getting this right? I've done this now, could you just double check I got this right and ACK 1. is fine.
2. I see. Let me get back to that point. (As I'm adding more test coverage I'm finding that other -more basic- things need a bit more work, first.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vorburger commented on a change in pull request #653: (DRAFT, WIP - DO NOT MERGE YET, BUT INITIAL REVIEW WELCOME) Release 2.x log4j-to-jul JDK Logging Bridge (LOG4J2-3282)
Posted by GitBox <gi...@apache.org>.
vorburger commented on a change in pull request #653:
URL: https://github.com/apache/logging-log4j2/pull/653#discussion_r775609860
##########
File path: log4j-to-jul/src/main/java/org/apache/logging/log4j/tojul/JULLogger.java
##########
@@ -0,0 +1,246 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.tojul;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Marker;
+import org.apache.logging.log4j.message.Message;
+import org.apache.logging.log4j.message.MessageFactory;
+import org.apache.logging.log4j.spi.AbstractLogger;
+
+/**
+ * Implementation of {@link org.apache.logging.log4j.Logger} that's backed by a {@link Logger}.
+ *
+ * This implementation currently ignores {@link Marker}.
+ *
+ * @author <a href="http://www.vorburger.ch">Michael Vorburger.ch</a> for Google
+ */
+public class JULLogger extends AbstractLogger {
Review comment:
Thanks! Resolving.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org