util-stats: store expressions indexed via a new caseclass instead of …

…string

Problem
Expressions are stored with a brittle String key. Leading to cases where expressions may clobber one another unexpectedly.

Solution
Swap the String key (which contains the name and serviceName of the expression) for a caseclass (composed of the name, serviceName, and namespaces of the expression).

JIRA Issues: CSL-10620

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

finagle-stats-core/src/main/scala/com/twitter/finagle/stats/Metrics.scala

@@ -45,7 +45,7 @@ object Metrics {
     gaugesMap = new ConcurrentHashMap[Seq[String], MetricsStore.StoreGauge](),
     /** Store MetricSchemas for each metric in order to surface metric metadata to users. */
     metricSchemas = new ConcurrentHashMap[String, MetricSchema](),
-    expressionSchemas = new ConcurrentHashMap[String, ExpressionSchema](),
+    expressionSchemas = new ConcurrentHashMap[ExpressionSchemaKey, ExpressionSchema](),
     // Memoizing metrics used for building expressions.
     // the key is the object reference hashcode shared between Expression and StatsReceiver,
     // and the value is the fully hydrated metric builder in StatsReceiver.
@@ -101,7 +101,7 @@ object Metrics {
     statsMap: ConcurrentHashMap[Seq[String], MetricsStore.StoreStat],
     gaugesMap: ConcurrentHashMap[Seq[String], MetricsStore.StoreGauge],
     metricSchemas: ConcurrentHashMap[String, MetricSchema],
-    expressionSchemas: ConcurrentHashMap[String, ExpressionSchema],
+    expressionSchemas: ConcurrentHashMap[ExpressionSchemaKey, ExpressionSchema],
     metricBuilders: ConcurrentHashMap[Int, MetricBuilder])
 }
 
@@ -257,11 +257,7 @@ private[finagle] class Metrics private (
   }
 
   private[stats] def registerExpression(exprSchema: ExpressionSchema): Unit = {
-    val expressionId = (exprSchema.labels.serviceName match {
-      case Some(serviceName) => exprSchema.name + "_" + serviceName
-      case None => exprSchema.name
-    }) + exprSchema.namespaces.mkString("_")
-    expressionSchemas.putIfAbsent(expressionId, exprSchema)
+    expressionSchemas.putIfAbsent(exprSchema.schemaKey(), exprSchema)
   }
 
   def registerGauge(schema: GaugeSchema, f: => Float): Unit =
@@ -345,16 +341,19 @@ private[finagle] class Metrics private (
   def schemas: util.Map[String, MetricSchema] =
     util.Collections.unmodifiableMap(metricSchemas)
 
-  private[this] val filledExpression: ConcurrentHashMap[String, ExpressionSchema] =
-    new ConcurrentHashMap[String, ExpressionSchema]()
+  private[this] val filledExpression: ConcurrentHashMap[
+    ExpressionSchemaKey,
+    ExpressionSchema
+  ] =
+    new ConcurrentHashMap[ExpressionSchemaKey, ExpressionSchema]()
 
-  def expressions: util.Map[String, ExpressionSchema] = {
+  def expressions: util.Map[ExpressionSchemaKey, ExpressionSchema] = {
     val added = expressionSchemas.keySet().asScala &~ filledExpression.keySet().asScala
 
-    added.foreach { name =>
-      val exprSchema = expressionSchemas.get(name)
+    added.foreach { key =>
+      val exprSchema = expressionSchemas.get(key)
       val newExpression = exprSchema.copy(expr = replaceExpression(exprSchema.expr, metricBuilders))
-      filledExpression.putIfAbsent(name, newExpression)
+      filledExpression.putIfAbsent(key, newExpression)
     }
 
     util.Collections.unmodifiableMap(filledExpression)

finagle-stats-core/src/main/scala/com/twitter/finagle/stats/MetricsStatsReceiver.scala

@@ -2,7 +2,7 @@ package com.twitter.finagle.stats
 
 import com.twitter.app.GlobalFlag
 import com.twitter.finagle.http.{HttpMuxHandler, Route, RouteIndex}
-import com.twitter.finagle.stats.exp.ExpressionSchema
+import com.twitter.finagle.stats.exp.{ExpressionSchema, ExpressionSchemaKey}
 import com.twitter.finagle.util.DefaultTimer
 import com.twitter.logging.{Level, Logger}
 import com.twitter.util.{Future, FuturePool, Time}
@@ -188,7 +188,8 @@ class MetricsExporter(val registry: Metrics, val logger: Logger)
    * Exposes Metric Expressions for ExpressionRegistry.
    * @return a map of expression names to their full ExpressionSchema.
    */
-  def expressions(): Map[String, ExpressionSchema] = registry.expressions.asScala.toMap
+  def expressions(): Map[ExpressionSchemaKey, ExpressionSchema] =
+    registry.expressions.asScala.toMap
 
   val pattern = "/admin/metrics.json"
 

finagle-stats-core/src/test/scala/com/twitter/finagle/stats/MetricsStatsReceiverTest.scala

@@ -1,6 +1,6 @@
 package com.twitter.finagle.stats
 
-import com.twitter.finagle.stats.exp.{Expression, ExpressionSchema}
+import com.twitter.finagle.stats.exp.{Expression, ExpressionSchema, ExpressionSchemaKey}
 import org.scalatest.FunSuite
 
 object MetricsStatsReceiverTest {
@@ -297,6 +297,37 @@ class MetricsStatsReceiverTest extends FunSuite {
       Expression(aaSchema).plus(
         Expression(bbSchema, Left(Expression.Min)).plus(Expression(ccSchema))))
 
-    assert(metrics.expressions.get("test_expression").expr == expected_expression.expr)
+    assert(
+      metrics.expressions
+        .get(ExpressionSchemaKey("test_expression", None, Seq())).expr == expected_expression.expr)
   }
+
+  test(
+    "expressions with different serviceNames or different namespaces are stored without clobbering each other") {
+    val metrics = Metrics.createDetached()
+    val sr = new MetricsStatsReceiver(metrics)
+    val exporter = new MetricsExporter(metrics)
+    val aSchema = CounterSchema(MetricBuilder(name = Seq("a"), statsReceiver = sr).withKernel)
+
+    val expression = ExpressionSchema("test_expression", Expression(aSchema))
+      .register()
+    assert(exporter.expressions.keySet.size == 1)
+
+    val expressionWithServiceName = ExpressionSchema("test_expression", Expression(aSchema))
+      .withServiceName("thrift")
+      .register()
+    assert(exporter.expressions.keySet.size == 2)
+
+    val expressionWithNamespace = ExpressionSchema("test_expression", Expression(aSchema))
+      .withNamespaces("a", "b")
+      .register()
+    assert(exporter.expressions.keySet.size == 3)
+
+    val expressionWithNamespaceAndServiceName =
+      ExpressionSchema("test_expression", Expression(aSchema))
+        .withNamespaces("a", "b").withServiceName("thrift")
+        .register()
+    assert(exporter.expressions.keySet.size == 4)
+  }
+
 }