diff --git a/core/src/main/java/hudson/logging/LogRecorder.java b/core/src/main/java/hudson/logging/LogRecorder.java index ac89fcc9e9ca..6f81846a3ad6 100644 --- a/core/src/main/java/hudson/logging/LogRecorder.java +++ b/core/src/main/java/hudson/logging/LogRecorder.java @@ -324,10 +324,14 @@ public Logger getLogger() { * Makes sure that the logger passes through messages at the correct level to us. */ public void enable() { + enableLogger(); + new SetLevel(name, getLevel()).broadcast(); + } + + private void enableLogger() { Logger l = getLogger(); if (!l.isLoggable(getLevel())) l.setLevel(getLevel()); - new SetLevel(name, getLevel()).broadcast(); } public void disable() { @@ -409,6 +413,63 @@ public LogRecorderManager getParent() { return Jenkins.get().getLog(); } + /** + * Updates controller logger levels after existing targets are reconfigured or removed. {@link Target#enable()} + * only makes a logger more verbose, so it cannot otherwise apply a change such as {@code FINEST} to {@code INFO}. + */ + private void updateLogLevels(List previousTargets) { + List recorders = getParent().getRecorders(); + Set changedTargetNames = new LinkedHashSet<>(); + for (Target previousTarget : previousTargets) { + Target target = getTarget(previousTarget.name, loggers); + if (target != null && target.getLevel().intValue() <= previousTarget.getLevel().intValue()) { + continue; + } + changedTargetNames.add(previousTarget.name); + if (sameLevel(previousTarget.getLogger().getLevel(), previousTarget.getLevel())) { + previousTarget.getLogger().setLevel(null); + } + } + + for (LogRecorder recorder : recorders) { + for (Target target : recorder.loggers) { + if (changedTargetNames.contains(target.name)) { + target.enableLogger(); + } + } + } + + List descendantTargets = new ArrayList<>(); + for (LogRecorder recorder : recorders) { + for (Target target : recorder.loggers) { + for (String name : changedTargetNames) { + if (isDescendant(name, target.name)) { + descendantTargets.add(target); + break; + } + } + } + } + descendantTargets.sort(Comparator.comparingInt(target -> target.name.length())); + descendantTargets.forEach(Target::enableLogger); + } + + private static Target getTarget(String loggerName, List targets) { + return targets.stream().filter(target -> target.name.equals(loggerName)).findFirst().orElse(null); + } + + private static boolean isDescendant(String loggerName, String candidate) { + return loggerName.isEmpty() + ? !candidate.isEmpty() + : candidate.startsWith(loggerName) + && candidate.length() > loggerName.length() + && candidate.charAt(loggerName.length()) == '.'; + } + + private static boolean sameLevel(Level first, Level second) { + return first == null ? second == null : second != null && first.intValue() == second.intValue(); + } + /** * Accepts submission from the configuration page. */ @@ -431,10 +492,11 @@ public synchronized void doConfigSubmit(StaplerRequest2 req, StaplerResponse2 rs redirect = "../" + Util.rawEncode(newName) + '/'; } + List previousTargets = List.copyOf(loggers); List newTargets = req.bindJSONToList(Target.class, src.get("loggers")); setLoggers(newTargets); - save(); + save(previousTargets); if (oldFile != null) oldFile.delete(); FormApply.success(redirect).generateResponse(req, rsp, null); } @@ -461,10 +523,17 @@ public synchronized void load() throws IOException { */ @Override public synchronized void save() throws IOException { + save(null); + } + + private synchronized void save(List previousTargets) throws IOException { if (BulkChange.contains(this)) return; getConfigFile().write(this); loggers.forEach(Target::enable); + if (previousTargets != null) { + updateLogLevels(previousTargets); + } SaveableListener.fireOnChange(this, getConfigFile()); } diff --git a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java index 799d62cc775e..92072cd844a5 100644 --- a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java +++ b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java @@ -33,7 +33,9 @@ import static org.hamcrest.Matchers.hasSize; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -53,6 +55,7 @@ import java.util.logging.SimpleFormatter; import jenkins.security.MasterToSlaveCallable; import org.htmlunit.FailingHttpStatusCodeException; +import org.htmlunit.WebClientUtil; import org.htmlunit.html.HtmlForm; import org.htmlunit.html.HtmlPage; import org.junit.jupiter.api.BeforeEach; @@ -149,6 +152,206 @@ void createLogRecorderWithNonAsciiName() throws Exception { j.createWebClient().goTo("log/" + Util.rawEncode(name) + "/configure"); } + @Issue("JENKINS-75310") + @Test + void changingRecorderToLessVerboseLevelUpdatesLogger() throws Exception { + String loggerName = getClass().getName() + ".lessVerbose"; + Logger logger = Logger.getLogger(loggerName); + LogRecorder recorder = new LogRecorder("recorder"); + recorder.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST))); + j.jenkins.getLog().getRecorders().add(recorder); + + try { + recorder.save(); + assertEquals(Level.FINEST, logger.getLevel()); + + configureLevel(recorder, Level.INFO); + + assertNotEquals(Level.FINEST, logger.getLevel()); + assertFalse(logger.isLoggable(Level.FINER)); + } finally { + logger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void changingParentTargetPreservesIntermediateManualLevel() throws Exception { + String parentLoggerName = getClass().getName() + ".manual"; + String intermediateLoggerName = parentLoggerName + ".intermediate"; + String childLoggerName = intermediateLoggerName + ".child"; + Logger parentLogger = Logger.getLogger(parentLoggerName); + Logger intermediateLogger = Logger.getLogger(intermediateLoggerName); + Logger childLogger = Logger.getLogger(childLoggerName); + LogRecorder parent = new LogRecorder("manual-parent"); + parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINE))); + LogRecorder child = new LogRecorder("manual-child"); + child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.WARNING))); + j.jenkins.getLog().getRecorders().addAll(List.of(parent, child)); + + try { + parent.save(); + child.save(); + intermediateLogger.setLevel(Level.FINEST); + assertNull(childLogger.getLevel()); + + configureLevel(parent, Level.INFO); + + assertFalse(parentLogger.isLoggable(Level.FINE)); + assertEquals(Level.FINEST, intermediateLogger.getLevel()); + assertNull(childLogger.getLevel()); + assertTrue(childLogger.isLoggable(Level.FINER)); + } finally { + parentLogger.setLevel(null); + intermediateLogger.setLevel(null); + childLogger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void changingRecorderPreservesMoreVerboseSharedTarget() throws Exception { + String loggerName = getClass().getName() + ".shared"; + Logger logger = Logger.getLogger(loggerName); + LogRecorder first = new LogRecorder("first"); + first.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST))); + LogRecorder second = new LogRecorder("second"); + second.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINE))); + j.jenkins.getLog().getRecorders().addAll(List.of(first, second)); + + try { + first.save(); + second.save(); + assertEquals(Level.FINEST, logger.getLevel()); + + configureLevel(first, Level.INFO); + + assertEquals(Level.FINE, logger.getLevel()); + } finally { + logger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void changingChildTargetDoesNotPinInheritedRecorderLevel() throws Exception { + String parentLoggerName = getClass().getName() + ".sharedParent"; + String childLoggerName = parentLoggerName + ".child"; + Logger parentLogger = Logger.getLogger(parentLoggerName); + Logger childLogger = Logger.getLogger(childLoggerName); + LogRecorder parent = new LogRecorder("shared-parent"); + parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINEST))); + LogRecorder child = new LogRecorder("shared-child"); + child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.INFO))); + j.jenkins.getLog().getRecorders().addAll(List.of(parent, child)); + + try { + parent.save(); + child.save(); + assertNull(childLogger.getLevel()); + + configureLevel(child, Level.WARNING); + + assertNull(childLogger.getLevel()); + parent.delete(); + assertNull(childLogger.getLevel()); + assertFalse(childLogger.isLoggable(Level.FINER)); + } finally { + parentLogger.setLevel(null); + childLogger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void changingParentTargetKeepsChildTargetLoggable() throws Exception { + String parentLoggerName = getClass().getName() + ".parent"; + String childLoggerName = parentLoggerName + ".child"; + Logger parentLogger = Logger.getLogger(parentLoggerName); + Logger childLogger = Logger.getLogger(childLoggerName); + LogRecorder parent = new LogRecorder("parent"); + parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINE))); + LogRecorder child = new LogRecorder("child"); + child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.FINE))); + j.jenkins.getLog().getRecorders().addAll(List.of(parent, child)); + + try { + parent.save(); + child.save(); + assertNull(childLogger.getLevel()); + + configureLevel(parent, Level.INFO); + + assertFalse(parentLogger.isLoggable(Level.FINE)); + assertEquals(Level.FINE, childLogger.getLevel()); + } finally { + parentLogger.setLevel(null); + childLogger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void removingRecorderTargetClearsLoggerLevel() throws Exception { + String loggerName = getClass().getName() + ".removed"; + Logger logger = Logger.getLogger(loggerName); + LogRecorder recorder = new LogRecorder("removed"); + recorder.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST))); + j.jenkins.getLog().getRecorders().add(recorder); + + try { + recorder.save(); + assertEquals(Level.FINEST, logger.getLevel()); + + removeTarget(recorder); + + assertTrue(recorder.getLoggers().isEmpty()); + assertNull(logger.getLevel()); + } finally { + logger.setLevel(null); + } + } + + @Issue("JENKINS-75310") + @Test + void removingRecorderTargetPreservesSharedTarget() throws Exception { + String loggerName = getClass().getName() + ".removedShared"; + Logger logger = Logger.getLogger(loggerName); + LogRecorder first = new LogRecorder("removed-first"); + first.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST))); + LogRecorder second = new LogRecorder("removed-second"); + second.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINE))); + j.jenkins.getLog().getRecorders().addAll(List.of(first, second)); + + try { + first.save(); + second.save(); + assertEquals(Level.FINEST, logger.getLevel()); + + removeTarget(first); + + assertEquals(Level.FINE, logger.getLevel()); + } finally { + logger.setLevel(null); + } + } + + private void configureLevel(LogRecorder recorder, Level level) throws Exception { + HtmlPage page = j.createWebClient().goTo("log/" + Util.rawEncode(recorder.getName()) + "/configure"); + HtmlForm form = page.getFormByName("config"); + form.getSelectByName("level").getOptionByValue(level.getName()).setSelected(true); + j.submit(form); + } + + private void removeTarget(LogRecorder recorder) throws Exception { + JenkinsRule.WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.goTo("log/" + Util.rawEncode(recorder.getName()) + "/configure"); + HtmlForm form = page.getFormByName("config"); + j.getButtonByCaption(form, "Delete").click(); + WebClientUtil.waitForJSExec(webClient); + j.submit(form); + } + @Issue({"JENKINS-18274", "JENKINS-63458"}) @Test void loggingOnSlaves() throws Exception {