newrelic / newrelic-java-agent

The New Relic Java agent
Apache License 2.0
202 stars 143 forks source link

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

Closed fieldju closed 2 years ago

fieldju commented 2 years ago

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 commented 2 years ago

CLA assistant check
All committers have signed the CLA.

kford-newrelic commented 2 years ago

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

fieldju commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

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

image