From b35e1ad232b5cf030cbb893aa51e92c71c8f135e Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Thu, 21 Jun 2018 14:48:25 +0100 Subject: [PATCH] Avoid unbounded metrics creation for requests not handled by Spring MVC Previously, if an HTTP request that used a templated URI was handled by something other than Spring MVC, a potentially unbounded number of metrics would be created. This happened because, in the absence of Spring MVC's best matching pattern attribute, MetricsFilter would fall back to using the request's path. If the handling route was templated, MetricsFilter would be unaware and would record different metrics for each different path, rather than a single metric for the matching pattern. This cimmit updates MetricsFilter so that it falls back to using unmapped when Spring MVC's best matching pattern attribute is not available. This ensures that an unbounded number of metrics will no longer be created, at the cost of losing specific metrics for requests that are not handled by Spring MVC and that do not use a templated path. Closes gh-5875 --- .../actuate/autoconfigure/MetricsFilter.java | 29 ++++-------------- .../MetricFilterAutoConfigurationTests.java | 30 +++++++++++++++++++ 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java index 5cde52b37b..31c259d47a 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java @@ -35,11 +35,9 @@ import org.springframework.boot.actuate.metrics.GaugeService; import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; import org.springframework.http.HttpStatus; -import org.springframework.http.HttpStatus.Series; import org.springframework.util.StopWatch; import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.servlet.HandlerMapping; -import org.springframework.web.util.UrlPathHelper; /** * Filter that counts requests and measures processing times. @@ -100,7 +98,6 @@ final class MetricsFilter extends OncePerRequestFilter { HttpServletResponse response, FilterChain chain) throws ServletException, IOException { StopWatch stopWatch = createStopWatchIfNecessary(request); - String path = new UrlPathHelper().getPathWithinApplication(request); int status = HttpStatus.INTERNAL_SERVER_ERROR.value(); try { chain.doFilter(request, response); @@ -113,7 +110,7 @@ final class MetricsFilter extends OncePerRequestFilter { } stopWatch.stop(); request.removeAttribute(ATTRIBUTE_STOP_WATCH); - recordMetrics(request, path, status, stopWatch.getTotalTimeMillis()); + recordMetrics(request, status, stopWatch.getTotalTimeMillis()); } } } @@ -137,27 +134,20 @@ final class MetricsFilter extends OncePerRequestFilter { } } - private void recordMetrics(HttpServletRequest request, String path, int status, - long time) { - String suffix = determineMetricNameSuffix(request, path, status); + private void recordMetrics(HttpServletRequest request, int status, long time) { + String suffix = determineMetricNameSuffix(request); submitMetrics(MetricsFilterSubmission.MERGED, request, status, time, suffix); submitMetrics(MetricsFilterSubmission.PER_HTTP_METHOD, request, status, time, suffix); } - private String determineMetricNameSuffix(HttpServletRequest request, String path, - int status) { + private String determineMetricNameSuffix(HttpServletRequest request) { Object bestMatchingPattern = request .getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); if (bestMatchingPattern != null) { return fixSpecialCharacters(bestMatchingPattern.toString()); } - Series series = getSeries(status); - if (Series.CLIENT_ERROR.equals(series) || Series.SERVER_ERROR.equals(series) - || Series.REDIRECTION.equals(series)) { - return UNKNOWN_PATH_SUFFIX; - } - return path; + return UNKNOWN_PATH_SUFFIX; } private String fixSpecialCharacters(String value) { @@ -174,15 +164,6 @@ final class MetricsFilter extends OncePerRequestFilter { return result; } - private Series getSeries(int status) { - try { - return HttpStatus.valueOf(status).series(); - } - catch (Exception ex) { - return null; - } - } - private void submitMetrics(MetricsFilterSubmission submission, HttpServletRequest request, int status, long time, String suffix) { String prefix = ""; diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfigurationTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfigurationTests.java index 36b1f29446..4d09992d9e 100644 --- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfigurationTests.java +++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfigurationTests.java @@ -51,6 +51,7 @@ import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.request.async.DeferredResult; import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.util.NestedServletException; import static org.assertj.core.api.Assertions.assertThat; @@ -98,6 +99,8 @@ public class MetricFilterAutoConfigurationTests { Filter filter = context.getBean(Filter.class); final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/test/path"); + request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, + "/test/path"); final MockHttpServletResponse response = new MockHttpServletResponse(); FilterChain chain = mock(FilterChain.class); willAnswer(new Answer() { @@ -114,6 +117,29 @@ public class MetricFilterAutoConfigurationTests { context.close(); } + @Test + public void usesUnmappedForInterationsWithNoBestMatchingPattern() throws Exception { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + Config.class, MetricFilterAutoConfiguration.class); + Filter filter = context.getBean(Filter.class); + final MockHttpServletRequest request = new MockHttpServletRequest("GET", + "/test/path"); + final MockHttpServletResponse response = new MockHttpServletResponse(); + FilterChain chain = mock(FilterChain.class); + willAnswer(new Answer() { + @Override + public Object answer(InvocationOnMock invocation) throws Throwable { + response.setStatus(200); + return null; + } + }).given(chain).doFilter(request, response); + filter.doFilter(request, response, chain); + verify(context.getBean(CounterService.class)).increment("status.200.unmapped"); + verify(context.getBean(GaugeService.class)).submit(eq("response.unmapped"), + anyDouble()); + context.close(); + } + @Test public void recordsHttpInteractionsWithTemplateVariable() throws Exception { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( @@ -362,6 +388,8 @@ public class MetricFilterAutoConfigurationTests { Filter filter = context.getBean(Filter.class); final MockHttpServletRequest request = new MockHttpServletRequest("PUT", "/test/path"); + request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, + "/test/path"); final MockHttpServletResponse response = new MockHttpServletResponse(); FilterChain chain = mock(FilterChain.class); willAnswer(new Answer() { @@ -419,6 +447,8 @@ public class MetricFilterAutoConfigurationTests { Filter filter = context.getBean(Filter.class); final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/test/path"); + request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, + "/test/path"); final MockHttpServletResponse response = new MockHttpServletResponse(); FilterChain chain = mock(FilterChain.class); willAnswer(new Answer() {