From e60194c7d558c2844e7970c1947299ee7320727c Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Mon, 13 Jan 2020 13:31:16 -0800 Subject: [PATCH] Ignore trailing slash when recording Web metrics Fixes gh-18207 --- .../metrics/MetricsProperties.java | 13 ++++++++++ .../WebFluxMetricsAutoConfiguration.java | 3 ++- .../WebMvcMetricsAutoConfiguration.java | 2 +- .../WebFluxMetricsAutoConfigurationTests.java | 14 +++++++++++ .../WebMvcMetricsAutoConfigurationTests.java | 14 +++++++++++ .../server/DefaultWebFluxTagsProvider.java | 14 +++++++++-- .../web/reactive/server/WebFluxTags.java | 24 ++++++++++++++++++- .../servlet/DefaultWebMvcTagsProvider.java | 16 ++++++++++--- .../metrics/web/servlet/WebMvcTags.java | 18 ++++++++++++++ .../server/MetricsWebFilterTests.java | 16 ++++++++++++- .../web/servlet/WebMvcMetricsFilterTests.java | 15 +++++++++++- 11 files changed, 139 insertions(+), 10 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java index de701cd6bd..42ebaf8cea 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/MetricsProperties.java @@ -243,6 +243,11 @@ public class MetricsProperties { */ private String metricName = "http.server.requests"; + /** + * Whether the trailing slash should be ignored when recording metrics. + */ + private boolean ignoreTrailingSlash = true; + /** * Auto-timed request settings. */ @@ -261,6 +266,14 @@ public class MetricsProperties { this.metricName = metricName; } + public boolean isIgnoreTrailingSlash() { + return this.ignoreTrailingSlash; + } + + public void setIgnoreTrailingSlash(boolean ignoreTrailingSlash) { + this.ignoreTrailingSlash = ignoreTrailingSlash; + } + } } diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfiguration.java index 670e896d16..6ae6883130 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfiguration.java @@ -59,7 +59,8 @@ public class WebFluxMetricsAutoConfiguration { @Bean @ConditionalOnMissingBean(WebFluxTagsProvider.class) public DefaultWebFluxTagsProvider webfluxTagConfigurer() { - return new DefaultWebFluxTagsProvider(); + return new DefaultWebFluxTagsProvider( + this.properties.getWeb().getServer().getRequest().isIgnoreTrailingSlash()); } @Bean diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfiguration.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfiguration.java index 7b81bf8972..03267c0ba0 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfiguration.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfiguration.java @@ -71,7 +71,7 @@ public class WebMvcMetricsAutoConfiguration { @Bean @ConditionalOnMissingBean(WebMvcTagsProvider.class) public DefaultWebMvcTagsProvider webMvcTagsProvider() { - return new DefaultWebMvcTagsProvider(); + return new DefaultWebMvcTagsProvider(this.properties.getWeb().getServer().getRequest().isIgnoreTrailingSlash()); } @Bean diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfigurationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfigurationTests.java index c5bfe12c5e..3824e2391b 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/reactive/WebFluxMetricsAutoConfigurationTests.java @@ -33,6 +33,7 @@ import org.springframework.boot.test.system.CapturedOutput; import org.springframework.boot.test.system.OutputCaptureExtension; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.test.web.reactive.server.WebTestClient; import static org.assertj.core.api.Assertions.assertThat; @@ -43,6 +44,7 @@ import static org.mockito.Mockito.mock; * * @author Brian Clozel * @author Dmytro Nosan + * @author Madhura Bhave */ @ExtendWith(OutputCaptureExtension.class) class WebFluxMetricsAutoConfigurationTests { @@ -55,9 +57,21 @@ class WebFluxMetricsAutoConfigurationTests { this.contextRunner.run((context) -> { assertThat(context).getBeans(MetricsWebFilter.class).hasSize(1); assertThat(context).getBeans(DefaultWebFluxTagsProvider.class).hasSize(1); + assertThat(ReflectionTestUtils.getField(context.getBean(DefaultWebFluxTagsProvider.class), + "ignoreTrailingSlash")).isEqualTo(true); }); } + @Test + void tagsProviderWhenIgnoreTrailingSlashIsFalse() { + this.contextRunner.withPropertyValues("management.metrics.web.server.request.ignore-trailing-slash=false") + .run((context) -> { + assertThat(context).hasSingleBean(DefaultWebFluxTagsProvider.class); + assertThat(ReflectionTestUtils.getField(context.getBean(DefaultWebFluxTagsProvider.class), + "ignoreTrailingSlash")).isEqualTo(false); + }); + } + @Test void shouldNotOverrideCustomTagsProvider() { this.contextRunner.withUserConfiguration(CustomWebFluxTagsProviderConfig.class) diff --git a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfigurationTests.java b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfigurationTests.java index 5b570ee300..141ab8ea1f 100644 --- a/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfigurationTests.java +++ b/spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/metrics/web/servlet/WebMvcMetricsAutoConfigurationTests.java @@ -48,6 +48,7 @@ import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.Ordered; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; import org.springframework.test.web.servlet.setup.MockMvcBuilders; @@ -62,6 +63,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers. * @author Andy Wilkinson * @author Dmytro Nosan * @author Tadaya Tsuyukubo + * @author Madhura Bhave */ @ExtendWith(OutputCaptureExtension.class) class WebMvcMetricsAutoConfigurationTests { @@ -79,12 +81,24 @@ class WebMvcMetricsAutoConfigurationTests { void definesTagsProviderAndFilterWhenMeterRegistryIsPresent() { this.contextRunner.run((context) -> { assertThat(context).hasSingleBean(DefaultWebMvcTagsProvider.class); + assertThat(ReflectionTestUtils.getField(context.getBean(DefaultWebMvcTagsProvider.class), + "ignoreTrailingSlash")).isEqualTo(true); assertThat(context).hasSingleBean(FilterRegistrationBean.class); assertThat(context.getBean(FilterRegistrationBean.class).getFilter()) .isInstanceOf(WebMvcMetricsFilter.class); }); } + @Test + void tagsProviderWhenIgnoreTrailingSlashIsFalse() { + this.contextRunner.withPropertyValues("management.metrics.web.server.request.ignore-trailing-slash=false") + .run((context) -> { + assertThat(context).hasSingleBean(DefaultWebMvcTagsProvider.class); + assertThat(ReflectionTestUtils.getField(context.getBean(DefaultWebMvcTagsProvider.class), + "ignoreTrailingSlash")).isEqualTo(false); + }); + } + @Test void tagsProviderBacksOff() { this.contextRunner.withUserConfiguration(TagsProviderConfiguration.class).run((context) -> { diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/DefaultWebFluxTagsProvider.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/DefaultWebFluxTagsProvider.java index fb54c496b9..e1d125ef85 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/DefaultWebFluxTagsProvider.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/DefaultWebFluxTagsProvider.java @@ -31,10 +31,20 @@ import org.springframework.web.server.ServerWebExchange; */ public class DefaultWebFluxTagsProvider implements WebFluxTagsProvider { + private final boolean ignoreTrailingSlash; + + public DefaultWebFluxTagsProvider() { + this(false); + } + + public DefaultWebFluxTagsProvider(boolean ignoreTrailingSlash) { + this.ignoreTrailingSlash = ignoreTrailingSlash; + } + @Override public Iterable httpRequestTags(ServerWebExchange exchange, Throwable exception) { - return Arrays.asList(WebFluxTags.method(exchange), WebFluxTags.uri(exchange), WebFluxTags.exception(exception), - WebFluxTags.status(exchange), WebFluxTags.outcome(exchange)); + return Arrays.asList(WebFluxTags.method(exchange), WebFluxTags.uri(exchange, this.ignoreTrailingSlash), + WebFluxTags.exception(exception), WebFluxTags.status(exchange), WebFluxTags.outcome(exchange)); } } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java index 764c069c34..1266630a18 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/reactive/server/WebFluxTags.java @@ -16,6 +16,8 @@ package org.springframework.boot.actuate.metrics.web.reactive.server; +import java.util.regex.Pattern; + import io.micrometer.core.instrument.Tag; import org.springframework.boot.actuate.metrics.http.Outcome; @@ -48,6 +50,8 @@ public final class WebFluxTags { private static final Tag EXCEPTION_NONE = Tag.of("exception", "None"); + private static final Pattern TRAILING_SLASH_PATTERN = Pattern.compile("/$"); + private WebFluxTags() { } @@ -87,9 +91,27 @@ public final class WebFluxTags { * @return the uri tag derived from the exchange */ public static Tag uri(ServerWebExchange exchange) { + return uri(exchange, false); + } + + /** + * Creates a {@code uri} tag based on the URI of the given {@code exchange}. Uses the + * {@link HandlerMapping#BEST_MATCHING_PATTERN_ATTRIBUTE} best matching pattern if + * available. Falling back to {@code REDIRECTION} for 3xx responses, {@code NOT_FOUND} + * for 404 responses, {@code root} for requests with no path info, and {@code UNKNOWN} + * for all other requests. + * @param exchange the exchange + * @param ignoreTrailingSlash whether to ignore the trailing slash + * @return the uri tag derived from the exchange + */ + public static Tag uri(ServerWebExchange exchange, boolean ignoreTrailingSlash) { PathPattern pathPattern = exchange.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (pathPattern != null) { - return Tag.of("uri", pathPattern.getPatternString()); + String patternString = pathPattern.getPatternString(); + if (ignoreTrailingSlash) { + patternString = TRAILING_SLASH_PATTERN.matcher(patternString).replaceAll(""); + } + return Tag.of("uri", patternString); } HttpStatus status = exchange.getResponse().getStatusCode(); if (status != null) { diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/DefaultWebMvcTagsProvider.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/DefaultWebMvcTagsProvider.java index 6d7ed11d24..6ac3aac758 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/DefaultWebMvcTagsProvider.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/DefaultWebMvcTagsProvider.java @@ -30,16 +30,26 @@ import io.micrometer.core.instrument.Tags; */ public class DefaultWebMvcTagsProvider implements WebMvcTagsProvider { + private final boolean ignoreTrailingSlash; + + public DefaultWebMvcTagsProvider() { + this(false); + } + + public DefaultWebMvcTagsProvider(boolean ignoreTrailingSlash) { + this.ignoreTrailingSlash = ignoreTrailingSlash; + } + @Override public Iterable getTags(HttpServletRequest request, HttpServletResponse response, Object handler, Throwable exception) { - return Tags.of(WebMvcTags.method(request), WebMvcTags.uri(request, response), WebMvcTags.exception(exception), - WebMvcTags.status(response), WebMvcTags.outcome(response)); + return Tags.of(WebMvcTags.method(request), WebMvcTags.uri(request, response, this.ignoreTrailingSlash), + WebMvcTags.exception(exception), WebMvcTags.status(response), WebMvcTags.outcome(response)); } @Override public Iterable getLongRequestTags(HttpServletRequest request, Object handler) { - return Tags.of(WebMvcTags.method(request), WebMvcTags.uri(request, null)); + return Tags.of(WebMvcTags.method(request), WebMvcTags.uri(request, null, this.ignoreTrailingSlash)); } } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java index 78c4c0b970..6f0702f061 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcTags.java @@ -94,9 +94,27 @@ public final class WebMvcTags { * @return the uri tag derived from the request */ public static Tag uri(HttpServletRequest request, HttpServletResponse response) { + return uri(request, response, false); + } + + /** + * Creates a {@code uri} tag based on the URI of the given {@code request}. Uses the + * {@link HandlerMapping#BEST_MATCHING_PATTERN_ATTRIBUTE} best matching pattern if + * available. Falling back to {@code REDIRECTION} for 3xx responses, {@code NOT_FOUND} + * for 404 responses, {@code root} for requests with no path info, and {@code UNKNOWN} + * for all other requests. + * @param request the request + * @param response the response + * @param ignoreTrailingSlash whether to ignore the trailing slash + * @return the uri tag derived from the request + */ + public static Tag uri(HttpServletRequest request, HttpServletResponse response, boolean ignoreTrailingSlash) { if (request != null) { String pattern = getMatchingPattern(request); if (pattern != null) { + if (ignoreTrailingSlash) { + pattern = TRAILING_SLASH_PATTERN.matcher(pattern).replaceAll(""); + } return Tag.of("uri", pattern); } if (response != null) { diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java index 33f1e14126..2485044c2a 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/reactive/server/MetricsWebFilterTests.java @@ -37,6 +37,7 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link MetricsWebFilter} * * @author Brian Clozel + * @author Madhura Bhave */ class MetricsWebFilterTests { @@ -50,7 +51,7 @@ class MetricsWebFilterTests { void setup() { MockClock clock = new MockClock(); this.registry = new SimpleMeterRegistry(SimpleConfig.DEFAULT, clock); - this.webFilter = new MetricsWebFilter(this.registry, new DefaultWebFluxTagsProvider(), REQUEST_METRICS_NAME, + this.webFilter = new MetricsWebFilter(this.registry, new DefaultWebFluxTagsProvider(true), REQUEST_METRICS_NAME, AutoTimer.ENABLED); } @@ -102,6 +103,19 @@ class MetricsWebFilterTests { assertMetricsContainsTag("status", "500"); } + @Test + void trailingSlashShouldNotRecordDuplicateMetrics() { + MockServerWebExchange exchange1 = createExchange("/projects/spring-boot", "/projects/{project}"); + MockServerWebExchange exchange2 = createExchange("/projects/spring-boot", "/projects/{project}/"); + this.webFilter.filter(exchange1, (serverWebExchange) -> exchange1.getResponse().setComplete()) + .block(Duration.ofSeconds(30)); + this.webFilter.filter(exchange2, (serverWebExchange) -> exchange2.getResponse().setComplete()) + .block(Duration.ofSeconds(30)); + assertThat(this.registry.get(REQUEST_METRICS_NAME).tag("uri", "/projects/{project}").timer().count()) + .isEqualTo(2); + assertThat(this.registry.get(REQUEST_METRICS_NAME).tag("status", "200").timer().count()).isEqualTo(2); + } + private MockServerWebExchange createExchange(String path, String pathPattern) { PathPatternParser parser = new PathPatternParser(); MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get(path).build()); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java index 438ab65452..bbd02a0336 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/web/servlet/WebMvcMetricsFilterTests.java @@ -289,6 +289,14 @@ class WebMvcMetricsFilterTests { assertThat(this.prometheusRegistry.scrape()).contains("le=\"30.0\""); } + @Test + void trailingSlashShouldNotRecordDuplicateMetrics() throws Exception { + this.mvc.perform(get("/api/c1/simple/10")).andExpect(status().isOk()); + this.mvc.perform(get("/api/c1/simple/10/")).andExpect(status().isOk()); + assertThat(this.registry.get("http.server.requests").tags("status", "200", "uri", "/api/c1/simple/{id}").timer() + .count()).isEqualTo(2); + } + @Target({ ElementType.METHOD }) @Retention(RetentionPolicy.RUNTIME) @Timed(percentiles = 0.95) @@ -356,7 +364,7 @@ class WebMvcMetricsFilterTests { @Bean WebMvcMetricsFilter webMetricsFilter(MeterRegistry registry, WebApplicationContext ctx) { - return new WebMvcMetricsFilter(registry, new DefaultWebMvcTagsProvider(), "http.server.requests", + return new WebMvcMetricsFilter(registry, new DefaultWebMvcTagsProvider(true), "http.server.requests", AutoTimer.ENABLED); } @@ -380,6 +388,11 @@ class WebMvcMetricsFilterTests { return id.toString(); } + @GetMapping("/simple/{id}") + String simpleMapping(@PathVariable Long id) { + return id.toString(); + } + @Timed @Timed(value = "my.long.request", extraTags = { "region", "test" }, longTask = true) @GetMapping("/callable/{id}")