finagle-core: Provide a mechanism for injecting a parameter in all cl…

…ients

Problem

Although it's possible to inject a parameter across all clients/servers with a
StackTransformer, it's quite error-prone, and it means that we need to consider
*when* to inject it.

Solution

Add a special mechanism just for injecting parameters.

JIRA Issues: CSL-9959

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

CHANGELOG.rst

@@ -27,6 +27,10 @@ New Features
 * finagle-core: Provide `com.twitter.finagle.naming.DisplayBoundName` for configuring how to
   display the bound `Name` for a given client in metrics metadata. ``PHAB_ID=D573905``
 
+* finagle-core: Provide `ClientParamsInjector`, a class that will be service-loaded at run-time
+  by Finagle clients, and will allow generic configuration of all sets of parameters.
+  ``PHAB_ID=D574861``
+
 Breaking API Changes
 ~~~~~~~~~~~~~~~~~~~~
 

finagle-core/src/main/scala/com/twitter/finagle/Init.scala

@@ -5,6 +5,7 @@ import com.twitter.finagle.exp.FinagleScheduler
 import com.twitter.finagle.loadbalancer.aperture
 import com.twitter.finagle.loadbalancer.aperture.ProcessCoordinate.FromInstanceId
 import com.twitter.finagle.stats.{DefaultStatsReceiver, FinagleStatsReceiver}
+import com.twitter.finagle.client.StackClient
 import com.twitter.finagle.server.StackServer
 import com.twitter.finagle.util.{DefaultLogger, LoadService}
 import com.twitter.jvm.JvmStats
@@ -110,6 +111,11 @@ private[twitter] object Init {
     _finagleBuildRevision.set(p.getProperty("build_revision", unknownVersion))
 
     LoadService[StackTransformer]().foreach { nt => StackServer.DefaultTransformer.append(nt) }
+    // we split out params injection from stack transformation because params are typically
+    // injected at the top of the stack, and it can create confusing deps to inject them via
+    // StackTransformers.  at the same time, we occasionally want to inject parameters
+    // before we ever call Stack#make.  this gives us an opportunity to do it.
+    LoadService[ClientParamsInjector]().foreach { nt => StackClient.DefaultInjectors.append(nt) }
 
     log.info(
       "Finagle version %s (rev=%s) built at %s".format(

finagle-core/src/main/scala/com/twitter/finagle/Stack.scala

@@ -478,6 +478,13 @@ object Stack {
     def apply[Req, Rep](stack: Stack[ServiceFactory[Req, Rep]]): Stack[ServiceFactory[Req, Rep]]
   }
 
+  /**
+   * Encodes parameter injection for [[Stack.Params]]
+   */
+  trait ParamsInjector {
+    def apply(params: Stack.Params): Stack.Params
+  }
+
   trait Transformable[+T] {
 
     /**
@@ -676,12 +683,24 @@ object Stack {
  * discouraged. Modifying params via transformers creates subtle dependencies
  * between modules and makes it difficult to reason about the value of
  * params, as it may change depending on the module's placement in the stack.
+ * Whenever possible, [[ClientParamsInjector]] should be used instead.
  */
 abstract class StackTransformer extends Stack.Transformer {
   def name: String
   override def toString: String = s"StackTransformer(name=$name)"
 }
 
+/**
+ * ClientsParamsInjector is the standard mechanism for injecting params into
+ * the client.  It is a ``Stack.ParamsInjector`` with a name.  The injection
+ * will run at materialization time for Finagle clients, so that the parameters
+ * for a Stack will be injected in a consistent way.
+ */
+abstract class ClientParamsInjector extends Stack.ParamsInjector {
+  def name: String
+  override def toString: String = s"ClientParamsInjector(name=$name)"
+}
+
 /**
  * `Stack.Params` forwarder to provide a clean Java API.
  */

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

@@ -109,6 +109,8 @@ trait EndpointerStackClient[Req, Rep, This <: EndpointerStackClient[Req, Rep, Th
     params: Stack.Params = this.params
   ): This
 
+  protected def injectors: Seq[ClientParamsInjector] = StackClient.DefaultInjectors.injectors
+
   /**
    * @inheritdoc
    *
@@ -143,10 +145,11 @@ trait EndpointerStackClient[Req, Rep, This <: EndpointerStackClient[Req, Rep, Th
       new RelativeNameMarkingStatsReceiver(stats.scope(clientLabel)),
       Client)
 
-    val clientParams = params +
-      Label(clientLabel) +
-      Stats(clientSr) +
-      BindingFactory.Dest(dest)
+    val clientParams = injectors.foldLeft(
+      params +
+        Label(clientLabel) +
+        Stats(clientSr) +
+        BindingFactory.Dest(dest)) { case (prms, injector) => injector(prms) }
 
     clientStack.make(clientParams)
   }

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

@@ -13,6 +13,7 @@ import com.twitter.finagle.stack.nilStack
 import com.twitter.finagle.stats.{ClientStatsReceiver, LoadedHostStatsReceiver}
 import com.twitter.finagle.tracing._
 import com.twitter.util.registry.GlobalRegistry
+import scala.collection.immutable.Queue
 
 object StackClient {
 
@@ -434,6 +435,19 @@ object StackClient {
     Stack.Params.empty +
       Stats(ClientStatsReceiver) +
       LoadBalancerFactory.HostStats(LoadedHostStatsReceiver)
+
+  /**
+   * A set of ClientParamsInjectors for transforming client params.
+   */
+  private[finagle] object DefaultInjectors {
+    @volatile private var underlying = Queue.empty[ClientParamsInjector]
+
+    def append(injector: ClientParamsInjector): Unit =
+      synchronized { underlying = underlying :+ injector }
+
+    def injectors: Seq[ClientParamsInjector] =
+      underlying
+  }
 }
 
 /**

finagle-core/src/test/resources/META-INF/services/com.twitter.finagle.ClientParamsInjector

@@ -0,0 +1 @@
+com.twitter.finagle.client.TestClientParamsInjector

finagle-core/src/test/scala/com/twitter/finagle/client/StackClientTest.scala

@@ -16,7 +16,7 @@ import com.twitter.finagle.service.FailFastFactory.FailFast
 import com.twitter.finagle.service.PendingRequestFilter
 import com.twitter.finagle.stats.{InMemoryStatsReceiver, NullStatsReceiver}
 import com.twitter.finagle.transport.{Transport, TransportContext}
-import com.twitter.finagle.util.StackRegistry
+import com.twitter.finagle.util.{StackRegistry, TestParam}
 import com.twitter.finagle.{Name, param}
 import com.twitter.util._
 import com.twitter.util.registry.{Entry, GlobalRegistry, SimpleRegistry}
@@ -753,4 +753,37 @@ abstract class AbstractStackClientTest
 
     assert(failure.toString == "Failure(boom!, flags=0x00) with Service -> stringClient")
   }
+
+  test("Injects the appropriate params") {
+    val listeningServer = StringServer.server
+      .serve(":*", Service.mk[String, String](Future.value))
+    val boundAddress = listeningServer.boundAddress.asInstanceOf[InetSocketAddress]
+    val label = "stringClient"
+
+    var testParamValue = 0
+
+    val verifyModule = new Stack.Module1[TestParam, ServiceFactory[String, String]] {
+      val role = Stack.Role("verify")
+      val description = "Verifies the value of the test param"
+
+      def make(testParam: TestParam, next: ServiceFactory[String, String]): ServiceFactory[String, String] = {
+        testParamValue = testParam.p1
+        new SimpleFilter[String, String] {
+          def apply(request: String, service: Service[String, String]): Future[String] = {
+            Future.value("world")
+          }
+        }.andThen(next)
+      }
+    }
+
+    // push the verification module onto the stack.  doesn't really matter where in the stack it
+    // goes
+    val svc = baseClient
+      .withStack(verifyModule +: _)
+      .newService(Name.bound(Address(boundAddress)), label)
+
+    await(svc("hello"))
+
+    assert(testParamValue == 37)
+  }
 }

finagle-core/src/test/scala/com/twitter/finagle/client/TestClientParamsInjector.scala

@@ -0,0 +1,9 @@
+package com.twitter.finagle.client
+
+import com.twitter.finagle.{ClientParamsInjector, Stack}
+import com.twitter.finagle.util.TestParam
+
+class TestClientParamsInjector extends ClientParamsInjector {
+  def name: String = "test-injector"
+  def apply(params: Stack.Params): Stack.Params = params + TestParam(37)
+}

finagle-core/src/test/scala/com/twitter/finagle/util/StackRegistryTest.scala

@@ -5,33 +5,6 @@ import com.twitter.util.Var
 import com.twitter.util.registry.{Entry, GlobalRegistry, SimpleRegistry}
 import org.scalatest.FunSuite
 
-case class TestParam(p1: Int) {
-  def mk() = (this, TestParam.param)
-}
-object TestParam {
-  implicit val param = Stack.Param(TestParam(1))
-}
-
-case class TestParam2(p2: Int) {
-  def mk() = (this, TestParam2.param)
-}
-object TestParam2 {
-  implicit val param = Stack.Param(TestParam2(1))
-}
-
-class NotCaseClassParam(val ncc: Var[Int]) {
-  def mk() = (this, NotCaseClassParam.param)
-
-}
-object NotCaseClassParam {
-
-  implicit val param = new Stack.Param[NotCaseClassParam] {
-    val default = new NotCaseClassParam(Var(3))
-    override def show(p: NotCaseClassParam): Seq[(String, () => String)] =
-      Seq(("ncc", () => p.ncc.sample().toString))
-  }
-}
-
 class StackRegistryTest extends FunSuite {
 
   val headRole = Stack.Role("head")

finagle-core/src/test/scala/com/twitter/finagle/util/params.scala

@@ -0,0 +1,31 @@
+package com.twitter.finagle.util
+
+import com.twitter.finagle.Stack
+import com.twitter.util.Var
+
+case class TestParam(p1: Int) {
+  def mk() = (this, TestParam.param)
+}
+object TestParam {
+  implicit val param = Stack.Param(TestParam(1))
+}
+
+case class TestParam2(p2: Int) {
+  def mk() = (this, TestParam2.param)
+}
+object TestParam2 {
+  implicit val param = Stack.Param(TestParam2(1))
+}
+
+class NotCaseClassParam(val ncc: Var[Int]) {
+  def mk() = (this, NotCaseClassParam.param)
+
+}
+object NotCaseClassParam {
+  implicit val param = new Stack.Param[NotCaseClassParam] {
+    val default = new NotCaseClassParam(Var(3))
+    override def show(p: NotCaseClassParam): Seq[(String, () => String)] =
+      Seq(("ncc", () => p.ncc.sample().toString))
+  }
+}
+