From 0c7708bf9f30c47b8d1cb12af7078bcce3ee3b07 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Tue, 14 Apr 2015 12:24:32 +0100 Subject: [PATCH] Update MetricFilter to treat an unsuccessful call to doFilter as a 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../MetricFilterAutoConfiguration.java | 26 +++++++++------ .../MetricFilterAutoConfigurationTests.java | 33 ++++++++++++++++++- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfiguration.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfiguration.java index 20369a5e527..b7f4c9a3ba5 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricFilterAutoConfiguration.java +++ b/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("/", "."); 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 f1c17785bc9..dee825a2b4c 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 @@ -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(); + } }