-
Notifications
You must be signed in to change notification settings - Fork 855
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
Remove mdc attributes prefix #9536
Remove mdc attributes prefix #9536
Conversation
990a25b
to
fd1c6b5
Compare
...java/io/opentelemetry/instrumentation/logback/appender/v1_0/internal/LoggingEventMapper.java
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.
4e5bea6
to
07b85e6
Compare
@laurit @mateuszrzeszutek do you think we should hold this for 2.0? |
This is related to: open-telemetry/opentelemetry-java#5880 If we put structured key value pairs in the body as AnyValue, there is no chance of collision with the attributes and the prefix is uneeded. |
one concern I have with putting MDC attributes in the body is that they won't be available to attribute-based processors (e.g. in the collector), or searchable in backends in the same way other log attributes are, at least without additional backend work / interpretation of the log bodies |
I think it is fine either way. We could put it in 2.0 along with other breaking changes or just outline this as a breaking change in the release notes of the next release. |
Do we need a configuration to enable the prefix of MDC? |
hi @SHaaD94! we are preparing to release v2.0 soon, can you rebase your PR and check any CI failures? thx |
313cda1
to
847ff0c
Compare
847ff0c
to
6c3bbb0
Compare
@trask sorry for the delay replying. I don't think we should put MDC attributes in the body - I think we should MapMessage / KeyValuePair in the body. Do you think we should do this and if so, do you think its worthwhile to bundle in with 2.0? |
Sorry for even more delayed reply 😂
Translating the |
Discussed in SIG. I will push commit to revert the map_message portion in order to avoid churn since we are planning to move map messages to log body in the future. |
… log body in the future
As discussed in #9506 removing MDC attributes prefix for both log4j and logback.