From a613e8a0e4110eb2a1ff48486628b14761092de7 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 6 Jun 2016 12:25:37 +0200 Subject: [PATCH] Changed aspect into filter with custom dispatch (#297) --- .../sleuth/instrument/web/TraceFilter.java | 20 ++++++++++++- .../sleuth/instrument/web/TraceWebAspect.java | 14 --------- .../web/TraceWebAutoConfiguration.java | 19 +++++++++++- .../web/TraceFilterIntegrationTests.java | 1 + .../instrument/web/TraceFilterTests.java | 4 +-- .../instrument/web/client/WebClientTests.java | 30 ++++++------------- 6 files changed, 49 insertions(+), 39 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java index 00b0d0b7f..9e7c0d3ec 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java @@ -111,7 +111,7 @@ public class TraceFilter extends OncePerRequestFilter { String uri = this.urlPathHelper.getPathWithinApplication(request); boolean skip = this.skipPattern.matcher(uri).matches() || Span.SPAN_NOT_SAMPLED.equals(ServletUtils.getHeader(request, response, Span.SAMPLED_NAME)); - Span spanFromRequest = (Span) request.getAttribute(TRACE_REQUEST_ATTR); + Span spanFromRequest = getSpanFromAttribute(request); if (spanFromRequest != null) { this.tracer.continueSpan(spanFromRequest); } @@ -152,11 +152,24 @@ public class TraceFilter extends OncePerRequestFilter { HttpStatus httpStatus = HttpStatus.valueOf(response.getStatus()); if (httpStatus.is2xxSuccessful() || httpStatus.is3xxRedirection()) { this.tracer.close(spanFromRequest); + } else if(isSpanContinued(request)) { + // it means that the span was already detached once and we're processing an error + this.tracer.close(spanFromRequest); + } else { + this.tracer.detach(spanFromRequest); } } } } + private Span getSpanFromAttribute(HttpServletRequest request) { + return (Span) request.getAttribute(TRACE_REQUEST_ATTR); + } + + private boolean isSpanContinued(HttpServletRequest request) { + return getSpanFromAttribute(request) != null; + } + private void addRequestTagsForParentSpan(HttpServletRequest request, Span spanFromRequest) { if (spanFromRequest.getName().contains("parent")) { addRequestTags(spanFromRequest, request); @@ -247,4 +260,9 @@ public class TraceFilter extends OncePerRequestFilter { return requestURI.append('?').append(queryString).toString(); } } + + @Override + protected boolean shouldNotFilterErrorDispatch() { + return false; + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java index 6b39dd1f7..a56ac27a3 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAspect.java @@ -21,7 +21,6 @@ import java.util.concurrent.Callable; import org.apache.commons.logging.Log; import org.aspectj.lang.ProceedingJoinPoint; -import org.aspectj.lang.annotation.After; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; import org.aspectj.lang.annotation.Pointcut; @@ -84,9 +83,6 @@ public class TraceWebAspect { @Pointcut("@within(org.springframework.stereotype.Controller)") private void anyControllerAnnotated() { } // NOSONAR - @Pointcut("target(org.springframework.boot.autoconfigure.web.ErrorController+)") - private void implementingErrorController() { } // NOSONAR - @Pointcut("execution(public java.util.concurrent.Callable *(..))") private void anyPublicMethodReturningCallable() { } // NOSONAR @@ -99,9 +95,6 @@ public class TraceWebAspect { @Pointcut("(anyRestControllerAnnotated() || anyControllerAnnotated()) && anyPublicMethodReturningWebAsyncTask()") private void anyControllerOrRestControllerWithPublicWebAsyncTaskMethod() { } // NOSONAR - @Pointcut("(anyRestControllerAnnotated() || anyControllerAnnotated()) && implementingErrorController()") - private void anyControllerOrRestControllerImplementingErrorController() { } // NOSONAR - @Around("anyControllerOrRestControllerWithPublicAsyncMethod()") @SuppressWarnings("unchecked") public Object wrapWithCorrelationId(ProceedingJoinPoint pjp) throws Throwable { @@ -134,11 +127,4 @@ public class TraceWebAspect { return webAsyncTask; } - @After("anyControllerOrRestControllerImplementingErrorController()") - public void wrapErrorController() throws Throwable { - if (this.tracer.isTracing()) { - this.tracer.close(this.tracer.getCurrentSpan()); - } - } - } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java index 453feeda5..5b7544dd5 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java @@ -30,6 +30,7 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingClass; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplication; +import org.springframework.boot.context.embedded.FilterRegistrationBean; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.cloud.sleuth.SpanInjector; import org.springframework.cloud.sleuth.SpanExtractor; @@ -42,6 +43,12 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.util.StringUtils; +import static javax.servlet.DispatcherType.ASYNC; +import static javax.servlet.DispatcherType.ERROR; +import static javax.servlet.DispatcherType.FORWARD; +import static javax.servlet.DispatcherType.INCLUDE; +import static javax.servlet.DispatcherType.REQUEST; + /** * {@link org.springframework.boot.autoconfigure.EnableAutoConfiguration Auto-configuration} * enables tracing to HTTP requests. @@ -72,7 +79,17 @@ public class TraceWebAutoConfiguration { } @Bean - @ConditionalOnMissingBean + public FilterRegistrationBean traceWebFilter(Tracer tracer, TraceKeys traceKeys, + SkipPatternProvider skipPatternProvider, SpanReporter spanReporter, + SpanExtractor spanExtractor, + SpanInjector spanInjector, + HttpTraceKeysInjector httpTraceKeysInjector, TraceFilter traceFilter) { + FilterRegistrationBean filterRegistrationBean = new FilterRegistrationBean(traceFilter); + filterRegistrationBean.setDispatcherTypes(ASYNC, ERROR, FORWARD, INCLUDE, REQUEST); + return filterRegistrationBean; + } + + @Bean public TraceFilter traceFilter(Tracer tracer, TraceKeys traceKeys, SkipPatternProvider skipPatternProvider, SpanReporter spanReporter, SpanExtractor spanExtractor, diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java index 10730ec1a..96fa42a16 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java @@ -117,6 +117,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { MvcResult mvcResult = whenSentToNonExistentEndpointWithTraceId(expectedTraceId); then(tracingHeaderFrom(mvcResult)).isEqualTo(expectedTraceId); + then(this.tracer.getCurrentSpan()).isNull(); } @Override diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java index b244a87b0..f970dfeb4 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java @@ -260,7 +260,7 @@ public class TraceFilterTests { } @Test - public void doesNotCloseSpanWhenResponseStatusIsNot2xx() throws Exception { + public void detachesSpanWhenResponseStatusIsNot2xx() throws Exception { this.request = builder().header(Span.SPAN_ID_NAME, 10L) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, @@ -269,7 +269,7 @@ public class TraceFilterTests { filter.doFilter(this.request, this.response, this.filterChain); - then(TestSpanContextHolder.getCurrentSpan()).isNotNull(); + then(TestSpanContextHolder.getCurrentSpan()).isNull(); } public void verifyParentSpanHttpTags() { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java index 407fdee1a..75805f34f 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientTests.java @@ -17,7 +17,6 @@ package org.springframework.cloud.sleuth.instrument.web.client; import javax.servlet.http.HttpServletRequest; -import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -52,6 +51,7 @@ import org.springframework.cloud.sleuth.SpanReporter; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.sampler.AlwaysSampler; import org.springframework.cloud.sleuth.trace.TestSpanContextHolder; +import org.springframework.cloud.sleuth.util.ArrayListSpanAccumulator; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.HttpHeaders; @@ -84,7 +84,7 @@ public class WebClientTests { @Autowired TestFeignInterface testFeignInterface; @Autowired @LoadBalanced RestTemplate template; - @Autowired Listener listener; + @Autowired ArrayListSpanAccumulator listener; @Autowired Tracer tracer; @Autowired TestErrorController testErrorController; @@ -210,6 +210,9 @@ public class WebClientTests { } catch (HttpClientErrorException e) { } then(this.tracer.getCurrentSpan()).isNull(); + Optional storedSpan = this.listener.getSpans().stream() + .filter(span -> "404".equals(span.tags().get("http.status_code"))).findFirst(); + then(storedSpan.isPresent()).isTrue(); then(this.testErrorController.getSpan()).isNotNull(); } @@ -261,11 +264,6 @@ public class WebClientTests { return new FooController(); } - @Bean - Listener listener() { - return new Listener(); - } - @LoadBalanced @Bean public RestTemplate restTemplate() { @@ -282,6 +280,10 @@ public class WebClientTests { return new TestErrorController(errorAttributes, tracer); } + @Bean + SpanReporter spanReporter() { + return new ArrayListSpanAccumulator(); + } } public static class TestErrorController extends BasicErrorController { @@ -310,20 +312,6 @@ public class WebClientTests { } } - @Component - public static class Listener implements SpanReporter { - private List events = new ArrayList<>(); - - public List getSpans() { - return this.events; - } - - @Override - public void report(Span span) { - this.events.add(span); - } - } - @RestController public static class FooController {