use route pattern string as server side operation name

lucasamoroso · lucasamoroso · commit 97d24daac3bf · 2022-10-10T20:49:29.000-03:00
diff --git a/instrumentation/kamon-armeria/src/main/java/kamon/instrumentation/armeria/server/ArmeriaHttpServerDecorator.java b/instrumentation/kamon-armeria/src/main/java/kamon/instrumentation/armeria/server/ArmeriaHttpServerDecorator.java
@@ -31,7 +31,7 @@
 
 public class ArmeriaHttpServerDecorator extends SimpleDecoratingHttpService {
   public static final AttributeKey<HttpServerInstrumentation.RequestHandler> REQUEST_HANDLER_TRACE_KEY =
-      AttributeKey.valueOf(HttpServerInstrumentation.RequestHandler.class, "REQUEST_HANDLER_TRACE");
+          AttributeKey.valueOf(HttpServerInstrumentation.RequestHandler.class, "REQUEST_HANDLER_TRACE");
 
   private final Map<Integer, HttpServerInstrumentation> serverInstrumentationMap;
 
@@ -47,15 +47,23 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc
     if (httpServerInstrumentation != null) {
 
       final HttpServerInstrumentation.RequestHandler requestHandler =
-          httpServerInstrumentation.createHandler(KamonArmeriaMessageConverter.toRequest(req));
+              httpServerInstrumentation.createHandler(KamonArmeriaMessageConverter.toRequest(req));
 
       ctx.log()
-          .whenComplete()
-          .thenAccept(log -> {
-              final Context context = ctx.attr(REQUEST_HANDLER_TRACE_KEY).context();
-              requestHandler.buildResponse(KamonArmeriaMessageConverter.toResponse(log),context);
-              requestHandler.responseSent();
-          });
+              .whenComplete()
+              .thenAccept(log -> {
+                final Context context = ctx.attr(REQUEST_HANDLER_TRACE_KEY).context();
+                /**
+                 * This is true only when no configured Route was matched.
+                 * Cases where the route is fallback should be managed here {@link kamon.instrumentation.armeria.server.HandleNotFoundMethodAdvisor#around}
+                 */
+                if (!ctx.config().route().isFallback()) {
+                  requestHandler.span().name(ctx.config().route().patternString());
+                }
+
+                requestHandler.buildResponse(KamonArmeriaMessageConverter.toResponse(log), context);
+                requestHandler.responseSent();
+              });
 
       try (Storage.Scope ignored = Kamon.storeContext(requestHandler.context())) {
         ctx.setAttr(REQUEST_HANDLER_TRACE_KEY, requestHandler);
diff --git a/instrumentation/kamon-armeria/src/main/resources/reference.conf b/instrumentation/kamon-armeria/src/main/resources/reference.conf
@@ -129,32 +129,6 @@ kamon.instrumentation.armeria {
         # a given request. Depending on the instrumented framework, it might be possible to apply this operation
         # name automatically or not, check the frameworks' instrumentation docs for more details.
         unhandled = "unhandled"
-
-        # FQCN for a HttpOperationNameGenerator implementation, or ony of the following shorthand forms:
-        #   - default: Uses the set default operation name
-        #   - method: Uses the request HTTP method as the operation name.
-        #
-        name-generator = "kamon.instrumentation.armeria.KamonArmeriaHttpOperationNameGenerator"
-
-        # Provides custom mappings from HTTP paths into operation names. Meant to be used in cases where the bytecode
-        # instrumentation is not able to provide a sensible operation name that is free of high cardinality values.
-        # For example, with the following configuration:
-        #   mappings {
-        #     "/organization/*/user/*/profile" = "/organization/:orgID/user/:userID/profile"
-        #     "/events/*/rsvps" = "EventRSVPs"
-        #   }
-        #
-        # Requests to "/organization/3651/user/39652/profile" and "/organization/22234/user/54543/profile" will have
-        # the same operation name "/organization/:orgID/user/:userID/profile".
-        #
-        # Similarly, requests to "/events/aaa-bb-ccc/rsvps" and "/events/1234/rsvps" will have the same operation
-        # name "EventRSVPs".
-        #
-        # The patterns are expressed as globs and the operation names are free form.
-        #
-        mappings {
-
-        }
       }
     }
   }
diff --git a/instrumentation/kamon-armeria/src/main/scala/kamon/instrumentation/armeria/server/ArmeriaHttpServerInstrumentation.scala b/instrumentation/kamon-armeria/src/main/scala/kamon/instrumentation/armeria/server/ArmeriaHttpServerInstrumentation.scala
@@ -16,7 +16,6 @@
 package kamon.instrumentation.armeria.server
 
 import java.util
-
 import com.linecorp.armeria.common.HttpStatus
 import com.linecorp.armeria.server._
 import com.typesafe.config.Config
@@ -78,13 +77,27 @@ object HandleNotFoundMethodAdvisor {
     */
   @Advice.OnMethodExit(onThrowable = classOf[Throwable])
   def around(@Advice.Argument(0) ctx: ServiceRequestContext,
+             @Advice.Argument(1) routingCtx: RoutingContext,
              @Advice.Argument(2) statusException: HttpStatusException,
              @Advice.Thrown throwable: Throwable): Unit = {
     lazy val unhandledOperationName: String = Kamon.config().getConfig("kamon.instrumentation.armeria.server").getString("tracing.operations.unhandled")
 
+    val requestHandler = ctx.attr(REQUEST_HANDLER_TRACE_KEY)
+
     if (throwable != null && statusException.httpStatus.code() == HttpStatus.NOT_FOUND.code()) {
-      val requestHandler = ctx.attr(REQUEST_HANDLER_TRACE_KEY)
-        requestHandler.span.name(unhandledOperationName)
+      requestHandler.span.name(unhandledOperationName)
+    } else {
+      /**
+        * If no exception was thrown then probably the request will be redirected because it doesn't ends with '/'.
+        * So here we are trying to find the Service config that will handle the request if we add a '/' to the end because Armeria will do that.
+        * This is the same strategy as the one Armeria uses here {@link com.linecorp.armeria.server.FallbackService# handleNotFound}
+        */
+      val oldPath = routingCtx.path
+      val newPath = oldPath + '/'
+      val serviceConfig = ctx.config.virtualHost.findServiceConfig(routingCtx.overridePath(newPath))
+      if (serviceConfig.isPresent) {
+        requestHandler.span.name(serviceConfig.value().route().patternString())
+      }
     }
   }
 }
diff --git a/instrumentation/kamon-armeria/src/test/resources/application.conf b/instrumentation/kamon-armeria/src/test/resources/application.conf
diff --git a/instrumentation/kamon-armeria/src/test/scala/kamon/instrumentation/armeria/server/ArmeriaGrpcServerTracingSpec.scala b/instrumentation/kamon-armeria/src/test/scala/kamon/instrumentation/armeria/server/ArmeriaGrpcServerTracingSpec.scala
@@ -47,7 +47,7 @@ class ArmeriaGrpcServerTracingSpec extends AnyWordSpec
 
       eventually(timeout(3 seconds)) {
         val span = testSpanReporter().nextSpan().value
-        span.operationName shouldBe "ArmeriaHelloService/Hello"
+        span.operationName shouldBe "/kamon.armeria.instrumentation.grpc.ArmeriaHelloService/Hello"
         span.hasError shouldBe false
         span.metricTags.get(plain("component")) shouldBe "armeria.http.server"
         span.metricTags.get(plain("http.method")) shouldBe "POST"
diff --git a/instrumentation/kamon-armeria/src/test/scala/kamon/instrumentation/armeria/server/ArmeriaHttpServerTracingSpec.scala b/instrumentation/kamon-armeria/src/test/scala/kamon/instrumentation/armeria/server/ArmeriaHttpServerTracingSpec.scala
@@ -47,7 +47,7 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
 
   private def testSuite(protocol: String, interface: String, port: Int): Unit = {
 
-    val webClient = newWebClient(protocol,port)
+    val webClient = newWebClient(protocol, port)
 
     s"The Armeria $protocol server" when {
 
@@ -75,8 +75,8 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
             val target = s"$protocol://$interface:$port/$pathNotFoundEndpoint"
             val expected = "unhandled"
 
-              val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, pathNotFoundEndpoint))
-              webClient.execute(request)
+            val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, pathNotFoundEndpoint))
+            webClient.execute(request)
 
             eventually(timeout(3 seconds)) {
               val span = testSpanReporter().nextSpan().value
@@ -91,12 +91,12 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
         }
 
         "set operation name with path + http method" when {
-            "resource doesn't exist" in {
-              val target = s"$protocol://$interface:$port/$usersEndpoint/not-found"
-              val expected = "/users/{}"
+          "resource doesn't exist" in {
+            val target = s"$protocol://$interface:$port/$usersEndpoint/not-found"
+            val expected = "/users/:userId"
 
-              val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$usersEndpoint/not-found"))
-              webClient.execute(request)
+            val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$usersEndpoint/not-found"))
+            webClient.execute(request)
 
             eventually(timeout(3 seconds)) {
               val span = testSpanReporter().nextSpan().value
@@ -111,7 +111,7 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
         }
 
         "not include path variables names" in {
-          val expected = "/users/{}/accounts/{}"
+          val expected = "/users/:userId/accounts/:accountId"
 
           val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, userAccountEndpoint))
           webClient.execute(request)
@@ -123,7 +123,7 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
         }
 
         "not fail when request url contains special regexp chars" in {
-          val expected = "/users/{}/accounts/{}"
+          val expected = "/users/:userId/accounts/:accountId"
 
           val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$userAccountEndpoint**"))
           val response = webClient.execute(request).aggregate().get()
@@ -137,7 +137,7 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
 
         "mark spans as failed when request fails" in {
           val target = s"$protocol://$interface:$port/$usersEndpoint/error"
-          val expected = "/users/{}"
+          val expected = "/users/:userId"
 
           val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$usersEndpoint/error"))
           webClient.execute(request)
@@ -156,9 +156,9 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
         "return a redirect status code" when {
           "a request to /docs is redirected to /docs/" in {
             val target = s"$protocol://$interface:$port/$docsEndpoint"
-            val expected = s"/docs"
+            val expected = s"/docs/*"
 
-            val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, docsEndpoint))
+            val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$docsEndpoint"))
             webClient.execute(request)
 
             eventually(timeout(3 seconds)) {
@@ -172,10 +172,10 @@ class ArmeriaHttpServerTracingSpec extends AnyWordSpec
           }
         }
 
-        "return a ok status code " when {
+        "return a ok status code" when {
           "a request to /docs/ is done" in {
             val target = s"$protocol://$interface:$port/$docsEndpoint/"
-            val expected = s"/docs"
+            val expected = s"/docs/*"
 
             val request = HttpRequest.of(RequestHeaders.of(HttpMethod.GET, s"$docsEndpoint/"))
             webClient.execute(request)
