Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix adding logback mdc appender in spring starter #13391

Merged
merged 5 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't follow how this compileOnly dep is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the scope to implementation, hopefully that is less confusing. A class from the logback-mdc-1.0:library is used in a test so it is needed to compile the test, I think it gets added to runtime dependencies because the main project depends on it and the suite depends on the main project.

// using the same versions as in the spring-boot-autoconfigure
implementation("ch.qos.logback:logback-classic") {
version {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,14 @@ private static void addMdcAppender(
initializeMdcAppenderFromProperties(applicationEnvironmentPreparedEvent, openTelemetryAppender);
openTelemetryAppender.start();
logger.addAppender(openTelemetryAppender);
// move existing appenders under otel mdc appender, so they could observe the added mdc values
for (Iterator<Appender<ILoggingEvent>> i = logger.iteratorForAppenders(); i.hasNext(); ) {
Appender<ILoggingEvent> appender = i.next();
if (appender != openTelemetryAppender) {
openTelemetryAppender.addAppender(appender);
logger.detachAppender(appender);
}
}
}

private static void initializeMdcAppenderFromProperties(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* 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;

@SuppressWarnings("OtelInternalJavadoc")
public class CustomListAppender extends ListAppender<ILoggingEvent> {
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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -191,6 +193,7 @@ void shouldInitializeMdcAppender() {
}

assertThat(testing.logRecords()).isEmpty();
assertThat(CustomListAppender.lastLogHadTraceId).isTrue();
assertThat(listAppender.list)
.satisfiesExactly(
event ->
Expand Down Expand Up @@ -238,13 +241,26 @@ void shouldNotInitializeMdcAppenderWhenDisabled() {
private static ListAppender<ILoggingEvent> getListAppender() {
Logger logger = LoggerFactory.getLogger(Logger.ROOT_LOGGER_NAME);
ch.qos.logback.classic.Logger logbackLogger = (ch.qos.logback.classic.Logger) logger;

ListAppender<ILoggingEvent> listAppender =
(ListAppender<ILoggingEvent>) logbackLogger.getAppender("List");
if (listAppender != null) {
return listAppender;
}

AppenderAttachable<?> mdcAppender =
(AppenderAttachable<?>) logbackLogger.getAppender("OpenTelemetryMdc");
if (mdcAppender == null) {
for (Iterator<Appender<ILoggingEvent>> i = logbackLogger.iteratorForAppenders();
i.hasNext(); ) {
Appender<ILoggingEvent> appender = i.next();
if (appender
instanceof io.opentelemetry.instrumentation.logback.mdc.v1_0.OpenTelemetryAppender) {
mdcAppender = (AppenderAttachable<?>) appender;
break;
}
}
}
return (ListAppender<ILoggingEvent>) mdcAppender.getAppender("List");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
</pattern>
</encoder>
</appender>
<appender name="List" class="ch.qos.logback.core.read.ListAppender" />
<appender name="List" class="io.opentelemetry.instrumentation.spring.autoconfigure.internal.instrumentation.logging.CustomListAppender" />

<root level="INFO">
<appender-ref ref="console"/>
Expand Down
Loading