Skip to content

Support log4j map messages - #4966

Merged
trask merged 11 commits into
open-telemetry:mainfrom
trask:log4j-map-messages
Jan 6, 2022
Merged

Support log4j map messages#4966
trask merged 11 commits into
open-telemetry:mainfrom
trask:log4j-map-messages

Conversation

@trask

@trask trask commented Dec 22, 2021

Copy link
Copy Markdown
Member

Introduces experimental setting otel.instrumentation.log4j-appender.experimental.capture-map-message-attributes to capture log4j MapMessage key/value pairs as log attributes.

It doesn't add any prefix to those keys because this is going to be the primary mechanism for users to report arbitrary key/value attributes on logs.

I would support enabling the default to true for this setting, since it is critical for supporting structured events, but it is disabled by default in this PR for now.

if (message != null) {
builder.setBody(message.getFormattedMessage());
if (message instanceof MapMessage) {
builder.setBody(message.getFormat());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird API :O

Looks like getFormat returns the message for StructuredDataMessage and empty string for StringMapMessage. From what I've seen, it's conventional to use a map message with an attribute message for the body. How about doing instanceof StructuredDataMessage for message.getFormat(), and checking the attribute for StringMapMessage, leaving Body empty if it's not set?

@trask
trask requested a review from a team January 5, 2022 21:54
@trask
trask requested a review from anuraaga January 6, 2022 00:51
@trask
trask merged commit 07ce0b5 into open-telemetry:main Jan 6, 2022
@trask
trask deleted the log4j-map-messages branch January 6, 2022 20:02
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Support log4j map messages

* Cache attribute keys

* Review

* Add tests

* Fix test

* Codenarc

* Better tests

* Remove sysout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants