gnodet-bot commented on code in PR #13832: URL: https://github.com/apache/maven/pull/13832#discussion_r4488420975
########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/LoggerManager.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.maven.api.logging; + +import java.util.ServiceConfigurationError; +import java.util.ServiceLoader; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.annotations.Provider; + +/** + * Central entry point for Maven's logging system. + * <p> + * Provides access to loggers, the active {@link LogSink}, level configuration, + * and severity tracking for {@code --fail-on-severity}. + * <p> + * The singleton is resolved lazily on the first {@link #get()} call using + * {@link ServiceLoader}. If no provider is found, a buffering fallback is + * installed that accumulates events until a real implementation is set via + * {@link #set(LoggerManager)}. + * + * @since 4.2.0 + */ +@Experimental +@Provider +public interface LoggerManager { + + /** + * Returns a {@link Logger} instance for the given name. + */ + @Nonnull + Logger getLogger(@Nonnull String name); + + /** + * Returns a {@link Logger} instance for the given class. + */ + @Nonnull + default Logger getLogger(@Nonnull Class<?> clazz) { + return getLogger(clazz.getName()); + } + + /** + * Returns the active log sink, or {@code null} if none is installed. + */ + @Nullable + LogSink getLogSink(); + + /** + * Installs or removes the active log sink. + */ + void setLogSink(@Nullable LogSink sink); + + /** + * Returns whether logging at {@code level} is enabled for the named logger. + */ + boolean isLevelEnabled(@Nonnull String loggerName, @Nonnull LogLevel level); + + /** + * Sets the root logger level (typically from {@code -X}, {@code -q} CLI flags). + * The new level takes effect immediately. + */ + void setRootLoggerLevel(@Nonnull LogLevel level); + + /** + * Reloads level configuration and resets severity tracking state. + */ + void reconfigure(); + + /** + * Returns {@code true} if any log event at or above {@code level} + * has been emitted since the last {@link #reconfigure()}. + * <p> + * Used by {@code --fail-on-severity}: the CLI stores the threshold + * and checks at build end. + */ + boolean hasReached(@Nonnull LogLevel level); + + /** + * Registers a classloader as belonging to a specific plugin. + * Used by the plugin manager to enable attribution of log events + * to the plugin that produced them. + * + * @param classLoader the plugin's classloader + * @param pluginId the plugin identifier (e.g. {@code "groupId:artifactId:version"}) + */ Review Comment: ⚠️ **Missing `@since` on `registerPluginClassLoader()`** `LogEvent.pluginId()` (added in this same commit) carries `@since 4.2.0` on the method. These two new default methods lack it, making the Javadoc coverage inconsistent within this PR. ```suggestion * * @param classLoader the plugin's classloader * @param pluginId the plugin identifier (e.g. {@code "groupId:artifactId:version"}) * @since 4.2.0 */ ``` ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/LoggerManager.java: ########## @@ -0,0 +1,202 @@ +/* + * 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.maven.api.logging; + +import java.util.ServiceConfigurationError; +import java.util.ServiceLoader; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.annotations.Provider; + +/** + * Central entry point for Maven's logging system. + * <p> + * Provides access to loggers, the active {@link LogSink}, level configuration, + * and severity tracking for {@code --fail-on-severity}. + * <p> + * The singleton is resolved lazily on the first {@link #get()} call using + * {@link ServiceLoader}. If no provider is found, a buffering fallback is + * installed that accumulates events until a real implementation is set via + * {@link #set(LoggerManager)}. + * + * @since 4.2.0 + */ +@Experimental +@Provider +public interface LoggerManager { + + /** + * Returns a {@link Logger} instance for the given name. + */ + @Nonnull + Logger getLogger(@Nonnull String name); + + /** + * Returns a {@link Logger} instance for the given class. + */ + @Nonnull + default Logger getLogger(@Nonnull Class<?> clazz) { + return getLogger(clazz.getName()); + } + + /** + * Returns the active log sink, or {@code null} if none is installed. + */ + @Nullable + LogSink getLogSink(); + + /** + * Installs or removes the active log sink. + */ + void setLogSink(@Nullable LogSink sink); + + /** + * Returns whether logging at {@code level} is enabled for the named logger. + */ + boolean isLevelEnabled(@Nonnull String loggerName, @Nonnull LogLevel level); + + /** + * Sets the root logger level (typically from {@code -X}, {@code -q} CLI flags). + * The new level takes effect immediately. + */ + void setRootLoggerLevel(@Nonnull LogLevel level); + + /** + * Reloads level configuration and resets severity tracking state. + */ + void reconfigure(); + + /** + * Returns {@code true} if any log event at or above {@code level} + * has been emitted since the last {@link #reconfigure()}. + * <p> + * Used by {@code --fail-on-severity}: the CLI stores the threshold + * and checks at build end. + */ + boolean hasReached(@Nonnull LogLevel level); + + /** + * Registers a classloader as belonging to a specific plugin. + * Used by the plugin manager to enable attribution of log events + * to the plugin that produced them. + * + * @param classLoader the plugin's classloader + * @param pluginId the plugin identifier (e.g. {@code "groupId:artifactId:version"}) + */ + default void registerPluginClassLoader(@Nonnull ClassLoader classLoader, @Nonnull String pluginId) {} + + /** + * Returns the plugin identifier for the given classloader, or {@code null} + * if the classloader is not a registered plugin classloader. + * + * @param classLoader the classloader to look up + * @return the plugin identifier, or {@code null} + */ Review Comment: ⚠️ **Missing `@since` on `getPluginId()`** Same issue as `registerPluginClassLoader()`: method-level `@since` is missing while the sibling `LogEvent.pluginId()` added in this commit carries it. ```suggestion * * @param classLoader the classloader to look up * @return the plugin identifier, or {@code null} * @since 4.2.0 */ ``` ########## impl/maven-logging-jul/src/main/java/org/apache/maven/logging/jul/MavenJulHandler.java: ########## @@ -0,0 +1,223 @@ +/* + * 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.maven.logging.jul; + +import java.text.MessageFormat; +import java.util.MissingResourceException; +import java.util.ResourceBundle; +import java.util.concurrent.atomic.AtomicLong; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogManager; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import org.apache.maven.api.MonotonicClock; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.LogLevel; +import org.apache.maven.api.logging.LogSink; +import org.apache.maven.api.logging.LoggerManager; +import org.apache.maven.logging.impl.DefaultLogEvent; +import org.apache.maven.logging.impl.DefaultLoggerManager; + +/** + * A JUL {@link Handler} that routes {@code java.util.logging} events into + * Maven's structured logging pipeline via {@link LoggerManager}, preserving + * the rich {@link LogRecord} metadata (source class name, source method name, + * thread ID) that the standard {@code SLF4JBridgeHandler} silently drops. + * <p> + * This handler has no SLF4J dependency — events go directly through + * {@link LoggerManager#get()} for level checking and sink dispatch. + * + * @since 4.2.0 + * @see #install() + */ +public class MavenJulHandler extends Handler { + + private static final AtomicLong SEQUENCE = new AtomicLong(); + + /** + * Re-entrancy guard: set to {@code true} while {@link #publish} is routing + * a JUL event through the sink on this thread. Prevents recursive JUL events + * from re-entering {@code publish} and causing infinite loops. + */ + private static final ThreadLocal<Boolean> IN_PUBLISH = new ThreadLocal<>(); + + static void setInPublishForTest(boolean inPublish) { + if (inPublish) { + IN_PUBLISH.set(Boolean.TRUE); + } else { + IN_PUBLISH.remove(); + } + } + + /** + * Installs this handler on the JUL root logger, removing any + * previously installed handlers. + */ + public static void install() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + for (Handler handler : rootLogger.getHandlers()) { + rootLogger.removeHandler(handler); + } + rootLogger.addHandler(new MavenJulHandler()); + } + + /** + * Returns {@code true} if a {@code MavenJulHandler} is installed + * on the JUL root logger. + */ + public static boolean isInstalled() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + for (Handler handler : rootLogger.getHandlers()) { + if (handler instanceof MavenJulHandler) { + return true; + } + } + return false; + } + + @Override + @SuppressWarnings("deprecation") // LogRecord.getThreadID() + public void publish(LogRecord record) { + if (record == null || !isLoggable(record)) { + return; + } + + if (IN_PUBLISH.get() != null) { + return; + } + + LoggerManager mgr = LoggerManager.get(); + + String loggerName = record.getLoggerName(); + if (loggerName == null) { + loggerName = ""; + } + + LogLevel level = julLevelToLogLevel(record.getLevel()); + if (!mgr.isLevelEnabled(loggerName, level)) { + return; + } + + IN_PUBLISH.set(Boolean.TRUE); + try { + LogSink sink = mgr.getLogSink(); + if (sink == null) { + return; + } + + String message = formatMessage(record); + Throwable throwable = record.getThrown(); + + String sourceClass = null; + String sourceMethod = null; + long threadId = -1; + String threadName = null; + + if (sink.needsMetadata()) { + sourceClass = record.getSourceClassName(); + sourceMethod = record.getSourceMethodName(); + threadId = record.getLongThreadID(); + threadName = Thread.currentThread().getName(); + } + + String pluginId = resolvePluginId(mgr); + + LogEvent event = new DefaultLogEvent( + MonotonicClock.now(), + level, + message, + loggerName, + throwable, + sourceClass, + sourceMethod, + threadId, + SEQUENCE.getAndIncrement(), + threadName, + null, + null, + pluginId, + null, + null, + (java.util.List<Object>) null); + + sink.accept(event); + + if (mgr instanceof DefaultLoggerManager dlm) { + dlm.record(level); + } + } finally { + IN_PUBLISH.remove(); + } + } + + @Override + public void flush() {} + + @Override + public void close() throws SecurityException {} + + private static String resolvePluginId(LoggerManager mgr) { + ClassLoader cl = Thread.currentThread().getContextClassLoader(); + return cl != null ? mgr.getPluginId(cl) : null; + } Review Comment: ⚠️ **Plugin attribution strategy diverges between JUL and SLF4J** JUL resolves `pluginId` at **log time** using only `Thread.currentThread().getContextClassLoader()` — a single classloader. SLF4J (in `MavenLoggerFactory.resolvePluginId()`) resolves it at **logger-creation time** by walking the full call stack with `StackWalker` to find any frame whose declaring class's classloader is a registered plugin classloader. These strategies can diverge in production: - A plugin using JUL and running on a **core thread** (e.g. a Maven-managed thread pool thread) whose context classloader is the core realm will produce events with `pluginId = null`, even if the call stack clearly originates from plugin code. - Conversely, a plugin using SLF4J that gets its logger from a framework-managed thread could have the `pluginId` resolved from a different call stack frame. Consider aligning the two modules: the JUL handler could also use a `StackWalker` scan for a registered classloader, the same way `MavenLoggerFactory` does. The `StackWalker` instance can be initialized as a static field (cost is per-call not per-init): ```suggestion private static final StackWalker STACK_WALKER = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); private static String resolvePluginId(LoggerManager mgr) { return STACK_WALKER.walk(frames -> frames .map(f -> mgr.getPluginId(f.getDeclaringClass().getClassLoader())) .filter(java.util.Objects::nonNull) .findFirst() .orElse(null)); } ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
