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-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
