finagle-core: Do less work for d-daperture balancers

Problem

Deterministic aperture load balancers track offered load but never
utilize that information as their apertures are a fixed size. As such
this work is unneccessary and can be avoided.

Solution

Do less in `LoadBand` for d-aperture balancers.

Result

Deterministic aperture (d-aperture) load balancers do less work and no
longer export "loadband" scoped metrics: "widen", "narrow",
"offered_load_ema".

JIRA Issues: CSL-7985

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

CHANGELOG.rst

@@ -7,6 +7,13 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+Runtime Behavior Changes
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+* finagle-core: Deterministic aperture (d-aperture) load balancers no longer export
+  "loadband" scoped metrics: "widen", "narrow", "offered_load_ema". These were not
+  necessary as d-aperture does not change the aperture size at runtime. ``PHAB_ID=D303833``
+
 19.4.0
 ------
 

finagle-core/src/main/scala/com/twitter/finagle/loadbalancer/aperture/LoadBand.scala

@@ -2,7 +2,7 @@ package com.twitter.finagle.loadbalancer.aperture
 
 import com.twitter.finagle._
 import com.twitter.finagle.loadbalancer.{BalancerNode, NodeT}
-import com.twitter.finagle.stats.StatsReceiver
+import com.twitter.finagle.stats.{NullStatsReceiver, StatsReceiver}
 import com.twitter.finagle.util.Ema
 import com.twitter.util.{Closable, Duration, Future, Return, Throw, Time}
 
@@ -20,14 +20,17 @@ import com.twitter.util.{Closable, Duration, Future, Return, Throw, Time}
  *
  * 1. Distributed clients should be able to converge on a uniform aperture size if
  * they are offered the same amount of load. The tighter the high and low bands, the
- * less "wiggle" room distributed clients have to diverge aperture sizes. This is an
- * important property to maintain, especially when using [[DeterministicAperture]], in
- * order to have a more uniform load distribution.
+ * less "wiggle" room distributed clients have to diverge aperture sizes.
  *
  * 2. Large changes or oscillations in the aperture window size are minimized in order to
  * avoid creating undue resource (e.g. sessions) churn. The `smoothWindow` allows to
  * dampen the rate of changes by rolling the offered load into an exponentially weighted
  * moving average.
+ *
+ * @note When using deterministic aperture, the aperture is not resized on the request
+ *       path. Instead it changes with server set updates. Due to the mechanism with
+ *       which this gets mixed in, this trait works as a no-op when [[Aperture.dapertureActive]]
+ *       is true.
  */
 private[loadbalancer] trait LoadBand[Req, Rep] extends BalancerNode[Req, Rep] with Closable {
   self: Aperture[Req, Rep] =>
@@ -62,7 +65,10 @@ private[loadbalancer] trait LoadBand[Req, Rep] extends BalancerNode[Req, Rep] wi
   private[this] val monoTime = new Ema.Monotime
   private[this] val ema = new Ema(smoothWin.inNanoseconds)
 
-  private[this] val sr = statsReceiver.scope("loadband")
+  // As noted above, d-aperture balancers do not need these metrics.
+  private[this] val sr =
+    if (dapertureActive) NullStatsReceiver
+    else statsReceiver.scope("loadband")
   private[this] val widenCounter = sr.counter("widen")
   private[this] val narrowCounter = sr.counter("narrow")
 
@@ -118,19 +124,23 @@ private[loadbalancer] trait LoadBand[Req, Rep] extends BalancerNode[Req, Rep] wi
 
   protected trait LoadBandNode extends NodeT[Req, Rep] {
     abstract override def apply(conn: ClientConnection): Future[Service[Req, Rep]] = {
-      adjustTotalLoad(1)
-      super.apply(conn).transform {
-        case Return(svc) =>
-          Future.value(new ServiceProxy(svc) {
-            override def close(deadline: Time): Future[Unit] =
-              super.close(deadline).ensure {
-                adjustTotalLoad(-1)
-              }
-          })
-
-        case t @ Throw(_) =>
-          adjustTotalLoad(-1)
-          Future.const(t)
+      if (dapertureActive) {
+        super.apply(conn)
+      } else {
+        adjustTotalLoad(1)
+        super.apply(conn).transform {
+          case Return(svc) =>
+            Future.value(new ServiceProxy(svc) {
+              override def close(deadline: Time): Future[Unit] =
+                super.close(deadline).ensure {
+                  adjustTotalLoad(-1)
+                }
+            })
+
+          case t @ Throw(_) =>
+            adjustTotalLoad(-1)
+            Future.const(t)
+        }
       }
     }
   }

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/ApertureTest.scala

@@ -78,6 +78,28 @@ class ApertureTest extends FunSuite with ApertureSuite {
     }
   }
 
+  test("dapertureActive does not create LoadBand metrics") {
+    val stats = new InMemoryStatsReceiver
+    val aperture = new ApertureLeastLoaded[Unit, Unit](
+      endpoints = Activity.pending,
+      smoothWin = Duration.Bottom,
+      lowLoad = 0,
+      highLoad = 0,
+      minAperture = 10,
+      maxEffort = 0,
+      rng = Rng.threadLocal,
+      statsReceiver = stats,
+      label = "",
+      timer = new NullTimer,
+      emptyException = new NoBrokersAvailableException,
+      useDeterministicOrdering = Some(true)
+    )
+
+    assert(!stats.gauges.contains(Seq("loadband", "offered_load_ema")))
+    assert(!stats.counters.contains(Seq("loadband", "widen")))
+    assert(!stats.counters.contains(Seq("loadband", "narrow")))
+  }
+
   test("closing ApertureLeastLoaded removes the loadband ema gauge") {
     val stats = new InMemoryStatsReceiver
     val aperture = new ApertureLeastLoaded[Unit, Unit](
@@ -92,7 +114,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       label = "",
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
-      useDeterministicOrdering = None
+      useDeterministicOrdering = Some(false)
     )
 
     assert(stats.gauges.contains(Seq("loadband", "offered_load_ema")))
@@ -116,7 +138,7 @@ class ApertureTest extends FunSuite with ApertureSuite {
       label = "",
       timer = new NullTimer,
       emptyException = new NoBrokersAvailableException,
-      useDeterministicOrdering = None
+      useDeterministicOrdering = Some(false)
     )
 
     assert(stats.gauges.contains(Seq("loadband", "offered_load_ema")))

finagle-core/src/test/scala/com/twitter/finagle/loadbalancer/aperture/LoadBandTest.scala

@@ -10,7 +10,10 @@ import org.scalatest.FunSuite
 class LoadBandTest extends FunSuite with ApertureSuite {
   private val rng = Rng()
 
-  private class Bal(protected val lowLoad: Double, protected val highLoad: Double)
+  private class Bal(
+    protected val lowLoad: Double,
+    protected val highLoad: Double,
+    override protected val useDeterministicOrdering: Option[Boolean] = Some(false))
       extends TestBal
       with LeastLoaded[Unit, Unit]
       with LoadBand[Unit, Unit] {