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