From be5868b6727a0d2c7e91a92497a9b1c915bd6453 Mon Sep 17 00:00:00 2001 From: s15r Date: Tue, 7 Nov 2023 14:16:55 +0100 Subject: [PATCH 1/3] prevent unmatched urls to pollute metrics --- .../web/ServerRequestMeterRegistryFilter.java | 5 ++-- .../binder/web/WebMetricsPublisher.java | 1 + .../metrics/binder/web/HttpMetricsSpec.groovy | 23 +++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java index 4f6de5cc7..ffac7f7cc 100644 --- a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java +++ b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java @@ -27,6 +27,7 @@ import java.util.Optional; +import static io.micronaut.configuration.metrics.binder.web.WebMetricsPublisher.URI_NO_ROUTE_MATCH; import static io.micronaut.core.util.StringUtils.FALSE; import static io.micronaut.http.HttpAttributes.URI_TEMPLATE; @@ -34,7 +35,7 @@ * Registers the timers and meters for each request. * *

The default is to intercept all paths /**, but using the - * property micronaut.metrics.http.path, this can be changed.

+ * property micronaut.metrics.http.path, this can be changed.

* * @author Christian Oestreich * @author graemerocher @@ -58,7 +59,7 @@ public ServerRequestMeterRegistryFilter(MeterRegistry meterRegistry) { private String resolvePath(HttpRequest request) { Optional route = request.getAttribute(URI_TEMPLATE, String.class); - return route.orElseGet(request::getPath); + return route.orElse(URI_NO_ROUTE_MATCH); } @Override diff --git a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java index 4972bcea4..1149232e1 100644 --- a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java +++ b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java @@ -58,6 +58,7 @@ public class WebMetricsPublisher> extends Flux { private static final Tag URI_NOT_FOUND = Tag.of("uri", "NOT_FOUND"); private static final Tag URI_REDIRECTION = Tag.of("uri", "REDIRECTION"); + static final String URI_NO_ROUTE_MATCH = "NO_ROUTE_MATCH"; private static final String UNKNOWN = "UNKNOWN"; private static final String METHOD = "method"; private static final String STATUS = "status"; diff --git a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy index 6fd90a4ce..880f77f93 100644 --- a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy +++ b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy @@ -6,10 +6,14 @@ import io.micrometer.core.instrument.Timer import io.micrometer.core.instrument.distribution.HistogramSnapshot import io.micrometer.core.instrument.search.MeterNotFoundException import io.micronaut.context.ApplicationContext +import io.micronaut.http.HttpHeaders import io.micronaut.http.HttpResponse +import io.micronaut.http.MediaType import io.micronaut.http.annotation.Controller import io.micronaut.http.annotation.Error import io.micronaut.http.annotation.Get +import io.micronaut.http.annotation.Header +import io.micronaut.http.annotation.Produces import io.micronaut.http.client.annotation.Client import io.micronaut.http.client.exceptions.HttpClientResponseException import io.micronaut.runtime.server.EmbeddedServer @@ -123,6 +127,16 @@ class HttpMetricsSpec extends Specification { then: noExceptionThrown() + when: "A request is made that does not match a route" + client.noRouteMatchForMediaType() + + then: + thrown(HttpClientResponseException) + registry.get(WebMetricsPublisher.METRIC_HTTP_CLIENT_REQUESTS).tags('uri', '/test-http-metrics-no-route-match').timer() + registry.get(WebMetricsPublisher.METRIC_HTTP_CLIENT_REQUESTS).tags("status", "406").timer() + registry.get(WebMetricsPublisher.METRIC_HTTP_SERVER_REQUESTS).tags('uri', 'NO_ROUTE_MATCH').timer() + registry.get(WebMetricsPublisher.METRIC_HTTP_SERVER_REQUESTS).tags("status", "406").timer() + cleanup: embeddedServer.close() @@ -174,6 +188,10 @@ class HttpMetricsSpec extends Specification { @Get("/test-http-metrics-not-found") HttpResponse notFound() + + @Get("/test-http-metrics-no-route-match") + @Header(name = HttpHeaders.ACCEPT, value = MediaType.TEXT_PLAIN) + HttpResponse noRouteMatchForMediaType() } @Controller('/') @@ -206,6 +224,11 @@ class HttpMetricsSpec extends Specification { HttpResponse myExceptionHandler() { return HttpResponse.badRequest() } + @Get("/test-http-metrics-no-route-match") + @Produces(MediaType.APPLICATION_JSON) + HttpResponse noRouteMatchForMediaType() { + return HttpResponse.ok() + } } @InheritConstructors From 9f6dab989a04c93747d8c5db6a70f837e1a261a6 Mon Sep 17 00:00:00 2001 From: Sergio del Amo Date: Wed, 8 Nov 2023 16:55:05 +0100 Subject: [PATCH 2/3] simpler test --- ...thVariableDoesNotPolluteMetricsSpec.groovy | 91 +++++++++++++++++++ .../metrics/binder/web/HttpMetricsSpec.groovy | 23 ----- 2 files changed, 91 insertions(+), 23 deletions(-) create mode 100644 micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy diff --git a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy new file mode 100644 index 000000000..2a8788752 --- /dev/null +++ b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy @@ -0,0 +1,91 @@ +package io.micronaut.configuration.metrics + +import io.micronaut.context.annotation.Property +import io.micronaut.context.annotation.Requires +import io.micronaut.core.type.Argument +import io.micronaut.http.HttpRequest +import io.micronaut.http.HttpStatus +import io.micronaut.http.MediaType +import io.micronaut.http.annotation.Controller +import io.micronaut.http.annotation.Get +import io.micronaut.http.annotation.PathVariable +import io.micronaut.http.annotation.Status +import io.micronaut.http.client.BlockingHttpClient +import io.micronaut.http.client.HttpClient +import io.micronaut.http.client.annotation.Client +import io.micronaut.http.client.exceptions.HttpClientResponseException +import io.micronaut.test.extensions.spock.annotation.MicronautTest +import jakarta.inject.Inject +import spock.lang.Specification + +@Property(name = "spec.name", value = "RoutePathVariableNotFoundMetricsSpec") +@MicronautTest +class NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec extends Specification { + + @Inject + @Client("/") + HttpClient httpClient + + void 'sending multiple requests to a route with a path variable does not pollute the metrics uri'() { + given: + BlockingHttpClient client = httpClient.toBlocking() + + when: + client.exchange(HttpRequest.GET("/books/1")) + + then: + noExceptionThrown() + + when: + client.exchange(HttpRequest.GET("/books/2")) + + then: + noExceptionThrown() + + when: + List uris = fetchHttpServerRequestMetricsUris(client) + + then: + noExceptionThrown() + uris == ['/books/{id}'] + + when: + client.exchange(HttpRequest.GET("/books/1").accept(MediaType.TEXT_PLAIN)) + + then: + HttpClientResponseException e = thrown() + HttpStatus.NOT_ACCEPTABLE == e.status + + when: + client.exchange(HttpRequest.GET("/books/2").accept(MediaType.TEXT_PLAIN)) + + then: + e = thrown() + HttpStatus.NOT_ACCEPTABLE == e.status + + when: + uris = fetchHttpServerRequestMetricsUris(client) + + then: + noExceptionThrown() + ['NO_ROUTE_MATCH', '/metrics/{name}','/books/{id}'] == uris + } + + private static List fetchHttpServerRequestMetricsUris(BlockingHttpClient client) { + fetchHttpServerRequestMetrics(client)["availableTags"].find { it.tag == "uri" }?.values + } + + private static Map fetchHttpServerRequestMetrics(BlockingHttpClient client) { + client.retrieve(HttpRequest.GET("/metrics/http.server.requests"), Argument.mapOf(String, Object)); + } + + @Requires(property = "spec.name", value = "RoutePathVariableNotFoundMetricsSpec") + @Controller + static class BookController { + @Get("/books/{id}") + @Status(HttpStatus.OK) + void index(@PathVariable String id) { + } + } +} + diff --git a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy index 880f77f93..6fd90a4ce 100644 --- a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy +++ b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/binder/web/HttpMetricsSpec.groovy @@ -6,14 +6,10 @@ import io.micrometer.core.instrument.Timer import io.micrometer.core.instrument.distribution.HistogramSnapshot import io.micrometer.core.instrument.search.MeterNotFoundException import io.micronaut.context.ApplicationContext -import io.micronaut.http.HttpHeaders import io.micronaut.http.HttpResponse -import io.micronaut.http.MediaType import io.micronaut.http.annotation.Controller import io.micronaut.http.annotation.Error import io.micronaut.http.annotation.Get -import io.micronaut.http.annotation.Header -import io.micronaut.http.annotation.Produces import io.micronaut.http.client.annotation.Client import io.micronaut.http.client.exceptions.HttpClientResponseException import io.micronaut.runtime.server.EmbeddedServer @@ -127,16 +123,6 @@ class HttpMetricsSpec extends Specification { then: noExceptionThrown() - when: "A request is made that does not match a route" - client.noRouteMatchForMediaType() - - then: - thrown(HttpClientResponseException) - registry.get(WebMetricsPublisher.METRIC_HTTP_CLIENT_REQUESTS).tags('uri', '/test-http-metrics-no-route-match').timer() - registry.get(WebMetricsPublisher.METRIC_HTTP_CLIENT_REQUESTS).tags("status", "406").timer() - registry.get(WebMetricsPublisher.METRIC_HTTP_SERVER_REQUESTS).tags('uri', 'NO_ROUTE_MATCH').timer() - registry.get(WebMetricsPublisher.METRIC_HTTP_SERVER_REQUESTS).tags("status", "406").timer() - cleanup: embeddedServer.close() @@ -188,10 +174,6 @@ class HttpMetricsSpec extends Specification { @Get("/test-http-metrics-not-found") HttpResponse notFound() - - @Get("/test-http-metrics-no-route-match") - @Header(name = HttpHeaders.ACCEPT, value = MediaType.TEXT_PLAIN) - HttpResponse noRouteMatchForMediaType() } @Controller('/') @@ -224,11 +206,6 @@ class HttpMetricsSpec extends Specification { HttpResponse myExceptionHandler() { return HttpResponse.badRequest() } - @Get("/test-http-metrics-no-route-match") - @Produces(MediaType.APPLICATION_JSON) - HttpResponse noRouteMatchForMediaType() { - return HttpResponse.ok() - } } @InheritConstructors From 776ec9e32f4ed1bd1711c32c1d5d5c4e8c336132 Mon Sep 17 00:00:00 2001 From: s15r Date: Wed, 8 Nov 2023 17:20:24 +0100 Subject: [PATCH 3/3] reuse existing UNKNOWN constant --- .../metrics/binder/web/ServerRequestMeterRegistryFilter.java | 4 ++-- .../configuration/metrics/binder/web/WebMetricsPublisher.java | 3 +-- ...tableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java index ffac7f7cc..4d2fcfc8e 100644 --- a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java +++ b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/ServerRequestMeterRegistryFilter.java @@ -27,7 +27,7 @@ import java.util.Optional; -import static io.micronaut.configuration.metrics.binder.web.WebMetricsPublisher.URI_NO_ROUTE_MATCH; +import static io.micronaut.configuration.metrics.binder.web.WebMetricsPublisher.UNKNOWN; import static io.micronaut.core.util.StringUtils.FALSE; import static io.micronaut.http.HttpAttributes.URI_TEMPLATE; @@ -59,7 +59,7 @@ public ServerRequestMeterRegistryFilter(MeterRegistry meterRegistry) { private String resolvePath(HttpRequest request) { Optional route = request.getAttribute(URI_TEMPLATE, String.class); - return route.orElse(URI_NO_ROUTE_MATCH); + return route.orElse(UNKNOWN); } @Override diff --git a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java index 1149232e1..1c2ec0345 100644 --- a/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java +++ b/micrometer-core/src/main/java/io/micronaut/configuration/metrics/binder/web/WebMetricsPublisher.java @@ -58,8 +58,7 @@ public class WebMetricsPublisher> extends Flux { private static final Tag URI_NOT_FOUND = Tag.of("uri", "NOT_FOUND"); private static final Tag URI_REDIRECTION = Tag.of("uri", "REDIRECTION"); - static final String URI_NO_ROUTE_MATCH = "NO_ROUTE_MATCH"; - private static final String UNKNOWN = "UNKNOWN"; + static final String UNKNOWN = "UNKNOWN"; private static final String METHOD = "method"; private static final String STATUS = "status"; private static final String URI = "uri"; diff --git a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy index 2a8788752..11dbcdbef 100644 --- a/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy +++ b/micrometer-core/src/test/groovy/io/micronaut/configuration/metrics/NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec.groovy @@ -68,7 +68,7 @@ class NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec extends Specif then: noExceptionThrown() - ['NO_ROUTE_MATCH', '/metrics/{name}','/books/{id}'] == uris + ['/metrics/{name}','/books/{id}','UNKNOWN'] == uris } private static List fetchHttpServerRequestMetricsUris(BlockingHttpClient client) { @@ -76,7 +76,7 @@ class NotAcceptableRouteWithPathVariableDoesNotPolluteMetricsSpec extends Specif } private static Map fetchHttpServerRequestMetrics(BlockingHttpClient client) { - client.retrieve(HttpRequest.GET("/metrics/http.server.requests"), Argument.mapOf(String, Object)); + client.retrieve(HttpRequest.GET("/metrics/http.server.requests"), Argument.mapOf(String, Object)) } @Requires(property = "spec.name", value = "RoutePathVariableNotFoundMetricsSpec")