From 97d24daac3bff6158c47bd519df2b3534e9db2e8 Mon Sep 17 00:00:00 2001 From: lucasamoroso Date: Mon, 10 Oct 2022 20:49:29 -0300 Subject: [PATCH] use route pattern string as server side operation name --- .../server/ArmeriaHttpServerDecorator.java | 24 ++++++++++----- .../src/main/resources/reference.conf | 26 ---------------- .../ArmeriaHttpServerInstrumentation.scala | 19 ++++++++++-- .../src/test/resources/application.conf | 4 --- .../server/ArmeriaGrpcServerTracingSpec.scala | 2 +- .../server/ArmeriaHttpServerTracingSpec.scala | 30 +++++++++---------- 6 files changed, 48 insertions(+), 57 deletions(-) delete mode 100644 instrumentation/kamon-armeria/src/test/resources/application.conf 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 index a20e78c94..6bf82f159 100644 --- 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 REQUEST_HANDLER_TRACE_KEY = - AttributeKey.valueOf(HttpServerInstrumentation.RequestHandler.class, "REQUEST_HANDLER_TRACE"); + AttributeKey.valueOf(HttpServerInstrumentation.RequestHandler.class, "REQUEST_HANDLER_TRACE"); private final Map 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 index 75e0d40f5..50d234f15 100644 --- 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 index d844a7cce..a071cf7e3 100644 --- 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 deleted file mode 100644 index 2a8579eb9..000000000 --- a/instrumentation/kamon-armeria/src/test/resources/application.conf +++ /dev/null @@ -1,4 +0,0 @@ -kamon.instrumentation.armeria.server.tracing.operations.mappings = { - "/users/*" = "/users/{}", - "/users/*/accounts/*" = "/users/{}/accounts/{}" -} \ No newline at end of file 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 index 30dfc46f8..e5eb42ccf 100644 --- 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 index f386a676b..4948b1563 100644 --- 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)