-
Notifications
You must be signed in to change notification settings - Fork 859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to capture logback key value pairs #8074
Conversation
import org.slf4j.LoggerFactory; | ||
import org.slf4j.MarkerFactory; | ||
|
||
public class Slf4j2Test { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking: Would be interesting to see what happens with Slf4j using log4j as the backend. I think key value pairs show up in log4j context data. When adding log4j context data, we prefix all the keys with log4j.context.
. So effectively, if you use SLF4j, the mapping experience differs based on whether you use log4j or logback as the backend.
We should strive to make the two experiences consistent.
Maybe we need a dedicated slf4j testing module where we apply the same abstract test to the logback and log4j implementations and verify the expected behavior?
...umentation/logback/logback-appender-1.0/library/src/slf4j2ApiTest/resources/logback-test.xml
Show resolved
Hide resolved
...java/io/opentelemetry/instrumentation/logback/appender/v1_0/internal/LoggingEventMapper.java
Outdated
Show resolved
Hide resolved
...rc/slf4j2ApiTest/java/io/opentelemetry/instrumentation/logback/appender/v1_0/Slf4j2Test.java
Outdated
Show resolved
Hide resolved
...rc/slf4j2ApiTest/java/io/opentelemetry/instrumentation/logback/appender/v1_0/Slf4j2Test.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking: Would be interesting to see what happens with Slf4j using log4j as the backend. I think key value pairs show up in log4j context data. When adding log4j context data, we prefix all the keys with
log4j.context.
. So effectively, if you use SLF4j, the mapping experience differs based on whether you use log4j or logback as the backend.We should strive to make the two experiences consistent.
it doesn't bother me too much at this point, since the instrumentation is at the logback/log4j layer (as opposed to at the slf4j layer), and we don't have any logging semantic conventions yet to unify the two experiences.
Maybe we need a dedicated slf4j testing module where we apply the same abstract test to the logback and log4j implementations and verify the expected behavior?
👍 once we have logging semantic conventions to unify them
...java/io/opentelemetry/instrumentation/logback/appender/v1_0/internal/LoggingEventMapper.java
Outdated
Show resolved
Hide resolved
96d7a4c
to
b2ce969
Compare
Resolves #8059