Skip to content
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

feature(logback-classic): add ability to include stack traces when forwarding logs #849

Closed
wants to merge 1 commit into from

Conversation

fieldju
Copy link

@fieldju fieldju commented May 13, 2022

Overview

This PR adds the ability to for logback classic log forwarding to include stack traces.

I complied this and included it in one of my apps.

With out the feature flag, the logs have no stack trace just like it is today:
image

With the feature flag, the logs now include the stack trace:
image
image

Related Github Issue

Include a link to the related GitHub issue, if applicable

Testing

The agent includes a suite of tests which 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.

@CLAassistant
Copy link

CLAassistant commented May 13, 2022

CLA assistant check
All committers have signed the CLA.

@@ -58,6 +62,13 @@ public static void recordNewRelicLogEvent(String message, long timeStampMillis,
logEventMap.put(LOG_LEVEL, level);
}

if (t != null && isApplicationForwardStackTracesEnabled()) {
Copy link
Author

Choose a reason for hiding this comment

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

My co-worker and expected this to be the default behavior, we'd be in-favor of this not even being behind a flag.
I assume this was omitted to save data egress / ingress.

@kford-newrelic
Copy link
Contributor

@fieldju thanks for the comprehensive PR! We'll add this to our backlog for review - thanks again!

@fieldju
Copy link
Author

fieldju commented Jun 10, 2022

@kford-newrelic I saw that you added the Represents a FY Q2 sustainability candidate label to #866.

I'm not entirely sure what that label means but I strongly feel that this PR and #866 are needed ASAP for the agent to be a feature complete logging solution.

I am currently forced to run a fork of this agent until these 2 PRs get merged/released.

@kford-newrelic
Copy link
Contributor

Not selected for next quarter's agent roadmap. Will consider for a future agent release.

@fieldju thank you for the feedback; that label essentially means we were considering this for our next quarter's (Jul-Sep) agent roadmap but unfortunately, we determined that we would need more engineering time to implement than we had available.

This is a good feature to have but we decided that we would want a consistent experience across other loggers and that we would want to communicate with other language agents, so this could potentially be implemented for those, again for consistency.

We will have to consider this for a future quarter/agent release.

@fieldju
Copy link
Author

fieldju commented Jun 15, 2022

@kford-newrelic, I am really confused why you are pushing the Agent as a production ready logging solution when you strip away arguably the most important peace of the logs when it comes to debugging production issues in the JVM.

It seems like you shouldn't be turning on log-forwarding by default and discouraging functional solutions like fluentd until you have this figured out.

@fieldju fieldju closed this Jun 15, 2022
@fieldju
Copy link
Author

fieldju commented Jun 15, 2022

Closing this PR.

We talked about it internally and decided that it'd be best to use Sleuth, Logback and Micrometer/Prometheus in combination with the NRI prometheus scrapper and Fluentd w/ the NR Plugin.

This means we get the features we need and avoid vendor lock in or heavy weight agents.

We also mulled over the merits of writing a Logback appender that could send logs straight to New Relic.
You can find a couple on google if you want, but it seems like fluentd will enable us to avoid vendor lock in and have a single pattern that is x-language/framework.

@fieldju
Copy link
Author

fieldju commented Jun 17, 2022

Closing the loop on this.

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

☝️ this will take forward any k,v pair that is in the json blob including any MDC kv pair.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants