Logback: don't make MDCPropertyMap of logging event immutable (open-telemetry#12718)

Author: laurit

instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java

@@ -22,7 +22,6 @@
 import io.opentelemetry.api.trace.SpanContext;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap;
 import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
 import io.opentelemetry.javaagent.bootstrap.internal.AgentCommonConfig;
 import io.opentelemetry.javaagent.bootstrap.internal.ConfiguredResourceAttributesHolder;
@@ -74,6 +73,9 @@ public static void onExit(
       }
 
       Map<String, String> spanContextData = new HashMap<>();
+      if (contextData != null) {
+        spanContextData.putAll(contextData);
+      }
 
       SpanContext spanContext = Java8BytecodeBridge.spanFromContext(context).getSpanContext();
 
@@ -96,11 +98,7 @@ public static void onExit(
         }
       }
 
-      if (contextData == null) {
-        contextData = spanContextData;
-      } else {
-        contextData = new UnionMap<>(contextData, spanContextData);
-      }
+      contextData = spanContextData;
     }
   }
 }

instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java

@@ -17,7 +17,6 @@
 import io.opentelemetry.api.trace.SpanContext;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.incubator.log.LoggingContextConstants;
-import io.opentelemetry.instrumentation.logback.mdc.v1_0.internal.UnionMap;
 import java.lang.reflect.Field;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -83,6 +82,9 @@ private void processEvent(ILoggingEvent event) {
     }
 
     Map<String, String> contextData = new HashMap<>();
+    if (eventContext != null) {
+      contextData.putAll(eventContext);
+    }
     Context context = Context.current();
     Span currentSpan = Span.fromContext(context);
 
@@ -102,20 +104,14 @@ private void processEvent(ILoggingEvent event) {
                   "baggage." + key, value.getValue()));
     }
 
-    if (eventContext == null) {
-      eventContext = contextData;
-    } else {
-      eventContext = new UnionMap<>(eventContext, contextData);
-    }
-    Map<String, String> eventContextMap = eventContext;
     LoggerContextVO oldVo = event.getLoggerContextVO();
     LoggerContextVO vo =
         oldVo != null
-            ? new LoggerContextVO(oldVo.getName(), eventContextMap, oldVo.getBirthTime())
+            ? new LoggerContextVO(oldVo.getName(), contextData, oldVo.getBirthTime())
             : null;
 
     try {
-      MDC_MAP_FIELD.set(event, eventContextMap);
+      MDC_MAP_FIELD.set(event, contextData);
     } catch (IllegalAccessException ignored) {
       // setAccessible(true) was called on the field
     }

instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMap.java

@@ -5,8 +5,12 @@
 
 package io.opentelemetry.instrumentation.logback.mdc.v1_0;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.LibraryInstrumentationExtension;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 class LogbackTest extends AbstractLogbackTest {
@@ -18,4 +22,15 @@ class LogbackTest extends AbstractLogbackTest {
   public InstrumentationExtension getInstrumentationExtension() {
     return testing;
   }
+
+  @Test
+  void testMdcMutable() {
+    TestAppender testAppender = TestAppender.instance;
+    runWithSpanAndBaggage("test", baggage, () -> logger.info("log message"));
+
+    assertThat(testAppender.lastEvent.getMessage()).isEqualTo("log message");
+    Map<String, String> map = testAppender.lastEvent.getMDCPropertyMap();
+    // verify that mdc map associated with the event is mutable
+    map.put("test", "test");
+  }
 }

instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.logback.mdc.v1_0;
+
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.Appender;
+import ch.qos.logback.core.UnsynchronizedAppenderBase;
+import ch.qos.logback.core.spi.AppenderAttachable;
+import ch.qos.logback.core.spi.AppenderAttachableImpl;
+import java.util.Iterator;
+
+public class TestAppender extends UnsynchronizedAppenderBase<ILoggingEvent>
+    implements AppenderAttachable<ILoggingEvent> {
+
+  static TestAppender instance;
+  private final AppenderAttachableImpl<ILoggingEvent> aai = new AppenderAttachableImpl<>();
+  ILoggingEvent lastEvent;
+
+  public TestAppender() {
+    instance = this;
+  }
+
+  private void processEvent(ILoggingEvent event) {
+    lastEvent = event;
+  }
+
+  @Override
+  protected void append(ILoggingEvent event) {
+    processEvent(event);
+    aai.appendLoopOnAppenders(event);
+  }
+
+  @Override
+  public void addAppender(Appender<ILoggingEvent> appender) {
+    aai.addAppender(appender);
+  }
+
+  @Override
+  public Iterator<Appender<ILoggingEvent>> iteratorForAppenders() {
+    return aai.iteratorForAppenders();
+  }
+
+  @Override
+  public Appender<ILoggingEvent> getAppender(String name) {
+    return aai.getAppender(name);
+  }
+
+  @Override
+  public boolean isAttached(Appender<ILoggingEvent> appender) {
+    return aai.isAttached(appender);
+  }
+
+  @Override
+  public void detachAndStopAllAppenders() {
+    aai.detachAndStopAllAppenders();
+  }
+
+  @Override
+  public boolean detachAppender(Appender<ILoggingEvent> appender) {
+    return aai.detachAppender(appender);
+  }
+
+  @Override
+  public boolean detachAppender(String name) {
+    return aai.detachAppender(name);
+  }
+}