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

Adds logback-classic-1.2 instrumentation #618

Merged

Conversation

jsubirat
Copy link
Contributor

@jsubirat jsubirat commented Dec 20, 2021

Overview

The following PR introduces instrumentation for the Logback logging framework. In particular, this instrumentation counts the amount of log lines emitted in total, as well as faceted by each of the log levels (DEBUG, INFO, etc.).

Note that this instrumentation doesn't simply count how many logger.info() calls have been performed, but how many log records have actually been accepted by the framework (that is, logs above the configured log level, and having been accepted by the turbofilters): we instrument the buildLoggingEventAndAppend method, which gets called when a log record has been accepted.

Testing

  • Unit tests have been provided that test the new functionality.
  • Internal performance tests have been performed using several services, which demonstrate no significant CPU/memory impact. Please get in touch with me to discuss about the results.

Checks

[x] Are your contributions backwards compatible with relevant frameworks and APIs?
[x] Does your code contain any breaking changes? Please describe.
[x] Does your code introduce any new dependencies? Please describe.

@jasonrclark
Copy link
Contributor

Question for the agent folks -- is there already a shared way built in if we needed to disable this instrumentation, or do we need to add a specific configuration of some sort?

@dylanmei
Copy link

Why shade another logging dependency instead of relying on simple java.util.logging?

@jsubirat jsubirat marked this pull request as ready for review December 20, 2021 15:42
@jsubirat
Copy link
Contributor Author

Why shade another logging dependency instead of relying on simple java.util.logging?

We focused on the Logback library since it is one of the most used, way above java.util.logging.

@jasonrclark
Copy link
Contributor

To clarify @dylanmei this is adding instrumentation for clients who use logback -- not using the library in the agent itself.

@dylanmei
Copy link

dylanmei commented Dec 20, 2021

Thanks! The overview is now much clearer. 😂

@kford-newrelic kford-newrelic added this to Triage in Java Engineering Board via automation Dec 20, 2021
@kford-newrelic kford-newrelic moved this from Triage to Needs Review in Java Engineering Board Dec 20, 2021
@jasonjkeller jasonjkeller self-requested a review January 13, 2022 22:18
@jasonjkeller
Copy link
Contributor

Question for the agent folks -- is there already a shared way built in if we needed to disable this instrumentation, or do we need to add a specific configuration of some sort?

Nothing special needs to be done. The Implementation-Title defined in the build.gradle can be used to disable the instrumentation as follows:

  class_transformer:
    com.newrelic.instrumentation.logback-classic-1.2:
      enabled: false

Is this instrumentation that we want enabled by default?

@jasonrclark
Copy link
Contributor

Great feedback @jasonjkeller! I definitely learned some new stuff.

Is this instrumentation that we want enabled by default?

Conversations internally have been that we will. Let's chat about it this week and we can loop back. There was also some internal conversation around naming that we should probably settle before we land anything here.

@jasonjkeller jasonjkeller changed the base branch from main to logging-initiative January 19, 2022 00:14
@jasonjkeller
Copy link
Contributor

FYI I've created a logging-initiative feature branch to merge PRs like this into and I've updated this PR to use that base instead of main. We'll have all PRs related to the logging initiative target the logging-initiative feature branch so that nothing makes it into an official agent release until absolutely ready.

@danybmx
Copy link
Contributor

danybmx commented Jan 19, 2022

Hi @jasonjkeller!

Comments from the review addressed on 5c73287, thanks for the review and the feedback on this!

@kford-newrelic kford-newrelic moved this from Needs Review to To do in Java Engineering Board Jan 19, 2022
@jasonjkeller jasonjkeller merged commit 520a605 into newrelic:logging-initiative Jan 19, 2022
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.

None yet

6 participants