-
Notifications
You must be signed in to change notification settings - Fork 143
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
NR-23881 - Give users possibility to add MDC to logging #866
Conversation
fcaf826
to
ec8712a
Compare
@kford-newrelic @jasonjkeller Hey guys! May I ask for a review, please? Thanks so much |
@StanlieK thanks for the PR, we really appreciate it! We're not in a position to review this at the moment, since we're working on high-priority work for our next release but rest assured that we'll add this to our roadmap and will consider it for a future release! |
ec8712a
to
e2aacd6
Compare
I want to plus one this PR. I ended up implementing this slightly different on my fork. I did a whitelist of MDC keys but I'd be perfectly happy having all the MDC keys too. @StanlieK I'd prefer if my vars are NOT prefixed by the key EX. Our java services and go services add On my fork I have implemented it like this
☝️ |
I'd also like to say that I need this PR and #849 for this to be a feature complete logging solution for my microservices. |
|
||
logEventMap.put(MESSAGE, formattedMessage); | ||
logEventMap.put(TIMESTAMP, event.getTimeMillis()); | ||
|
||
if (isApplicationLoggingForwardingIncludeMdcEnabled() && event.getContextData() != null) { | ||
event.getContextData().forEach((key, value) -> logEventMap.put(MDC_ATTRIBUTE_PREFIX + key, value)); |
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.
I'd love to make the prefix optional.
When working with distributed microservices that are multi-language / framework its nice to have a single context key to search for.
I wouldn't want to have to write queries that are WHERE mdc.foo = 'bar' AND foo = 'bar'
if I could just write WHERE foo = 'bar'
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.
I completely agree with you, I'll incorporate it.
@fieldju I like the idea of MDC properties not being prefixed. Or at least an optional prefix. Also, I like the idea of whitelisted/blacklisted properties. It would be nice if all the properties will be allowed by default, as we have lots of properties from project to project. It would be madness to control each separately. In this case, it would be useful to have allowed and disallowed properties. If you'll set disallowed to |
@StanlieK Since they aren't too interesting in merging these PRs, we pivoted to an alternate way of shipping logs to New Relic that gives us total control. We ended up making a custom logback layout and using the New Relic Fluent Bit helm chart You can see/use/copy that config here in this gist point_up this will take forward any k,v pair that is in the json blob including any MDC kv pair. So if I add some random MDC k,v pairs like so @Scheduled(fixedRate = 10, timeUnit = TimeUnit.SECONDS)
public void foo() {
MDC.put("foo", "bar");
MDC.put("uuid", UUID.randomUUID().toString());
log.info("preparing to log fake errors");
var err = new RuntimeException("fake error");
var err2 = new RuntimeException("another fake error", err);
log.error("fake error message", err2);
MDC.clear();
} ☝️ foo=bar is present in the log data in NR. Good Luck! |
@fieldju sorry that it's taking us a while to get to this PR - as you can probably imagine, it's not as simple as merging in this one PR - we need to make sure we have solutions that will work for as wide an audience as possible and that we can (hopefully) implement relatively consistently across other logging frameworks, etc. The good new though is that we have adding MDC to our logs on our roadmap for the Jul-Sep quarter, so we expect to get to this reasonably soon! |
@StanlieK writing to let you know that I started working on merging this PR. Because of that, this will not be in our upcoming 7.10 release, but most likely in our 7.11 release. |
…o NR from log4j-2 and logback
e2aacd6
to
ea97eb3
Compare
Filtering mapped context attributes given include/exclude lists. Extracting common methods from AgentUtils classes to AppLogginUtil in agent-bridge
ea97eb3
to
66bbdec
Compare
This has been tested and reviewed in #1016 |
Please help to change this document as it is not correct
|
@mcflythekid this PR was modified before it was added to the codebase. |
Include MDC data into logs forwarded to NR from log4j-2 and logback
Overview
This PR will add support for including MDC data in logs forwarded to NR from log4j-2 and logback. It maps the MDC map into log events prefixed by
mdc.
. E.g. if you have the MDC keyexample
, it will have the keymdc.example
in New Relic.The whole feature is disabled by default and can be enabled by setting:
application_logging.forwarding.include_mdc.enabled
totrue
NEW_RELIC_APPLICATION_LOGGING_FORWARDING_INCLUDE_MDC_ENABLED
totrue
Important: This feature is not available for log4j 1.x as the agent does not support forwarding of the logs at all.
Example - Logback with disabled INCLUDE_MDC feature
Example - Logback with enabled INCLUDE_MDC feature
Example - Log4j with disabled INCLUDE_MDC feature
Example - Log4j with enabled INCLUDE_MDC feature
Related Github Issue
The feature has been requested in this issue #801.
Testing
The agent includes a suite of tests that should be used to
verify your changes don't break existing functionality. These tests will run with
Github Actions when a pull request is made. More details on running the tests locally can be found
here.
Checks
[x] Are your contributions backwards compatible with relevant frameworks and APIs?
[ ] Does your code contain any breaking changes? Please describe.
[ ] Does your code introduce any new dependencies? Please describe.