Merge pull request #926 from yarosman/master-play-route-name-generator

Allow to define custom router name generator for play server

instrumentation/kamon-play/build.sbt

@@ -1,9 +1,9 @@
 import sbt.Tests._
 import play.grpc.gen.scaladsl.{PlayScalaServerCodeGenerator, PlayScalaClientCodeGenerator}
 
-val `Play-2.6-version` = "2.6.23"
-val `Play-2.7-version` = "2.7.3"
-val `Play-2.8-version` = "2.8.2"
+val `Play-2.6-version` = "2.6.25"
+val `Play-2.7-version` = "2.7.9"
+val `Play-2.8-version` = "2.8.7"
 
 /**
   * Test Configurations
@@ -45,8 +45,8 @@ libraryDependencies ++= { if(scalaBinaryVersion.value == "2.13") Seq.empty else
 )}
 
 libraryDependencies ++= { if(scalaBinaryVersion.value == "2.11") Seq.empty else Seq(
-  "com.lightbend.play" %%  "play-grpc-runtime"    % "0.9.0" % "test-play-2.8",
-  "com.lightbend.play" %%  "play-grpc-scalatest"  % "0.9.0" % "test-play-2.8",
+  "com.lightbend.play" %%  "play-grpc-runtime"    % "0.9.1" % "test-play-2.8",
+  "com.lightbend.play" %%  "play-grpc-scalatest"  % "0.9.1" % "test-play-2.8",
   "com.typesafe.play" %%  "play-akka-http2-support" % `Play-2.8-version` % "test-play-2.8",
   "com.typesafe.play" %%  "play"                  % `Play-2.8-version` % "test-play-2.8",
   "com.typesafe.play" %%  "play-netty-server"     % `Play-2.8-version` % "test-play-2.8",
@@ -73,12 +73,12 @@ lazy val baseTestSettings = Seq(
 )
 
 inConfig(TestCommon)(Defaults.testSettings ++ instrumentationSettings ++ baseTestSettings ++ Seq(
-  crossScalaVersions := Seq("2.11.12", "2.12.11")
+  crossScalaVersions := Seq("2.11.12", "2.12.13")
 ))
 
 inConfig(`Test-Play-2.6`)(Defaults.testSettings ++ instrumentationSettings ++ baseTestSettings ++ Seq(
   sources := joinSources(TestCommon, `Test-Play-2.6`).value,
-  crossScalaVersions := Seq("2.11.12", "2.12.11"),
+  crossScalaVersions := Seq("2.11.12", "2.12.13"),
   testGrouping := singleTestPerJvm(definedTests.value, javaOptions.value),
   unmanagedResourceDirectories ++= (unmanagedResourceDirectories in Compile).value,
   unmanagedResourceDirectories ++= (unmanagedResourceDirectories in TestCommon).value,
@@ -93,7 +93,7 @@ inConfig(`Test-Play-2.7`)(Defaults.testSettings ++ instrumentationSettings ++ ba
 
 inConfig(`Test-Play-2.8`)(Defaults.testSettings ++ instrumentationSettings ++ baseTestSettings ++ Seq(
   sources := joinSources(TestCommon, `Test-Play-2.8`).value,
-  crossScalaVersions := Seq("2.12.11", "2.13.1"),
+  crossScalaVersions := Seq("2.12.13", "2.13.3"),
   akkaGrpcGeneratedSources := Seq(AkkaGrpc.Server, AkkaGrpc.Client),
   akkaGrpcGeneratedLanguages := Seq(AkkaGrpc.Scala),
   akkaGrpcExtraGenerators += PlayScalaServerCodeGenerator,

instrumentation/kamon-play/src/main/resources/reference.conf

@@ -107,6 +107,12 @@ kamon.instrumentation.play.http {
       # Custom mappings between routes and operation names.
       operations {
 
+        # Default configuration for HttpOperationNameGenerator implementation, but it has never been used
+        #   - default: Uses the set default operation name
+        #   - method: Uses the request HTTP method as the operation name.
+        #
+        name-generator = "default"
+
         # The default operation name to be used when creating Spans to handle the HTTP server requests. In most
         # cases it is not possible to define an operation name right at the moment of starting the HTTP server Span
         # and in those cases, this operation name will be initially assigned to the Span. Instrumentation authors
@@ -139,6 +145,29 @@ kamon.instrumentation.play.http {
         }
       }
     }
+
+    #
+    # Configure specefic tracing/monitoring elements which depends on `play-framework` internal APIs that might change in the future
+    #
+    extra {
+      # Framework-specific way to generate operations names instead of using `kamon.instrumentation.play.http.server.tracing.operations.name-generator`
+      # Used by default with implementations that converts paths of form `/foo/bar/$paramname<regexp>/blah` to `/foo/bar/paramname/blah`
+      #
+      # FQCN for a kamon.instrumentation.play.RouterOperationNameGenerator 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.play.DefaultRouterOperationNameGenerator"
+
+      # Framework-specific way to generate operations names instead of using `kamon.instrumentation.play.http.server.tracing.operations.name-generator`
+      # Used by default with setting operation name to `request.uri.toRelative`
+      #
+      # FQCN for a kamon.instrumentation.play.GrpcRouterNameGenerator 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.
+      #
+      grpc-name-generator = "kamon.instrumentation.play.DefaultGrpcRouterNameGenerator"
+    }
   }
 
   client {

instrumentation/kamon-play/src/main/scala/kamon/instrumentation/play/GrpcRouterNameGenerator.scala

@@ -0,0 +1,15 @@
+package kamon.instrumentation.play
+
+import akka.http.scaladsl.model.HttpRequest
+
+trait GrpcRouterNameGenerator {
+
+  def generateOperationName(request: akka.http.scaladsl.model.HttpRequest): String
+
+}
+
+class DefaultGrpcRouterNameGenerator extends GrpcRouterNameGenerator {
+
+  override def generateOperationName(request: HttpRequest): String = request.uri.toRelative.toString
+
+}

instrumentation/kamon-play/src/main/scala/kamon/instrumentation/play/PlayServerInstrumentation.scala

@@ -19,10 +19,11 @@ package kamon.instrumentation.play
 import java.time.Duration
 import java.util.concurrent.atomic.AtomicLong
 
+import com.typesafe.config.Config
 import io.netty.channel.Channel
 import io.netty.handler.codec.http.{HttpRequest, HttpResponse}
 import io.netty.util.concurrent.GenericFutureListener
-import kamon.Kamon
+import kamon.{ClassLoading, Kamon}
 import kamon.context.Storage
 import kamon.instrumentation.akka.http.ServerFlowWrapper
 import kamon.instrumentation.context.{CaptureCurrentTimestampOnExit, HasTimestamp}
@@ -33,12 +34,13 @@ import kanela.agent.api.instrumentation.InstrumentationBuilder
 import kanela.agent.api.instrumentation.classloader.ClassRefiner
 import kanela.agent.api.instrumentation.mixin.Initializer
 import kanela.agent.libs.net.bytebuddy.asm.Advice
+import org.slf4j.LoggerFactory
 import play.api.mvc.RequestHeader
 import play.api.routing.{HandlerDef, Router}
 import play.core.server.NettyServer
 
 import scala.collection.JavaConverters.asScalaBufferConverter
-import scala.collection.concurrent.TrieMap
+import scala.util.Try
 import scala.util.{Failure, Success}
 
 class PlayServerInstrumentation extends InstrumentationBuilder {
@@ -243,32 +245,61 @@ object HasServerInstrumentation {
 
 object GenerateOperationNameOnFilterHandler {
 
-  private val _operationNameCache = TrieMap.empty[String, String]
-  private val _normalizePattern = """\$([^<]+)<[^>]+>""".r
+  private val defaultRouterNameGenerator = new DefaultRouterOperationNameGenerator()
+  private val _logger = LoggerFactory.getLogger(GenerateOperationNameOnFilterHandler.getClass)
+
+  @volatile private var _routerNameGenerator: RouterOperationNameGenerator = rebuildRouterNameGenerator(Kamon.config())
+
+  Kamon.onReconfigure(newConfig => _routerNameGenerator = rebuildRouterNameGenerator(newConfig))
+
+  private def rebuildRouterNameGenerator(config: Config): RouterOperationNameGenerator = {
+    val nameGeneratorClazz = config.getString("kamon.instrumentation.play.http.server.extra.name-generator")
+    Try(ClassLoading.createInstance[RouterOperationNameGenerator](nameGeneratorClazz)) match {
+      case Failure(exception) =>
+        _logger.error(s"Exception occurred on $nameGeneratorClazz instance creation, used default", exception)
+        defaultRouterNameGenerator
+      case Success(value) =>
+        value
+    }
+  }
 
   @Advice.OnMethodEnter
   def enter(@Advice.Argument(0) request: RequestHeader): Unit = {
     request.attrs.get(Router.Attrs.HandlerDef).map(handler => {
       val span = Kamon.currentSpan()
-      span.name(generateOperationName(handler))
+      span.name(_routerNameGenerator.generateOperationName(handler))
       span.takeSamplingDecision()
     })
   }
 
-  private def generateOperationName(handlerDef: HandlerDef): String =
-    _operationNameCache.getOrElseUpdate(handlerDef.path, {
-      // Convert paths of form /foo/bar/$paramname<regexp>/blah to /foo/bar/paramname/blah
-      _normalizePattern.replaceAllIn(handlerDef.path, "$1")
-  })
-
 }
 
 object GenerateGRPCOperationName {
+
+  private val defaultGrpcRouterNameGenerator = new DefaultGrpcRouterNameGenerator()
+  private val _logger = LoggerFactory.getLogger(GenerateOperationNameOnFilterHandler.getClass)
+
+  @volatile private var _grpcRouterNameGenerator: GrpcRouterNameGenerator = rebuildRouterNameGenerator(Kamon.config())
+
+  Kamon.onReconfigure(newConfig => _grpcRouterNameGenerator = rebuildRouterNameGenerator(newConfig))
+
+  private def rebuildRouterNameGenerator(config: Config): GrpcRouterNameGenerator = {
+    val nameGeneratorClazz = config.getString("kamon.instrumentation.play.http.server.extra.grpc-name-generator")
+    Try(ClassLoading.createInstance[GrpcRouterNameGenerator](nameGeneratorClazz)) match {
+      case Failure(exception) =>
+        _logger.error(s"Exception occurred on $nameGeneratorClazz instance creation, used default", exception)
+        defaultGrpcRouterNameGenerator
+      case Success(value) =>
+        value
+    }
+  }
+
   @Advice.OnMethodEnter
   def enter(@Advice.Argument(0) request: akka.http.scaladsl.model.HttpRequest): Unit = {
     val span = Kamon.currentSpan()
-    span.name(request.uri.toRelative.toString)
+    span.name(_grpcRouterNameGenerator.generateOperationName(request))
     span.tag("http.protocol", request.protocol.value)
     span.takeSamplingDecision()
   }
+
 }

instrumentation/kamon-play/src/main/scala/kamon/instrumentation/play/RouterOperationNameGenerator.scala

@@ -0,0 +1,23 @@
+package kamon.instrumentation.play
+
+import play.api.routing.HandlerDef
+
+import scala.collection.concurrent.TrieMap
+
+trait RouterOperationNameGenerator {
+  def generateOperationName(handlerDef: HandlerDef): String
+}
+
+class DefaultRouterOperationNameGenerator extends RouterOperationNameGenerator {
+
+  private val _operationNameCache = TrieMap.empty[String, String]
+  private val _normalizePattern = """\$([^<]+)<[^>]+>""".r
+
+  def generateOperationName(handlerDef: HandlerDef): String = {
+    _operationNameCache.getOrElseUpdate(handlerDef.path, {
+      // Convert paths of form /foo/bar/$paramname<regexp>/blah to /foo/bar/paramname/blah
+      _normalizePattern.replaceAllIn(handlerDef.path, "$1")
+    })
+  }
+
+}