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

Author: wilkinsona

spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java

@@ -35,11 +35,9 @@
 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 @@ protected void doFilterInternal(HttpServletRequest request,
 			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 @@ protected void doFilterInternal(HttpServletRequest request,
 				}
 				stopWatch.stop();
 				request.removeAttribute(ATTRIBUTE_STOP_WATCH);
-				recordMetrics(request, path, status, stopWatch.getTotalTimeMillis());
+				recordMetrics(request, status, stopWatch.getTotalTimeMillis());
 			}
 		}
 	}
@@ -137,27 +134,20 @@ private int getStatus(HttpServletResponse response) {
 		}
 	}
 
-	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 @@ private String fixSpecialCharacters(String value) {
 		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 = "";

spring-boot-actuator/src/test/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfigurationTests.java

@@ -51,6 +51,7 @@
 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 void recordsHttpInteractions() throws Exception {
 		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<Object>() {
@@ -114,6 +117,29 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
 		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<Object>() {
+			@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 void additionallyRecordsMetricsWithHttpMethodNameIfConfigured()
 		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<Object>() {
@@ -419,6 +447,8 @@ public void whenExceptionIsThrownResponseStatusIsUsedWhenResponseHasBeenCommitte
 		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<Object>() {