From df0ba5c03a77e46403d682811584132a79da0e9d Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 2 Jun 2015 14:30:59 -0700 Subject: [PATCH] Guard against metric failures in MetricsFilter Update MetricsFilter so that failures to record metrics are logged and ignored. Fixes gh-2777 --- .../MetricFilterAutoConfiguration.java | 102 +---------- .../actuate/autoconfigure/MetricsFilter.java | 160 ++++++++++++++++++ .../MetricFilterAutoConfigurationTests.java | 60 ++++--- 3 files changed, 198 insertions(+), 124 deletions(-) create mode 100644 spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java 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 e29e6e7f82..01133efdbe 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 @@ -16,15 +16,9 @@ package org.springframework.boot.actuate.autoconfigure; -import java.io.IOException; - import javax.servlet.Filter; -import javax.servlet.FilterChain; import javax.servlet.Servlet; -import javax.servlet.ServletException; import javax.servlet.ServletRegistration; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.metrics.CounterService; @@ -35,13 +29,8 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.core.Ordered; -import org.springframework.core.annotation.Order; -import org.springframework.http.HttpStatus; -import org.springframework.util.StopWatch; import org.springframework.web.filter.OncePerRequestFilter; import org.springframework.web.servlet.HandlerMapping; -import org.springframework.web.util.UrlPathHelper; /** * {@link EnableAutoConfiguration Auto-configuration} that records Servlet interactions @@ -58,10 +47,6 @@ import org.springframework.web.util.UrlPathHelper; @AutoConfigureAfter(MetricRepositoryAutoConfiguration.class) public class MetricFilterAutoConfiguration { - private static final int UNDEFINED_HTTP_STATUS = 999; - - private static final String UNKNOWN_PATH_SUFFIX = "/unmapped"; - @Autowired private CounterService counterService; @@ -70,91 +55,6 @@ public class MetricFilterAutoConfiguration { @Bean public Filter metricFilter() { - return new MetricsFilter(); + return new MetricsFilter(this.counterService, this.gaugeService); } - - /** - * Filter that counts requests and measures processing times. - */ - @Order(Ordered.HIGHEST_PRECEDENCE) - private final class MetricsFilter extends OncePerRequestFilter { - - @Override - protected void doFilterInternal(HttpServletRequest request, - HttpServletResponse response, FilterChain chain) throws ServletException, - IOException { - UrlPathHelper helper = new UrlPathHelper(); - 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(); - Object bestMatchingPattern = request - .getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); - if (bestMatchingPattern != null) { - suffix = fixSpecialCharacters(bestMatchingPattern.toString()); - } - else if (is4xxClientError(status)) { - suffix = UNKNOWN_PATH_SUFFIX; - } - String gaugeKey = getKey("response" + suffix); - MetricFilterAutoConfiguration.this.gaugeService.submit(gaugeKey, - stopWatch.getTotalTimeMillis()); - String counterKey = getKey("status." + status + suffix); - MetricFilterAutoConfiguration.this.counterService.increment(counterKey); - } - } - - private String fixSpecialCharacters(String value) { - String result = value.replaceAll("[{}]", "-"); - result = result.replace("**", "-star-star-"); - result = result.replace("*", "-star-"); - result = result.replace("/-", "/"); - result = result.replace("-/", "/"); - if (result.endsWith("-")) { - result = result.substring(0, result.length() - 1); - } - if (result.startsWith("-")) { - result = result.substring(1); - } - return result; - } - - private int getStatus(HttpServletResponse response) { - try { - return response.getStatus(); - } - catch (Exception ex) { - return UNDEFINED_HTTP_STATUS; - } - } - - private boolean is4xxClientError(int status) { - try { - return HttpStatus.valueOf(status).is4xxClientError(); - } - catch (Exception ex) { - return false; - } - } - - private String getKey(String string) { - // graphite compatible metric names - String value = string.replace("/", "."); - value = value.replace("..", "."); - if (value.endsWith(".")) { - value = value + "root"; - } - if (value.startsWith("_")) { - value = value.substring(1); - } - return value; - } - } - } 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 new file mode 100644 index 0000000000..d006be6c58 --- /dev/null +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/autoconfigure/MetricsFilter.java @@ -0,0 +1,160 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.actuate.autoconfigure; + +import java.io.IOException; + +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.springframework.boot.actuate.metrics.CounterService; +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.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. + */ +@Order(Ordered.HIGHEST_PRECEDENCE) +final class MetricsFilter extends OncePerRequestFilter { + + private static final int UNDEFINED_HTTP_STATUS = 999; + + private static final String UNKNOWN_PATH_SUFFIX = "/unmapped"; + + private static final Log logger = LogFactory.getLog(MetricsFilter.class); + + private final CounterService counterService; + + private final GaugeService gaugeService; + + public MetricsFilter(CounterService counterService, GaugeService gaugeService) { + this.counterService = counterService; + this.gaugeService = gaugeService; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, + HttpServletResponse response, FilterChain chain) throws ServletException, + IOException { + StopWatch stopWatch = new StopWatch(); + stopWatch.start(); + String path = new UrlPathHelper().getPathWithinApplication(request); + int status = HttpStatus.INTERNAL_SERVER_ERROR.value(); + try { + chain.doFilter(request, response); + status = getStatus(response); + } + finally { + stopWatch.stop(); + recordMetrics(request, path, status, stopWatch.getTotalTimeMillis()); + } + } + + private int getStatus(HttpServletResponse response) { + try { + return response.getStatus(); + } + catch (Exception ex) { + return UNDEFINED_HTTP_STATUS; + } + } + + private void recordMetrics(HttpServletRequest request, String path, int status, + long time) { + String suffix = getFinalStatus(request, path, status); + submitToGauge(getKey("response" + suffix), time); + incrementCounter(getKey("status." + status + suffix)); + } + + private String getFinalStatus(HttpServletRequest request, String path, int status) { + Object bestMatchingPattern = request + .getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE); + if (bestMatchingPattern != null) { + return fixSpecialCharacters(bestMatchingPattern.toString()); + } + if (is4xxClientError(status)) { + return UNKNOWN_PATH_SUFFIX; + } + return path; + } + + private String fixSpecialCharacters(String value) { + String result = value.replaceAll("[{}]", "-"); + result = result.replace("**", "-star-star-"); + result = result.replace("*", "-star-"); + result = result.replace("/-", "/"); + result = result.replace("-/", "/"); + if (result.endsWith("-")) { + result = result.substring(0, result.length() - 1); + } + if (result.startsWith("-")) { + result = result.substring(1); + } + return result; + } + + private boolean is4xxClientError(int status) { + try { + return HttpStatus.valueOf(status).is4xxClientError(); + } + catch (Exception ex) { + return false; + } + } + + private String getKey(String string) { + // graphite compatible metric names + String value = string.replace("/", "."); + value = value.replace("..", "."); + if (value.endsWith(".")) { + value = value + "root"; + } + if (value.startsWith("_")) { + value = value.substring(1); + } + return value; + } + + private void submitToGauge(String metricName, double value) { + try { + this.gaugeService.submit(metricName, value); + } + catch (Exception ex) { + logger.warn("Unable to submit gauge metric '" + metricName + "'", ex); + } + } + + private void incrementCounter(String metricName) { + try { + this.counterService.increment(metricName); + } + catch (Exception ex) { + logger.warn("Unable to submit counter metric '" + metricName + "'", ex); + } + } + +} 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 dee825a2b4..09742c9214 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 @@ -42,7 +42,9 @@ import org.springframework.web.util.NestedServletException; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.BDDMockito.willAnswer; +import static org.mockito.BDDMockito.willThrow; import static org.mockito.Matchers.anyDouble; +import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -89,7 +91,6 @@ public class MetricFilterAutoConfigurationTests { MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) .addFilter(filter).build(); mvc.perform(get("/templateVarTest/foo")).andExpect(status().isOk()); - verify(context.getBean(CounterService.class)).increment( "status.200.templateVarTest.someVariable"); verify(context.getBean(GaugeService.class)).submit( @@ -106,7 +107,6 @@ public class MetricFilterAutoConfigurationTests { MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) .addFilter(filter).build(); mvc.perform(get("/knownPath/foo")).andExpect(status().isNotFound()); - verify(context.getBean(CounterService.class)).increment( "status.404.knownPath.someVariable"); verify(context.getBean(GaugeService.class)).submit( @@ -122,9 +122,7 @@ public class MetricFilterAutoConfigurationTests { MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) .addFilter(filter).build(); mvc.perform(get("/unknownPath/1")).andExpect(status().isNotFound()); - mvc.perform(get("/unknownPath/2")).andExpect(status().isNotFound()); - verify(context.getBean(CounterService.class), times(2)).increment( "status.404.unmapped"); verify(context.getBean(GaugeService.class), times(2)).submit( @@ -147,7 +145,6 @@ public class MetricFilterAutoConfigurationTests { Filter filter = context.getBean(Filter.class); MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) .addFilter(filter).build(); - try { mvc.perform(get("/unhandledException")).andExpect( status().isInternalServerError()); @@ -155,7 +152,6 @@ public class MetricFilterAutoConfigurationTests { catch (NestedServletException ex) { // Expected } - verify(context.getBean(CounterService.class)).increment( "status.500.unhandledException"); verify(context.getBean(GaugeService.class)).submit( @@ -163,6 +159,24 @@ public class MetricFilterAutoConfigurationTests { context.close(); } + @Test + public void gaugeServiceThatThrows() throws Exception { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + Config.class, MetricFilterAutoConfiguration.class); + GaugeService gaugeService = context.getBean(GaugeService.class); + willThrow(new IllegalStateException()).given(gaugeService).submit(anyString(), + anyDouble()); + Filter filter = context.getBean(Filter.class); + MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) + .addFilter(filter).build(); + mvc.perform(get("/templateVarTest/foo")).andExpect(status().isOk()); + verify(context.getBean(CounterService.class)).increment( + "status.200.templateVarTest.someVariable"); + verify(context.getBean(GaugeService.class)).submit( + eq("response.templateVarTest.someVariable"), anyDouble()); + context.close(); + } + @Configuration public static class Config { @@ -178,26 +192,26 @@ public class MetricFilterAutoConfigurationTests { } -} + @RestController + class MetricFilterTestController { -@RestController -class MetricFilterTestController { + @RequestMapping("templateVarTest/{someVariable}") + public String testTemplateVariableResolution(@PathVariable String someVariable) { + return someVariable; + } - @RequestMapping("templateVarTest/{someVariable}") - public String testTemplateVariableResolution(@PathVariable String someVariable) { - return someVariable; - } + @RequestMapping("knownPath/{someVariable}") + @ResponseStatus(HttpStatus.NOT_FOUND) + @ResponseBody + public String testKnownPathWith404Response(@PathVariable String someVariable) { + return someVariable; + } - @RequestMapping("knownPath/{someVariable}") - @ResponseStatus(HttpStatus.NOT_FOUND) - @ResponseBody - public String testKnownPathWith404Response(@PathVariable String someVariable) { - return someVariable; + @ResponseBody + @RequestMapping("unhandledException") + public String testException() { + throw new RuntimeException(); + } } - @ResponseBody - @RequestMapping("unhandledException") - public String testException() { - throw new RuntimeException(); - } }