From c6f5961ad660c9d24500a64d747b936020bef80a Mon Sep 17 00:00:00 2001 From: Taylor Wicksell Date: Mon, 4 Aug 2014 16:23:36 -0700 Subject: [PATCH 1/3] fix for gh-1331 MetricFilter now supports uri template variables when available --- .../MetricFilterAutoConfiguration.java | 5 ++ .../MetricFilterAutoConfigurationTests.java | 51 ++++++++++++++++--- 2 files changed, 48 insertions(+), 8 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 f74978ddd2..84f20c08e3 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 @@ -39,6 +39,7 @@ import org.springframework.core.Ordered; import org.springframework.core.annotation.Order; import org.springframework.util.StopWatch; import org.springframework.web.filter.OncePerRequestFilter; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.util.UrlPathHelper; /** @@ -86,6 +87,10 @@ public class MetricFilterAutoConfiguration { } finally { stopWatch.stop(); + if(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) != null) + { + suffix = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE).toString().replaceAll("[{}]", "-"); + } String gaugeKey = getKey("response" + suffix); MetricFilterAutoConfiguration.this.gaugeService.submit(gaugeKey, stopWatch.getTotalTimeMillis()); 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 0f396291d8..dcd96e6971 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 @@ -16,6 +16,16 @@ package org.springframework.boot.actuate.autoconfigure; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.willAnswer; +import static org.mockito.Matchers.anyDouble; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -29,14 +39,10 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; - -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertThat; -import static org.mockito.BDDMockito.willAnswer; -import static org.mockito.Matchers.anyDouble; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; /** * Tests for {@link MetricFilterAutoConfiguration}. @@ -44,6 +50,7 @@ import static org.mockito.Mockito.verify; * @author Phillip Webb */ public class MetricFilterAutoConfigurationTests { + @Test public void recordsHttpInteractions() throws Exception { @@ -67,6 +74,21 @@ public class MetricFilterAutoConfigurationTests { anyDouble()); context.close(); } + + @Test + public void recordsHttpInteractionsWithTemplateVariable() 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(); + 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(); + } @Test public void skipsFilterIfMissingServices() throws Exception { @@ -88,6 +110,19 @@ public class MetricFilterAutoConfigurationTests { public GaugeService gaugeService() { return mock(GaugeService.class); } + } } + + +@RestController +class MetricFilterTestController +{ + + @RequestMapping("templateVarTest/{someVariable}") + public String testTemplateVariableResolution(String someVariable) + { + return someVariable; + } +} \ No newline at end of file From 733b22f46aa741175192009a98c367e1ecdd8f75 Mon Sep 17 00:00:00 2001 From: Taylor Wicksell Date: Mon, 4 Aug 2014 18:38:53 -0700 Subject: [PATCH 2/3] gh-1331 defaulting 404 response metrics to "unkownPath" when no matching handler can be found Fixes gh-1331, Fixes gh-1333 --- .../MetricFilterAutoConfiguration.java | 6 +++ .../MetricFilterAutoConfigurationTests.java | 48 ++++++++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) 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 84f20c08e3..8d4be258a8 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 @@ -37,6 +37,7 @@ 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; @@ -56,6 +57,7 @@ import org.springframework.web.util.UrlPathHelper; public class MetricFilterAutoConfiguration { private static final int UNDEFINED_HTTP_STATUS = 999; + private static final String UNKNOWN_PATH_SUFFIX = "/unknownPath"; @Autowired private CounterService counterService; @@ -91,6 +93,10 @@ public class MetricFilterAutoConfiguration { { suffix = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE).toString().replaceAll("[{}]", "-"); } + else if(getStatus(response) == HttpStatus.NOT_FOUND.value()) + { + suffix=UNKNOWN_PATH_SUFFIX; + } String gaugeKey = getKey("response" + suffix); MetricFilterAutoConfiguration.this.gaugeService.submit(gaugeKey, stopWatch.getTotalTimeMillis()); 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 dcd96e6971..c7de70d7e0 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 @@ -22,6 +22,7 @@ import static org.mockito.BDDMockito.willAnswer; import static org.mockito.Matchers.anyDouble; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -37,11 +38,15 @@ import org.springframework.boot.actuate.metrics.GaugeService; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.http.HttpStatus; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.web.bind.annotation.PathVariable; 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; /** @@ -89,6 +94,39 @@ public class MetricFilterAutoConfigurationTests { anyDouble()); context.close(); } + + @Test + public void recordsKnown404HttpInteractionsAsSingleMetricWithPathAndTemplateVariable() 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(); + mvc.perform(get("/knownPath/foo")) + .andExpect(status().isNotFound()); + + verify(context.getBean(CounterService.class)).increment("status.404.knownPath.-someVariable-"); + verify(context.getBean(GaugeService.class)).submit(eq("response.knownPath.-someVariable-"), + anyDouble()); + context.close(); + } + + @Test + public void records404HttpInteractionsAsSingleMetric() 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(); + 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.unknownPath"); + verify(context.getBean(GaugeService.class), times(2)).submit(eq("response.unknownPath"), + anyDouble()); + context.close(); + } @Test public void skipsFilterIfMissingServices() throws Exception { @@ -121,7 +159,15 @@ class MetricFilterTestController { @RequestMapping("templateVarTest/{someVariable}") - public String testTemplateVariableResolution(String 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; } From 1259057acbf14f2fd9e81a072b7c9302cb92b848 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 5 Aug 2014 07:56:02 -0700 Subject: [PATCH 3/3] Change unknownPath to unmapped --- .../MetricFilterAutoConfiguration.java | 17 ++-- .../MetricFilterAutoConfigurationTests.java | 98 +++++++++---------- 2 files changed, 57 insertions(+), 58 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 8d4be258a8..4f2f6edffa 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 @@ -57,7 +57,7 @@ import org.springframework.web.util.UrlPathHelper; public class MetricFilterAutoConfiguration { private static final int UNDEFINED_HTTP_STATUS = 999; - private static final String UNKNOWN_PATH_SUFFIX = "/unknownPath"; + private static final String UNKNOWN_PATH_SUFFIX = "/unmapped"; @Autowired private CounterService counterService; @@ -89,18 +89,19 @@ public class MetricFilterAutoConfiguration { } finally { stopWatch.stop(); - if(request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) != null) - { - suffix = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE).toString().replaceAll("[{}]", "-"); + int status = getStatus(response); + if (request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) != null) { + suffix = request + .getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE) + .toString().replaceAll("[{}]", "-"); } - else if(getStatus(response) == HttpStatus.NOT_FOUND.value()) - { - suffix=UNKNOWN_PATH_SUFFIX; + else if (HttpStatus.valueOf(status).is4xxClientError()) { + suffix = UNKNOWN_PATH_SUFFIX; } String gaugeKey = getKey("response" + suffix); MetricFilterAutoConfiguration.this.gaugeService.submit(gaugeKey, stopWatch.getTotalTimeMillis()); - String counterKey = getKey("status." + getStatus(response) + suffix); + String counterKey = getKey("status." + status + suffix); MetricFilterAutoConfiguration.this.counterService.increment(counterKey); } } 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 c7de70d7e0..5f2ec79bba 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 @@ -16,17 +16,6 @@ package org.springframework.boot.actuate.autoconfigure; -import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertThat; -import static org.mockito.BDDMockito.willAnswer; -import static org.mockito.Matchers.anyDouble; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; - import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -49,13 +38,23 @@ import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestController; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.willAnswer; +import static org.mockito.Matchers.anyDouble; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + /** * Tests for {@link MetricFilterAutoConfiguration}. * * @author Phillip Webb */ public class MetricFilterAutoConfigurationTests { - @Test public void recordsHttpInteractions() throws Exception { @@ -79,52 +78,55 @@ public class MetricFilterAutoConfigurationTests { anyDouble()); context.close(); } - + @Test public void recordsHttpInteractionsWithTemplateVariable() 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(); - 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()); + 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(); } - + @Test - public void recordsKnown404HttpInteractionsAsSingleMetricWithPathAndTemplateVariable() throws Exception { + public void recordsKnown404HttpInteractionsAsSingleMetricWithPathAndTemplateVariable() + 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(); - mvc.perform(get("/knownPath/foo")) - .andExpect(status().isNotFound()); - - verify(context.getBean(CounterService.class)).increment("status.404.knownPath.-someVariable-"); - verify(context.getBean(GaugeService.class)).submit(eq("response.knownPath.-someVariable-"), - anyDouble()); + 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( + eq("response.knownPath.-someVariable-"), anyDouble()); context.close(); } - + @Test public void records404HttpInteractionsAsSingleMetric() 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(); - 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.unknownPath"); - verify(context.getBean(GaugeService.class), times(2)).submit(eq("response.unknownPath"), - anyDouble()); + 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( + eq("response.unmapped"), anyDouble()); context.close(); } @@ -148,27 +150,23 @@ public class MetricFilterAutoConfigurationTests { public GaugeService gaugeService() { return mock(GaugeService.class); } - + } } - @RestController -class MetricFilterTestController -{ - +class MetricFilterTestController { + @RequestMapping("templateVarTest/{someVariable}") - public String testTemplateVariableResolution(@PathVariable String someVariable) - { + public String testTemplateVariableResolution(@PathVariable String someVariable) { return someVariable; } - + @RequestMapping("knownPath/{someVariable}") @ResponseStatus(HttpStatus.NOT_FOUND) @ResponseBody - public String testKnownPathWith404Response(@PathVariable String someVariable) - { + public String testKnownPathWith404Response(@PathVariable String someVariable) { return someVariable; } } \ No newline at end of file