Guard against metric failures in MetricsFilter

Update MetricsFilter so that failures to record metrics are logged and
ignored.

Fixes gh-2777

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;
-		}
-	}
-
 }

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

+/*
+ * 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);
+		}
+	}
+
+}

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();
-	}
 }