From f1f6e576762ee2ba5ff59a77c64706a59261cdd6 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 13 Nov 2024 10:23:28 +0200 Subject: [PATCH] Logback: don't make MDCPropertyMap of logging event immutable --- .../mdc/v1_0/LoggingEventInstrumentation.java | 10 +- .../mdc/v1_0/OpenTelemetryAppender.java | 14 +- .../logback/mdc/v1_0/internal/UnionMap.java | 214 ------------------ .../logback/mdc/v1_0/LogbackTest.java | 15 ++ .../logback/mdc/v1_0/TestAppender.java | 70 ++++++ .../mdc/v1_0/internal/UnionMapTest.java | 104 --------- .../library/src/test/resources/logback.xml | 2 + 7 files changed, 96 insertions(+), 333 deletions(-) delete mode 100644 instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMap.java create mode 100644 instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/TestAppender.java delete mode 100644 instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMapTest.java diff --git a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java b/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java index 93768e2f9857..5ff59fcb3f39 100644 --- a/instrumentation/logback/logback-mdc-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/logback/mdc/v1_0/LoggingEventInstrumentation.java +++ b/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 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; } } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java index 99b480efada0..8fdb009f06a3 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/OpenTelemetryAppender.java +++ b/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 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 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 } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMap.java b/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMap.java deleted file mode 100644 index e84b136d7d0d..000000000000 --- a/instrumentation/logback/logback-mdc-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMap.java +++ /dev/null @@ -1,214 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.logback.mdc.v1_0.internal; - -import java.io.Serializable; -import java.util.AbstractMap; -import java.util.AbstractSet; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.LinkedHashSet; -import java.util.Map; -import java.util.Set; - -/** - * An immutable view over two maps, with keys resolving from the first map first, or otherwise the - * second if not present in the first. - * - *

This class is internal and is hence not for public use. Its APIs are unstable and can change - * at any time. - */ -public final class UnionMap extends AbstractMap implements Serializable { - private static final long serialVersionUID = 1L; - - private final Map first; - private final Map second; - private int size = -1; - private Set> entrySet; - - public UnionMap(Map first, Map second) { - this.first = first; - this.second = second; - } - - @Override - public int size() { - if (size >= 0) { - return size; - } - - Map a; - Map b; - if (first.size() >= second.size()) { - a = first; - b = second; - } else { - a = second; - b = first; - } - - int size = a.size(); - if (!b.isEmpty()) { - for (K k : b.keySet()) { - if (!a.containsKey(k)) { - size++; - } - } - } - - return this.size = size; - } - - @Override - public boolean isEmpty() { - return first.isEmpty() && second.isEmpty(); - } - - @Override - public boolean containsKey(Object key) { - return first.containsKey(key) || second.containsKey(key); - } - - @Override - public boolean containsValue(Object value) { - return first.containsValue(value) || second.containsValue(value); - } - - @Override - public V get(Object key) { - V value = first.get(key); - return value != null ? value : second.get(key); - } - - @Override - public V put(K key, V value) { - throw new UnsupportedOperationException(); - } - - @Override - public V remove(Object key) { - throw new UnsupportedOperationException(); - } - - @Override - public void clear() { - throw new UnsupportedOperationException(); - } - - @Override - public Set> entrySet() { - if (entrySet != null) { - return entrySet; - } - - // Check for dupes first to reduce allocations on the vastly more common case where there aren't - // any. - boolean secondHasDupes = false; - for (Entry entry : second.entrySet()) { - if (first.containsKey(entry.getKey())) { - secondHasDupes = true; - break; - } - } - - Set> filteredSecond; - if (!secondHasDupes) { - filteredSecond = second.entrySet(); - } else { - filteredSecond = new LinkedHashSet<>(); - for (Entry entry : second.entrySet()) { - if (!first.containsKey(entry.getKey())) { - filteredSecond.add(entry); - } - } - } - return entrySet = - Collections.unmodifiableSet(new ConcatenatedSet<>(first.entrySet(), filteredSecond)); - } - - private Object writeReplace() { - // serialize this object as HashMap - return new HashMap<>(this); - } - - // Member sets must be deduped by caller. - static final class ConcatenatedSet extends AbstractSet { - - private final Set first; - private final Set second; - - private final int size; - - ConcatenatedSet(Set first, Set second) { - this.first = first; - this.second = second; - - size = first.size() + second.size(); - } - - @Override - public int size() { - return size; - } - - @Override - public boolean add(T t) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean remove(Object o) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean addAll(Collection c) { - throw new UnsupportedOperationException(); - } - - @Override - public boolean retainAll(Collection c) { - throw new UnsupportedOperationException(); - } - - @Override - public void clear() { - throw new UnsupportedOperationException(); - } - - @Override - public Iterator iterator() { - return new ConcatenatedSetIterator(); - } - - class ConcatenatedSetIterator implements Iterator { - final Iterator firstItr = first.iterator(); - final Iterator secondItr = second.iterator(); - - ConcatenatedSetIterator() {} - - @Override - public boolean hasNext() { - return firstItr.hasNext() || secondItr.hasNext(); - } - - @Override - public T next() { - if (firstItr.hasNext()) { - return firstItr.next(); - } - return secondItr.next(); - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - } - } -} diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.java b/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.java index d0de1c10e672..f676e8f3cf4e 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.java +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/LogbackTest.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 map = testAppender.lastEvent.getMDCPropertyMap(); + // verify that mdc map associated with the event is mutable + map.put("test", "test"); + } } diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/TestAppender.java b/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/TestAppender.java new file mode 100644 index 000000000000..e001771dd6ec --- /dev/null +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/TestAppender.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 + implements AppenderAttachable { + + static TestAppender instance; + private final AppenderAttachableImpl 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 appender) { + aai.addAppender(appender); + } + + @Override + public Iterator> iteratorForAppenders() { + return aai.iteratorForAppenders(); + } + + @Override + public Appender getAppender(String name) { + return aai.getAppender(name); + } + + @Override + public boolean isAttached(Appender appender) { + return aai.isAttached(appender); + } + + @Override + public void detachAndStopAllAppenders() { + aai.detachAndStopAllAppenders(); + } + + @Override + public boolean detachAppender(Appender appender) { + return aai.detachAppender(appender); + } + + @Override + public boolean detachAppender(String name) { + return aai.detachAppender(name); + } +} diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMapTest.java b/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMapTest.java deleted file mode 100644 index 1656c55f41c1..000000000000 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/java/io/opentelemetry/instrumentation/logback/mdc/v1_0/internal/UnionMapTest.java +++ /dev/null @@ -1,104 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.logback.mdc.v1_0.internal; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.google.common.collect.ImmutableMap; -import java.util.Collections; -import java.util.Map; -import java.util.Set; -import java.util.stream.Stream; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.MethodSource; - -class UnionMapTest { - - @ParameterizedTest - @MethodSource("providesMapsArguments") - void testMaps(Map first, Map second) { - UnionMap union = new UnionMap<>(first, second); - - assertThat(union.get("cat")).isEqualTo("meow"); - assertThat(union.get("dog")).isEqualTo("bark"); - assertThat(union.get("foo")).isEqualTo("bar"); - assertThat(union.get("hello")).isEqualTo("world"); - assertThat(union.get("giraffe")).isNull(); - - assertThat(union.isEmpty()).isFalse(); - assertThat(union.size()).isEqualTo(4); - assertThat(union.containsKey("cat")).isTrue(); - assertThat(union.containsKey("dog")).isTrue(); - assertThat(union.containsKey("foo")).isTrue(); - assertThat(union.containsKey("hello")).isTrue(); - assertThat(union.containsKey("giraffe")).isFalse(); - - Set> set = union.entrySet(); - assertThat(set.isEmpty()).isFalse(); - assertThat(set.size()).isEqualTo(4); - assertThat(set.toArray().length).isEqualTo(4); - } - - private static Stream providesMapsArguments() { - ImmutableMap firstArg = - ImmutableMap.of( - "cat", "meow", - "dog", "bark"); - - return Stream.of( - Arguments.of( - firstArg, - ImmutableMap.of( - "foo", "bar", - "hello", "world")), - Arguments.of( - firstArg, - ImmutableMap.of( - "foo", "bar", - "hello", "world", - "cat", "moo"))); - } - - @Test - void testBothEmpty() { - UnionMap union = new UnionMap<>(Collections.emptyMap(), Collections.emptyMap()); - - assertThat(union.isEmpty()).isTrue(); - assertThat(union.size()).isEqualTo(0); - assertThat(union.get("cat")).isNull(); - - Set> set = union.entrySet(); - assertThat(set.isEmpty()).isTrue(); - assertThat(set.size()).isEqualTo(0); - - assertThat(set.toArray().length).isEqualTo(0); - } - - @ParameterizedTest - @MethodSource("providesOneEmptyArguments") - void testOneEmpty(Map first, Map second) { - UnionMap union = new UnionMap<>(first, second); - - assertThat(union.isEmpty()).isFalse(); - assertThat(union.size()).isEqualTo(1); - assertThat(union.get("cat")).isEqualTo("meow"); - assertThat(union.get("dog")).isNull(); - - Set> set = union.entrySet(); - assertThat(set.isEmpty()).isFalse(); - assertThat(set.size()).isEqualTo(1); - - assertThat(set.toArray().length).isEqualTo(1); - } - - private static Stream providesOneEmptyArguments() { - return Stream.of( - Arguments.of(ImmutableMap.of("cat", "meow"), Collections.emptyMap()), - Arguments.of(Collections.emptyMap(), ImmutableMap.of("cat", "meow"))); - } -} diff --git a/instrumentation/logback/logback-mdc-1.0/library/src/test/resources/logback.xml b/instrumentation/logback/logback-mdc-1.0/library/src/test/resources/logback.xml index b934356e3af4..aed73c5b3f47 100644 --- a/instrumentation/logback/logback-mdc-1.0/library/src/test/resources/logback.xml +++ b/instrumentation/logback/logback-mdc-1.0/library/src/test/resources/logback.xml @@ -17,10 +17,12 @@ + +