finagle-mysql: Less aggressive annotations

Problem

We emit a lot of noise in our tracing annotations.

Solution

Emit less noise.

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

CHANGELOG.rst

@@ -13,6 +13,11 @@ Bug Fixes
 * finagle-thriftmux: Fixed a bug where connections were not established eagerly in ThriftMux
   MethodBuilder even when eager connections was enabled. ``PHAB_ID=D589592``
 
+Runtime Behavior Changes
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+ * finagle-mysql: Don't use the full query when adding tracing annotations. ``PHAB_ID=D593944``
+
 20.12.0
 -------
 

finagle-mysql/src/main/scala/com/twitter/finagle/mysql/MySqlTracingFilter.scala

@@ -26,14 +26,31 @@ private[finagle] object MysqlTracingFilter {
       new MysqlTracingFilter(_credentials.username, _database.db).andThen(next)
     }
   }
+
+  // For security purposes we only emit the first token which will always be a SQL
+  // command (like SELECT) or a fragment (DROP [DATABASE|TABLE]). We don't want to
+  // implement the full parser here so we won't deal with the two token commands.
+  private def firstToken(str: String): String =
+    str.substring(0, math.max(0, str.indexOf(' ')))
 }
 
 private class MysqlTracingFilter(username: Option[String], database: Option[String])
     extends SimpleFilter[Request, Result] {
   import MysqlTracingFilter._
 
   // we cache based on the class name of the request to avoid string concat
-  val unknownReqAnnoCache = mutable.Map.empty[String, String]
+  private[this] val unknownReqAnnoCache = mutable.Map.empty[String, String]
+
+  private[this] def getClassName(request: Request): String = {
+    val className = request.getClass.getSimpleName
+    // We need to synchronize or else we risk map corruption and that can result
+    // in infinite loops.
+    unknownReqAnnoCache.synchronized {
+      unknownReqAnnoCache.getOrElseUpdate(
+        className,
+        UnknownReqAnnotationPrefix + className.replace("$", ""))
+    }
+  }
 
   def apply(request: Request, service: Service[Request, Result]): Future[Result] = {
     val trace = Trace()
@@ -49,20 +66,13 @@ private class MysqlTracingFilter(username: Option[String], database: Option[Stri
 
       request match {
         case QueryRequest(sqlStatement) =>
-          trace.recordBinary(QueryAnnotationKey, sqlStatement)
+          trace.recordBinary(QueryAnnotationKey, firstToken(sqlStatement))
         case PrepareRequest(sqlStatement) =>
-          trace.recordBinary(PrepareAnnotationKey, sqlStatement)
+          trace.recordBinary(PrepareAnnotationKey, firstToken(sqlStatement))
         case ExecuteRequest(id, _, _, _) =>
           trace.recordBinary(ExecuteAnnotationKey, id)
         case _ =>
-          val className = request.getClass.getSimpleName
-
-          // not thread safe but also not worth the synchronization
-          val anno = unknownReqAnnoCache.getOrElseUpdate(
-            className,
-            UnknownReqAnnotationPrefix + className.replace("$", ""))
-
-          trace.record(anno)
+          trace.record(getClassName(request))
       }
     }
 

finagle-mysql/src/test/scala/com/twitter/finagle/mysql/integration/MysqlBuilderTest.scala

@@ -42,8 +42,8 @@ class MysqlBuilderTest extends FunSuite with IntegrationClient {
       ready(client.ping())
 
       val mysqlTraces = annotations.collect {
-        case Annotation.BinaryAnnotation("clnt/mysql.query", "SELECT 1") => ()
-        case Annotation.BinaryAnnotation("clnt/mysql.prepare", "SELECT ?") => ()
+        case Annotation.BinaryAnnotation("clnt/mysql.query", "SELECT") => ()
+        case Annotation.BinaryAnnotation("clnt/mysql.prepare", "SELECT") => ()
         case Annotation.Message("clnt/mysql.PingRequest") => ()
       }