InstrumentationConfig part 5: library logging appenders (open-telemet…

…ry#6321) * InstrumentationConfig part 5: library logging appenders * Logback * remove log4j hackery * fix tests * Remove unused Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
helios · Oct 23, 2022 · b9aee50 · b9aee50
1 parent 09abf33
commit b9aee50
Show file tree

Hide file tree

Showing 13 changed files with 200 additions and 84 deletions.
diff --git a/...ain/java/io/opentelemetry/javaagent/instrumentation/log4j/appender/v2_17/Log4jHelper.java b/...ain/java/io/opentelemetry/javaagent/instrumentation/log4j/appender/v2_17/Log4jHelper.java
@@ -5,10 +5,14 @@
 
 package io.opentelemetry.javaagent.instrumentation.log4j.appender.v2_17;
 
+import static java.util.Collections.emptyList;
+
 import io.opentelemetry.instrumentation.api.appender.internal.LogBuilder;
 import io.opentelemetry.instrumentation.log4j.appender.v2_17.internal.ContextDataAccessor;
 import io.opentelemetry.instrumentation.log4j.appender.v2_17.internal.LogEventMapper;
 import io.opentelemetry.javaagent.bootstrap.AgentLogEmitterProvider;
+import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig;
+import java.util.List;
 import java.util.Map;
 import java.util.function.BiConsumer;
 import javax.annotation.Nullable;
@@ -19,8 +23,29 @@
 
 public final class Log4jHelper {
 
-  private static final LogEventMapper<Map<String, String>> mapper =
-      new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE);
+  private static final LogEventMapper<Map<String, String>> mapper;
+
+  static {
+    InstrumentationConfig config = InstrumentationConfig.get();
+
+    boolean captureExperimentalAttributes =
+        config.getBoolean("otel.instrumentation.log4j-appender.experimental-log-attributes", false);
+    boolean captureMapMessageAttributes =
+        config.getBoolean(
+            "otel.instrumentation.log4j-appender.experimental.capture-map-message-attributes",
+            false);
+    List<String> captureContextDataAttributes =
+        config.getList(
+            "otel.instrumentation.log4j-appender.experimental.capture-context-data-attributes",
+            emptyList());
+
+    mapper =
+        new LogEventMapper<>(
+            ContextDataAccessorImpl.INSTANCE,
+            captureExperimentalAttributes,
+            captureMapMessageAttributes,
+            captureContextDataAttributes);
+  }
 
   public static void capture(Logger logger, Level level, Message message, Throwable throwable) {
     String instrumentationName = logger.getName();

diff --git a/instrumentation/log4j/log4j-appender-2.17/library/build.gradle.kts b/instrumentation/log4j/log4j-appender-2.17/library/build.gradle.kts
@@ -12,9 +12,3 @@ dependencies {
   testImplementation("io.opentelemetry:opentelemetry-sdk-logs")
   testImplementation("io.opentelemetry:opentelemetry-sdk-testing")
 }
-
-tasks.withType<Test>().configureEach {
-  // TODO run tests both with and without experimental log attributes
-  jvmArgs("-Dotel.instrumentation.log4j-appender.experimental.capture-map-message-attributes=true")
-  jvmArgs("-Dotel.instrumentation.log4j-appender.experimental.capture-context-data-attributes=*")
-}
diff --git a/...ain/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/OpenTelemetryAppender.java b/...ain/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/OpenTelemetryAppender.java
@@ -5,6 +5,8 @@
 
 package io.opentelemetry.instrumentation.log4j.appender.v2_17;
 
+import static java.util.Collections.emptyList;
+
 import io.opentelemetry.instrumentation.api.appender.internal.LogBuilder;
 import io.opentelemetry.instrumentation.api.appender.internal.LogEmitterProvider;
 import io.opentelemetry.instrumentation.api.appender.internal.LogEmitterProviderHolder;
@@ -13,9 +15,13 @@
 import io.opentelemetry.instrumentation.sdk.appender.internal.DelegatingLogEmitterProvider;
 import io.opentelemetry.sdk.logs.SdkLogEmitterProvider;
 import java.io.Serializable;
+import java.util.Arrays;
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
+import org.apache.logging.log4j.ThreadContext;
 import org.apache.logging.log4j.core.Appender;
 import org.apache.logging.log4j.core.Core;
 import org.apache.logging.log4j.core.Filter;
@@ -24,8 +30,10 @@
 import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.logging.log4j.core.config.Property;
 import org.apache.logging.log4j.core.config.plugins.Plugin;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
 import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
 import org.apache.logging.log4j.core.time.Instant;
+import org.apache.logging.log4j.message.MapMessage;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
 
 @Plugin(
@@ -39,8 +47,7 @@ public class OpenTelemetryAppender extends AbstractAppender {
   private static final LogEmitterProviderHolder logEmitterProviderHolder =
       new LogEmitterProviderHolder();
 
-  private static final LogEventMapper<ReadOnlyStringMap> mapper =
-      new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE);
+  private final LogEventMapper<ReadOnlyStringMap> mapper;
 
   @PluginBuilderFactory
   public static <B extends Builder<B>> B builder() {
@@ -50,10 +57,43 @@ public static <B extends Builder<B>> B builder() {
   static class Builder<B extends Builder<B>> extends AbstractAppender.Builder<B>
       implements org.apache.logging.log4j.core.util.Builder<OpenTelemetryAppender> {
 
+    @PluginBuilderAttribute private boolean captureExperimentalAttributes;
+    @PluginBuilderAttribute private boolean captureMapMessageAttributes;
+    @PluginBuilderAttribute private String captureContextDataAttributes;
+
+    /**
+     * Sets whether experimental attributes should be set to logs. These attributes may be changed
+     * or removed in the future, so only enable this if you know you do not require attributes
+     * filled by this instrumentation to be stable across versions.
+     */
+    public B setCaptureExperimentalAttributes(boolean captureExperimentalAttributes) {
+      this.captureExperimentalAttributes = captureExperimentalAttributes;
+      return asBuilder();
+    }
+
+    /** Sets whether log4j {@link MapMessage} attributes should be copied to logs. */
+    public B setCaptureMapMessageAttributes(boolean captureMapMessageAttributes) {
+      this.captureMapMessageAttributes = captureMapMessageAttributes;
+      return asBuilder();
+    }
+
+    /** Configures the {@link ThreadContext} attributes that will be copied to logs. */
+    public B setCaptureContextDataAttributes(String captureContextDataAttributes) {
+      this.captureContextDataAttributes = captureContextDataAttributes;
+      return asBuilder();
+    }
+
     @Override
     public OpenTelemetryAppender build() {
       return new OpenTelemetryAppender(
-          getName(), getLayout(), getFilter(), isIgnoreExceptions(), getPropertyArray());
+          getName(),
+          getLayout(),
+          getFilter(),
+          isIgnoreExceptions(),
+          getPropertyArray(),
+          captureExperimentalAttributes,
+          captureMapMessageAttributes,
+          captureContextDataAttributes);
     }
   }
 
@@ -62,8 +102,28 @@ private OpenTelemetryAppender(
       Layout<? extends Serializable> layout,
       Filter filter,
       boolean ignoreExceptions,
-      Property[] properties) {
+      Property[] properties,
+      boolean captureExperimentalAttributes,
+      boolean captureMapMessageAttributes,
+      String captureContextDataAttributes) {
+
     super(name, filter, layout, ignoreExceptions, properties);
+    this.mapper =
+        new LogEventMapper<>(
+            ContextDataAccessorImpl.INSTANCE,
+            captureExperimentalAttributes,
+            captureMapMessageAttributes,
+            splitAndFilterBlanksAndNulls(captureContextDataAttributes));
+  }
+
+  private static List<String> splitAndFilterBlanksAndNulls(String value) {
+    if (value == null) {
+      return emptyList();
+    }
+    return Arrays.stream(value.split(","))
+        .map(String::trim)
+        .filter(s -> !s.isEmpty())
+        .collect(Collectors.toList());
   }
 
   @Override

diff --git a/...n/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapper.java b/...n/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapper.java
@@ -5,15 +5,12 @@
 
 package io.opentelemetry.instrumentation.log4j.appender.v2_17.internal;
 
-import static java.util.Collections.emptyList;
-
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.appender.internal.LogBuilder;
 import io.opentelemetry.instrumentation.api.appender.internal.Severity;
-import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.instrumentation.api.internal.cache.Cache;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.io.PrintWriter;
@@ -33,44 +30,26 @@ public final class LogEventMapper<T> {
 
   private static final String SPECIAL_MAP_MESSAGE_ATTRIBUTE = "message";
 
-  private static final boolean captureExperimentalAttributes =
-      Config.get()
-          .getBoolean("otel.instrumentation.log4j-appender.experimental-log-attributes", false);
-
   private static final Cache<String, AttributeKey<String>> contextDataAttributeKeyCache =
       Cache.bounded(100);
   private static final Cache<String, AttributeKey<String>> mapMessageAttributeKeyCache =
       Cache.bounded(100);
 
-  private final boolean captureMapMessageAttributes;
+  private final ContextDataAccessor<T> contextDataAccessor;
 
+  private final boolean captureExperimentalAttributes;
+  private final boolean captureMapMessageAttributes;
   private final List<String> captureContextDataAttributes;
-
-  // cached as an optimization
   private final boolean captureAllContextDataAttributes;
 
-  private final ContextDataAccessor<T> contextDataAccessor;
-
-  public LogEventMapper(ContextDataAccessor<T> contextDataAccessor) {
-    this(
-        contextDataAccessor,
-        Config.get()
-            .getBoolean(
-                "otel.instrumentation.log4j-appender.experimental.capture-map-message-attributes",
-                false),
-        Config.get()
-            .getList(
-                "otel.instrumentation.log4j-appender.experimental.capture-context-data-attributes",
-                emptyList()));
-  }
-
-  // visible for testing
-  LogEventMapper(
+  public LogEventMapper(
       ContextDataAccessor<T> contextDataAccessor,
+      boolean captureExperimentalAttributes,
       boolean captureMapMessageAttributes,
       List<String> captureContextDataAttributes) {
 
     this.contextDataAccessor = contextDataAccessor;
+    this.captureExperimentalAttributes = captureExperimentalAttributes;
     this.captureMapMessageAttributes = captureMapMessageAttributes;
     this.captureContextDataAttributes = captureContextDataAttributes;
     this.captureAllContextDataAttributes =

diff --git a/...va/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapperTest.java b/...va/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapperTest.java
@@ -24,15 +24,15 @@
 import javax.annotation.Nullable;
 import org.apache.logging.log4j.message.StringMapMessage;
 import org.apache.logging.log4j.message.StructuredDataMessage;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
 
 class LogEventMapperTest {
 
   @Test
   void testDefault() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, emptyList());
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, false, emptyList());
     Map<String, String> contextData = new HashMap<>();
     contextData.put("key1", "value1");
     contextData.put("key2", "value2");
@@ -49,7 +49,7 @@ void testDefault() {
   void testSome() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, singletonList("key2"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, false, singletonList("key2"));
     Map<String, String> contextData = new HashMap<>();
     contextData.put("key1", "value1");
     contextData.put("key2", "value2");
@@ -67,7 +67,7 @@ void testSome() {
   void testAll() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, singletonList("*"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, false, singletonList("*"));
     Map<String, String> contextData = new HashMap<>();
     contextData.put("key1", "value1");
     contextData.put("key2", "value2");
@@ -87,7 +87,7 @@ void testAll() {
   void testCaptureMapMessageDisabled() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, singletonList("*"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, false, singletonList("*"));
 
     StringMapMessage message = new StringMapMessage();
     message.put("key1", "value1");
@@ -108,7 +108,7 @@ void testCaptureMapMessageDisabled() {
   void testCaptureMapMessageWithSpecialAttribute() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, true, singletonList("*"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, true, singletonList("*"));
 
     StringMapMessage message = new StringMapMessage();
     message.put("key1", "value1");
@@ -129,7 +129,7 @@ void testCaptureMapMessageWithSpecialAttribute() {
   void testCaptureMapMessageWithoutSpecialAttribute() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, true, singletonList("*"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, true, singletonList("*"));
 
     StringMapMessage message = new StringMapMessage();
     message.put("key1", "value1");
@@ -153,7 +153,7 @@ void testCaptureMapMessageWithoutSpecialAttribute() {
   void testCaptureStructuredDataMessage() {
     // given
     LogEventMapper<Map<String, String>> mapper =
-        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, true, singletonList("*"));
+        new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, true, singletonList("*"));
 
     StructuredDataMessage message = new StructuredDataMessage("an id", "a message", "a type");
     message.put("key1", "value1");

diff --git a/instrumentation/log4j/log4j-appender-2.17/library/src/test/resources/log4j2-test.xml b/instrumentation/log4j/log4j-appender-2.17/library/src/test/resources/log4j2-test.xml
@@ -6,13 +6,12 @@
       <PatternLayout
         pattern="%d{HH:mm:ss.SSS} [%t] %-5level %logger{36} traceId: %X{trace_id} spanId: %X{span_id} flags: %X{trace_flags} - %msg%n"/>
     </Console>
-    <ListAppender name="ListAppender"/>
-    <OpenTelemetry name="OpenTelemetryAppender"/>
+    <!-- TODO run tests both with and without experimental log attributes -->
+    <OpenTelemetry name="OpenTelemetryAppender" captureMapMessageAttributes="true" captureContextDataAttributes="*"/>
   </Appenders>
   <Loggers>
     <Logger name="TestLogger" level="All">
       <AppenderRef ref="OpenTelemetryAppender" level="All"/>
-      <AppenderRef ref="ListAppender" level="All"/>
       <AppenderRef ref="Console" level="All"/>
     </Logger>
     <Root>

diff --git a/...opentelemetry/javaagent/instrumentation/logback/appender/v1_0/LogbackInstrumentation.java b/...opentelemetry/javaagent/instrumentation/logback/appender/v1_0/LogbackInstrumentation.java
@@ -5,6 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.logback.appender.v1_0;
 
+import static io.opentelemetry.javaagent.instrumentation.logback.appender.v1_0.LogbackSingletons.mapper;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.isPublic;
 import static net.bytebuddy.matcher.ElementMatchers.named;
@@ -13,7 +14,6 @@
 
 import ch.qos.logback.classic.spi.ILoggingEvent;
 import io.opentelemetry.instrumentation.api.appender.internal.LogEmitterProvider;
-import io.opentelemetry.instrumentation.logback.appender.v1_0.internal.LoggingEventMapper;
 import io.opentelemetry.javaagent.bootstrap.AgentLogEmitterProvider;
 import io.opentelemetry.javaagent.bootstrap.CallDepth;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -51,7 +51,7 @@ public static void methodEnter(
       // logging framework delegates to another
       callDepth = CallDepth.forClass(LogEmitterProvider.class);
       if (callDepth.getAndIncrement() == 0) {
-        LoggingEventMapper.INSTANCE.emit(AgentLogEmitterProvider.get(), event);
+        mapper().emit(AgentLogEmitterProvider.get(), event);
       }
     }
 

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/logback/appender/v1_0/LogbackSingletons.java b/...a/io/opentelemetry/javaagent/instrumentation/logback/appender/v1_0/LogbackSingletons.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.logback.appender.v1_0;
+
+import static java.util.Collections.emptyList;
+
+import io.opentelemetry.instrumentation.logback.appender.v1_0.internal.LoggingEventMapper;
+import io.opentelemetry.javaagent.bootstrap.internal.InstrumentationConfig;
+import java.util.List;
+
+public class LogbackSingletons {
+
+  private static final LoggingEventMapper mapper;
+
+  static {
+    InstrumentationConfig config = InstrumentationConfig.get();
+
+    boolean captureExperimentalAttributes =
+        config.getBoolean(
+            "otel.instrumentation.logback-appender.experimental-log-attributes", false);
+    List<String> captureMdcAttributes =
+        config.getList(
+            "otel.instrumentation.logback-appender.experimental.capture-mdc-attributes",
+            emptyList());
+
+    mapper = new LoggingEventMapper(captureExperimentalAttributes, captureMdcAttributes);
+  }
+
+  public static LoggingEventMapper mapper() {
+    return mapper;
+  }
+}