diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f1e966f0..cc4d9d2a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,11 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com Unreleased ---------- +* Make lookup of Admin `LoggingHandler` more resilient when multiple implementations are detected. + Now instead of perhaps using an incorrect handler the server will instead emit a lint rule violation + and not attempt to install a logging handler ensuring that only when a single `LoggingHandler` + is located that the functionality is enabled. ``PHAB_ID=D484965`` + Runtime Behavior Changes ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/logback-classic/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler b/logback-classic/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler similarity index 100% rename from logback-classic/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler rename to logback-classic/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler diff --git a/logback-classic/src/main/scala/com/twitter/server/handler/logback/classic/LoggingHandler.scala b/logback-classic/src/main/scala/com/twitter/server/handler/logback/classic/LoggingHandler.scala index ac3fe070..8ff8b191 100644 --- a/logback-classic/src/main/scala/com/twitter/server/handler/logback/classic/LoggingHandler.scala +++ b/logback-classic/src/main/scala/com/twitter/server/handler/logback/classic/LoggingHandler.scala @@ -4,9 +4,8 @@ import ch.qos.logback.classic.{Level, Logger, LoggerContext} import com.twitter.finagle.http._ import com.twitter.io.Buf import com.twitter.server.Admin.Grouping -import com.twitter.server.handler.AdminHttpMuxHandler -import com.twitter.server.util.HttpUtils._ import com.twitter.server.util.HtmlUtils.escapeHtml +import com.twitter.server.util.HttpUtils._ import com.twitter.util.Future import com.twitter.util.logging.Logging import java.net.URLEncoder @@ -15,7 +14,10 @@ import org.slf4j.LoggerFactory import scala.annotation.tailrec import scala.collection.JavaConverters._ -private class LoggingHandler extends AdminHttpMuxHandler with Logging { +private class LoggingHandler extends com.twitter.server.handler.LoggingHandler with Logging { + + /** Implementation name */ + override val name: String = "logback-classic" implicit val loggerOrder: Ordering[Logger] = Ordering.by(_.getName) implicit val levelOrder: Ordering[Level] = Ordering.by(_.levelInt) diff --git a/server/src/main/scala/com/twitter/server/AdminHttpServer.scala b/server/src/main/scala/com/twitter/server/AdminHttpServer.scala index 449b8d5f..82c1e630 100644 --- a/server/src/main/scala/com/twitter/server/AdminHttpServer.scala +++ b/server/src/main/scala/com/twitter/server/AdminHttpServer.scala @@ -1,6 +1,6 @@ package com.twitter.server -import com.twitter.app.App +import com.twitter.app.{App, Flag} import com.twitter.finagle.client.ClientRegistry import com.twitter.finagle.filter.ServerAdmissionControl import com.twitter.finagle.http.{HttpMuxer, Method, Request, Response, Route => HttpRoute} @@ -11,7 +11,7 @@ import com.twitter.finagle.util.LoadService import com.twitter.finagle.{Http, ListeningServer, NullServer, Service} import com.twitter.server.Admin.Grouping import com.twitter.server.filters.AdminThreadPoolFilter -import com.twitter.server.handler.{AdminHttpMuxHandler, NoLoggingHandler} +import com.twitter.server.handler.{AdminHttpMuxHandler, LoggingHandler, NoLoggingHandler} import com.twitter.server.lint.LoggingRules import com.twitter.server.util.HttpUtils import com.twitter.server.view.{IndexView, NotFoundView} @@ -21,7 +21,6 @@ import com.twitter.util.{Future, Monitor, Time} import java.net.InetSocketAddress import org.slf4j.LoggerFactory import scala.language.reflectiveCalls -import com.twitter.app.Flag object AdminHttpServer { @@ -100,6 +99,22 @@ object AdminHttpServer { ): Route = { Route(path, handler, alias, group, includeInIndex, method) } + + /** Convert an AdminHttpMuxHandler to a AdminHttpServer.Route */ + private def muxHandlerToRoute(handler: AdminHttpMuxHandler): Route = { + AdminThreadPoolFilter.isolateRoute(Route.from(handler.route)) + } + + private val defaultLoggingHandlerRoute: Route = + AdminThreadPoolFilter.isolateRoute( + Route( + path = "/admin/logging", + handler = new NoLoggingHandler, + alias = "Logging", + group = Some(Grouping.Utilities), + includeInIndex = true + ) + ) } trait AdminHttpServer { self: App with Stats => @@ -132,12 +147,27 @@ trait AdminHttpServer { self: App with Stats => @volatile protected var adminHttpServer: ListeningServer = NullServer + // Look up a logging handler, will only be added if a single one is found. + private[this] val loggingHandlerRoute: Seq[Route] = { + val handlers = LoadService[LoggingHandler]() + if (handlers.length > 1) { + // add linting issue for multiple logging handlers + GlobalRules.get.add(LoggingRules.multipleLoggingHandlers(handlers.map(_.name))) + Seq(defaultLoggingHandlerRoute) + } else if (handlers.length == 1) { + // add the logging handler + handlers.map(muxHandlerToRoute) + } else { + // add linting issue for missing logging handler + GlobalRules.get.add(LoggingRules.NoLoggingHandler) + Seq(defaultLoggingHandlerRoute) + } + } + // We start with routes added via load service, note that these will be overridden // by any routes added in any call to updateMuxer(). private[this] val loadServiceRoutes: Seq[Route] = - LoadService[AdminHttpMuxHandler]() - .map(handler => Route.from(handler.route)) - .map(AdminThreadPoolFilter.isolateRoute) + LoadService[AdminHttpMuxHandler]().map(muxHandlerToRoute) ++ loggingHandlerRoute private[this] var allRoutes: Seq[Route] = loadServiceRoutes @@ -190,28 +220,7 @@ trait AdminHttpServer { self: App with Stats => */ protected def configureAdminHttpServer(server: Http.Server): Http.Server = server - /** Provide a "catch-all" LoggingHandler that displays a message about configuring a logging implementation */ - private[this] def addLoggingHandler(): Unit = { - if (!allRoutes.exists(_.path == "/admin/logging")) { - // add linting issue for un-configured logging handler - GlobalRules.get.add(LoggingRules.NoLoggingHandler) - - allRoutes = allRoutes ++ - Seq( - Route( - path = "/admin/logging", - handler = new NoLoggingHandler, - alias = "Logging", - group = Some(Grouping.Utilities), - includeInIndex = true - ) - ).map(AdminThreadPoolFilter.isolateRoute) - } - } - private[this] def updateMuxer(): Unit = { - addLoggingHandler() // ensure there is an /admin/logging handler - // create a service which multiplexes across all endpoints. val localMuxer = allRoutes.foldLeft(new HttpMuxer) { case (muxer, route) => diff --git a/server/src/main/scala/com/twitter/server/handler/LoggingHandler.scala b/server/src/main/scala/com/twitter/server/handler/LoggingHandler.scala new file mode 100644 index 00000000..9381d4f5 --- /dev/null +++ b/server/src/main/scala/com/twitter/server/handler/LoggingHandler.scala @@ -0,0 +1,8 @@ +package com.twitter.server.handler + +/** Marker trait for LoggingHandlers */ +trait LoggingHandler extends AdminHttpMuxHandler { + + /** Implementation name */ + def name: String +} diff --git a/server/src/main/scala/com/twitter/server/handler/NoLoggingHandler.scala b/server/src/main/scala/com/twitter/server/handler/NoLoggingHandler.scala index e28c2729..b6efebb5 100644 --- a/server/src/main/scala/com/twitter/server/handler/NoLoggingHandler.scala +++ b/server/src/main/scala/com/twitter/server/handler/NoLoggingHandler.scala @@ -9,10 +9,12 @@ import com.twitter.util.logging.Logger private[server] object NoLoggingHandler { val MissingLoggingImplMessageHeader: String = - "You have not configured a logging handler implementation for TwitterServer." + "You have not properly configured a logging handler implementation for TwitterServer." val MissingLoggingImplMessageBody: String = - "Please add a dependency on one of: twitter-server-logback-classic, " + - "twitter-server-slf4j-jdk14, or twitter-server-slf4j-log4j12." + "Please add a dependency on only one of: twitter-server-logback-classic, " + + "twitter-server-slf4j-jdk14, or twitter-server-slf4j-log4j12. Note: this page will appear " + + "when there is no dependency configured or when multiple conflicting dependencies have been " + + "detected. Please check the /admin/lint page for more information." } class NoLoggingHandler extends Service[Request, Response] { diff --git a/server/src/main/scala/com/twitter/server/lint/LoggingRules.scala b/server/src/main/scala/com/twitter/server/lint/LoggingRules.scala index 0ed662ef..01677136 100644 --- a/server/src/main/scala/com/twitter/server/lint/LoggingRules.scala +++ b/server/src/main/scala/com/twitter/server/lint/LoggingRules.scala @@ -48,4 +48,14 @@ object LoggingRules { ) { Seq(Issue("No logging handler implementation configured.")) } + + def multipleLoggingHandlers(names: Seq[String]): Rule = Rule( + Category.Configuration, + "Multiple Admin logging handler implementations detected", + "To properly configure the ability to dynamically change log levels, please specify only " + + "one of the supported TwitterServer logging implementations on your " + + "classpath: logback-classic, slf4j-log4j12, or slf4j-jdk14." + ) { + Seq(Issue(s"Multiple logging handler implementations found: ${names.mkString(", ")}")) + } } diff --git a/slf4j-jdk14/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler b/slf4j-jdk14/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler similarity index 100% rename from slf4j-jdk14/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler rename to slf4j-jdk14/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler diff --git a/slf4j-jdk14/src/main/scala/com/twitter/server/handler/slf4j/jdk14/LoggingHandler.scala b/slf4j-jdk14/src/main/scala/com/twitter/server/handler/slf4j/jdk14/LoggingHandler.scala index 9a5c5fc1..13ab82cb 100644 --- a/slf4j-jdk14/src/main/scala/com/twitter/server/handler/slf4j/jdk14/LoggingHandler.scala +++ b/slf4j-jdk14/src/main/scala/com/twitter/server/handler/slf4j/jdk14/LoggingHandler.scala @@ -4,7 +4,6 @@ import com.twitter.finagle.http._ import com.twitter.io.Buf import com.twitter.logging.{Level, Logger} import com.twitter.server.Admin.Grouping -import com.twitter.server.handler.AdminHttpMuxHandler import com.twitter.server.handler.slf4j.jdk14.LoggingHandler._ import com.twitter.server.util.HtmlUtils._ import com.twitter.server.util.HttpUtils._ @@ -83,7 +82,10 @@ private object LoggingHandler { * [[com.twitter.logging.Logger]] configuration state and allows for runtime changes * via HTTP query strings (?logger=&level=). */ -private class LoggingHandler extends AdminHttpMuxHandler { +private class LoggingHandler extends com.twitter.server.handler.LoggingHandler { + + /** Implementation name */ + override val name: String = "slf4j-jdk14" private[this] val levels = Logger.levels.values.toSeq.sorted(LoggingHandler.levelOrder) private[this] val logManager = java.util.logging.LogManager.getLogManager diff --git a/slf4j-log4j12/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler b/slf4j-log4j12/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler similarity index 100% rename from slf4j-log4j12/src/main/resources/META-INF/services/com.twitter.server.handler.AdminHttpMuxHandler rename to slf4j-log4j12/src/main/resources/META-INF/services/com.twitter.server.handler.LoggingHandler diff --git a/slf4j-log4j12/src/main/scala/com/twitter/server/handler/slf4j/log4j12/LoggingHandler.scala b/slf4j-log4j12/src/main/scala/com/twitter/server/handler/slf4j/log4j12/LoggingHandler.scala index 311c9689..c6c1e790 100644 --- a/slf4j-log4j12/src/main/scala/com/twitter/server/handler/slf4j/log4j12/LoggingHandler.scala +++ b/slf4j-log4j12/src/main/scala/com/twitter/server/handler/slf4j/log4j12/LoggingHandler.scala @@ -3,7 +3,6 @@ package com.twitter.server.handler.slf4j.log4j12 import com.twitter.finagle.http._ import com.twitter.io.Buf import com.twitter.server.Admin.Grouping -import com.twitter.server.handler.AdminHttpMuxHandler import com.twitter.server.util.HtmlUtils._ import com.twitter.server.util.HttpUtils._ import com.twitter.util.Future @@ -12,7 +11,10 @@ import java.net.URLEncoder import org.apache.log4j.{Level, LogManager, Logger} import scala.collection.JavaConverters._ -private class LoggingHandler extends AdminHttpMuxHandler with Logging { +private class LoggingHandler extends com.twitter.server.handler.LoggingHandler with Logging { + + /** Implementation name */ + override def name: String = "slf4j-log412" implicit val loggerOrder: Ordering[Logger] = Ordering.by(_.getName) implicit val levelOrder: Ordering[Level] = Ordering.by(_.toInt)