From 07e84e8a8b31397ec63d401c8257c10172310421 Mon Sep 17 00:00:00 2001 From: Wu Sheng Date: Tue, 6 Oct 2026 23:16:00 +0800 Subject: [PATCH 1/2] Support logback 1.6.x in the logback toolkit layouts Logback 1.6.0 removed PatternLayout.defaultConverterMap, which TraceIdPatternLogbackLayout and TraceIdMDCPatternLogbackLayout wrote their conversion words to in a static initializer, so both failed with NoSuchFieldError: defaultConverterMap (apache/skywalking#14120). The layouts now register their conversion words in the logging context's conversion rule registry (CoreConstants.PATTERN_RULE_REGISTRY) when they start, updating it in place as logback does for . Logback 1.2.x to 1.6.x all read class names from that registry, so the same toolkit artifact keeps working on older logback. Logback 1.5.13 is not supported because of an upstream regression fixed in 1.5.14 (qos-ch/logback#885). The toolkit now compiles against logback 1.6.5, so an API removal fails the build. LogbackVersionCompatibilityTest renders both layouts against logback 1.2.13, 1.3.16, 1.4.14, 1.5.12, 1.5.14, 1.5.38 and 1.6.5, each loaded in an isolated class loader. Add apm-toolkit-logback-scenario, the first plugin test of the logback toolkit and its agent activation. It runs logback 1.2.x to 1.5.x with the released toolkit and checks the trace ID and SkyWalking context rendered by both layouts, including behind an AsyncAppender, through the gRPC log reporter. --- .github/workflows/plugins-jdk17-test.0.yaml | 1 + CHANGES.md | 7 + .../apm-toolkit-logback-1.x/pom.xml | 119 +++++++++++++++- .../AbstractTraceIdPatternLogbackLayout.java | 76 +++++++++++ .../v1/x/TraceIdPatternLogbackLayout.java | 11 +- .../x/mdc/TraceIdMDCPatternLogbackLayout.java | 15 ++- .../toolkit/log/logback/v1/x/LayoutProbe.java | 62 +++++++++ .../v1/x/LogbackVersionCompatibilityTest.java | 106 +++++++++++++++ .../v1/x/TraceIdPatternLogbackLayoutTest.java | 97 +++++++++++++ .../Application-toolkit-logback-1.x.md | 35 +++++ .../bin/startup.sh | 22 +++ .../config/expectedData.yaml | 83 ++++++++++++ .../configuration.yml | 20 +++ .../apm-toolkit-logback-scenario/pom.xml | 127 ++++++++++++++++++ .../src/main/assembly/assembly.xml | 41 ++++++ .../apm/testcase/logback/Application.java | 33 +++++ .../apm/testcase/logback/CaseHandler.java | 88 ++++++++++++ .../src/main/resources/logback.xml | 53 ++++++++ .../support-version.list | 23 ++++ 19 files changed, 1007 insertions(+), 12 deletions(-) create mode 100644 apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java create mode 100644 apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LayoutProbe.java create mode 100644 apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LogbackVersionCompatibilityTest.java create mode 100644 apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java create mode 100755 test/plugin/scenarios/apm-toolkit-logback-scenario/bin/startup.sh create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/config/expectedData.yaml create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/configuration.yml create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/pom.xml create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/assembly/assembly.xml create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/Application.java create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/CaseHandler.java create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/resources/logback.xml create mode 100644 test/plugin/scenarios/apm-toolkit-logback-scenario/support-version.list diff --git a/.github/workflows/plugins-jdk17-test.0.yaml b/.github/workflows/plugins-jdk17-test.0.yaml index 60a230e812..73aa926c3c 100644 --- a/.github/workflows/plugins-jdk17-test.0.yaml +++ b/.github/workflows/plugins-jdk17-test.0.yaml @@ -86,6 +86,7 @@ jobs: - graphql-20plus-scenario - spring-kafka-3.x-scenario - undertow-2.3.x-scenario + - apm-toolkit-logback-scenario steps: - uses: actions/checkout@v2 with: diff --git a/CHANGES.md b/CHANGES.md index add61965cb..ca82a5c427 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -66,6 +66,13 @@ Release Notes. ... Could not find artifact org.apache.skywalking:java-agent:pom:9.7.0` (apache/skywalking#13988). * Stop reporting datasource timeout configuration as metrics. The c3p0 `maxIdleTime`, DBCP `maxWaitMillis` and HikariCP `connectionTimeout`, `validationTimeout`, `idleTimeout` and `leakDetectionThreshold` gauges are no longer reported. +* Support logback 1.6.x in `apm-toolkit-logback-1.x` (apache/skywalking#14120). `TraceIdPatternLogbackLayout` and + `TraceIdMDCPatternLogbackLayout` failed with `NoSuchFieldError: defaultConverterMap`, because logback 1.6.0 removed + `PatternLayout.defaultConverterMap`. The layouts now register their conversion words in the logging context's + conversion rule registry when they start, which logback 1.2.x to 1.6.x all read, so the same toolkit artifact keeps + working on older logback. Logback 1.5.13 is not supported because of an upstream regression fixed in 1.5.14 + (qos-ch/logback#885). The words are no longer registered for the whole JVM, so using `%tid` or `%sw_ctx` in + other encoders, such as a plain ``, requires declaring them as ``s. All issues and pull requests are [here](https://github.com/apache/skywalking/milestone/263?closed=1) diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/pom.xml b/apm-application-toolkit/apm-toolkit-logback-1.x/pom.xml index 7a99852187..3a1c209e23 100644 --- a/apm-application-toolkit/apm-toolkit-logback-1.x/pom.xml +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/pom.xml @@ -28,8 +28,11 @@ apm-toolkit-logback-1.x - 1.2.3 + + 1.6.5 6.1 + ${project.build.directory}/logback-compat @@ -46,4 +49,118 @@ provided + + + + + + org.apache.maven.plugins + maven-dependency-plugin + + + copy-logback-compat-versions + generate-test-resources + + copy + + + ${maven.test.skip} + ${logback-compat.dir} + + + ch.qos.logback + logback-core + 1.2.13 + + + ch.qos.logback + logback-classic + 1.2.13 + + + ch.qos.logback + logback-core + 1.3.16 + + + ch.qos.logback + logback-classic + 1.3.16 + + + ch.qos.logback + logback-core + 1.4.14 + + + ch.qos.logback + logback-classic + 1.4.14 + + + ch.qos.logback + logback-core + 1.5.12 + + + ch.qos.logback + logback-classic + 1.5.12 + + + ch.qos.logback + logback-core + 1.5.14 + + + ch.qos.logback + logback-classic + 1.5.14 + + + ch.qos.logback + logback-core + 1.5.38 + + + ch.qos.logback + logback-classic + 1.5.38 + + + ch.qos.logback + logback-core + 1.6.5 + + + ch.qos.logback + logback-classic + 1.6.5 + + + org.slf4j + slf4j-api + 1.7.36 + + + org.slf4j + slf4j-api + 2.0.19 + + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + + + ${logback-compat.dir} + + + + + diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java new file mode 100644 index 0000000000..a34665e50c --- /dev/null +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java @@ -0,0 +1,76 @@ +/* + * 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.skywalking.apm.toolkit.log.logback.v1.x; + +import ch.qos.logback.classic.PatternLayout; +import ch.qos.logback.core.Context; +import ch.qos.logback.core.CoreConstants; +import java.util.HashMap; +import java.util.Map; + +/** + * Registers the SkyWalking conversion words into the logging context before the pattern is compiled. + *

+ * The context rule registry, keyed by {@link CoreConstants#PATTERN_RULE_REGISTRY} and holding converter class + * names, is read by every logback release from 1.2 to 1.6. Logback 1.5.14+ adapts the class names into converter + * suppliers itself. The static {@code PatternLayout.defaultConverterMap} used before was removed in logback 1.6.0. + *

+ * The words are registered the same way as a {@code }, so pattern layouts of the context started + * later can use them too. The registry is cleared when the context is reset, and the layouts register them again + * when they start. + *

+ * Logback 1.5.13 is not supported: it reads this registry as a map of suppliers, which was reverted in 1.5.14. + */ +public abstract class AbstractTraceIdPatternLogbackLayout extends PatternLayout { + + @Override + @SuppressWarnings("unchecked") + public void start() { + Context context = getContext(); + if (context == null) { + addError("A logging context is required to register the SkyWalking conversion words"); + return; + } + + Map rules = new HashMap<>(); + registerConverters(rules); + + // Logback reads the registry while compiling the pattern in super.start(), so starting under the same lock + // keeps concurrently started layouts from modifying it during that read. + synchronized (context) { + Map registry = (Map) context.getObject(CoreConstants.PATTERN_RULE_REGISTRY); + if (registry == null) { + registry = new HashMap<>(); + context.putObject(CoreConstants.PATTERN_RULE_REGISTRY, registry); + } + // Updated in place, as logback does for . Rules already registered, such as + // user-defined s, keep their precedence. + for (Map.Entry rule : rules.entrySet()) { + registry.putIfAbsent(rule.getKey(), rule.getValue()); + } + + super.start(); + } + } + + /** + * @param rules conversion word to converter class name + */ + protected abstract void registerConverters(Map rules); +} diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayout.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayout.java index 8a85b4dc47..03be856e07 100644 --- a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayout.java +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayout.java @@ -18,16 +18,17 @@ package org.apache.skywalking.apm.toolkit.log.logback.v1.x; -import ch.qos.logback.classic.PatternLayout; +import java.util.Map; /** * Based on the logback-component convert register mechanism, register {@link LogbackPatternConverter} as a new * convert, match to "tid" and "sw_ctx". You can use "%tid" or "sw_ctx" in logback config file, "Pattern" section. *

*/ -public class TraceIdPatternLogbackLayout extends PatternLayout { - static { - defaultConverterMap.put("tid", LogbackPatternConverter.class.getName()); - defaultConverterMap.put("sw_ctx", LogbackSkyWalkingContextPatternConverter.class.getName()); +public class TraceIdPatternLogbackLayout extends AbstractTraceIdPatternLogbackLayout { + @Override + protected void registerConverters(Map rules) { + rules.put("tid", LogbackPatternConverter.class.getName()); + rules.put("sw_ctx", LogbackSkyWalkingContextPatternConverter.class.getName()); } } diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/mdc/TraceIdMDCPatternLogbackLayout.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/mdc/TraceIdMDCPatternLogbackLayout.java index 861705965a..6003a19bef 100644 --- a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/mdc/TraceIdMDCPatternLogbackLayout.java +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/mdc/TraceIdMDCPatternLogbackLayout.java @@ -18,14 +18,17 @@ package org.apache.skywalking.apm.toolkit.log.logback.v1.x.mdc; -import ch.qos.logback.classic.PatternLayout; +import java.util.Map; +import org.apache.skywalking.apm.toolkit.log.logback.v1.x.AbstractTraceIdPatternLogbackLayout; /** - * Override "X" and "mdc",SuperClass run before Subclass. + * Override "X" and "mdc", so "%X{tid}" and "%X{sw_ctx}" print the SkyWalking context. Other MDC keys keep the + * default MDC output. */ -public class TraceIdMDCPatternLogbackLayout extends PatternLayout { - static { - defaultConverterMap.put("X", LogbackMDCPatternConverter.class.getName()); - defaultConverterMap.put("mdc", LogbackMDCPatternConverter.class.getName()); +public class TraceIdMDCPatternLogbackLayout extends AbstractTraceIdPatternLogbackLayout { + @Override + protected void registerConverters(Map rules) { + rules.put("X", LogbackMDCPatternConverter.class.getName()); + rules.put("mdc", LogbackMDCPatternConverter.class.getName()); } } diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LayoutProbe.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LayoutProbe.java new file mode 100644 index 0000000000..70d7f3749e --- /dev/null +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LayoutProbe.java @@ -0,0 +1,62 @@ +/* + * 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.skywalking.apm.toolkit.log.logback.v1.x; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.PatternLayout; +import ch.qos.logback.classic.spi.LoggingEvent; +import org.apache.skywalking.apm.toolkit.log.logback.v1.x.mdc.TraceIdMDCPatternLogbackLayout; + +/** + * Runs inside the class loader of the logback release under test, so it only uses logback APIs that exist from + * 1.2 to 1.6. + */ +public final class LayoutProbe { + + private LayoutProbe() { + } + + /** + * Starts both SkyWalking layouts in one context, as a logback.xml using both of them does, and renders an event + * with each. + */ + public static String[] render(String tidPattern, String mdcPattern) { + LoggerContext context = new LoggerContext(); + PatternLayout tidLayout = start(new TraceIdPatternLogbackLayout(), context, tidPattern); + PatternLayout mdcLayout = start(new TraceIdMDCPatternLogbackLayout(), context, mdcPattern); + + LoggingEvent event = new LoggingEvent(); + event.setLevel(Level.INFO); + return new String[] { + tidLayout.doLayout(event), + mdcLayout.doLayout(event) + }; + } + + private static PatternLayout start(PatternLayout layout, LoggerContext context, String pattern) { + layout.setContext(context); + layout.setPattern(pattern); + layout.start(); + if (!layout.isStarted()) { + throw new IllegalStateException(layout.getClass().getSimpleName() + " failed to start"); + } + return layout; + } +} diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LogbackVersionCompatibilityTest.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LogbackVersionCompatibilityTest.java new file mode 100644 index 0000000000..050f7c85d1 --- /dev/null +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/LogbackVersionCompatibilityTest.java @@ -0,0 +1,106 @@ +/* + * 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.skywalking.apm.toolkit.log.logback.v1.x; + +import java.io.File; +import java.lang.reflect.InvocationTargetException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.Arrays; +import java.util.Collection; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertTrue; + +/** + * Renders the layouts against each supported logback release. The jars are copied by maven-dependency-plugin, and + * each release gets its own class loader with the toolkit classes, so no other logback version is visible. + */ +@RunWith(Parameterized.class) +public class LogbackVersionCompatibilityTest { + + @Parameterized.Parameters(name = "logback {0}") + public static Collection versions() { + return Arrays.asList(new Object[][] { + {"1.2.13", "1.7.36"}, + {"1.3.16", "2.0.19"}, + {"1.4.14", "2.0.19"}, + // The last release reading class names from the context registry directly. + {"1.5.12", "2.0.19"}, + // The first release adapting class names in the context registry into converter suppliers. + {"1.5.14", "2.0.19"}, + {"1.5.38", "2.0.19"}, + // PatternLayout.defaultConverterMap removed since 1.6.0. + {"1.6.5", "2.0.19"} + }); + } + + private final String logbackVersion; + private final String slf4jVersion; + + public LogbackVersionCompatibilityTest(String logbackVersion, String slf4jVersion) { + this.logbackVersion = logbackVersion; + this.slf4jVersion = slf4jVersion; + } + + @Test + public void rendersSkyWalkingConversionWords() throws Exception { + try (URLClassLoader classLoader = new URLClassLoader(classpath(), ClassLoader.getSystemClassLoader().getParent())) { + Class probe = Class.forName(LayoutProbe.class.getName(), true, classLoader); + String[] rendered = (String[]) invoke(probe, "%tid|%sw_ctx", "%X{tid}|%mdc{sw_ctx}"); + + assertArrayEquals(new String[] { + "TID: N/A|SW_CTX: N/A", + "TID: N/A|SW_CTX: N/A" + }, rendered); + } + } + + private URL[] classpath() throws Exception { + File dir = new File(System.getProperty("logback.compat.dir", "target/logback-compat")); + File[] jars = { + new File(dir, "logback-core-" + logbackVersion + ".jar"), + new File(dir, "logback-classic-" + logbackVersion + ".jar"), + new File(dir, "slf4j-api-" + slf4jVersion + ".jar") + }; + URL[] urls = new URL[jars.length + 2]; + for (int i = 0; i < jars.length; i++) { + assertTrue(jars[i] + " is missing, it is copied by maven-dependency-plugin in generate-test-resources", + jars[i].isFile()); + urls[i] = jars[i].toURI().toURL(); + } + urls[jars.length] = AbstractTraceIdPatternLogbackLayout.class.getProtectionDomain().getCodeSource().getLocation(); + urls[jars.length + 1] = LayoutProbe.class.getProtectionDomain().getCodeSource().getLocation(); + return urls; + } + + private static Object invoke(Class probe, String tidPattern, String mdcPattern) throws Exception { + try { + return probe.getMethod("render", String.class, String.class).invoke(null, tidPattern, mdcPattern); + } catch (InvocationTargetException e) { + if (e.getCause() instanceof Exception) { + throw (Exception) e.getCause(); + } + throw e; + } + } +} diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java new file mode 100644 index 0000000000..456917d809 --- /dev/null +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java @@ -0,0 +1,97 @@ +/* + * 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.skywalking.apm.toolkit.log.logback.v1.x; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.pattern.ClassicConverter; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.classic.spi.LoggingEvent; +import ch.qos.logback.core.CoreConstants; +import java.util.HashMap; +import java.util.Map; +import org.apache.skywalking.apm.toolkit.log.logback.v1.x.mdc.TraceIdMDCPatternLogbackLayout; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertSame; + +public class TraceIdPatternLogbackLayoutTest { + + @Test + public void existingRulesKeepPrecedence() { + LoggerContext context = new LoggerContext(); + Map userRules = new HashMap<>(); + userRules.put("tid", FixedConverter.class.getName()); + userRules.put("user", FixedConverter.class.getName()); + context.putObject(CoreConstants.PATTERN_RULE_REGISTRY, userRules); + + TraceIdPatternLogbackLayout layout = new TraceIdPatternLogbackLayout(); + layout.setContext(context); + layout.setPattern("%tid|%sw_ctx|%user"); + layout.start(); + + assertEquals("fixed|SW_CTX: N/A|fixed", layout.doLayout(event())); + // Updated in place, so a rule logback adds to the same registry concurrently is kept. + assertSame(userRules, context.getObject(CoreConstants.PATTERN_RULE_REGISTRY)); + assertEquals(LogbackSkyWalkingContextPatternConverter.class.getName(), userRules.get("sw_ctx")); + } + + @Test + public void layoutsInOneContextKeepEachOthersRules() { + LoggerContext context = new LoggerContext(); + start(new TraceIdPatternLogbackLayout(), context); + start(new TraceIdMDCPatternLogbackLayout(), context); + + @SuppressWarnings("unchecked") + Map rules = (Map) context.getObject(CoreConstants.PATTERN_RULE_REGISTRY); + assertEquals(LogbackPatternConverter.class.getName(), rules.get("tid")); + assertEquals(LogbackSkyWalkingContextPatternConverter.class.getName(), rules.get("sw_ctx")); + assertEquals(4, rules.size()); + } + + @Test + public void notStartedWithoutContext() { + TraceIdPatternLogbackLayout layout = new TraceIdPatternLogbackLayout(); + layout.setPattern("%tid"); + layout.start(); + + assertFalse(layout.isStarted()); + } + + private static void start(AbstractTraceIdPatternLogbackLayout layout, LoggerContext context) { + layout.setContext(context); + layout.setPattern("%msg"); + layout.start(); + } + + private static ILoggingEvent event() { + LoggingEvent event = new LoggingEvent(); + event.setLevel(Level.INFO); + return event; + } + + public static class FixedConverter extends ClassicConverter { + @Override + public String convert(ILoggingEvent event) { + return "fixed"; + } + } +} diff --git a/docs/en/setup/service-agent/java-agent/Application-toolkit-logback-1.x.md b/docs/en/setup/service-agent/java-agent/Application-toolkit-logback-1.x.md index 8023dfed50..f1651d3ca9 100644 --- a/docs/en/setup/service-agent/java-agent/Application-toolkit-logback-1.x.md +++ b/docs/en/setup/service-agent/java-agent/Application-toolkit-logback-1.x.md @@ -8,6 +8,22 @@ ``` +# Supported logback versions + +The toolkit layouts support logback 1.2.x to 1.6.x in the same artifact, except logback **1.5.13**. +Using the layouts with logback 1.6.x requires toolkit 9.8.0 or later, as earlier toolkit releases fail on it with +`NoSuchFieldError: defaultConverterMap`. With an earlier toolkit release, declare the conversion words as +``s and use them in plain encoders instead, see [Print trace ID in your logs](#print-trace-id-in-your-logs). + +Logback 1.5.13 has an upstream regression, [qos-ch/logback#885](https://github.com/qos-ch/logback/issues/885). +The layouts register `%tid`, `%sw_ctx`, `%X` and `%mdc` in the logging context's conversion rule registry +(`CoreConstants.PATTERN_RULE_REGISTRY`), which holds converter class names in every other release from 1.2 to 1.6. +Logback 1.5.13 changed the registry to hold converter suppliers, so class names that code puts into it break, +including this toolkit's and Spring Boot's. On 1.5.13, a layout whose pattern uses one of these words fails to start +with `ClassCastException: class java.lang.String cannot be cast to class java.util.function.Supplier`, and a +logback.xml containing it fails to configure. Logback 1.5.14 reverted the change, so upgrade to 1.5.14 or later. +``s declared in logback.xml are not affected. + # Print trace ID in your logs * set `%tid` in `Pattern` section of logback.xml @@ -32,6 +48,25 @@ ``` +* to use `%tid` or `%sw_ctx` in other encoders, such as a plain ``, declare them as conversion rules + in logback.xml. Declare `X` with `org.apache.skywalking.apm.toolkit.log.logback.v1.x.mdc.LogbackMDCPatternConverter` + in the same way for `%X{tid}`. Logback 1.5.7+ also accepts `class` in place of the deprecated `converterClass`. +```xml + + + + + + %d{yyyy-MM-dd HH:mm:ss.SSS} [%tid] [%thread] %-5level %logger{36} -%msg%n + + +``` + Before 9.8.0, the layouts registered these words for the whole JVM once their classes were loaded, so other encoders + could pick them up by chance, depending on the configuration order. Since 9.8.0, the layouts register them in their + logging context when they start, and the conversion rules are the way to use them anywhere else. + * Support logback AsyncAppender(MDC also support), No additional configuration is required. Refer to the demo of logback.xml below. For details: [Logback AsyncAppender](https://logback.qos.ch/manual/appenders.html#AsyncAppender) ```xml diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/bin/startup.sh b/test/plugin/scenarios/apm-toolkit-logback-scenario/bin/startup.sh new file mode 100755 index 0000000000..0a9664ba2a --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/bin/startup.sh @@ -0,0 +1,22 @@ +#!/bin/bash +# +# 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. + +home="$(cd "$(dirname $0)"; pwd)" + +# A fixed instance name, so the SkyWalking context printed in the logs is known in the expected data. +java -jar ${agent_opts} -Dskywalking.agent.instance_name=logback-scenario-instance ${home}/../libs/apm-toolkit-logback-scenario.jar & diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/config/expectedData.yaml b/test/plugin/scenarios/apm-toolkit-logback-scenario/config/expectedData.yaml new file mode 100644 index 0000000000..5c4bce6a95 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/config/expectedData.yaml @@ -0,0 +1,83 @@ +# 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. +segmentItems: +- serviceName: apm-toolkit-logback-scenario + segmentSize: ge 1 + segments: + - segmentId: not null + spans: + - operationName: logback-case + parentSpanId: -1 + spanId: 0 + spanLayer: Unknown + startTime: nq 0 + endTime: nq 0 + componentId: 0 + isError: false + spanType: Entry + peer: '' + skipAnalysis: false + refs: + - {parentEndpoint: /upstream, networkAddress: 'upstream:8080', refType: CrossProcess, + parentSpanId: 1, parentTraceSegmentId: upstream-segment-id, parentServiceInstance: upstream-instance, + parentService: upstream-service, traceId: logback-scenario-trace-id} +logItems: +- serviceName: apm-toolkit-logback-scenario + logSize: 3 + logs: + # Rendered on the AsyncAppender worker thread from the context captured when the event was created. + # The gRPC reporter does not attach a trace context there, so only the text is checked. + - timestamp: nq 0 + body: + type: TEXT + content: + text: 'start with async-mdc-layout user=skywalking TID:logback-scenario-trace-id SW_CTX:[apm-toolkit-logback-scenario,logback-scenario-instance,logback-scenario-trace-id,' + - timestamp: nq 0 + endpoint: logback-case + traceContext: + traceId: logback-scenario-trace-id + traceSegmentId: not blank + spanId: 0 + body: + type: TEXT + content: + text: 'start with mdc-layout user=skywalking TID:logback-scenario-trace-id SW_CTX:[apm-toolkit-logback-scenario,logback-scenario-instance,logback-scenario-trace-id,' + tags: + data: + - key: level + value: INFO + - key: logger + value: test.apache.skywalking.apm.testcase.logback.CaseHandler + - key: thread + value: not blank + - timestamp: nq 0 + endpoint: logback-case + traceContext: + traceId: logback-scenario-trace-id + traceSegmentId: not blank + spanId: 0 + body: + type: TEXT + content: + text: 'start with tid-layout TID:logback-scenario-trace-id SW_CTX:[apm-toolkit-logback-scenario,logback-scenario-instance,logback-scenario-trace-id,' + tags: + data: + - key: level + value: INFO + - key: logger + value: test.apache.skywalking.apm.testcase.logback.CaseHandler + - key: thread + value: not blank diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/configuration.yml b/test/plugin/scenarios/apm-toolkit-logback-scenario/configuration.yml new file mode 100644 index 0000000000..4934b914bb --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/configuration.yml @@ -0,0 +1,20 @@ +# 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. + +type: jvm +entryService: http://localhost:8080/apm-toolkit-logback-scenario/case/logback +healthCheck: http://localhost:8080/apm-toolkit-logback-scenario/case/healthCheck +startScript: ./bin/startup.sh diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/pom.xml b/test/plugin/scenarios/apm-toolkit-logback-scenario/pom.xml new file mode 100644 index 0000000000..a8cd02cd97 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/pom.xml @@ -0,0 +1,127 @@ + + + + + org.apache.skywalking.apm.testcase + apm-toolkit-logback-scenario + 1.0.0 + jar + + 4.0.0 + + + UTF-8 + 17 + 3.8.1 + logback-classic + 1.5.38 + 9.7.0 + + + skywalking-apm-toolkit-logback-scenario + + + + + ch.qos.logback + logback-classic + ${test.framework.version} + + + org.apache.skywalking + apm-toolkit-logback-1.x + ${apm-toolkit.version} + + + org.apache.skywalking + apm-toolkit-trace + ${apm-toolkit.version} + + + + + apm-toolkit-logback-scenario + + + org.apache.maven.plugins + maven-shade-plugin + 3.1.0 + + + package + + shade + + + + + *:* + + META-INF/*.SF + META-INF/*.DSA + META-INF/*.RSA + + + + + + + + test.apache.skywalking.apm.testcase.logback.Application + + + + + + + + maven-compiler-plugin + ${maven-compiler-plugin.version} + + ${compiler.version} + ${compiler.version} + ${project.build.sourceEncoding} + + + + org.apache.maven.plugins + maven-assembly-plugin + + + assemble + package + + single + + + + src/main/assembly/assembly.xml + + ./target/ + + + + + + + diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/assembly/assembly.xml b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/assembly/assembly.xml new file mode 100644 index 0000000000..56e63cbce3 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/assembly/assembly.xml @@ -0,0 +1,41 @@ + + + + + zip + + + + + ./bin + 0775 + + + + + + ${project.build.directory}/apm-toolkit-logback-scenario.jar + ./libs + 0775 + + + diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/Application.java b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/Application.java new file mode 100644 index 0000000000..607e31a977 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/Application.java @@ -0,0 +1,33 @@ +/* + * 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 test.apache.skywalking.apm.testcase.logback; + +import com.sun.net.httpserver.HttpServer; +import java.io.IOException; +import java.net.InetSocketAddress; + +public class Application { + + public static void main(String[] args) throws IOException { + HttpServer server = HttpServer.create(new InetSocketAddress(8080), 0); + server.createContext("/apm-toolkit-logback-scenario/case/healthCheck", CaseHandler::respond); + server.createContext("/apm-toolkit-logback-scenario/case/logback", new CaseHandler()); + server.start(); + } +} diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/CaseHandler.java b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/CaseHandler.java new file mode 100644 index 0000000000..3cfe5918a4 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/java/test/apache/skywalking/apm/testcase/logback/CaseHandler.java @@ -0,0 +1,88 @@ +/* + * 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 test.apache.skywalking.apm.testcase.logback; + +import com.sun.net.httpserver.HttpExchange; +import com.sun.net.httpserver.HttpHandler; +import java.io.IOException; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import org.apache.skywalking.apm.toolkit.trace.CarrierItemRef; +import org.apache.skywalking.apm.toolkit.trace.ContextCarrierRef; +import org.apache.skywalking.apm.toolkit.trace.Tracer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.slf4j.MDC; + +public class CaseHandler implements HttpHandler { + + private static final Logger LOGGER = LoggerFactory.getLogger(CaseHandler.class); + + /** + * The trace continues a fixed upstream context, so the trace ID printed in the logs is known in the expected data. + */ + private static final String SW8 = String.join( + "-", "1", encode("logback-scenario-trace-id"), encode("upstream-segment-id"), "1", + encode("upstream-service"), encode("upstream-instance"), encode("/upstream"), encode("upstream:8080") + ); + + @Override + public void handle(HttpExchange exchange) throws IOException { + log(); + respond(exchange); + } + + private void log() { + ContextCarrierRef carrier = new ContextCarrierRef(); + CarrierItemRef item = carrier.items(); + while (item.hasNext()) { + item = item.next(); + if ("sw8".equals(item.getHeadKey())) { + item.setHeadValue(SW8); + } + } + Tracer.createEntrySpan("logback-case", carrier); + // A key of the application's own, which the SkyWalking MDC converter leaves to logback. + MDC.put("user", "skywalking"); + try { + LOGGER.info("logback-scenario"); + } finally { + MDC.remove("user"); + Tracer.stopSpan(); + } + } + + static void respond(HttpExchange exchange) throws IOException { + if ("HEAD".equals(exchange.getRequestMethod())) { + exchange.sendResponseHeaders(200, -1); + exchange.close(); + return; + } + byte[] body = "Success".getBytes(StandardCharsets.UTF_8); + exchange.sendResponseHeaders(200, body.length); + try (OutputStream out = exchange.getResponseBody()) { + out.write(body); + } + } + + private static String encode(String value) { + return Base64.getEncoder().encodeToString(value.getBytes(StandardCharsets.UTF_8)); + } +} diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/resources/logback.xml b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/resources/logback.xml new file mode 100644 index 0000000000..ac1770418f --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/src/main/resources/logback.xml @@ -0,0 +1,53 @@ + + + + + + + tid-layout %tid %sw_ctx %msg + + + + + + + + mdc-layout user=%X{user} %X{tid} %X{sw_ctx} %msg + + + + + + + + + async-mdc-layout user=%X{user} %X{tid} %X{sw_ctx} %msg + + + + + + + + + + + + + diff --git a/test/plugin/scenarios/apm-toolkit-logback-scenario/support-version.list b/test/plugin/scenarios/apm-toolkit-logback-scenario/support-version.list new file mode 100644 index 0000000000..b0bc5ed058 --- /dev/null +++ b/test/plugin/scenarios/apm-toolkit-logback-scenario/support-version.list @@ -0,0 +1,23 @@ +# 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. + +# The scenario uses the released apm-toolkit-logback-1.x, so the agent is verified with the toolkit users have. +# logback 1.6.x requires apm-toolkit-logback-1.x 9.8.0+. Add it once released, e.g. `1.6.5,apm-toolkit.version=9.8.0`. +# Do not add logback 1.5.13, the toolkit 9.8.0+ does not work on it (qos-ch/logback#885, fixed in 1.5.14). +1.2.13 +1.3.16 +1.4.14 +1.5.38 From 8dd4de0ff48a5cf3e53734ddd33b5bec8e5ac896 Mon Sep 17 00:00:00 2001 From: Wu Sheng Date: Wed, 7 Oct 2026 01:09:27 +0800 Subject: [PATCH 2/2] Replace the conversion rule registry instead of modifying it Register the SkyWalking conversion words by replacing the context's conversion rule registry with an updated copy, under the context monitor held only for the copy. Layouts starting concurrently, from any class loader, keep each other's words, and logback reads a registry that is never modified afterwards, so no lock is held while logback creates and starts the converters. Nothing is copied once the words are registered. On logback before 1.5.14, which also writes s to this registry, a rule logback registers at the same time as a layout starts in another thread may be lost; this is documented. --- .../AbstractTraceIdPatternLogbackLayout.java | 34 ++++----- .../v1/x/TraceIdPatternLogbackLayoutTest.java | 71 ++++++++++++++++++- 2 files changed, 86 insertions(+), 19 deletions(-) diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java index a34665e50c..dec4406783 100644 --- a/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/main/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/AbstractTraceIdPatternLogbackLayout.java @@ -31,9 +31,11 @@ * names, is read by every logback release from 1.2 to 1.6. Logback 1.5.14+ adapts the class names into converter * suppliers itself. The static {@code PatternLayout.defaultConverterMap} used before was removed in logback 1.6.0. *

- * The words are registered the same way as a {@code }, so pattern layouts of the context started - * later can use them too. The registry is cleared when the context is reset, and the layouts register them again - * when they start. + * The words are registered in the context, so pattern layouts of the context started later can use them too. The + * registry is cleared when the context is reset, and the layouts register them again when they start. The registry + * is replaced by an updated copy, never modified, as layouts starting concurrently may be reading it. On logback + * before 1.5.14, which also writes {@code }s to this registry, a rule logback registers at the same + * time as a layout starts in another thread may be lost. *

* Logback 1.5.13 is not supported: it reads this registry as a map of suppliers, which was reverted in 1.5.14. */ @@ -51,26 +53,26 @@ public void start() { Map rules = new HashMap<>(); registerConverters(rules); - // Logback reads the registry while compiling the pattern in super.start(), so starting under the same lock - // keeps concurrently started layouts from modifying it during that read. + // Layouts starting concurrently, from any class loader, do not lose each other's words. The monitor is only + // held for the copy, and released before logback creates and starts the converters. synchronized (context) { - Map registry = (Map) context.getObject(CoreConstants.PATTERN_RULE_REGISTRY); - if (registry == null) { - registry = new HashMap<>(); + Map existing = (Map) context.getObject(CoreConstants.PATTERN_RULE_REGISTRY); + if (existing == null || !existing.keySet().containsAll(rules.keySet())) { + Map registry = existing == null ? new HashMap<>() : new HashMap<>(existing); + // Rules already registered, such as user-defined s, keep their precedence. + for (Map.Entry rule : rules.entrySet()) { + registry.putIfAbsent(rule.getKey(), rule.getValue()); + } context.putObject(CoreConstants.PATTERN_RULE_REGISTRY, registry); } - // Updated in place, as logback does for . Rules already registered, such as - // user-defined s, keep their precedence. - for (Map.Entry rule : rules.entrySet()) { - registry.putIfAbsent(rule.getKey(), rule.getValue()); - } - - super.start(); } + + super.start(); } /** - * @param rules conversion word to converter class name + * Puts the conversion words of this layout into {@code rules}, as conversion word to converter class name. + * Subclasses override it to change or add words. Words already registered in the context keep their precedence. */ protected abstract void registerConverters(Map rules); } diff --git a/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java index 456917d809..490a12cde0 100644 --- a/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java +++ b/apm-application-toolkit/apm-toolkit-logback-1.x/src/test/java/org/apache/skywalking/apm/toolkit/log/logback/v1/x/TraceIdPatternLogbackLayoutTest.java @@ -31,6 +31,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertSame; public class TraceIdPatternLogbackLayoutTest { @@ -49,9 +50,28 @@ public void existingRulesKeepPrecedence() { layout.start(); assertEquals("fixed|SW_CTX: N/A|fixed", layout.doLayout(event())); - // Updated in place, so a rule logback adds to the same registry concurrently is kept. - assertSame(userRules, context.getObject(CoreConstants.PATTERN_RULE_REGISTRY)); - assertEquals(LogbackSkyWalkingContextPatternConverter.class.getName(), userRules.get("sw_ctx")); + } + + @Test + public void registryIsReplacedNotModified() { + LoggerContext context = new LoggerContext(); + Map userRules = new HashMap<>(); + userRules.put("user", FixedConverter.class.getName()); + context.putObject(CoreConstants.PATTERN_RULE_REGISTRY, userRules); + + start(new TraceIdPatternLogbackLayout(), context); + + // A layout starting concurrently may be reading the registry it got before. + assertEquals(1, userRules.size()); + @SuppressWarnings("unchecked") + Map registry = (Map) context.getObject(CoreConstants.PATTERN_RULE_REGISTRY); + assertNotSame(userRules, registry); + assertEquals(FixedConverter.class.getName(), registry.get("user")); + assertEquals(LogbackPatternConverter.class.getName(), registry.get("tid")); + + // Nothing is copied once the words are registered. + start(new TraceIdPatternLogbackLayout(), context); + assertSame(registry, context.getObject(CoreConstants.PATTERN_RULE_REGISTRY)); } @Test @@ -67,6 +87,21 @@ public void layoutsInOneContextKeepEachOthersRules() { assertEquals(4, rules.size()); } + @Test + public void convertersStartWithoutHoldingTheContextLock() { + LoggerContext context = new LoggerContext(); + Map userRules = new HashMap<>(); + userRules.put("lock", ContextLockConverter.class.getName()); + context.putObject(CoreConstants.PATTERN_RULE_REGISTRY, userRules); + + TraceIdPatternLogbackLayout layout = new TraceIdPatternLogbackLayout(); + layout.setContext(context); + layout.setPattern("%tid|%lock"); + layout.start(); + + assertEquals("TID: N/A|unlocked", layout.doLayout(event())); + } + @Test public void notStartedWithoutContext() { TraceIdPatternLogbackLayout layout = new TraceIdPatternLogbackLayout(); @@ -94,4 +129,34 @@ public String convert(ILoggingEvent event) { return "fixed"; } } + + /** + * Takes the context monitor from another thread while starting, as logback's synchronized context methods do, + * e.g. getScheduledExecutorService(). It would block, or deadlock if waited for, while the layout held it. + */ + public static class ContextLockConverter extends ClassicConverter { + private boolean unlocked; + + @Override + public void start() { + Thread thread = new Thread(() -> { + synchronized (getContext()) { + // Holds nothing, only proves the monitor is available. + } + }); + thread.start(); + try { + thread.join(5000); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + unlocked = !thread.isAlive(); + super.start(); + } + + @Override + public String convert(ILoggingEvent event) { + return unlocked ? "unlocked" : "locked"; + } + } }