From b7bb5afcebeb5c1e98bf8b82e2af3aab006fcb46 Mon Sep 17 00:00:00 2001 From: Moses Nakamura Date: Tue, 10 Nov 2020 22:41:50 +0000 Subject: [PATCH] finagle-core: Provide a mechanism for injecting a parameter in all clients 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 | 4 +++ .../main/scala/com/twitter/finagle/Init.scala | 6 ++++ .../scala/com/twitter/finagle/Stack.scala | 19 ++++++++++ .../client/EndpointerStackClient.scala | 11 +++--- .../twitter/finagle/client/StackClient.scala | 14 ++++++++ .../com.twitter.finagle.ClientParamsInjector | 1 + .../finagle/client/StackClientTest.scala | 35 ++++++++++++++++++- .../client/TestClientParamsInjector.scala | 9 +++++ .../finagle/util/StackRegistryTest.scala | 27 -------------- .../com/twitter/finagle/util/params.scala | 31 ++++++++++++++++ 10 files changed, 125 insertions(+), 32 deletions(-) create mode 100644 finagle-core/src/test/resources/META-INF/services/com.twitter.finagle.ClientParamsInjector create mode 100644 finagle-core/src/test/scala/com/twitter/finagle/client/TestClientParamsInjector.scala create mode 100644 finagle-core/src/test/scala/com/twitter/finagle/util/params.scala diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b914e16063..df2cf4e3cd 100644 --- a/CHANGELOG.rst +++ b/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 ~~~~~~~~~~~~~~~~~~~~ diff --git a/finagle-core/src/main/scala/com/twitter/finagle/Init.scala b/finagle-core/src/main/scala/com/twitter/finagle/Init.scala index c0079f00ec..f0b0d7e996 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/Init.scala +++ b/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( diff --git a/finagle-core/src/main/scala/com/twitter/finagle/Stack.scala b/finagle-core/src/main/scala/com/twitter/finagle/Stack.scala index f6de9d77f0..a27090fe86 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/Stack.scala +++ b/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. */ diff --git a/finagle-core/src/main/scala/com/twitter/finagle/client/EndpointerStackClient.scala b/finagle-core/src/main/scala/com/twitter/finagle/client/EndpointerStackClient.scala index 2c58ddd213..92851761c7 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/client/EndpointerStackClient.scala +++ b/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) } diff --git a/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala b/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala index 42c77d505d..b93cf3908c 100644 --- a/finagle-core/src/main/scala/com/twitter/finagle/client/StackClient.scala +++ b/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 + } } /** diff --git a/finagle-core/src/test/resources/META-INF/services/com.twitter.finagle.ClientParamsInjector b/finagle-core/src/test/resources/META-INF/services/com.twitter.finagle.ClientParamsInjector new file mode 100644 index 0000000000..c85cfa9684 --- /dev/null +++ b/finagle-core/src/test/resources/META-INF/services/com.twitter.finagle.ClientParamsInjector @@ -0,0 +1 @@ +com.twitter.finagle.client.TestClientParamsInjector diff --git a/finagle-core/src/test/scala/com/twitter/finagle/client/StackClientTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/client/StackClientTest.scala index 3ad9645986..102ecd2e46 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/client/StackClientTest.scala +++ b/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) + } } diff --git a/finagle-core/src/test/scala/com/twitter/finagle/client/TestClientParamsInjector.scala b/finagle-core/src/test/scala/com/twitter/finagle/client/TestClientParamsInjector.scala new file mode 100644 index 0000000000..2e8259e7b5 --- /dev/null +++ b/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) +} diff --git a/finagle-core/src/test/scala/com/twitter/finagle/util/StackRegistryTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/util/StackRegistryTest.scala index 3e074f3c5f..59ebb6e375 100644 --- a/finagle-core/src/test/scala/com/twitter/finagle/util/StackRegistryTest.scala +++ b/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") diff --git a/finagle-core/src/test/scala/com/twitter/finagle/util/params.scala b/finagle-core/src/test/scala/com/twitter/finagle/util/params.scala new file mode 100644 index 0000000000..1d43fd8103 --- /dev/null +++ b/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)) + } +} +