From 78d93fde6cdf18534d6990ec7c8d4e97260e5c57 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 23 Dec 2020 00:18:28 +0000 Subject: [PATCH] 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 | 9 +++ .../com/twitter/finagle/http/TraceInfo.scala | 11 ---- .../main/scala/com/twitter/finagle/Http.scala | 2 + .../http/filter/HttpTracingFilter.scala | 60 +++++++++++++++++++ .../http/TraceInitializationTest.scala | 8 ++- 5 files changed, 77 insertions(+), 13 deletions(-) create mode 100644 finagle-http/src/main/scala/com/twitter/finagle/http/filter/HttpTracingFilter.scala 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 ) )