OpenTelemetry Log4j appender does not handle non string values

[rupinder10]

When the Log4j LogEvent context data contains non string values e.g string arrays or java maps, the resulting attributes are incorrect.

Create a class extending ObjectThreadContextMap and set it as the default using the log4j system property log4j2.threadContextMap

Add the following code:

public class MyObjectContextMap implements ReadOnlyThreadContextMap, ObjectThreadContextMap {
//Implementation methods
}

In the logging code add the following

MyObjectContextMap map = (MyObjectContextMap)ThreadContext.getThreadContextMap();
map.putValue("numbers", new String[]{"one", "two", "three"});
HashMap testmap = new HashMap();
testmap.put("fn", "firstname");
testmap.put("ln", "lastname");
testmap.put(age, 25);
map.put("testmap", testmap);

logger.log(Level.TRACE, "test message");

What did you expect to see?
I would expect that the nested value and the array value would be included in attributes properly.

What did you see instead?
I saw the .toString() representations of the array and map. So numbers showed as an attribute with value [Ljava.lang.String and testmap showed up as a single value with the map values as {fn=firstname, ln=lastname}.

What version are you using?
1.23.0-alpha
Log4j: 2.17.1

[breedx-splk]

The root cause might be this? https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/log4j/log4j-appender-2.17/library/src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/internal/LogEventMapper.java#L163

It doesn't really look like the current design is intended to handle heterogeneous types, especially list/collection types.

This test can be added to the LogEventMapperTest to verify the behavior:

@Test
  void testObjects() {
    // given
    ContextDataAccessor<Map<String, Object>> accessor = new ContextDataAccessor<Map<String, Object>>() {
      @Nullable
      @Override
      public Object getValue(Map<String, Object> contextData, String key) {
        return contextData.get(key);
      }

      @Override
      public void forEach(Map<String, Object> contextData, BiConsumer<String, Object> action) {
        contextData.forEach(action);
      }
    };
    Map<String, Object> map = new HashMap<>();
    LogEventMapper<Map<String, Object>> mapper =
        new LogEventMapper<>(accessor, false, false, false, singletonList("*"));
    Map<String, Object> contextData = new HashMap<>();
    contextData.put("key1", "value1");
    contextData.put("key2", new String[]{"one", "two", "three"});
    map.put("fn", "Joe");
    map.put("ln", "Smitty");
    contextData.put("key3", map);
    AttributesBuilder attributes = Attributes.builder();

    // when
    mapper.captureContextDataAttributes(attributes, contextData);

    // then
    Attributes result = attributes.build();
    assertThat(result.get(stringKey("log4j.context_data.key1"))).isEqualTo("value1");
    assertThat(result.get(stringKey("log4j.context_data.key2"))).startsWith("[Ljava.lang.String;");
    assertThat(result.get(stringKey("log4j.context_data.key3"))).startsWith("{ln=Smitty, fn=Joe}");
  }

Open to ideas.

[zeitlinger]

What about flattening maps (unless they are pat of an array)?

testmap.fn = firstname
testmap.numbers = one, two, three

[mateuszrzeszutek]

I think that nested attributes (open-telemetry/opentelemetry-specification#2888) would probably be something we'd want to use here; they're not specced yet though.

[trask]

I think that nested attributes (open-telemetry/opentelemetry-specification#2888) would probably be something we'd want to use here; they're not specced yet though.

👍

any thoughts what to do in the meantime?

• json serialize maps/arrays
• skip maps/arrays

skipping them is probably better from a stability perspective if we are hoping to use nested attributes in the future

[mateuszrzeszutek]

Agree on skipping them 👍
Let's wait for the decision on nested attributes.