KTOR-5420 Fix DropwizardMetricsPlugin when using with StatusPages plu…

…gin (#3359)
ktorio · Jan 19, 2023 · aa16db0 · aa16db0
1 parent df902e1
commit aa16db0
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 32 deletions.
diff --git a/ktor-server/ktor-server-plugins/ktor-server-metrics/build.gradle.kts b/ktor-server/ktor-server-plugins/ktor-server-metrics/build.gradle.kts
@@ -7,5 +7,11 @@ kotlin {
                 api(libs.dropwizard.jvm)
             }
         }
+        jvmTest {
+            dependencies {
+                api(project(":ktor-server:ktor-server-plugins:ktor-server-status-pages"))
+                api(project(":ktor-server:ktor-server-plugins:ktor-server-cors"))
+            }
+        }
     }
 }
diff --git a/...lugins/ktor-server-metrics/jvm/src/io/ktor/server/metrics/dropwizard/DropwizardMetrics.kt b/...lugins/ktor-server-metrics/jvm/src/io/ktor/server/metrics/dropwizard/DropwizardMetrics.kt
@@ -9,6 +9,7 @@ import com.codahale.metrics.MetricRegistry.*
 import com.codahale.metrics.jvm.*
 import io.ktor.server.application.*
 import io.ktor.server.application.hooks.*
+import io.ktor.server.request.*
 import io.ktor.server.routing.*
 import io.ktor.util.*
 import java.util.concurrent.*
@@ -46,9 +47,11 @@ public class DropwizardMetricsConfig {
  */
 public val DropwizardMetrics: ApplicationPlugin<DropwizardMetricsConfig> =
     createApplicationPlugin("DropwizardMetrics", ::DropwizardMetricsConfig) {
-        val duration = pluginConfig.registry.timer(name(pluginConfig.baseName, "duration"))
-        val active = pluginConfig.registry.counter(name(pluginConfig.baseName, "active"))
-        val exceptions = pluginConfig.registry.meter(name(pluginConfig.baseName, "exceptions"))
+        val registry = pluginConfig.registry
+        val baseName = pluginConfig.baseName
+        val duration = registry.timer(name(baseName, "duration"))
+        val active = registry.counter(name(baseName, "active"))
+        val exceptions = registry.meter(name(baseName, "exceptions"))
         val httpStatus = ConcurrentHashMap<Int, Meter>()
 
         if (pluginConfig.registerJvmMetricSets) {
@@ -59,8 +62,8 @@ public val DropwizardMetrics: ApplicationPlugin<DropwizardMetricsConfig> =
                 "jvm.files" to ::FileDescriptorRatioGauge,
                 "jvm.attributes" to ::JvmAttributeGaugeSet
             ).filter { (name, _) ->
-                !pluginConfig.registry.names.any { existingName -> existingName.startsWith(name) }
-            }.forEach { (name, metric) -> pluginConfig.registry.register(name, metric()) }
+                !registry.names.any { existingName -> existingName.startsWith(name) }
+            }.forEach { (name, metric) -> registry.register(name, metric()) }
         }
 
         on(CallFailed) { _, _ ->
@@ -69,8 +72,8 @@ public val DropwizardMetrics: ApplicationPlugin<DropwizardMetricsConfig> =
 
         on(MonitoringEvent(Routing.RoutingCallStarted)) { call ->
             val name = call.route.toString()
-            val meter = pluginConfig.registry.meter(name(pluginConfig.baseName, name, "meter"))
-            val timer = pluginConfig.registry.timer(name(pluginConfig.baseName, name, "timer"))
+            val meter = registry.meter(name(baseName, name, "meter"))
+            val timer = registry.timer(name(baseName, name, "timer"))
             meter.mark()
             val context = timer.time()
             call.attributes.put(
@@ -79,29 +82,34 @@ public val DropwizardMetrics: ApplicationPlugin<DropwizardMetricsConfig> =
             )
         }
 
-        on(MonitoringEvent(Routing.RoutingCallFinished)) { call ->
-            val routingMetrics = call.attributes.take(routingMetricsKey)
-            val status = call.response.status()?.value ?: 0
-            val statusMeter =
-                pluginConfig.registry.meter(name(pluginConfig.baseName, routingMetrics.name, status.toString()))
-            statusMeter.mark()
-            routingMetrics.context.stop()
-        }
-
         @OptIn(InternalAPI::class)
         on(Metrics) { call ->
             active.inc()
             call.attributes.put(measureKey, CallMeasure(duration.time()))
         }
 
-        on(AfterCall) { call ->
+        on(ResponseSent) { call ->
+            val routingMetrics = call.attributes.takeOrNull(routingMetricsKey)
+            val name = routingMetrics?.name ?: call.request.routeName
+            val status = call.response.status()?.value ?: 0
+            val statusMeter =
+                registry.meter(name(baseName, name, status.toString()))
+            statusMeter.mark()
+            routingMetrics?.context?.stop()
+
             active.dec()
             val meter = httpStatus.computeIfAbsent(call.response.status()?.value ?: 0) {
-                pluginConfig.registry.meter(name(pluginConfig.baseName, "status", it.toString()))
+                registry.meter(name(baseName, "status", it.toString()))
             }
             meter.mark()
             call.attributes.getOrNull(measureKey)?.apply {
                 timer.stop()
             }
         }
     }
+
+private val ApplicationRequest.routeName: String
+    get() {
+        val metricUri = uri.ifEmpty { "/" }.let { if (it.endsWith('/')) it else "$it/" }
+        return "$metricUri(method:${httpMethod.value})"
+    }
diff --git a/...s/ktor-server-metrics/jvm/src/io/ktor/server/metrics/dropwizard/DropwizardMetricsUtils.kt b/...s/ktor-server-metrics/jvm/src/io/ktor/server/metrics/dropwizard/DropwizardMetricsUtils.kt
@@ -5,23 +5,10 @@
 package io.ktor.server.metrics.dropwizard
 
 import com.codahale.metrics.*
-import io.ktor.server.application.*
 import io.ktor.util.*
 
 internal class RoutingMetrics(val name: String, val context: Timer.Context)
-internal val routingMetricsKey = AttributeKey<RoutingMetrics>("metrics")
+internal val routingMetricsKey = AttributeKey<RoutingMetrics>("routingMetrics")
 
 internal data class CallMeasure constructor(val timer: Timer.Context)
 internal val measureKey = AttributeKey<CallMeasure>("metrics")
-
-internal object AfterCall : Hook<(ApplicationCall) -> Unit> {
-    override fun install(pipeline: ApplicationCallPipeline, handler: (ApplicationCall) -> Unit) {
-        pipeline.intercept(ApplicationCallPipeline.Monitoring) {
-            try {
-                proceed()
-            } finally {
-                handler(call)
-            }
-        }
-    }
-}
diff --git a/.../ktor-server-metrics/jvm/test/io/ktor/server/metrics/dropwizard/DropwizardMetricsTests.kt b/.../ktor-server-metrics/jvm/test/io/ktor/server/metrics/dropwizard/DropwizardMetricsTests.kt
@@ -6,7 +6,11 @@ package io.ktor.server.metrics.dropwizard
 
 import com.codahale.metrics.*
 import com.codahale.metrics.jvm.*
+import io.ktor.client.request.*
+import io.ktor.http.*
 import io.ktor.server.application.*
+import io.ktor.server.plugins.cors.routing.*
+import io.ktor.server.plugins.statuspages.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
 import io.ktor.server.testing.*
@@ -95,4 +99,54 @@ class DropwizardMetricsTests {
 
         assertThat(registry.names, everyItem(startsWith(prefix)))
     }
+
+    @Test
+    fun `with StatusPages plugin`() = testApplication {
+        install(StatusPages) {
+            exception<Throwable> { call, _ ->
+                call.respond(HttpStatusCode.InternalServerError)
+            }
+        }
+
+        val testRegistry = MetricRegistry()
+        install(DropwizardMetrics) {
+            registry = testRegistry
+            baseName = ""
+        }
+
+        routing {
+            get("/uri") {
+                throw RuntimeException("Oops")
+            }
+        }
+
+        client.get("/uri")
+
+        assertEquals(1, testRegistry.meter("/uri/(method:GET).500").count)
+    }
+
+    @Test
+    fun `with CORS plugin`() = testApplication {
+        val testRegistry = MetricRegistry()
+        install(DropwizardMetrics) {
+            registry = testRegistry
+        }
+        install(CORS) {
+            anyHost()
+        }
+
+        routing {
+            get("/") {
+                call.respond("Hello, World")
+            }
+        }
+
+        val response = client.options("") {
+            header("Access-Control-Request-Method", "GET")
+            header("Origin", "https://ktor.io")
+        }
+
+        assertEquals(HttpStatusCode.OK, response.status)
+        assertEquals(1, testRegistry.meter("ktor.calls./(method:OPTIONS).200").count)
+    }
 }