review

Author: laurit

…ver/route/AkkaRouteHolderSingletons.java …akkahttp/server/route/AkkaRouteUtil.javainstrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/AkkaRouteHolderSingletons.java renamed to instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/AkkaRouteUtil.java instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/AkkaRouteHolderSingletons.java renamed to instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/AkkaRouteUtil.java

@@ -8,10 +8,10 @@
 import akka.http.scaladsl.server.PathMatcher;
 import io.opentelemetry.instrumentation.api.util.VirtualField;
 
-public final class AkkaRouteHolderSingletons {
+public final class AkkaRouteUtil {
 
   public static final VirtualField<PathMatcher<?>, String> PREFIX =
       VirtualField.find(PathMatcher.class, String.class);
 
-  private AkkaRouteHolderSingletons() {}
+  private AkkaRouteUtil() {}
 }

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/AkkaRouteWrapper.java

@@ -8,7 +8,6 @@
 import akka.http.scaladsl.server.RequestContext;
 import akka.http.scaladsl.server.RouteResult;
 import io.opentelemetry.context.Context;
-import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
 import scala.Function1;
 import scala.concurrent.Future;
 import scala.runtime.AbstractFunction1;
@@ -22,7 +21,7 @@ public AkkaRouteWrapper(Function1<RequestContext, Future<RouteResult>> route) {
 
   @Override
   public Future<RouteResult> apply(RequestContext ctx) {
-    Context context = Java8BytecodeBridge.currentContext();
+    Context context = Context.current();
     AkkaRouteHolder routeHolder = AkkaRouteHolder.get(context);
     if (routeHolder == null) {
       return route.apply(ctx);

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/PathMatcherInstrumentation.java

@@ -5,7 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.akkahttp.server.route;
 
-import static io.opentelemetry.javaagent.instrumentation.akkahttp.server.route.AkkaRouteHolderSingletons.PREFIX;
+import static io.opentelemetry.javaagent.instrumentation.akkahttp.server.route.AkkaRouteUtil.PREFIX;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.returns;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/PathMatcherStaticInstrumentation.java

@@ -6,7 +6,7 @@
 package io.opentelemetry.javaagent.instrumentation.akkahttp.server.route;
 
 import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.extendsClass;
-import static io.opentelemetry.javaagent.instrumentation.akkahttp.server.route.AkkaRouteHolderSingletons.PREFIX;
+import static io.opentelemetry.javaagent.instrumentation.akkahttp.server.route.AkkaRouteUtil.PREFIX;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/server/route/PathMatcherUtil.java

@@ -8,17 +8,6 @@ package io.opentelemetry.javaagent.instrumentation.akkahttp
 import akka.actor.ActorSystem
 import akka.http.scaladsl.Http
 import akka.http.scaladsl.server.Directives._
-import akka.http.scaladsl.server.Directives.{
-  IntNumber,
-  complete,
-  concat,
-  path,
-  pathEndOrSingleSlash,
-  pathPrefix,
-  pathSingleSlash,
-  JavaUUID,
-  Segment
-}
 import akka.http.scaladsl.server.Route
 import akka.stream.ActorMaterializer
 import io.opentelemetry.instrumentation.test.utils.PortUtils
@@ -32,6 +21,8 @@ import io.opentelemetry.testing.internal.armeria.common.{
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.{AfterAll, Test, TestInstance}
 import org.junit.jupiter.api.extension.RegisterExtension
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.CsvSource
 
 import java.net.{URI, URISyntaxException}
 import java.util.function.Consumer
@@ -62,24 +53,164 @@ class AkkaHttpServerRouteTest {
     test(route, "/test", "GET /test")
   }
 
-  @Test def testRoute(): Unit = {
+  @Test def testPathPrefix(): Unit = {
+    val route =
+      pathPrefix("a") {
+        pathPrefix("b") {
+          path("c") {
+            complete("ok")
+          }
+        }
+      }
+
+    test(route, "/a/b/c", "GET /a/b/c")
+  }
+
+  @Test def testTrailingSlash(): Unit = {
+    val route = path("foo"./) { complete("ok") }
+    test(route, "/foo/", "GET /foo/")
+  }
+
+  @Test def testSlash(): Unit = {
+    val route = path("foo" / "bar") { complete("ok") }
+    test(route, "/foo/bar", "GET /foo/bar")
+  }
+
+  @Test def testEncodedSlash(): Unit = {
+    val route = path("foo/bar") { complete("ok") }
+    test(route, "/foo%2Fbar", "GET /foo%2Fbar")
+  }
+
+  @Test def testSeparateOnSlashes(): Unit = {
+    val route = path(separateOnSlashes("foo/bar")) { complete("ok") }
+    test(route, "/foo/bar", "GET /foo/bar")
+  }
+
+  @Test def testMatchRegex(): Unit = {
+    val route = path("foo" / """number-\d+""".r) { _ => complete("ok") }
+    test(route, "/foo/number-123", "GET /foo/*")
+  }
+
+  @ParameterizedTest
+  @CsvSource(
+    Array(
+      "/i42, GET /i*",
+      "/hCAFE, GET /h*"
+    )
+  )
+  def testPipe(requestPath: String, expectedName: String): Unit = {
+    val route = path("i" ~ IntNumber | "h" ~ HexIntNumber) { _ =>
+      complete("ok")
+    }
+    test(route, requestPath, expectedName)
+  }
+
+  @ParameterizedTest
+  @CsvSource(
+    Array(
+      "/colours/red, GET /colours/red",
+      "/colours/green, GET /colours/green"
+    )
+  )
+  def testMapExtractor(requestPath: String, expectedName: String): Unit = {
+    val route = path("colours" / Map("red" -> 1, "green" -> 2, "blue" -> 3)) {
+      _ => complete("ok")
+    }
+    test(route, requestPath, expectedName)
+  }
+
+  @ParameterizedTest
+  @CsvSource(
+    Array(
+      "/fooish, GET /foo*",
+      "/fooish/123, GET /foo*"
+    )
+  )
+  def testNotMatch(requestPath: String, expectedName: String): Unit = {
+    val route = pathPrefix("foo" ~ !"bar") { complete("ok") }
+    test(route, requestPath, expectedName)
+  }
+
+  @Test def testProvide(): Unit = {
+    val route = pathPrefix("foo") {
+      provide("hi") { _ =>
+        path("bar") {
+          complete("ok")
+        }
+      }
+    }
+    test(route, "/foo/bar", "GET /foo/bar")
+  }
+
+  @ParameterizedTest
+  @CsvSource(
+    Array(
+      "/foo/bar/X42/edit, GET /foo/bar/X*/edit",
+      "/foo/bar/X/edit, GET /foo/bar/X/edit"
+    )
+  )
+  def testOptional(requestPath: String, expectedName: String): Unit = {
+    val route = path("foo" / "bar" / "X" ~ IntNumber.? / ("edit" | "create")) {
+      _ => complete("ok")
+    }
+    test(route, requestPath, expectedName)
+  }
+
+  @Test def testNoMatches(): Unit = {
+    val route = path("foo" / "bar") { complete("ok") }
+    test(
+      route,
+      "/foo/wrong",
+      "GET",
+      404,
+      "The requested resource could not be found."
+    )
+  }
+
+  @Test def testError(): Unit = {
+    val route = path("foo" / IntNumber) { _ =>
+      failWith(new RuntimeException("oops"))
+    }
+    test(
+      route,
+      "/foo/123",
+      "GET /foo/*",
+      500,
+      "There was an internal server error."
+    )
+  }
+
+  @Test def testConcat(): Unit = {
     val route = concat(
       pathEndOrSingleSlash {
         complete("root")
       },
+      path(".+".r / "wrong1") { _ =>
+        complete("wrong1")
+      },
       pathPrefix("test") {
         concat(
           pathSingleSlash {
             complete("test")
           },
-          path(IntNumber) { _ =>
-            complete("ok")
+          pathPrefix("foo") {
+            concat(
+              path(IntNumber) { _ =>
+                complete("ok")
+              }
+            )
+          },
+          path("something-else") {
+            complete("test")
           }
         )
+      },
+      path("test" / "wrong2") {
+        complete("wrong2")
       }
     )
 
-    test(route, "/test/1", "GET /test/*")
+    test(route, "/test/foo/1", "GET /test/foo/*")
   }
 
   @Test def testRouteWithUUID(): Unit = {
@@ -127,7 +258,13 @@ class AkkaHttpServerRouteTest {
     test(route, "/api/v2/orders/order123/status", "GET /api/v2/orders/*/status")
   }
 
-  def test(route: Route, path: String, spanName: String): Unit = {
+  def test(
+      route: Route,
+      path: String,
+      spanName: String,
+      expectedStatus: Int = 200,
+      expectedMsg: String = "ok"
+  ): Unit = {
     val port = PortUtils.findOpenPort
     val address: URI = buildAddress(port)
     val binding =
@@ -138,8 +275,8 @@ class AkkaHttpServerRouteTest {
         address.resolve(path).toString
       )
       val response = client.execute(request).aggregate.join
-      assertThat(response.status.code).isEqualTo(200)
-      assertThat(response.contentUtf8).isEqualTo("ok")
+      assertThat(response.status.code).isEqualTo(expectedStatus)
+      assertThat(response.contentUtf8).isEqualTo(expectedMsg)
 
       testing.waitAndAssertTraces(new Consumer[TraceAssert] {
         override def accept(trace: TraceAssert): Unit =