finagle: Remove param.Logger

Problem

We want to promote a unified way of configuring logging in Finagle and its related libraries. Configuring logging via
passing a specific logger implementation (like we do with `c.t.f.param.Logger` stack param) is considered anti-pattern
in the modern logging world. One should use a supported configuration channel (aka `logback.xml`) to override
and amend logging settings. This way logging behavior can be altered w/o the need to recompile.

Solution

Remove `c.t.f.param.Logger`. Users willing to adjust Finagle's logging settings (for a `com.twitter.finagle` logger)
should do so via means supported by their logging backend (i.e., `logback.xml` for logback).

JIRA Issues: CSL-7495

Differential Revision: https://phabricator.twitter.biz/D618667

CHANGELOG.rst

@@ -7,6 +7,12 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+Breaking API Changes
+~~~~~~~~~~~~~~~~~~~~
+
+* finagle-core: `c.t.f.param.Logger` has been removed. Use external configuration supported by
+  your logging backend to alter settings of `com.twitter.finagle` logger.  ``PHAB_ID=D618667``
+
 21.4.0
 ------
 

finagle-core/src/main/scala/com/twitter/finagle/builder/ClientBuilder.scala

@@ -1011,21 +1011,6 @@ class ClientBuilder[Req, Rep, HasCluster, HasCodec, HasHostConnectionLimit] priv
   def monitor(mFactory: String => com.twitter.util.Monitor): This =
     configured(MonitorFactory(mFactory))
 
-  /**
-   * Log very detailed debug information to the given logger.
-   *
-   * To migrate to the Stack-based APIs, use `configured`.
-   * For example:
-   * {{{
-   * import com.twitter.finagle.Http
-   * import com.twitter.finagle.param.Logger
-   *
-   * Http.client.configured(Logger(logger))
-   * }}}
-   */
-  def logger(logger: java.util.logging.Logger): This =
-    configured(Logger(logger))
-
   /**
    * Use the given parameters for failure accrual.  The first parameter
    * is the number of *successive* failures that are required to mark
@@ -1290,7 +1275,7 @@ private[finagle] object ClientBuilderClient {
   ): ServiceFactory[Req, Rep] = {
     val params = client.params
     val Daemonize(daemon) = params[Daemonize]
-    val Logger(logger) = params[Logger]
+    val logger = DefaultLogger
     val MonitorFactory(mFactory) = params[MonitorFactory]
 
     val clientParams = params + Monitor(mFactory(label))
@@ -1341,7 +1326,7 @@ private[finagle] object ClientBuilderClient {
       private[this] val released = new AtomicBoolean(false)
       override def close(deadline: Time): Future[Unit] = {
         if (!released.compareAndSet(false, true)) {
-          val Logger(logger) = client.params[Logger]
+          val logger = DefaultLogger
           logger.log(
             java.util.logging.Level.WARNING,
             "Release on Service called multiple times!",

finagle-core/src/main/scala/com/twitter/finagle/builder/ServerBuilder.scala

@@ -310,19 +310,6 @@ class ServerBuilder[Req, Rep, HasCodec, HasBindTo, HasName] private[builder] (
   def bindTo(address: SocketAddress): ServerBuilder[Req, Rep, HasCodec, Yes, HasName] =
     _configured(BindTo(address))
 
-  /**
-   * To migrate to the Stack-based APIs, use `configured`.
-   * For example:
-   * {{{
-   * import com.twitter.finagle.Http
-   * import com.twitter.finagle.param
-   *
-   * Http.server.configured(param.Logger(logger))
-   * }}}
-   */
-  def logger(logger: java.util.logging.Logger): This =
-    _configured(Logger(logger))
-
   /**
    * To migrate to the Stack-based APIs, use `ServerTransportParams.verbose`.
    * For example:
@@ -662,7 +649,7 @@ class ServerBuilder[Req, Rep, HasCodec, HasBindTo, HasName] private[builder] (
     val label = if (lbl == "") "server" else lbl
 
     val BindTo(addr) = params[BindTo]
-    val Logger(logger) = params[Logger]
+    val logger = DefaultLogger
     val Daemonize(daemon) = params[Daemonize]
     val MonitorFactory(newMonitor) = params[MonitorFactory]
     val statsReceiver = new RoleConfiguredStatsReceiver(

finagle-core/src/main/scala/com/twitter/finagle/client/ClientRegistry.scala

@@ -6,7 +6,7 @@ import com.twitter.finagle.naming.BindingFactory
 import com.twitter.finagle.param.{Label, ProtocolLibrary}
 import com.twitter.finagle.stats.FinagleStatsReceiver
 import com.twitter.finagle.util.StackRegistry.Entry
-import com.twitter.finagle.util.{Showable, StackRegistry}
+import com.twitter.finagle.util.{DefaultLogger, Showable, StackRegistry}
 import com.twitter.util.registry.GlobalRegistry
 import com.twitter.util.{Closable, Future, Time}
 import java.util.logging.Level
@@ -33,7 +33,7 @@ private[twitter] object ClientRegistry extends StackRegistry {
       case StackRegistry.Entry(_, _, params) =>
         val param.Label(name) = params[param.Label]
         val LoadBalancerFactory.Dest(va) = params[LoadBalancerFactory.Dest]
-        val param.Logger(log) = params[param.Logger]
+        val log = DefaultLogger
 
         val resolved = va.changes.filter(_ != Addr.Pending).toFuture()
         resolved.map { resolution =>

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/LoadBalancerFactory.scala

@@ -6,7 +6,7 @@ import com.twitter.finagle.loadbalancer.aperture.EagerConnections
 import com.twitter.finagle.loadbalancer.distributor.AddressedFactory
 import com.twitter.finagle.service.FailFastFactory
 import com.twitter.finagle.stats._
-import com.twitter.finagle.util.DefaultMonitor
+import com.twitter.finagle.util.{DefaultMonitor, DefaultLogger}
 import com.twitter.util.{Activity, Event, Var}
 import java.util.logging.{Level, Logger}
 import com.twitter.finagle.loadbalancer.distributor.AddrLifecycle
@@ -283,7 +283,6 @@ object LoadBalancerFactory {
       implicitly[Stack.Param[HostStats]],
       implicitly[Stack.Param[AddressOrdering]],
       implicitly[Stack.Param[param.Stats]],
-      implicitly[Stack.Param[param.Logger]],
       implicitly[Stack.Param[param.Monitor]],
       implicitly[Stack.Param[param.Reporter]]
     )
@@ -299,9 +298,7 @@ object LoadBalancerFactory {
       val EnableProbation(probationEnabled) = params[EnableProbation]
 
       val param.Stats(statsReceiver) = params[param.Stats]
-      val param.Logger(log) = params[param.Logger]
       val param.Label(label) = params[param.Label]
-      val param.Monitor(monitor) = params[param.Monitor]
 
       val rawStatsReceiver = statsReceiver match {
         case sr: RollupStatsReceiver => sr.underlying.head
@@ -321,7 +318,7 @@ object LoadBalancerFactory {
           catch {
             case NonFatal(exc) =>
               val res = set.toVector
-              log.log(
+              DefaultLogger.log(
                 Level.WARNING,
                 s"Unable to order endpoints via ($ordering): \n${res.mkString("\n")}",
                 exc

finagle-core/src/main/scala/com/twitter/finagle/param/Params.scala

@@ -154,18 +154,6 @@ object HighResTimer {
     Stack.Param(HighResTimer(Default))
 }
 
-/**
- * A class eligible for configuring a [[java.util.logging.Logger]]
- * used throughout finagle clients and servers.
- */
-case class Logger(log: java.util.logging.Logger) {
-  def mk(): (Logger, Stack.Param[Logger]) =
-    (this, Logger.param)
-}
-object Logger {
-  implicit val param: Stack.Param[Logger] = Stack.Param(Logger(util.DefaultLogger))
-}
-
 /**
  * A class eligible for configuring a
  * [[com.twitter.finagle.stats.StatsReceiver]] throughout finagle

finagle-core/src/main/scala/com/twitter/finagle/service/FailFastFactory.scala

@@ -60,12 +60,11 @@ object FailFastFactory {
    * Creates a [[com.twitter.finagle.Stackable]] [[FailFastFactory]] when enabled.
    */
   def module[Req, Rep]: Stackable[ServiceFactory[Req, Rep]] =
-    new Stack.Module6[
+    new Stack.Module5[
       FailFast,
       param.Stats,
       param.Timer,
       param.Label,
-      param.Logger,
       Transporter.EndpointAddr,
       ServiceFactory[Req, Rep]
     ] {
@@ -76,7 +75,6 @@ object FailFastFactory {
         _stats: param.Stats,
         _timer: param.Timer,
         _label: param.Label,
-        _logger: param.Logger,
         _endpoint: Transporter.EndpointAddr,
         next: ServiceFactory[Req, Rep]
       ): ServiceFactory[Req, Rep] = {
@@ -87,15 +85,14 @@ object FailFastFactory {
             val param.Stats(statsReceiver) = _stats
             val param.Timer(timer) = _timer
             val param.Label(label) = _label
-            val param.Logger(logger) = _logger
             val Transporter.EndpointAddr(endpoint) = _endpoint
 
             new FailFastFactory(
               next,
               statsReceiver.scope("failfast"),
               timer,
               label,
-              logger,
+              DefaultLogger,
               endpoint
             )
         }

finagle-core/src/test/java/com/twitter/finagle/StackParamCompilationTest.java

@@ -23,7 +23,6 @@
 import com.twitter.finagle.naming.BindingFactory;
 import com.twitter.finagle.param.ExceptionStatsHandler;
 import com.twitter.finagle.param.Label;
-import com.twitter.finagle.param.Logger;
 import com.twitter.finagle.param.Monitor;
 import com.twitter.finagle.param.Reporter;
 import com.twitter.finagle.param.Stats;
@@ -56,7 +55,6 @@ public void testParams() {
       ClientBuilder.get()
         .configured(new Label("").mk())
         .configured(new Timer(DefaultTimer.getInstance()).mk())
-        .configured(new Logger(java.util.logging.Logger.getLogger("com.twitter.finagle")).mk())
         .configured(new Stats(com.twitter.finagle.stats.DefaultStatsReceiver.get()).mk())
         .configured(new Monitor(RootMonitor.getInstance()).mk())
         .configured(new Monitor(NullMonitor.getInstance()).mk())

finagle-core/src/test/scala/com/twitter/finagle/filter/MonitorFilterTest.scala

@@ -7,10 +7,9 @@ import com.twitter.finagle.client.utils.StringClient
 import com.twitter.finagle.server.utils.StringServer
 import com.twitter.util._
 import java.net.{InetAddress, InetSocketAddress}
-import java.util.logging.{Level, Logger, StreamHandler}
 import org.mockito.Matchers._
 import org.mockito.Mockito.{times, verify, when}
-import org.mockito.{Matchers, Mockito}
+import org.mockito.Mockito
 import org.scalatest.FunSuite
 import org.scalatestplus.mockito.MockitoSugar
 
@@ -88,11 +87,6 @@ class MonitorFilterTest extends FunSuite with MockitoSugar {
     val monitor = Mockito.spy(new MockMonitor)
     val inner = new MockSourcedException("FakeService1")
     val outer = new MockSourcedException(inner, SourcedException.UnspecifiedServiceName)
-
-    val mockLogger = Mockito.spy(Logger.getLogger("MockServer"))
-    // add handler to redirect and mute output, so that it doesn't show up in the console during a test run.
-    mockLogger.setUseParentHandlers(false)
-    mockLogger.addHandler(new StreamHandler())
   }
 
   test("MonitorFilter should when attached to a server, report source for sourced exceptions") {
@@ -107,7 +101,6 @@ class MonitorFilterTest extends FunSuite with MockitoSugar {
       .name("FakeService2")
       .bindTo(address)
       .monitor((_, _) => monitor)
-      .logger(mockLogger)
       .build(service)
 
     // We cannot mock "service" directly, because we are testing an internal filter defined in the ServerBuilder
@@ -128,13 +121,6 @@ class MonitorFilterTest extends FunSuite with MockitoSugar {
 
     verify(monitor, times(0)).handle(inner)
     verify(monitor).handle(outer)
-    verify(mockLogger).log(
-      Matchers.eq(Level.SEVERE),
-      Matchers.eq(
-        "The 'FakeService2' service FakeService2 on behalf of FakeService1 threw an exception"
-      ),
-      Matchers.eq(outer)
-    )
 
     // need to properly close the client and the server, otherwise they will prevent ExitGuard from exiting and interfere with ExitGuardTest
     Await.ready(client.close(), 1.second)
@@ -152,7 +138,6 @@ class MonitorFilterTest extends FunSuite with MockitoSugar {
     val client = ClientBuilder()
       .stack(StringClient.client.withEndpoint(mockService))
       .monitor(_ => monitor)
-      .logger(mockLogger)
       .hosts(Seq(new InetSocketAddress(0)))
       .build()
 

finagle-memcached/src/main/scala/com/twitter/finagle/Memcached.scala

@@ -49,6 +49,7 @@ import com.twitter.finagle.service._
 import com.twitter.finagle.stats.{ExceptionStatsHandler, StatsReceiver}
 import com.twitter.finagle.tracing.{ClientDestTracingFilter, Tracer}
 import com.twitter.finagle.transport.{Transport, TransportContext}
+import com.twitter.finagle.util.DefaultLogger
 import com.twitter.io.Buf
 import com.twitter.util._
 import com.twitter.util.registry.GlobalRegistry
@@ -251,14 +252,13 @@ object Memcached extends finagle.Client[Command, Response] with finagle.Server[C
         Resolver.eval(Client.mkDestination("localhost", LocalMemcached.port))
       } else dest
 
-      val Logger(logger) = params[Logger]
       val label0 = if (label == "") params[Label].label else label
 
       val KeyHasher(hasher) = params[KeyHasher]
       registerClient(label0, hasher.toString)
 
       def partitionAwareFinagleClient() = {
-        logger.fine(s"Using the new partitioning finagle client for memcached: $destination")
+        DefaultLogger.fine(s"Using the new partitioning finagle client for memcached: $destination")
         val rawClient: Service[Command, Response] = {
           val stk = stack.insertAfter(
             BindingFactory.role,
@@ -270,7 +270,7 @@ object Memcached extends finagle.Client[Command, Response] with finagle.Server[C
       }
 
       def oldMemcachedClient(va: Var[Addr]) = {
-        logger.fine(s"Using the old memcached client: $destination")
+        DefaultLogger.fine(s"Using the old memcached client: $destination")
 
         val finagle.param.Stats(sr) = params[finagle.param.Stats]
         val NumReps(numReps) = params[NumReps]

finagle-memcached/src/main/scala/com/twitter/finagle/memcached/partitioning/MemcachedPartitioningService.scala

@@ -2,11 +2,11 @@ package com.twitter.finagle.memcached.partitioning
 
 import com.twitter.finagle.Stack.Params
 import com.twitter.finagle.memcached.protocol._
-import com.twitter.finagle.param.Logger
 import com.twitter.finagle.partitioning.ConsistentHashPartitioningService.NoPartitioningKeys
 import com.twitter.finagle.partitioning.PartitioningService.PartitionedResults
 import com.twitter.finagle.partitioning.param.NumReps
 import com.twitter.finagle.partitioning.{ConsistentHashPartitioningService, param}
+import com.twitter.finagle.util.DefaultLogger
 import com.twitter.finagle.{param => _, _}
 import com.twitter.hashing.KeyHasher
 import com.twitter.io.Buf
@@ -59,7 +59,7 @@ private[finagle] class MemcachedPartitioningService(
     ) {
   import MemcachedPartitioningService._
 
-  private[this] val logger = params[Logger].log
+  private[this] val logger = DefaultLogger
 
   final override protected def getKeyBytes(key: Buf): Array[Byte] = {
     Buf.ByteArray.Owned.extract(key)

finagle-netty4-http/src/main/scala/com/twitter/finagle/netty4/http/package.scala

@@ -10,7 +10,7 @@ import com.twitter.finagle.netty4.http.handler.{
   UnpoolHttpHandler,
   UriValidatorHandler
 }
-import com.twitter.finagle.param.{Logger, Stats}
+import com.twitter.finagle.param.Stats
 import com.twitter.finagle.http.param._
 import com.twitter.finagle.server.Listener
 import com.twitter.finagle.transport.TransportContext
@@ -146,7 +146,6 @@ package object http {
     val maxRequestSize = params[MaxRequestSize].size
     val decompressionEnabled = params[Decompression].enabled
     val compressionLevel = params[CompressionLevel].level
-    val log = params[Logger].log
     val stats = params[Stats].statsReceiver
 
     { pipeline: ChannelPipeline =>

finagle-netty4/src/main/scala/com/twitter/finagle/netty4/channel/AbstractNetty4ClientChannelInitializer.scala

@@ -4,8 +4,9 @@ import com.twitter.finagle.Stack
 import com.twitter.finagle.client.Transporter
 import com.twitter.finagle.netty4.proxy.{HttpProxyConnectHandler, Netty4ProxyConnectHandler}
 import com.twitter.finagle.netty4.ssl.client.Netty4ClientSslChannelInitializer
-import com.twitter.finagle.param.{Label, Logger, Stats}
+import com.twitter.finagle.param.{Label, Stats}
 import com.twitter.finagle.transport.Transport
+import com.twitter.finagle.util.DefaultLogger
 import com.twitter.util.Duration
 import io.netty.channel.{Channel, ChannelInitializer}
 import io.netty.handler.proxy.{HttpProxyHandler, Socks5ProxyHandler}
@@ -21,7 +22,6 @@ private[netty4] abstract class AbstractNetty4ClientChannelInitializer(params: St
   import Netty4ClientChannelInitializer._
 
   private[this] val Transport.Liveness(readTimeout, writeTimeout, _) = params[Transport.Liveness]
-  private[this] val Logger(logger) = params[Logger]
   private[this] val Label(label) = params[Label]
   private[this] val Stats(stats) = params[Stats]
   private[this] val Transporter.HttpProxyTo(httpHostAndCredentials) =
@@ -33,7 +33,7 @@ private[netty4] abstract class AbstractNetty4ClientChannelInitializer(params: St
 
   private[this] val channelSnooper =
     if (params[Transport.Verbose].enabled)
-      Some(ChannelSnooper.byteSnooper(label)(logger.log(Level.INFO, _, _)))
+      Some(ChannelSnooper.byteSnooper(label)(DefaultLogger.log(Level.INFO, _, _)))
     else
       None
 
@@ -47,7 +47,7 @@ private[netty4] abstract class AbstractNetty4ClientChannelInitializer(params: St
       Some(sharedChannelStatsFn(params))
     } else None
 
-  private[this] val exceptionHandler = new ChannelExceptionHandler(stats, logger)
+  private[this] val exceptionHandler = new ChannelExceptionHandler(stats, DefaultLogger)
 
   def initChannel(ch: Channel): Unit = {
 

finagle-netty4/src/main/scala/com/twitter/finagle/netty4/channel/Netty4FramedServerChannelInitializer.scala

@@ -3,6 +3,7 @@ package com.twitter.finagle.netty4.channel
 import com.twitter.finagle.param._
 import com.twitter.finagle.transport.Transport
 import com.twitter.finagle.Stack
+import com.twitter.finagle.util.DefaultLogger
 import com.twitter.util.Duration
 import io.netty.channel._
 import io.netty.handler.timeout._
@@ -23,13 +24,12 @@ private[netty4] class Netty4FramedServerChannelInitializer(params: Stack.Params)
 
   import Netty4FramedServerChannelInitializer._
 
-  private[this] val Logger(logger) = params[Logger]
   private[this] val Stats(stats) = params[Stats]
   private[this] val Transport.Liveness(readTimeout, writeTimeout, _) = params[Transport.Liveness]
   private[this] val sharedChannelRequestStats =
     if (!stats.isNull) Some(new ChannelRequestStatsHandler.SharedChannelRequestStats(stats))
     else None
-  private[this] val exceptionHandler = new ChannelExceptionHandler(stats, logger)
+  private[this] val exceptionHandler = new ChannelExceptionHandler(stats, DefaultLogger)
 
   override def initChannel(ch: Channel): Unit = {
     val pipeline = ch.pipeline