Skip to content

Commit

Permalink
inject-slf4j: Move the SLF4J API logging bridges.
Browse files Browse the repository at this point in the history
Problem/Solution

Update to move the SLF4J API logging bridges from dependencies of
`inject-sl4j` to dependencies of `inject-app` and `inject-server`
to allow other modules in the inject framework to be usable in
environments where having the bridges on the classpath can cause
issues.

JIRA Issues: CSL-6480

Differential Revision: https://phabricator.twitter.biz/D179652
  • Loading branch information
cacoco authored and jenkins committed Jun 8, 2018
1 parent 7b983dc commit 9e57dec
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 24 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ All notable changes to this project will be documented in this file. Note that `

### Changed

* inject-slf4j: Move the SLF4J API logging bridges from `inject-slf4j` to `inject-app`
and `inject-server`. This allows code in the inject framework to be mostly useful in
environments where having the bridges on the classpath causes issues. ``PHAB_ID=D179652``

### Fixed

* finatra-http: Fail startup for incorrect Controller callback functions. Controller route callback
Expand Down
15 changes: 10 additions & 5 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,12 @@ lazy val injectApp = (project in file("inject/inject-app"))
name := "inject-app",
moduleName := "inject-app",
libraryDependencies ++= Seq(
"com.novocode" % "junit-interface" % "0.11" % Test,
"com.twitter" %% "util-core" % versions.twLibVersion,
"com.novocode" % "junit-interface" % "0.11" % Test
"org.slf4j" % "jcl-over-slf4j" % versions.slf4j,
"org.slf4j" % "jul-to-slf4j" % versions.slf4j,
"org.slf4j" % "log4j-over-slf4j" % versions.slf4j,
"org.slf4j" % "slf4j-api" % versions.slf4j
),
ScoverageKeys.coverageExcludedPackages := "<empty>;.*TypeConverter.*",
publishArtifact in Test := true,
Expand Down Expand Up @@ -401,7 +405,11 @@ lazy val injectServer = (project in file("inject/inject-server"))
libraryDependencies ++= Seq(
"com.google.guava" % "guava" % versions.guava % Test,
"com.twitter" %% "finagle-stats" % versions.twLibVersion,
"com.twitter" %% "twitter-server" % versions.twLibVersion
"com.twitter" %% "twitter-server" % versions.twLibVersion,
"org.slf4j" % "jcl-over-slf4j" % versions.slf4j,
"org.slf4j" % "jul-to-slf4j" % versions.slf4j,
"org.slf4j" % "log4j-over-slf4j" % versions.slf4j,
"org.slf4j" % "slf4j-api" % versions.slf4j
),
publishArtifact in Test := true,
mappings in (Test, packageBin) := {
Expand Down Expand Up @@ -432,9 +440,6 @@ lazy val injectSlf4j = (project in file("inject/inject-slf4j"))
"com.fasterxml.jackson.core" % "jackson-annotations" % versions.jackson,
"com.twitter" %% "finagle-core" % versions.twLibVersion,
"com.twitter" %% "util-slf4j-api" % versions.twLibVersion,
"org.slf4j" % "jcl-over-slf4j" % versions.slf4j,
"org.slf4j" % "jul-to-slf4j" % versions.slf4j,
"org.slf4j" % "log4j-over-slf4j" % versions.slf4j,
"org.slf4j" % "slf4j-api" % versions.slf4j)
)

Expand Down
48 changes: 39 additions & 9 deletions doc/src/sphinx/user-guide/logging/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
Introduction to Logging With Finatra
====================================

Finatra uses the `SLF4J <http://www.slf4j.org/manual.html>`__ API for framework logging. By coupling the framework to only the
`SLF4J <http://www.slf4j.org/manual.html>`__ API, application developers are free to choose their logging implementation.
Finatra uses the `SLF4J <http://www.slf4j.org/manual.html>`__ API for framework logging. By coupling
the framework to only the `SLF4J <http://www.slf4j.org/manual.html>`__ API, application developers
are free to choose their actual logging implementation.

.. admonition:: From the SLF4J documentation

Expand All @@ -13,15 +14,37 @@ Finatra uses the `SLF4J <http://www.slf4j.org/manual.html>`__ API for framework
java.util.logging, Logback and log4j. SLF4J allows the end-user to
plug in the desired logging framework at deployment time."

`finatra/inject/inject-slf4j <https://github.com/twitter/finatra/tree/develop/inject/inject-slf4j>`__ provides transitively SLF4J bridges for the following logging providers:
`finatra/inject/inject-app <https://github.com/twitter/finatra/tree/develop/inject/inject-app>`__
and `finatra/inject/inject-server <https://github.com/twitter/finatra/tree/develop/inject/inject-server>`__
transitively provide SLF4J bridges for the following logging providers:

- `Log4j <http://en.wikipedia.org/wiki/Log4j>`__
- `commons-logging <http://commons.apache.org/proper/commons-logging/>`__
- `java.util.logging <http://docs.oracle.com/javase/7/docs/api/index.html?java/util/logging/package-summary.html>`__: there is a performance penalty for intercepting jul log messages, so `c.t.inject.server.TwitterServer` will install the `SLF4JBridgeHandler <http://www.slf4j.org/api/org/slf4j/bridge/SLF4JBridgeHandler.html>`__ which mitigates most of the performance penalty. Note, if you are not using the `c.t.inject.server.TwitterServer` or a subclass, e.g., you are building a command line application directly with `c.t.inject.app.App`, make sure the framework includes the `LoggingModule <https://github.com/twitter/finatra/blob/develop/inject/inject-modules/src/main/scala/com/twitter/inject/modules/LoggerModule.scala>`__ as a framework module.
- `java.util.logging <http://docs.oracle.com/javase/7/docs/api/index.html?java/util/logging/package-summary.html>`__:
there is a performance penalty for intercepting jul log messages, so `c.t.inject.server.TwitterServer`
will install the `SLF4JBridgeHandler <http://www.slf4j.org/api/org/slf4j/bridge/SLF4JBridgeHandler.html>`__
which mitigates most of the performance penalty.

Since `SLF4J <http://www.slf4j.org/manual.html>`__ is an interface, it requires an actual logging implementation. However, you should ensure that you **do not** end-up with *multiple* logging implementations on your classpath, e.g., you should not have multiple SLF4J bindings (`slf4j-nop`, `slf4j-log4j12`, `slf4j-jdk14`, etc.) nor a `java.util.logging` implementation, etc. on your classpath as these are all competing implementations and since classpath order is non-deterministic this will lead to unexpected logging behavior.
.. tip::

While there are several scala-wrappers for SLF4J, Finatra uses and exposes some additional features on top of the `TwitterUtil <https://twitter.github.io/util/>`__ `util-slf4j-api <https://github.com/twitter/util/tree/develop/util-slf4j-api>`__ project.
Note, if you are not using `c.t.inject.server.TwitterServer` or a subclass, e.g., you are
building a command line application directly with `c.t.inject.app.App`, you can include the
`LoggingModule <https://github.com/twitter/finatra/blob/develop/inject/inject-modules/src/main/scala/com/twitter/inject/modules/LoggerModule.scala>`__
to attempt installation the `SLF4JBridgeHandler <http://www.slf4j.org/api/org/slf4j/bridge/SLF4JBridgeHandler.html>`__.

For more information on the SLF4J bridges see the SLF4J
`Bridging legacy APIs <https://www.slf4j.org/legacy.html>`__ documentation.

Since `SLF4J <http://www.slf4j.org/manual.html>`__ is an interface, it requires an actual logging
implementation. However, you should ensure that you **do not** end-up with *multiple* logging
implementations on your classpath, e.g., you should not have multiple SLF4J bindings (`slf4j-nop`,
`slf4j-log4j12`, `slf4j-jdk14`, etc.) nor the `java.util.logging` implementation, etc. on your
classpath as these are all competing implementations and since classpath order is non-deterministic
this will lead to unexpected logging behavior.

While there are several scala-wrappers for SLF4J, Finatra uses and exposes some additional features
on top of the `TwitterUtil <https://twitter.github.io/util/>`__
`util-slf4j-api <https://github.com/twitter/util/tree/develop/util-slf4j-api>`__ project.

The main logging utility is the `c.t.inject.Logging <https://github.com/twitter/finatra/blob/develop/inject/inject-slf4j/src/main/scala/com/twitter/inject/Logging.scala>`__
trait which can be mixed into any object or class:
Expand All @@ -38,8 +61,15 @@ trait which can be mixed into any object or class:
}
This trait is a wrapper with some added utility over the `c.t.util.logging.Logging <https://github.com/twitter/util/blob/develop/util-slf4j-api/src/main/scala/com/twitter/util/logging/Logging.scala>`__.
This trait is a wrapper with some added utility over the
`c.t.util.logging.Logging <https://github.com/twitter/util/blob/develop/util-slf4j-api/src/main/scala/com/twitter/util/logging/Logging.scala>`__.

.. important::

Scala users should prefer using the logging methods of the `c.t.inject.Logging <https://github.com/twitter/finatra/blob/develop/inject/inject-slf4j/src/main/scala/com/twitter/inject/Logging.scala>`__ trait (as opposed to directly accessing the Logger instance) as these methods use "call-by-name" parameters.
Scala users should prefer using the logging methods of the
`c.t.inject.Logging <https://github.com/twitter/finatra/blob/develop/inject/inject-slf4j/src/main/scala/com/twitter/inject/Logging.scala>`__
trait as opposed to directly accessing the underlying Logger instance in the Logging as these
methods use "call-by-name" parameters.

For more information see the `scaladocs <https://twitter.github.io/finatra/scaladocs/index.html#com.twitter.inject.Logging>`__ for `c.t.inject.Logging` or the `util-slf4j-api README <https://github.com/twitter/util/blob/develop/util-slf4j-api/README.md>`__.
For more information see the `c.t.inject.Logging` `scaladocs <https://twitter.github.io/finatra/scaladocs/index.html#com.twitter.inject.Logging>`__
or the `util-slf4j-api README <https://github.com/twitter/util/blob/develop/util-slf4j-api/README.md>`__.
3 changes: 3 additions & 0 deletions inject/inject-app/src/main/scala/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ scala_library(
"3rdparty/jvm/joda-time",
"3rdparty/jvm/net/codingwell:scala-guice",
"3rdparty/jvm/org/joda:joda-convert",
"3rdparty/jvm/org/slf4j:jcl-over-slf4j",
"3rdparty/jvm/org/slf4j:jul-to-slf4j",
"3rdparty/jvm/org/slf4j:log4j-over-slf4j",
"3rdparty/jvm/org/slf4j:slf4j-api",
"finatra/inject/inject-core/src/main/scala",
"finatra/inject/inject-slf4j/src/main/scala",
Expand Down
3 changes: 3 additions & 0 deletions inject/inject-server/src/main/scala/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ scala_library(
"3rdparty/jvm/com/google/inject/extensions:guice-assistedinject",
"3rdparty/jvm/com/google/inject/extensions:guice-multibindings",
"3rdparty/jvm/net/codingwell:scala-guice",
"3rdparty/jvm/org/slf4j:jcl-over-slf4j",
"3rdparty/jvm/org/slf4j:jul-to-slf4j",
"3rdparty/jvm/org/slf4j:log4j-over-slf4j",
"3rdparty/jvm/org/slf4j:slf4j-api",
"finagle/finagle-core/src/main/scala",
"finagle/finagle-http/src/main/scala",
Expand Down
10 changes: 0 additions & 10 deletions inject/inject-slf4j/src/main/scala/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,10 @@ scala_library(
dependencies = [
"3rdparty/jvm/com/fasterxml/jackson/core:jackson-annotations",
"3rdparty/jvm/com/google/guava",
"3rdparty/jvm/org/slf4j:jcl-over-slf4j",
"3rdparty/jvm/org/slf4j:jul-to-slf4j",
"3rdparty/jvm/org/slf4j:log4j-over-slf4j",
"3rdparty/jvm/org/slf4j:slf4j-api",
"finagle/finagle-core/src/main/scala",
"util/util-core/src/main/scala",
"util/util-slf4j-api/src/main/scala",
],
excludes = [
exclude(
org = "org.clapper",
name = "grizzled-slf4j",
),
],
exports = [
"3rdparty/jvm/com/fasterxml/jackson/core:jackson-annotations",
"util/util-core/src/main/scala",
Expand Down

0 comments on commit 9e57dec

Please sign in to comment.