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
This commit is contained in:
		
							parent
							
								
									fdb83ec338
								
							
						
					
					
						commit
						d0cf6b534b
					
				| 
						 | 
				
			
			@ -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
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -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("/", ".");
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -16,29 +16,6 @@
 | 
			
		|||
 | 
			
		||||
package org.springframework.boot.actuate.autoconfigure;
 | 
			
		||||
 | 
			
		||||
import javax.servlet.Filter;
 | 
			
		||||
import javax.servlet.FilterChain;
 | 
			
		||||
 | 
			
		||||
import org.junit.Test;
 | 
			
		||||
import org.mockito.invocation.InvocationOnMock;
 | 
			
		||||
import org.mockito.stubbing.Answer;
 | 
			
		||||
import org.springframework.boot.actuate.metrics.CounterService;
 | 
			
		||||
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;
 | 
			
		||||
import org.springframework.web.util.NestedServletException;
 | 
			
		||||
 | 
			
		||||
import static org.hamcrest.Matchers.equalTo;
 | 
			
		||||
import static org.junit.Assert.assertThat;
 | 
			
		||||
import static org.mockito.BDDMockito.willAnswer;
 | 
			
		||||
| 
						 | 
				
			
			@ -52,6 +29,37 @@ 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;
 | 
			
		||||
import org.mockito.stubbing.Answer;
 | 
			
		||||
import org.springframework.boot.actuate.metrics.CounterService;
 | 
			
		||||
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;
 | 
			
		||||
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;
 | 
			
		||||
 | 
			
		||||
/**
 | 
			
		||||
 * 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");
 | 
			
		||||
		}
 | 
			
		||||
 | 
			
		||||
	}
 | 
			
		||||
 | 
			
		||||
}
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
| 
						 | 
				
			
			@ -19,6 +19,10 @@
 | 
			
		|||
		<main.basedir>${basedir}/../..</main.basedir>
 | 
			
		||||
	</properties>
 | 
			
		||||
	<dependencies>
 | 
			
		||||
		<dependency>
 | 
			
		||||
			<groupId>org.springframework.boot</groupId>
 | 
			
		||||
			<artifactId>spring-boot-starter-actuator</artifactId>
 | 
			
		||||
		</dependency>
 | 
			
		||||
		<dependency>
 | 
			
		||||
			<groupId>org.springframework.boot</groupId>
 | 
			
		||||
			<artifactId>spring-boot-starter-security</artifactId>
 | 
			
		||||
| 
						 | 
				
			
			
 | 
			
		|||
		Loading…
	
		Reference in New Issue