Update MetricFilter to treat an unsuccessful call to doFilter as a 500

Previously, if a call to doFilter in MetricFilter failed (i.e. it threw
an exception), it would be handled as if it had a response status of
200. This is because the servlet container was yet to handle the
exception and set the response status to 500.

This commit updates MetricFilter to assume that an exception thrown from
doFilter will result in a response with a status of 500. Strictly
speaking, even though the filter has highest precedence and will
therefore run last on the way back out, this may not always be the case.
For example, a custom Tomcat Valve could handle the exception and result
in a 200 response but that’s an edge case that’s into shooting yourself
in the foot territory.

Closes gh-2818

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2014 the original author or authors.
+ * Copyright 2012-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -49,6 +49,7 @@ import org.springframework.web.util.UrlPathHelper;
  *
  * @author Dave Syer
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 @Configuration
 @ConditionalOnBean({ CounterService.class, GaugeService.class })
@@ -86,25 +87,19 @@ public class MetricFilterAutoConfiguration {
 			String suffix = helper.getPathWithinApplication(request);
 			StopWatch stopWatch = new StopWatch();
 			stopWatch.start();
+			int status = HttpStatus.INTERNAL_SERVER_ERROR.value();
 			try {
 				chain.doFilter(request, response);
+				status = getStatus(response);
 			}
 			finally {
 				stopWatch.stop();
-				int status = getStatus(response);
 				Object bestMatchingPattern = request
 						.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
-				HttpStatus httpStatus = HttpStatus.OK;
-				try {
-					httpStatus = HttpStatus.valueOf(status);
-				}
-				catch (Exception ex) {
-					// not convertible
-				}
 				if (bestMatchingPattern != null) {
 					suffix = fixSpecialCharacters(bestMatchingPattern.toString());
 				}
-				else if (httpStatus.is4xxClientError()) {
+				else if (is4xxClientError(status)) {
 					suffix = UNKNOWN_PATH_SUFFIX;
 				}
 				String gaugeKey = getKey("response" + suffix);
@@ -139,6 +134,17 @@ public class MetricFilterAutoConfiguration {
 			}
 		}
 
+		private boolean is4xxClientError(int status) {
+			HttpStatus httpStatus = HttpStatus.OK;
+			try {
+				httpStatus = HttpStatus.valueOf(status);
+			}
+			catch (Exception ex) {
+				// not convertible
+			}
+			return httpStatus.is4xxClientError();
+		}
+
 		private String getKey(String string) {
 			// graphite compatible metric names
 			String value = string.replace("/", ".");

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2013 the original author or authors.
+ * Copyright 2012-2015 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -37,6 +37,7 @@ import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.ResponseBody;
 import org.springframework.web.bind.annotation.ResponseStatus;
 import org.springframework.web.bind.annotation.RestController;
+import org.springframework.web.util.NestedServletException;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
@@ -53,6 +54,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
  * Tests for {@link MetricFilterAutoConfiguration}.
  *
  * @author Phillip Webb
+ * @author Andy Wilkinson
  */
 public class MetricFilterAutoConfigurationTests {
 
@@ -138,6 +140,29 @@ public class MetricFilterAutoConfigurationTests {
 		context.close();
 	}
 
+	@Test
+	public void controllerMethodThatThrowsUnhandledException() throws Exception {
+		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
+				Config.class, MetricFilterAutoConfiguration.class);
+		Filter filter = context.getBean(Filter.class);
+		MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController())
+				.addFilter(filter).build();
+
+		try {
+			mvc.perform(get("/unhandledException")).andExpect(
+					status().isInternalServerError());
+		}
+		catch (NestedServletException ex) {
+			// Expected
+		}
+
+		verify(context.getBean(CounterService.class)).increment(
+				"status.500.unhandledException");
+		verify(context.getBean(GaugeService.class)).submit(
+				eq("response.unhandledException"), anyDouble());
+		context.close();
+	}
+
 	@Configuration
 	public static class Config {
 
@@ -169,4 +194,10 @@ class MetricFilterTestController {
 	public String testKnownPathWith404Response(@PathVariable String someVariable) {
 		return someVariable;
 	}
+
+	@ResponseBody
+	@RequestMapping("unhandledException")
+	public String testException() {
+		throw new RuntimeException();
+	}
 }