Merge pull request #84 from newrelic/fix-mdc-logback

Fix mdc logback

Author: kanderson250

examples/logback-app/build.gradle.kts

@@ -12,11 +12,11 @@ repositories {
 
 dependencies {
     implementation(project(":logback"))
-    implementation("ch.qos.logback:logback-core:1.2.3")
-    implementation("ch.qos.logback:logback-classic:1.2.3")
+    implementation("ch.qos.logback:logback-core:1.3.9")
+    implementation("ch.qos.logback:logback-classic:1.3.9")
     implementation("com.fasterxml.jackson.core:jackson-core:2.11.1")
 
-    implementation("com.newrelic.agent.java:newrelic-api:7.6.0")
+    implementation("com.newrelic.agent.java:newrelic-api:8.6.0")
 }
 
 

logback/build.gradle.kts

@@ -27,6 +27,8 @@ dependencies {
 
     includeInJar(project(":core"))
 
+    testImplementation("ch.qos.logback:logback-core:1.3.9");
+    testImplementation("ch.qos.logback:logback-classic:1.3.9");
     testImplementation("org.junit.jupiter:junit-jupiter:5.6.2")
     testImplementation("com.google.guava:guava:30.0-jre")
     testImplementation("org.mockito:mockito-core:3.4.4")

logback/src/main/java/com/newrelic/logging/logback/NewRelicAsyncAppender.java

@@ -40,13 +40,13 @@ public class NewRelicAsyncAppender extends AsyncAppender {
     protected void preprocess(ILoggingEvent eventObject) {
         Map<String, String> linkingMetadata = agentSupplier.get().getLinkingMetadata();
         // NR linking metadata is added to the MDC map
-        if (!isNoOpMDC) {
+        if (!IsNoOpMDCHolder.isNoOpMDC) {
             for (Map.Entry<String, String> entry : linkingMetadata.entrySet()) {
                 MDC.put(NEW_RELIC_PREFIX + entry.getKey(), entry.getValue());
             }
         }
         super.preprocess(eventObject);
-        if (isNoOpMDC) {
+        if (IsNoOpMDCHolder.isNoOpMDC) {
             for (Map.Entry<String, String> entry : linkingMetadata.entrySet()) {
                 /*
                  * This only works if there is at least one entry in the MDC map. If the MDC map is empty when
@@ -59,8 +59,14 @@ protected void preprocess(ILoggingEvent eventObject) {
         }
     }
 
+    /*
+    Due to issues with simultaneous classloading of the NRAsyncAppender and the Logback classes,
+    the isNoOpMDC field is wrapped to load it lazily. Visible for testing.
+     */
+    static class IsNoOpMDCHolder {
+        static boolean isNoOpMDC = MDC.getMDCAdapter() instanceof NOPMDCAdapter;
+    }
+
     //visible for testing
     public static Supplier<Agent> agentSupplier = NewRelic::getAgent;
-    @SuppressWarnings("WeakerAccess") //visible for testing
-    static boolean isNoOpMDC = MDC.getMDCAdapter() instanceof NOPMDCAdapter;
 }

logback/src/test/java/com/newrelic/logging/logback/NewRelicLogbackTests.java

@@ -22,6 +22,7 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.Timeout;
 import org.mockito.Mockito;
+import org.slf4j.LoggerFactory;
 import org.slf4j.MDC;
 
 import java.io.BufferedReader;
@@ -30,6 +31,7 @@
 import java.io.PipedInputStream;
 import java.io.PipedOutputStream;
 import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
 import java.util.function.Supplier;
 import java.util.regex.Pattern;
 
@@ -60,8 +62,8 @@ void shouldWrapJsonConsoleAppenderCorrectly() throws Throwable {
     void shouldAllWorkCorrectlyEvenWithoutMDC() throws Throwable {
         givenMockAgentData();
         givenARedirectedAppender();
-        givenMDCIsANoOp();
         givenALoggingEventWithMDCDisabled();
+        givenMDCIsANoOp();
         whenTheEventIsAppended();
         thenMockAgentDataIsInTheMessage();
         thenJsonLayoutWasUsed();
@@ -129,13 +131,16 @@ private void givenMockAgentData() {
     }
 
     private void givenMDCIsANoOp() {
-        NewRelicAsyncAppender.isNoOpMDC = true;
+        NewRelicAsyncAppender.IsNoOpMDCHolder.isNoOpMDC = true;
+        // Wipe the MDC to mimic a NOPMDCAdapter.
+        event.setMDCPropertyMap(new HashMap<>());
     }
 
     private void givenALoggingEvent() {
         event = new LoggingEvent();
         event.setMessage("test_error_message");
         event.setLevel(Level.ERROR);
+        event.setLoggerContext((LoggerContext) LoggerFactory.getILoggerFactory());
     }
 
     private void givenALoggingEventWithExceptionData() {
@@ -283,7 +288,7 @@ private String getOutput() throws IOException {
     void setUp() throws Exception {
         // Clear MDC data before each test
         MDC.clear();
-        isNoOpMDC = NewRelicAsyncAppender.isNoOpMDC;
+        isNoOpMDC = NewRelicAsyncAppender.IsNoOpMDCHolder.isNoOpMDC;
         outputStream = new PipedOutputStream();
         PipedInputStream inputStream = new PipedInputStream(outputStream);
         bufferedReader = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8));
@@ -293,7 +298,7 @@ void setUp() throws Exception {
     void tearDown() throws Exception {
         // Clear MDC data before each test
         MDC.clear();
-        NewRelicAsyncAppender.isNoOpMDC = isNoOpMDC;
+        NewRelicAsyncAppender.IsNoOpMDCHolder.isNoOpMDC = isNoOpMDC;
         appender.stop();
         appender.detachAndStopAllAppenders();
         outputStream.close();