Add marker attribute for Log4j 2 #6680
Conversation
| assertThat(log.getAttributes().get(SemanticAttributes.EXCEPTION_TYPE)).isEqualTo(IllegalStateException.getName()) | ||
| assertThat(log.getAttributes().get(SemanticAttributes.EXCEPTION_MESSAGE)).isEqualTo("hello") | ||
| assertThat(log.getAttributes().get(SemanticAttributes.EXCEPTION_STACKTRACE)).contains(Log4j2Test.name) | ||
| OpenTelemetryAssertions.assertThat(log.getAttributes()).containsEntry(SemanticAttributes.EXCEPTION_TYPE, IllegalStateException.getName()) | ||
| OpenTelemetryAssertions.assertThat(log.getAttributes()).containsEntry(SemanticAttributes.EXCEPTION_MESSAGE, "hello") | ||
| OpenTelemetryAssertions.assertThat(log.getAttributes().get(SemanticAttributes.EXCEPTION_STACKTRACE)).contains(Log4j2Test.name) |
There was a problem hiding this comment.
is it not possible to static import here due to conflicting assertThat signatures and/or because groovy?
There was a problem hiding this comment.
I have tried to add a static import with IntelliJ and some assertions were failing after that.😅 After, I did not go further in this refactoring on the assertions.
| new LogEventMapper<>(ContextDataAccessorImpl.INSTANCE, false, true, singletonList("*")); | ||
| new LogEventMapper<>( | ||
| ContextDataAccessorImpl.INSTANCE, false, true, false, singletonList("*")); |
There was a problem hiding this comment.
it may be worth introducing LogEventMapperBuilder in the future
There was a problem hiding this comment.
Agree, same thing for Logback instrumentation.
instrumentation/log4j/log4j-appender-2.17/javaagent/src/test/groovy/Log4j2Test.groovy
Outdated
Show resolved
Hide resolved
| @@ -35,22 +36,27 @@ | |||
| private static final Cache<String, AttributeKey<String>> mapMessageAttributeKeyCache = | |||
| Cache.bounded(100); | |||
|
|
|||
| private static final AttributeKey<String> LOG_MARKER = AttributeKey.stringKey("log4j.marker"); | |||
There was a problem hiding this comment.
Logback has markers too -- do we want to encode library name in the attribute name? I'm worried that we'll run into a situation similar to #5765, perhaps it'd be better to avoid it right from the start and just name it log.marker or sth similar.
There was a problem hiding this comment.
@trask Do you agree with @mateuszrzeszutek? Or would it be something we would like to discuss at the next OpenTelemetry Java + Instrumentation SIG?
There was a problem hiding this comment.
@mateuszrzeszutek I gave contradictory advice here: #6652 (comment)
so that it doesn't look like a spec'd attribute
I'm not sure what the best compromise here is
There was a problem hiding this comment.
It looks like we're already prefixing MDC/context data with the library name (log4j.context_data...), so I suppose doing the same for markers is not out of place.
I guess we can merge it as it is -- we'll most likely have to rename things anyway once the log semantic conventions start to take shape.
…roovy/Log4j2Test.groovy Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
No description provided.