finagle-http: record binary annotations when spans complete

Problem:
It would be useful to record more high-level information when spans complete

Solution:
Add the following annotations:

```
http.status_code = int (HTTP code.... 400/500/200 etc...)
http.method String (HTTP method...ie POST, GET)
```

JIRA Issues: CSL-10325

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

CHANGELOG.rst

@@ -7,6 +7,14 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
 Unreleased
 ----------
 
+New Features
+~~~~~~~~~~~~
+
+* finagle-http: Record http-specific annotations including `http.status_code` and
+  `http.method`. See details at
+  https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/trace
+  ``PHAB_ID=D580894``
+
 Bug Fixes
 ~~~~~~~~~
 
@@ -23,6 +31,7 @@ Runtime Behavior Changes
 
 New Features
 ~~~~~~~~~~~~
+
 * finagle-benchmark: Add a benchmark for LocalContext. ``PHAB_ID=D588632``
 
 * finagle-core: Add a new filter, `ClientExceptionTracingFilter`, that records error annotations for

finagle-base-http/src/main/scala/com/twitter/finagle/http/TraceInfo.scala

@@ -101,11 +101,9 @@ private object TraceInfo {
     id match {
       case Some(tid) =>
         Trace.letId(tid) {
-          traceRpc(request)
           f
         }
       case None =>
-        traceRpc(request)
         f
     }
   }
@@ -136,15 +134,6 @@ private object TraceInfo {
     if (traceId.flags.toLong != 0L) {
       request.headerMap.addUnsafe(Header.Flags, JLong.toString(traceId.flags.toLong))
     }
-    traceRpc(request)
-  }
-
-  def traceRpc(request: Request): Unit = {
-    val trace = Trace()
-    if (trace.isActivelyTracing) {
-      trace.recordRpc(request.method.toString)
-      trace.recordBinary("http.uri", stripParameters(request.uri))
-    }
   }
 
   /**

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

@@ -152,6 +152,7 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re
           TraceInitializerFilter.role,
           PayloadSizeFilter.module(PayloadSizeFilter.clientTraceKeyPrefix)
         )
+        .replace(StackClient.Role.protoTracing, HttpTracingFilter.module)
         // The pooling strategy depends on whether we're using H2, and if so, what variant.
         // We use a custom module to isolate the selection logic.
         .replace(DefaultPool.Role, HttpPool)
@@ -373,6 +374,7 @@ object Http extends Client[Request, Response] with HttpRichClient with Server[Re
           TraceInitializerFilter.role,
           PayloadSizeFilter.module(PayloadSizeFilter.serverTraceKeyPrefix)
         )
+        .replace(StackServer.Role.protoTracing, HttpTracingFilter.module)
         .replace(TraceInitializerFilter.role, new HttpServerTraceInitializer[Request, Response])
         .replace(StackServer.Role.preparer, HttpNackFilter.module)
         .prepend(ServerDtabContextFilter.module)

finagle-http/src/main/scala/com/twitter/finagle/http/filter/HttpTracingFilter.scala

@@ -0,0 +1,60 @@
+package com.twitter.finagle.http.filter
+
+import com.twitter.finagle._
+import com.twitter.finagle.tracing.{Trace, Tracing}
+import com.twitter.util.Future
+
+private[finagle] object HttpTracingFilter extends SimpleFilter[http.Request, http.Response] {
+  val Role: Stack.Role = Stack.Role("HttpTracing")
+  def apply(
+    request: http.Request,
+    service: Service[http.Request, http.Response]
+  ): Future[http.Response] = {
+    val tracing = Trace()
+    val rep = service(request)
+    traceHttpRequest(request, tracing)
+    traceHttpResponse(rep, tracing)
+    rep
+  }
+
+  def traceHttpRequest(request: http.Request, tracing: Tracing): Unit = {
+    if (tracing.isActivelyTracing) {
+      tracing.recordRpc(request.method.toString)
+      tracing.recordBinary("http.method", request.method.toString)
+      tracing.recordBinary("http.uri", stripParameters(request.uri))
+    }
+  }
+
+  def traceHttpResponse(rep: Future[http.Response], tracing: Tracing): Unit = {
+    if (tracing.isActivelyTracing) {
+      rep.onSuccess { r =>
+        tracing.recordBinary("http.status_code", r.statusCode)
+      }
+    }
+  }
+
+  /**
+   * Remove any parameters from url.
+   */
+  private[this] def stripParameters(uri: String): String = {
+    uri.indexOf('?') match {
+      case -1 => uri
+      case n => uri.substring(0, n)
+    }
+  }
+
+  /**
+   * Creates a [[com.twitter.finagle.Stackable]] `HttpTracingFilter` which will add binary
+   * annotations `http.uri`, `http.status_code`, and `http.method` to completed http spans.
+   */
+  def module: Stackable[ServiceFactory[http.Request, http.Response]] =
+    new Stack.Module0[ServiceFactory[http.Request, http.Response]] {
+      val role = HttpTracingFilter.Role
+      val description = "Record http annotation for completed spans"
+      def make(
+        next: ServiceFactory[http.Request, http.Response]
+      ): ServiceFactory[http.Request, http.Response] = {
+        HttpTracingFilter.andThen(next)
+      }
+    }
+}

finagle-http/src/test/scala/com/twitter/finagle/http/TraceInitializationTest.scala

@@ -34,15 +34,19 @@ class TraceInitializationTest extends FunSuite {
     assertAnnotationsInOrder(
       tracer.toSeq,
       Seq(
-        Annotation.Rpc("GET"),
-        Annotation.BinaryAnnotation("http.uri", "/this/is/a/uri/path"),
         Annotation.ServiceName("theClient"),
         Annotation.ClientSend,
         Annotation.Rpc("GET"),
+        Annotation.BinaryAnnotation("http.method", "GET"),
         Annotation.BinaryAnnotation("http.uri", "/this/is/a/uri/path"),
         Annotation.ServiceName("theServer"),
         Annotation.ServerRecv,
+        Annotation.Rpc("GET"),
+        Annotation.BinaryAnnotation("http.method", "GET"),
+        Annotation.BinaryAnnotation("http.uri", "/this/is/a/uri/path"),
         Annotation.ServerSend,
+        Annotation.BinaryAnnotation("http.status_code", 200),
+        Annotation.BinaryAnnotation("http.status_code", 200),
         Annotation.ClientRecv
       )
     )