diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 0fb86af2e8..a4659ceb35 100644 --- a/CHANGELOG.rst +++ b/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 diff --git a/finagle-base-http/src/main/scala/com/twitter/finagle/http/TraceInfo.scala b/finagle-base-http/src/main/scala/com/twitter/finagle/http/TraceInfo.scala index 78481e017e..ba3d5de02e 100644 --- a/finagle-base-http/src/main/scala/com/twitter/finagle/http/TraceInfo.scala +++ b/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)) - } } /** diff --git a/finagle-http/src/main/scala/com/twitter/finagle/Http.scala b/finagle-http/src/main/scala/com/twitter/finagle/Http.scala index 6f707e9a74..cb9638c3a8 100644 --- a/finagle-http/src/main/scala/com/twitter/finagle/Http.scala +++ b/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) diff --git a/finagle-http/src/main/scala/com/twitter/finagle/http/filter/HttpTracingFilter.scala b/finagle-http/src/main/scala/com/twitter/finagle/http/filter/HttpTracingFilter.scala new file mode 100644 index 0000000000..2608ad25aa --- /dev/null +++ b/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) + } + } +} diff --git a/finagle-http/src/test/scala/com/twitter/finagle/http/TraceInitializationTest.scala b/finagle-http/src/test/scala/com/twitter/finagle/http/TraceInitializationTest.scala index 66e93b8662..be187e1bc8 100644 --- a/finagle-http/src/test/scala/com/twitter/finagle/http/TraceInitializationTest.scala +++ b/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 ) )