-
Notifications
You must be signed in to change notification settings - Fork 146
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 local decorating support for adding linking metadata when using Log4j2 JsonLayout #1472
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… to Log4j2 JsonLayout
Codecov Report
@@ Coverage Diff @@
## main #1472 +/- ##
============================================
+ Coverage 70.58% 70.59% +0.01%
- Complexity 9744 9747 +3
============================================
Files 813 813
Lines 39259 39269 +10
Branches 5961 5963 +2
============================================
+ Hits 27711 27722 +11
Misses 8862 8862
+ Partials 2686 2685 -1
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…nto two modules to support log4j 2.6-2.14 and 2.15+
…was already added by other logger instrumentation (e.g. JUL).
jasonjkeller
changed the title
Prototype two potential approaches to support adding linking metadata…
Add local decorating support for adding linking metadata when using Log4j2 JsonLayout
Sep 15, 2023
meiao
reviewed
Sep 25, 2023
instrumentation/apache-log4j-2/src/main/java/com/nr/agent/instrumentation/log4j2/AgentUtil.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/logging/log4j/core/layout/AbstractJacksonLayout_Instrumentation.java
Outdated
Show resolved
Hide resolved
meiao
approved these changes
Sep 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… to Log4j2 JsonLayout
Resolves #1467
This PR prototypes a few different approaches to support adding NR-LINKING metadata to the message when using JsonLayout with Log4j2.
OPTION 1: RECOMMENDED
TL;DR Recommended as it does not break other Log4j2 layouts, limits the scope of layouts it could alter to just Json/Yaml/Xml (though only modifies Json), and does not requires additional user config or splitting the instrumentation across multiple modules .
This approach weaves
AbstractJacksonLayout.toSerializable
and injects the NR-LINKING metadata into a JSON string (both pretty and compact formats) representing a LogEvent.https://github.com/apache/logging-log4j2/blob/7e745b42bda9bf6f8ea681d38992d18036fc021e/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractJacksonLayout.java#L282-L299
The result of this PR is that when logging using the
JsonLayout
and with local decorating enabled, theNR-LINKING
metadata blob (NR-LINKING|entity.guid|hostname|trace.id|span.id|entity.name|
) will be appended to the message string as shown below:OPTION 2: NOT RECOMMENDED
TL;DR Not recommended as it breaks other Log4j2 layouts and also requires splitting the instrumentation across two modules.
One option is to weave
AbstractStringLayout.getBytes(final String s)
and modify the string object which represents the JSON string that will be logged. This is the preferred option as it does not require any additional effort from customers.https://github.com/apache/logging-log4j2/blob/7e745b42bda9bf6f8ea681d38992d18036fc021e/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java#L213-L222
This involves some string parsing and manipulation that could be costly when it comes to overhead. You have to find the "message", navigate to the end of that line, and then append the NR-LINKING metadata. Further complicating matters is that the NR-LINKING might already exist if the logged line passed through a different logging library that is already adding the NR-LINKING metadata, such as the JUL example below. This means that first we would have to check if NR-LINKING already exists. The overhead is likely acceptable.
Major Caveat: It seems that this approach will break as of 2.15.0 as they seem to have refactoredAbstractStringLayout.getBytes
. We would have to introduce another instrumentation module to support 2.15.0+, assuming a similar approach can be taken with those newer versions.This caveat has been resolved with the most recent changes on this PR. There is now aapache-log4j-2
instrumentation module that supports versions 2.6 - 2.14 and aapache-log4j-2.15
instrumentation module that supports versions 2.15+.OPTION 3: NOT RECOMMENDED
TL;DR Not recommended as it breaks backwards compatibly with Log4j2 versions below 2.10.0 and it also requires additional config from customers.
Another approach is to weave the constructor for
AbstractJacksonLayout$LogEventWithAdditionalFields(final Object logEvent, final Map<String, String> additionalFields)
and add the NR-LINKING metadata to theadditionalFields
map.https://github.com/apache/logging-log4j2/blob/7e745b42bda9bf6f8ea681d38992d18036fc021e/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractJacksonLayout.java#L359-L362
However the code that instantiates
LogEventWithAdditionalFields
will never be hit unlessadditionalFields.length > 0
, which means that the customer will be required to have at least one additional field set in their JSON layout like shown below.An example of what this instrumentation looks like can be seen here: 4645256#diff-9975c5b457f8a20647825bca0aaa786ec7180120a77f69d8d3f0c1ab8d046deb
Major Caveat: Our apache-log4j-2 currently supports versions 2.6+ but this approach would force us to drop support for versions 2.9.1 and below. Effectively, the instrumentation would support 2.10.0+. This is because
AbstractJacksonLayout
has been refactored over these versions and theLogEventWithAdditionalFields
class that we would target to weave doesn't exist prior to 2.10.0.It might be technically possible to for us to modify the logic via weave instrumentation so that
additionalFields.length > 0
always evaluates totrue
so that it enters the first conditional statement and creates an instance ofLogEventWithAdditionalFields
but that feels dangerous as we would always force the logic to evaluate that way and it wouldn't be able to hit the other conditional branches after it.https://github.com/apache/logging-log4j2/blob/7e745b42bda9bf6f8ea681d38992d18036fc021e/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractJacksonLayout.java#L308-L321