From d0cf6b534bf55b718ea8bf0f2e311a974f523e1b Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 13 Jul 2015 13:33:30 +0100 Subject: [PATCH 1/4] Add 3xx redirects to the "unmapped" class of requests for metrics When Spring Security sends 302 responses to a login page we don't get any information about the request matching in Spring MVC. Consequently apps can end up with a lot of counter.status.302.* metrics (where "*" can be whatever the user sent). This change treats 3xx the same as 4xx (if it is unmapped it just gets added to a metric called "unmapped" instead of using the actual request path). Fixes gh-2563 --- .../MetricFilterAutoConfiguration.java | 2 + .../actuate/autoconfigure/MetricsFilter.java | 12 ++++ .../MetricFilterAutoConfigurationTests.java | 66 +++++++++++++++---- .../spring-boot-sample-web-secure/pom.xml | 4 ++ 4 files changed, 71 insertions(+), 13 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 01133efdbe..158dd240b4 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 @@ -27,6 +27,7 @@ import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.web.filter.OncePerRequestFilter; @@ -45,6 +46,7 @@ import org.springframework.web.servlet.HandlerMapping; @ConditionalOnClass({ Servlet.class, ServletRegistration.class, OncePerRequestFilter.class, HandlerMapping.class }) @AutoConfigureAfter(MetricRepositoryAutoConfiguration.class) +@ConditionalOnProperty(name="endpoints.metrics.filter.enabled", matchIfMissing=true) public class MetricFilterAutoConfiguration { @Autowired 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 index d006be6c58..b49f25697f 100644 --- 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 @@ -99,6 +99,9 @@ final class MetricsFilter extends OncePerRequestFilter { if (is4xxClientError(status)) { return UNKNOWN_PATH_SUFFIX; } + if (is3xxRedirection(status)) { + return UNKNOWN_PATH_SUFFIX; + } return path; } @@ -126,6 +129,15 @@ final class MetricsFilter extends OncePerRequestFilter { } } + private boolean is3xxRedirection(int status) { + try { + return HttpStatus.valueOf(status).is3xxRedirection(); + } + catch (Exception ex) { + return false; + } + } + 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 09742c9214..ecd1237ed7 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,8 +16,26 @@ 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.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; +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 java.io.IOException; + import javax.servlet.Filter; import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.junit.Test; import org.mockito.invocation.InvocationOnMock; @@ -27,9 +45,11 @@ 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.core.annotation.Order; import org.springframework.http.HttpStatus; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.stereotype.Component; import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import org.springframework.web.bind.annotation.PathVariable; @@ -37,21 +57,9 @@ 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.filter.OncePerRequestFilter; 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; -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}. * @@ -130,6 +138,23 @@ public class MetricFilterAutoConfigurationTests { context.close(); } + @Test + public void records302HttpInteractionsAsSingleMetric() throws Exception { + AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( + Config.class, MetricFilterAutoConfiguration.class, RedirectFilter.class); + MetricsFilter filter = context.getBean(MetricsFilter.class); + MockMvc mvc = MockMvcBuilders.standaloneSetup(new MetricFilterTestController()) + .addFilter(filter).addFilter(context.getBean(RedirectFilter.class)) + .build(); + mvc.perform(get("/unknownPath/1")).andExpect(status().is3xxRedirection()); + mvc.perform(get("/unknownPath/2")).andExpect(status().is3xxRedirection()); + verify(context.getBean(CounterService.class), times(2)).increment( + "status.302.unmapped"); + verify(context.getBean(GaugeService.class), times(2)).submit( + eq("response.unmapped"), anyDouble()); + context.close(); + } + @Test public void skipsFilterIfMissingServices() throws Exception { AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext( @@ -214,4 +239,19 @@ public class MetricFilterAutoConfigurationTests { } } + @Component + @Order(0) + public static class RedirectFilter extends OncePerRequestFilter { + + @Override + protected void doFilterInternal(HttpServletRequest request, + HttpServletResponse response, FilterChain chain) throws ServletException, + IOException { + // send redirect before filter chain is executed, like Spring Security sending + // us back to a login page + response.sendRedirect("http://example.com"); + } + + } + } diff --git a/spring-boot-samples/spring-boot-sample-web-secure/pom.xml b/spring-boot-samples/spring-boot-sample-web-secure/pom.xml index a4646d5295..692dcc566b 100644 --- a/spring-boot-samples/spring-boot-sample-web-secure/pom.xml +++ b/spring-boot-samples/spring-boot-sample-web-secure/pom.xml @@ -19,6 +19,10 @@ ${basedir}/../.. + + org.springframework.boot + spring-boot-starter-actuator + org.springframework.boot spring-boot-starter-security From f6c395d0a62019f12cc139daef181e6b9f1b7aad Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Fri, 10 Jul 2015 15:56:27 +0100 Subject: [PATCH 2/4] Add Maven incantation to make STS 3.7 happy --- spring-boot-cli/pom.xml | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/spring-boot-cli/pom.xml b/spring-boot-cli/pom.xml index 7659040c24..a176bdbb9e 100644 --- a/spring-boot-cli/pom.xml +++ b/spring-boot-cli/pom.xml @@ -304,5 +304,40 @@ + + + + org.eclipse.m2e + lifecycle-mapping + 1.0.0 + + + + + + + org.apache.maven.plugins + + + maven-dependency-plugin + + + [1.0.0,) + + + unpack + copy-dependencies + + + + + + + + + + + + From 67933ab01f91b864d79f0a23526f66d88b3576dc Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 13 Jul 2015 14:21:18 +0100 Subject: [PATCH 3/4] Add boolean flag spring.rabbit.listener.useTransactionManager If set *and* there is a transaction manager in the context, then the listener container created by Spring Boot will use the transaction manager to execute the listeners. Fixes gh-3432 (by virtue of not requiring a JtaTransactionManager any more). --- .../RabbitAnnotationDrivenConfiguration.java | 9 +++++---- .../amqp/RabbitAutoConfiguration.java | 2 +- .../autoconfigure/amqp/RabbitProperties.java | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java index 61383d5a0c..ebbf2933ff 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java @@ -25,7 +25,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.transaction.jta.JtaTransactionManager; +import org.springframework.transaction.PlatformTransactionManager; /** * Configuration for Spring AMQP annotation driven endpoints. @@ -38,15 +38,16 @@ import org.springframework.transaction.jta.JtaTransactionManager; class RabbitAnnotationDrivenConfiguration { @Autowired(required = false) - private JtaTransactionManager transactionManager; + private PlatformTransactionManager transactionManager; @Bean @ConditionalOnMissingBean(name = "rabbitListenerContainerFactory") public SimpleRabbitListenerContainerFactory rabbitListenerContainerFactory( - ConnectionFactory connectionFactory) { + ConnectionFactory connectionFactory, RabbitProperties config) { SimpleRabbitListenerContainerFactory factory = new SimpleRabbitListenerContainerFactory(); factory.setConnectionFactory(connectionFactory); - if (this.transactionManager != null) { + if (this.transactionManager != null + && config.getListener().isUseTransactionManager()) { factory.setTransactionManager(this.transactionManager); } return factory; diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java index 5385148cfe..6fee9ffc4e 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAutoConfiguration.java @@ -91,7 +91,7 @@ public class RabbitAutoConfiguration { @Bean @ConditionalOnMissingBean(RabbitTemplate.class) - public RabbitTemplate rabbitTemplate() { + public RabbitTemplate rabbitTemplate(RabbitProperties config) { return new RabbitTemplate(this.connectionFactory); } diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java index 773253d831..83cabdc788 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java @@ -62,6 +62,12 @@ public class RabbitProperties { */ private String addresses; + private Listener listener = new Listener(); + + public Listener getListener() { + return this.listener; + } + public String getHost() { if (this.addresses == null) { return this.host; @@ -156,4 +162,16 @@ public class RabbitProperties { this.virtualHost = ("".equals(virtualHost) ? "/" : virtualHost); } + public static class Listener { + private boolean useTransactionManager = false; + + public boolean isUseTransactionManager() { + return this.useTransactionManager; + } + + public void setUseTransactionManager(boolean useTransactionManager) { + this.useTransactionManager = useTransactionManager; + } + } + } From ff79138aa51ac8b563b994f40f88bd42bcc8ae01 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Mon, 13 Jul 2015 14:57:31 +0100 Subject: [PATCH 4/4] Alternative fix for Rabbit listener transactions Never inject a transaction manager (user can create his own "rabbitListenerContainerFactory" bean if he wants an actual non-JTA transaction manager. See gh-3432 --- .../RabbitAnnotationDrivenConfiguration.java | 9 --------- .../autoconfigure/amqp/RabbitProperties.java | 18 ------------------ 2 files changed, 27 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java index ebbf2933ff..2bff6719dd 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java @@ -20,12 +20,10 @@ import org.springframework.amqp.rabbit.annotation.EnableRabbit; import org.springframework.amqp.rabbit.config.RabbitListenerConfigUtils; import org.springframework.amqp.rabbit.config.SimpleRabbitListenerContainerFactory; import org.springframework.amqp.rabbit.connection.ConnectionFactory; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.transaction.PlatformTransactionManager; /** * Configuration for Spring AMQP annotation driven endpoints. @@ -37,19 +35,12 @@ import org.springframework.transaction.PlatformTransactionManager; @ConditionalOnClass(EnableRabbit.class) class RabbitAnnotationDrivenConfiguration { - @Autowired(required = false) - private PlatformTransactionManager transactionManager; - @Bean @ConditionalOnMissingBean(name = "rabbitListenerContainerFactory") public SimpleRabbitListenerContainerFactory rabbitListenerContainerFactory( ConnectionFactory connectionFactory, RabbitProperties config) { SimpleRabbitListenerContainerFactory factory = new SimpleRabbitListenerContainerFactory(); factory.setConnectionFactory(connectionFactory); - if (this.transactionManager != null - && config.getListener().isUseTransactionManager()) { - factory.setTransactionManager(this.transactionManager); - } return factory; } diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java index 83cabdc788..773253d831 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java @@ -62,12 +62,6 @@ public class RabbitProperties { */ private String addresses; - private Listener listener = new Listener(); - - public Listener getListener() { - return this.listener; - } - public String getHost() { if (this.addresses == null) { return this.host; @@ -162,16 +156,4 @@ public class RabbitProperties { this.virtualHost = ("".equals(virtualHost) ? "/" : virtualHost); } - public static class Listener { - private boolean useTransactionManager = false; - - public boolean isUseTransactionManager() { - return this.useTransactionManager; - } - - public void setUseTransactionManager(boolean useTransactionManager) { - this.useTransactionManager = useTransactionManager; - } - } - }