Repository: tipsy/javalin
Pull request #959 (2020-05-12 12:36:58)

Improve Micrometer plugin to tag requests by URI, outcome, status, etc.
Hi there! I'm one of the Micrometer maintainers.

This PR upgrades to Micrometer 1.5.1 and makes several improvements to `MicrometerPlugin`.

Use the new Micrometer `TimedHandler`. Improve the plugin to ship a metric called `jetty.server.requests` with tags for `uri`, `exception`, `method`, `status`, and `outcome`. Much of the logic is around making the `uri` tag have meaning in the aggregate (represent the route and not the request path) and limit tag cardinality. It should look something like this:

![image](https://user-images.githubusercontent.com/1697736/81692400-5a23f800-942c-11ea-8357-9f0d91786f8a.png)

I did find it kind of difficult to capture the exception for use as a tag in `TimedHandler` because:
 
- Javalin assumes there is only one exception handler per `Exception` class type
- Javalin always delegates to the closest matching handler by type. 
- Unless you manually set the status code to 500 in an exception handler, Javalin will return a 200.

It leads to an awkward situation where if you want exception tagging and also to add some user-provided exception handler, you have to specifically delegate to the handler defined in the `MicrometerPlugin`:

```java
// must manually delegate to Micrometer exception handler for excepton tags to be correct
app.exception(IllegalArgumentException::class.java) { e, ctx ->
  MicrometerPlugin.EXCEPTION_HANDLER.handle(e, ctx);
  e.printStackTrace();
  ctx.status(200); // for Javalin's default behavior you have to set this status code back
};
```

Unless the framework can do something to improve this usability concern, I think exception tagging should be left off by default. I've added an option in the `MicrometerPlugin` constructor:

```kotlin
MicrometerPlugin @JvmOverloads constructor(
  private val registry: MeterRegistry = Metrics.globalRegistry,
  private val tags: Iterable<Tag> = Tags.empty(),
  private val tagExceptionName: Boolean = false
)
```

tipsy: Thanks @jkschneider, I'll try to get this merged before the end of the week! 
tipsy: This looks great @jkschneider. It does make the plugin a bit more complex, so a bit of documentation would be good. Would you be interested in creating a small docs page for http://javalin.io/plugins/micrometer ?

It involves creating a markdown file here: https://github.com/javalin/javalin.github.io/tree/master/pages/plugins, but you can also write your docs as a comment in this PR, and I can take care of putting it on the website.

Patch:
diff --git a/javalin/src/main/java/io/javalin/plugin/metrics/MicrometerPlugin.kt b/javalin/src/main/java/io/javalin/plugin/metrics/MicrometerPlugin.kt
index fae0e9ab..b21311aa 100644
--- a/javalin/src/main/java/io/javalin/plugin/metrics/MicrometerPlugin.kt
+++ b/javalin/src/main/java/io/javalin/plugin/metrics/MicrometerPlugin.kt
@@ -1,25 +1,78 @@
+/*
+ * Javalin - https://javalin.io
+ * Copyright 2020 David Åse
+ * Licensed under Apache 2.0: https://github.com/tipsy/javalin/blob/master/LICENSE
+ */
+
 package io.javalin.plugin.metrics
 
 import io.javalin.Javalin
 import io.javalin.core.plugin.Plugin
-import io.javalin.core.util.OptionalDependency
-import io.javalin.core.util.Util
+import io.javalin.http.Context
+import io.javalin.http.ExceptionHandler
+import io.javalin.http.HandlerEntry
+import io.javalin.http.HandlerType
 import io.micrometer.core.instrument.MeterRegistry
 import io.micrometer.core.instrument.Metrics
 import io.micrometer.core.instrument.Tag
+import io.micrometer.core.instrument.Tags
+import io.micrometer.core.instrument.binder.http.DefaultHttpServletRequestTagsProvider
+import io.micrometer.core.instrument.binder.jetty.JettyConnectionMetrics
 import io.micrometer.core.instrument.binder.jetty.JettyServerThreadPoolMetrics
-import io.micrometer.core.instrument.binder.jetty.JettyStatisticsMetrics
-import org.eclipse.jetty.server.handler.StatisticsHandler
+import io.micrometer.core.instrument.binder.jetty.TimedHandler
+import org.apache.commons.lang3.StringUtils
+import javax.servlet.http.HttpServletRequest
+import javax.servlet.http.HttpServletResponse
 
-class MicrometerPlugin @JvmOverloads constructor(val registry: MeterRegistry = Metrics.globalRegistry) : Plugin {
+class MicrometerPlugin @JvmOverloads constructor(private val registry: MeterRegistry = Metrics.globalRegistry,
+                                                 private val tags: Iterable<Tag> = Tags.empty(),
+                                                 private val tagExceptionName: Boolean = false) : Plugin {
     override fun apply(app: Javalin) {
         app.server()?.server()?.let { server ->
-            Util.ensureDependencyPresent(OptionalDependency.MICROMETER)
-            val statisticsHandler = StatisticsHandler()
-            server.insertHandler(statisticsHandler)
-            JettyStatisticsMetrics.monitor(this.registry, statisticsHandler)
-            val threadPoolMetrics = JettyServerThreadPoolMetrics(server.threadPool, emptyList<Tag>())
-            threadPoolMetrics.bindTo(this.registry)
+            if(tagExceptionName) {
+                app.exception(Exception::class.java, EXCEPTION_HANDLER)
+            }
+
+            server.insertHandler(TimedHandler(registry, tags, object : DefaultHttpServletRequestTagsProvider() {
+                override fun getTags(request: HttpServletRequest, response: HttpServletResponse): Iterable<Tag> {
+                    val exceptionName = if(tagExceptionName) {
+                        response.getHeader(EXCEPTION_HEADER)
+                    } else {
+                        "Unknown"
+                    }
+
+                    response.setHeader(EXCEPTION_HEADER, null)
+                    val uri = app.servlet()
+                            .matcher
+                            .findEntries(HandlerType.valueOf(request.method), request.pathInfo)
+                            .stream()
+                            .findAny()
+                            .map(HandlerEntry::path)
+                            .map { path: String -> if (path == "/" || StringUtils.isBlank(path)) "root" else path }
+                            .map { path: String -> if (response.status in 300..399) "REDIRECTION" else path }
+                            .map { path: String -> if (response.status == 404) "NOT_FOUND" else path }
+                            .orElse("NOT_FOUND")
+                    return Tags.concat(
+                            super.getTags(request, response),
+                            "uri", uri,
+                            "exception", exceptionName ?: "None"
+                    )
+                }
+            }))
+
+            JettyServerThreadPoolMetrics(server.threadPool, tags).bindTo(registry)
+        }
+
+        JettyConnectionMetrics(registry, tags)
+    }
+
+    companion object {
+        private const val EXCEPTION_HEADER = "__micrometer_exception_name"
+
+        var EXCEPTION_HANDLER = ExceptionHandler { e: Exception, ctx: Context ->
+            val simpleName = e.javaClass.simpleName
+            ctx.header(EXCEPTION_HEADER, if (StringUtils.isNotBlank(simpleName)) simpleName else e.javaClass.name)
+            ctx.status(500)
         }
     }
 }
diff --git a/pom.xml b/pom.xml
index 9851054b..c9e5459b 100644
--- a/pom.xml
+++ b/pom.xml
@@ -72,7 +72,7 @@
         <jtwig.version>5.87.0.RELEASE</jtwig.version>
         <jvmbrotli.version>0.2.0</jvmbrotli.version>
         <logback.version>1.2.3</logback.version>
-        <micrometer.version>1.3.6</micrometer.version>
+        <micrometer.version>1.5.1</micrometer.version>
         <mustache.version>0.9.6</mustache.version>
         <pebble.version>3.1.2</pebble.version>
         <redoc.version>2.0.0-rc.23</redoc.version>


Test patch:
diff --git a/javalin/src/test/java/io/javalin/TestMicrometerPlugin.kt b/javalin/src/test/java/io/javalin/TestMicrometerPlugin.kt
new file mode 100644
index 00000000..40efb883
--- /dev/null
+++ b/javalin/src/test/java/io/javalin/TestMicrometerPlugin.kt
@@ -0,0 +1,95 @@
+/*
+ * Javalin - https://javalin.io
+ * Copyright 2020 David Åse
+ * Licensed under Apache 2.0: https://github.com/tipsy/javalin/blob/master/LICENSE
+ */
+
+package io.javalin
+
+import io.javalin.plugin.metrics.MicrometerPlugin
+import io.javalin.testing.HttpUtil
+import io.micrometer.core.instrument.MeterRegistry
+import io.micrometer.core.instrument.Tags
+import io.micrometer.core.instrument.simple.SimpleMeterRegistry
+import org.junit.After
+import org.junit.Before
+import org.junit.Test
+
+class TestMicrometerPlugin {
+    private val meterRegistry: MeterRegistry = SimpleMeterRegistry()
+    val app: Javalin = Javalin.create { config -> config.registerPlugin(MicrometerPlugin(meterRegistry, Tags.empty(), true)) }.start(0);
+    val http = HttpUtil(app.port())
+
+    @Before
+    fun before() {
+        // must manually delegate to Micrometer exception handler for excepton tags to be correct
+        app.exception(IllegalArgumentException::class.java) { e, ctx ->
+            MicrometerPlugin.EXCEPTION_HANDLER.handle(e, ctx);
+            e.printStackTrace();
+        };
+    }
+
+    @After
+    fun after() {
+        app.stop()
+    }
+
+    @Test
+    fun `successful request`() {
+        app.get("/hello/:name") { ctx -> ctx.result("Hello: " + ctx.pathParam("name")) }
+        http.get("/hello/jon")
+        meterRegistry.get("jetty.server.requests")
+                .tag("uri", "/hello/:name")
+                .tag("method", "GET")
+                .tag("exception", "None")
+                .tag("status", "200")
+                .tag("outcome", "SUCCESS")
+                .timer()
+    }
+
+    @Test
+    fun `request throwing exception`() {
+        app.get("/boom") { ctx -> throw IllegalArgumentException("boom") }
+        http.get("/boom")
+        meterRegistry.get("jetty.server.requests")
+                .tag("uri", "/boom")
+                .tag("method", "GET")
+                .tag("exception", "IllegalArgumentException")
+                .tag("status", "500")
+                .tag("outcome", "SERVER_ERROR")
+                .timer()
+    }
+
+    @Test
+    fun redirect() {
+        app.get("/hello") { ctx -> ctx.result("Hello") }
+        app.get("/redirect") { ctx -> ctx.redirect("/hello") }
+        http.get("/redirect")
+        meterRegistry.get("jetty.server.requests")
+                .tag("uri", "REDIRECTION")
+                .tag("method", "GET")
+                .tag("exception", "None")
+                .tag("status", "302")
+                .tag("outcome", "REDIRECTION")
+                .timer()
+        meterRegistry.get("jetty.server.requests")
+                .tag("uri", "/hello")
+                .tag("method", "GET")
+                .tag("exception", "None")
+                .tag("status", "200")
+                .tag("outcome", "SUCCESS")
+                .timer()
+    }
+
+    @Test
+    fun `not found`() {
+        http.get("/doesNotExist")
+        meterRegistry.get("jetty.server.requests")
+                .tag("uri", "NOT_FOUND")
+                .tag("method", "GET")
+                .tag("exception", "None")
+                .tag("status", "404")
+                .tag("outcome", "CLIENT_ERROR")
+                .timer()
+    }
+}