newrelic / newrelic-java-agent

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

Adds logback-classic-1.2 instrumentation #618

Closed jsubirat closed 2 years ago

jsubirat commented 2 years ago

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

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

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

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

jsubirat commented 2 years ago

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

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

dylanmei commented 2 years ago

Thanks! The overview is now much clearer. :joy:

jasonjkeller commented 2 years ago

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

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

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

Hi @jasonjkeller!

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