From 720b15d53e6d2fb82150f50595836fadf2e2cf92 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Tue, 25 Feb 2025 16:21:24 +0200 Subject: [PATCH 1/3] Fix adding logback mdc appender in spring starter --- .../build.gradle.kts | 1 + .../logging/LogbackAppenderInstaller.java | 8 ++++++ .../logging/CustomListAppender.java | 27 +++++++++++++++++++ .../logging/LogbackAppenderTest.java | 16 +++++++++++ .../resources/logback-no-otel-appenders.xml | 2 +- 5 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java diff --git a/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts b/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts index 99fbcbd98b8e..d37cab3f84e0 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts +++ b/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts @@ -132,6 +132,7 @@ testing { implementation("org.springframework.boot:spring-boot-autoconfigure:$springBootVersion") implementation(project(":instrumentation:logback:logback-appender-1.0:library")) + compileOnly(project(":instrumentation:logback:logback-mdc-1.0:library")) // using the same versions as in the spring-boot-autoconfigure implementation("ch.qos.logback:logback-classic") { version { diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java index 4e8af4b11703..2c3a79d4b501 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java +++ b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java @@ -172,6 +172,14 @@ private static void addMdcAppender( initializeMdcAppenderFromProperties(applicationEnvironmentPreparedEvent, openTelemetryAppender); openTelemetryAppender.start(); logger.addAppender(openTelemetryAppender); + // move existing appenders under otel mdc appender + for (Iterator> i = logger.iteratorForAppenders(); i.hasNext(); ) { + Appender appender = i.next(); + if (appender != openTelemetryAppender) { + openTelemetryAppender.addAppender(appender); + logger.detachAppender(appender); + } + } } private static void initializeMdcAppenderFromProperties( diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java new file mode 100644 index 000000000000..9041f28b8269 --- /dev/null +++ b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java @@ -0,0 +1,27 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.spring.autoconfigure.internal.instrumentation.logging; + +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; + +/** + * This class is internal and is hence not for public use. Its APIs are unstable and can change at + * any time. + */ +public class CustomListAppender extends ListAppender { + public static boolean lastLogHadTraceId; + + @Override + protected void append(ILoggingEvent event) { + // Since list appender just captures the event object it is possible that the trace_id is not + // present when list appender was called but is added at a later time. Here we record whether + // trace_id was present in mdc at the time when the event was processed by the list appender. + // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/13383 + lastLogHadTraceId = event.getMDCPropertyMap().get("trace_id") != null; + super.append(event); + } +} diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderTest.java index c11631cb7d4d..53d841d30bec 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderTest.java +++ b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderTest.java @@ -8,6 +8,7 @@ import static org.assertj.core.api.Assertions.assertThat; import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.Appender; import ch.qos.logback.core.read.ListAppender; import ch.qos.logback.core.spi.AppenderAttachable; import io.opentelemetry.api.OpenTelemetry; @@ -22,6 +23,7 @@ import io.opentelemetry.instrumentation.testing.junit.LibraryInstrumentationExtension; import io.opentelemetry.sdk.logs.data.LogRecordData; import java.util.HashMap; +import java.util.Iterator; import java.util.List; import java.util.Map; import org.junit.jupiter.api.BeforeEach; @@ -191,6 +193,7 @@ void shouldInitializeMdcAppender() { } assertThat(testing.logRecords()).isEmpty(); + assertThat(CustomListAppender.lastLogHadTraceId).isTrue(); assertThat(listAppender.list) .satisfiesExactly( event -> @@ -238,13 +241,26 @@ void shouldNotInitializeMdcAppenderWhenDisabled() { private static ListAppender getListAppender() { Logger logger = LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME); ch.qos.logback.classic.Logger logbackLogger = (ch.qos.logback.classic.Logger) logger; + ListAppender listAppender = (ListAppender) logbackLogger.getAppender("List"); if (listAppender != null) { return listAppender; } + AppenderAttachable mdcAppender = (AppenderAttachable) logbackLogger.getAppender("OpenTelemetryMdc"); + if (mdcAppender == null) { + for (Iterator> i = logbackLogger.iteratorForAppenders(); + i.hasNext(); ) { + Appender appender = i.next(); + if (appender + instanceof io.opentelemetry.instrumentation.logback.mdc.v1_0.OpenTelemetryAppender) { + mdcAppender = (AppenderAttachable) appender; + break; + } + } + } return (ListAppender) mdcAppender.getAppender("List"); } } diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/resources/logback-no-otel-appenders.xml b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/resources/logback-no-otel-appenders.xml index d77f5158c476..4d160ba2a8a7 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/resources/logback-no-otel-appenders.xml +++ b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/resources/logback-no-otel-appenders.xml @@ -8,7 +8,7 @@ - + From 3277ded1fbd25ff737bd290ccbca3edade28e184 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 28 Feb 2025 16:21:05 +0200 Subject: [PATCH 2/3] address review comments --- .../instrumentation/logging/LogbackAppenderInstaller.java | 2 +- .../internal/instrumentation/logging/CustomListAppender.java | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java index 2c3a79d4b501..fd13848aaf2f 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java +++ b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/LogbackAppenderInstaller.java @@ -172,7 +172,7 @@ private static void addMdcAppender( initializeMdcAppenderFromProperties(applicationEnvironmentPreparedEvent, openTelemetryAppender); openTelemetryAppender.start(); logger.addAppender(openTelemetryAppender); - // move existing appenders under otel mdc appender + // move existing appenders under otel mdc appender, so they could observe the added mdc values for (Iterator> i = logger.iteratorForAppenders(); i.hasNext(); ) { Appender appender = i.next(); if (appender != openTelemetryAppender) { diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java index 9041f28b8269..c2ecb450282b 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java +++ b/instrumentation/spring/spring-boot-autoconfigure/src/testLogbackAppender/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/logging/CustomListAppender.java @@ -8,10 +8,7 @@ import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.read.ListAppender; -/** - * This class is internal and is hence not for public use. Its APIs are unstable and can change at - * any time. - */ +@SuppressWarnings("OtelInternalJavadoc") public class CustomListAppender extends ListAppender { public static boolean lastLogHadTraceId; From d1e344b02e734a42c9c05e09d64b3fe79631ae3f Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 28 Feb 2025 19:24:21 +0200 Subject: [PATCH 3/3] change dependency scope --- .../spring/spring-boot-autoconfigure/build.gradle.kts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts b/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts index d37cab3f84e0..681fdcb45b81 100644 --- a/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts +++ b/instrumentation/spring/spring-boot-autoconfigure/build.gradle.kts @@ -132,7 +132,7 @@ testing { implementation("org.springframework.boot:spring-boot-autoconfigure:$springBootVersion") implementation(project(":instrumentation:logback:logback-appender-1.0:library")) - compileOnly(project(":instrumentation:logback:logback-mdc-1.0:library")) + implementation(project(":instrumentation:logback:logback-mdc-1.0:library")) // using the same versions as in the spring-boot-autoconfigure implementation("ch.qos.logback:logback-classic") { version {