util-lint: Add lint package to flag questionable practices

Motivation

There are a variety of best practices we can identify at runtime and
use to help our users improve their service.

Solution

Introduce a new util-lint module at com.twitter.util.lint which allows
us to register `Rules` which when run can return 0 or more `Issues`.

This commit adds a few rules:

 * Multiple `StatsReceivers` registered.

 * Large cumulative gauges.

 * Large number of requests to `StatsReceiver.{stat,counter,addGauge}`.

 * Duplicate names used for clients or servers.

Result

Users can visit /admin/lint with TwitterServer and see if there is
anything they can improve with their service.

RB_ID=754348

CHANGES

@@ -7,6 +7,12 @@ Note that ``RB_ID=#`` correspond to associated messages in commits.
 1.x
 -----
 
+New Features:
+-------------
+
+  * Add new admin endpoint "/admin/lint" which checks for possible issues with
+    performance or configuration. ``RB_ID=754348``
+
 Runtime Behavior Changes
 ------------------------
 

doc/src/sphinx/Features.rst

@@ -174,7 +174,9 @@ series of default endpoints:
   /admin/clients
   /admin/announcer
   /admin/shutdown
+  /admin/lint
   /admin/logging
+  /admin/registry
   /admin/resolver
   /admin/tracing
   /admin/threads
@@ -311,6 +313,13 @@ See the :ref:`metrics <metrics_label>` section for more information.
   Surface server information exposed by Finagle. Per-server configuration parameters and
   values for each module are available at /admin/clients/<client name>.
 
+**/admin/registry.json**
+  Displays how the service is currently configured across a variety of dimensions
+  including the client stack, server stack, flags, service loader values, and more.
+
+**/admin/lint**
+  Runs and displays the results for all registered linters to check for various service issues.
+
 **/admin/shutdown**
   Stop the process gracefully.
 

project/Build.scala

@@ -117,6 +117,7 @@ object TwitterServer extends Build {
       util("app"),
       util("core"),
       util("jvm"),
+      util("lint"),
       util("logging"),
       util("registry"),
       "com.github.spullara.mustache.java" % "compiler" % mustacheVersion

src/main/scala/BUILD

@@ -16,6 +16,7 @@ scala_library(name='scala',
     'util/util-app',
     'util/util-core',
     'util/util-jvm',
+    'util/util-lint',
     'util/util-logging',
     'util/util-registry',
   ],

src/main/scala/com/twitter/server/Admin.scala

@@ -35,6 +35,12 @@ trait Admin { self: App with AdminHttpServer =>
     Route(
       path = "/admin/contention", handler = new TextBlockView andThen new ContentionHandler,
       alias = "Contention", group = Some(Grouping.ProcessInfo), includeInIndex = true),
+    Route(
+      path = "/admin/lint", handler = new LintHandler(),
+      alias = "Lint", group = Some(Grouping.ProcessInfo), includeInIndex = true),
+    Route(
+      path = "/admin/lint.json", handler = new LintHandler(),
+      alias = "Lint", group = Some(Grouping.ProcessInfo), includeInIndex = false),
     Route(
       path = "/admin/threads", handler = new ThreadsHandler,
       alias = "Threads", group = Some(Grouping.ProcessInfo), includeInIndex = true),

src/main/scala/com/twitter/server/Linters.scala

@@ -0,0 +1,87 @@
+package com.twitter.server
+
+import com.twitter.app.App
+import com.twitter.finagle.client.ClientRegistry
+import com.twitter.finagle.server.ServerRegistry
+import com.twitter.finagle.stats.{StatsReceiverWithCumulativeGauges, LoadedStatsReceiver, BroadcastStatsReceiver}
+import com.twitter.finagle.util.StackRegistry
+import com.twitter.util.lint._
+
+/**
+ * Registers any global linter [[Rule rules]].
+ */
+trait Linters { app: App =>
+
+  private[this] def rules: Rules =
+    GlobalRules.get
+
+  premain {
+    registerLinters()
+  }
+
+  /** Exposed for testing */
+  private[server] def registerLinters(): Unit = {
+    numberOfStatsReceivers()
+    tooManyCumulativeGauges()
+    Seq(ClientRegistry, ServerRegistry).foreach(stackRegistryDuplicates)
+  }
+
+  private[this] def numberOfStatsReceivers(): Unit = {
+    val rule = Rule(
+      Category.Performance,
+      "Number of StatsReceivers",
+      "More than one StatsReceiver loaded causes a larger than necessary " +
+        "memory footprint and slower runtime usage. Examine your (transitive) " +
+        "dependencies and remove unwanted StatsReceivers either via dependency " +
+        "management or the com.twitter.finagle.util.loadServiceIgnoredPaths flag. " +
+        "Alternatively, having none loaded indicates that the service will not " +
+        "have any telemetry reported which is dangerous way to operate a service."
+    ) {
+      LoadedStatsReceiver.self match {
+        case bsr: BroadcastStatsReceiver =>
+          val srs = bsr.statsReceivers
+          if (srs.size == 1) {
+            Nil
+          } else if (srs.isEmpty) {
+            Seq(Issue("No StatsReceivers registered"))
+          } else {
+            Seq(Issue(s"Multiple StatsReceivers registered: ${srs.mkString(", ")}"))
+          }
+        case _ =>
+          Nil
+      }
+    }
+    rules.add(rule)
+  }
+
+  private[this] def tooManyCumulativeGauges(): Unit = {
+    val srs = LoadedStatsReceiver.self match {
+      case bsr: BroadcastStatsReceiver => bsr.statsReceivers
+      case s => Seq(s)
+    }
+    srs.foreach {
+      case srwg: StatsReceiverWithCumulativeGauges =>
+        srwg.registerLargeGaugeLinter(rules)
+      case _ =>
+    }
+  }
+
+  private[this] def stackRegistryDuplicates(stackReg: StackRegistry): Unit = {
+    val rule = Rule(
+      Category.Configuration,
+      s"Duplicate ${stackReg.registryName} StackRegistry names",
+      "Duplicate registry entries indicate that multiple clients or servers are " +
+        "being registered with the same `com.twitter.finagle.param.Label`."
+    ) {
+      val dups = stackReg.registeredDuplicates
+      if (dups.isEmpty) Nil
+      else {
+        dups.map { e =>
+          Issue(s"name=${e.name} protocolLib=${e.protocolLibrary} addr=${e.addr}")
+        }
+      }
+    }
+    rules.add(rule)
+  }
+
+}

src/main/scala/com/twitter/server/TwitterServer.scala

@@ -27,6 +27,7 @@ import com.twitter.logging.Logging
  * }}}
  */
 trait TwitterServer extends App
+  with Linters
   with Logging
   with EventSink
   with LogFormat

src/main/scala/com/twitter/server/handler/LintHandler.scala

@@ -0,0 +1,74 @@
+package com.twitter.server.handler
+
+import com.twitter.finagle.Service
+import com.twitter.finagle.http.{Response, Request}
+import com.twitter.server.util.HttpUtils._
+import com.twitter.server.util.JsonConverter
+import com.twitter.util.Future
+import com.twitter.util.lint.{Rule, GlobalRules}
+
+/**
+ * UI for running the globally registered lint [[Rule Rules]].
+ */
+class LintHandler extends Service[Request, Response] {
+
+  def apply(req: Request): Future[Response] =
+    if (expectsHtml(req)) htmlResponse(req) else jsonResponse(req)
+
+  private[this] def htmlResponse(req: Request): Future[Response] = {
+    // todo: an HTML version with a reasonable UI
+    jsonResponse(req)
+  }
+
+  private[this] def jsonString(s: String): String =
+    s.replace("\n", " ").trim
+
+  private[this] def jsonRule(rule: Rule): Map[String, Any] =
+    Map(
+      "category" -> rule.category.toString,
+      "id" -> rule.id,
+      "name" -> rule.name,
+      "description" -> jsonString(rule.description)
+    )
+
+  private[this] def jsonResponse(req: Request): Future[Response] = {
+    val rules = GlobalRules.get.iterable.toSeq
+    val jsonMap = jsonMapFromRules(rules)
+    newOk(JsonConverter.writeToString(jsonMap))
+  }
+
+  /** exposed for testing */
+  private[handler] def jsonMapFromRules(rules: Seq[Rule]): Map[String, Any] = {
+    val (oks, nots) = rules
+      .map(rule => rule -> rule())
+      .partition { case (_, res) => res.isEmpty }
+
+    val numIssues = nots.foldLeft(0) { case (total, (_, issues)) =>
+      total + issues.size
+    }
+
+    val failureIssues = nots.map { case (rule, issues) =>
+      jsonRule(rule) ++
+        Map("issues" -> issues.map(i => jsonString(i.details)))
+    }
+
+    val okRules = oks.map { case (rule, _) =>
+      rule.id
+    }
+
+    Map("lint_results" ->
+      Map(
+        "metadata" ->
+          Map(
+            "num_rules_run" -> rules.size,
+            "num_rules_ok" -> oks.size,
+            "num_rules_failed" -> nots.size,
+            "num_issues_found" -> numIssues
+          ),
+        "failed_rules" -> failureIssues,
+        "ok_rules" -> okRules
+      )
+    )
+  }
+
+}

src/test/scala/com/twitter/server/handler/LintHandlerTest.scala

@@ -0,0 +1,31 @@
+package com.twitter.server.handler
+
+import com.twitter.util.lint.{Issue, Category, Rule}
+import org.junit.runner.RunWith
+import org.scalatest.FunSuite
+import org.scalatest.junit.JUnitRunner
+
+@RunWith(classOf[JUnitRunner])
+class LintHandlerTest extends FunSuite {
+
+  private val ruleOk = Rule(Category.Performance, "ok", "desc") { Nil }
+  private def nFailed(n: Int): Rule = Rule(Category.Performance, "failures", "desc") {
+    Seq.fill(n) { Issue("fail") }
+  }
+
+  test("json metadata") {
+    val handler = new LintHandler()
+    val numFailures = 3
+    val map = handler.jsonMapFromRules(Seq(ruleOk, nFailed(numFailures)))
+
+    val metadata = map("lint_results")
+      .asInstanceOf[Map[String, Any]]("metadata")
+      .asInstanceOf[Map[String, Any]]
+
+    assert(metadata("num_rules_run") == 2)
+    assert(metadata("num_rules_ok") == 1)
+    assert(metadata("num_rules_failed") == 1)
+    assert(metadata("num_issues_found") == numFailures)
+  }
+
+}

src/test/scala/com/twitter/server/handler/ThreadsHandlerTest.scala

@@ -2,7 +2,7 @@ package com.twitter.server.handler
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.module.scala.DefaultScalaModule
-import com.twitter.finagle.http.{Request, Response}
+import com.twitter.finagle.http.Request
 import com.twitter.util.Await
 import org.junit.runner.RunWith
 import org.scalatest.FunSuite