From 192964751a203b7cfa7b05ffa4e0a4e6c1724ab9 Mon Sep 17 00:00:00 2001 From: Dave Syer Date: Tue, 1 Dec 2015 17:08:45 +0000 Subject: [PATCH] Normalize usage of IdGenerator (only in TraceManager) Also ensures TraceFilter sends traceId and spanId in response if they are generated in the request. Fixes gh-65 --- .../scheduling/TraceSchedulingAspect.java | 16 ++------- .../TraceSchedulingAutoConfiguration.java | 5 ++- .../sleuth/instrument/web/TraceFilter.java | 36 ++++++++++++------- .../BaseConfigurationForITests.java | 20 ----------- .../DefaultTestAutoConfiguration.java | 9 ++++- .../ScheduledTestConfiguration.java | 18 ---------- .../TestBeanWithScheduledMethod.java | 19 ---------- .../scheduling/TracingOnScheduledITest.java | 35 ++++++++++++++++-- .../instrument/web/TraceAsyncITest.java | 7 +--- .../instrument/web/TraceFilterITest.java | 29 +++++++++------ 10 files changed, 87 insertions(+), 107 deletions(-) delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/BaseConfigurationForITests.java delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/ScheduledTestConfiguration.java delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TestBeanWithScheduledMethod.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAspect.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAspect.java index 966b00679..41c2c3500 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAspect.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAspect.java @@ -19,12 +19,9 @@ package org.springframework.cloud.sleuth.instrument.scheduling; import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; -import org.springframework.cloud.sleuth.MilliSpan; -import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Trace; import org.springframework.cloud.sleuth.TraceManager; import org.springframework.scheduling.annotation.Scheduled; -import org.springframework.util.IdGenerator; /** * Aspect that creates a new Span for running threads executing methods annotated with @@ -42,20 +39,14 @@ import org.springframework.util.IdGenerator; public class TraceSchedulingAspect { private final TraceManager trace; - private final IdGenerator idGenerator; - public TraceSchedulingAspect(TraceManager trace, IdGenerator idGenerator) { + public TraceSchedulingAspect(TraceManager trace) { this.trace = trace; - this.idGenerator = idGenerator; } @Around("execution (@org.springframework.scheduling.annotation.Scheduled * *.*(..))") public Object traceBackgroundThread(final ProceedingJoinPoint pjp) throws Throwable { - final Span span = this.trace.isTracing() ? this.trace.getCurrentSpan() - : MilliSpan.builder().begin(System.currentTimeMillis()) - .traceId(createId()).spanId(createId()) - .build(); - Trace scope = this.trace.startSpan(pjp.toShortString(), span); + Trace scope = this.trace.startSpan(pjp.toShortString()); try { return pjp.proceed(); } @@ -64,7 +55,4 @@ public class TraceSchedulingAspect { } } - private String createId() { - return this.idGenerator.generateId().toString(); - } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAutoConfiguration.java index 231752d04..caf6b964a 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/scheduling/TraceSchedulingAutoConfiguration.java @@ -30,7 +30,6 @@ import org.springframework.cloud.sleuth.autoconfig.TraceAutoConfiguration; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.EnableAspectJAutoProxy; -import org.springframework.util.IdGenerator; /** * Registers beans related to task scheduling. @@ -49,8 +48,8 @@ public class TraceSchedulingAutoConfiguration { @ConditionalOnClass(ProceedingJoinPoint.class) @Bean - public TraceSchedulingAspect traceSchedulingAspect(TraceManager trace, IdGenerator idGenerator) { - return new TraceSchedulingAspect(trace, idGenerator); + public TraceSchedulingAspect traceSchedulingAspect(TraceManager trace) { + return new TraceSchedulingAspect(trace); } } 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 4b9cca34e..043b4cef5 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 @@ -55,13 +55,14 @@ import org.springframework.web.util.UrlPathHelper; * @author Dave Syer */ @Order(Ordered.HIGHEST_PRECEDENCE + 5) -public class TraceFilter extends OncePerRequestFilter implements ApplicationEventPublisherAware { +public class TraceFilter extends OncePerRequestFilter + implements ApplicationEventPublisherAware { protected static final String TRACE_REQUEST_ATTR = TraceFilter.class.getName() + ".TRACE"; - public static final Pattern DEFAULT_SKIP_PATTERN = Pattern - .compile("/api-docs.*|/autoconfig|/configprops|/dump|/info|/metrics.*|/mappings|/trace|/swagger.*|.*\\.png|.*\\.css|.*\\.js|.*\\.html|/favicon.ico|/hystrix.stream"); + public static final Pattern DEFAULT_SKIP_PATTERN = Pattern.compile( + "/api-docs.*|/autoconfig|/configprops|/dump|/info|/metrics.*|/mappings|/trace|/swagger.*|.*\\.png|.*\\.css|.*\\.js|.*\\.html|/favicon.ico|/hystrix.stream"); private final TraceManager traceManager; private final Pattern skipPattern; @@ -128,10 +129,10 @@ public class TraceFilter extends OncePerRequestFilter implements ApplicationEven publish(new ServerReceivedEvent(this, parent, trace.getSpan())); request.setAttribute(TRACE_REQUEST_ATTR, trace); // Send new span id back - addToResponseIfNotPresent(response, Trace.TRACE_ID_NAME, trace.getSpan() - .getTraceId()); - addToResponseIfNotPresent(response, Trace.SPAN_ID_NAME, trace.getSpan() - .getSpanId()); + addToResponseIfNotPresent(response, Trace.TRACE_ID_NAME, + trace.getSpan().getTraceId()); + addToResponseIfNotPresent(response, Trace.SPAN_ID_NAME, + trace.getSpan().getSpanId()); } else { trace = this.traceManager.startSpan(name); @@ -151,9 +152,11 @@ public class TraceFilter extends OncePerRequestFilter implements ApplicationEven return; } if (trace != null) { + addResponseHeaders(response, trace.getSpan()); addResponseAnnotations(response); - if (trace.getSavedTrace()!=null) { - publish(new ServerSentEvent(this, trace.getSavedTrace().getSpan(), trace.getSpan())); + if (trace.getSavedTrace() != null) { + publish(new ServerSentEvent(this, trace.getSavedTrace().getSpan(), + trace.getSpan())); } // Double close to clean up the parent (remote span as well) this.traceManager.close(this.traceManager.close(trace)); @@ -161,17 +164,24 @@ public class TraceFilter extends OncePerRequestFilter implements ApplicationEven } } + private void addResponseHeaders(HttpServletResponse response, Span span) { + if (span != null) { + response.addHeader(Trace.SPAN_ID_NAME, span.getSpanId()); + response.addHeader(Trace.TRACE_ID_NAME, span.getTraceId()); + } + } + private void publish(ApplicationEvent event) { - if (this.publisher!=null) { + if (this.publisher != null) { this.publisher.publishEvent(event); } } - //TODO: move annotation keys to constants + // TODO: move annotation keys to constants protected void addRequestAnnotations(HttpServletRequest request) { String uri = this.urlPathHelper.getPathWithinApplication(request); - this.traceManager.addAnnotation("/http/request/uri", request.getRequestURL() - .toString()); + this.traceManager.addAnnotation("/http/request/uri", + request.getRequestURL().toString()); this.traceManager.addAnnotation("/http/request/endpoint", uri); this.traceManager.addAnnotation("/http/request/method", request.getMethod()); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/BaseConfigurationForITests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/BaseConfigurationForITests.java deleted file mode 100644 index feb492e65..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/BaseConfigurationForITests.java +++ /dev/null @@ -1,20 +0,0 @@ -package org.springframework.cloud.sleuth.instrument; - -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; -import org.springframework.boot.autoconfigure.jmx.JmxAutoConfiguration; -import org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.EnableAspectJAutoProxy; -import org.springframework.context.support.PropertySourcesPlaceholderConfigurer; - -@EnableAspectJAutoProxy(proxyTargetClass = true) -@Configuration -@EnableAutoConfiguration(exclude = {LoadBalancerAutoConfiguration.class, JmxAutoConfiguration.class}) -public class BaseConfigurationForITests { - - @Bean static PropertySourcesPlaceholderConfigurer placeholderConfigurer() { - return new PropertySourcesPlaceholderConfigurer(); - } - -} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/DefaultTestAutoConfiguration.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/DefaultTestAutoConfiguration.java index f65295a14..5a5409921 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/DefaultTestAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/DefaultTestAutoConfiguration.java @@ -6,15 +6,22 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.autoconfigure.jmx.JmxAutoConfiguration; import org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration; import org.springframework.cloud.netflix.archaius.ArchaiusAutoConfiguration; import org.springframework.cloud.sleuth.instrument.integration.TraceSpringIntegrationAutoConfiguration; +import org.springframework.cloud.sleuth.sampler.AlwaysSampler; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.EnableAspectJAutoProxy; +import org.springframework.context.annotation.Import; @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME) -@EnableAutoConfiguration(exclude = { TraceSpringIntegrationAutoConfiguration.class, +@EnableAutoConfiguration(exclude = { LoadBalancerAutoConfiguration.class, + JmxAutoConfiguration.class, TraceSpringIntegrationAutoConfiguration.class, ArchaiusAutoConfiguration.class, LoadBalancerAutoConfiguration.class }) +@EnableAspectJAutoProxy(proxyTargetClass = true) +@Import(AlwaysSampler.class) @Configuration public @interface DefaultTestAutoConfiguration { } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/ScheduledTestConfiguration.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/ScheduledTestConfiguration.java deleted file mode 100644 index 5bb42243b..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/ScheduledTestConfiguration.java +++ /dev/null @@ -1,18 +0,0 @@ -package org.springframework.cloud.sleuth.instrument.scheduling; - -import org.springframework.cloud.sleuth.instrument.BaseConfigurationForITests; -import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; - -@Configuration -@Import(BaseConfigurationForITests.class) -@DefaultTestAutoConfiguration -class ScheduledTestConfiguration { - - @Bean TestBeanWithScheduledMethod testBeanWithScheduledMethod() { - return new TestBeanWithScheduledMethod(); - } - -} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TestBeanWithScheduledMethod.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TestBeanWithScheduledMethod.java deleted file mode 100644 index addad9f62..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TestBeanWithScheduledMethod.java +++ /dev/null @@ -1,19 +0,0 @@ -package org.springframework.cloud.sleuth.instrument.scheduling; - -import org.springframework.cloud.sleuth.Span; -import org.springframework.cloud.sleuth.trace.TraceContextHolder; -import org.springframework.scheduling.annotation.Scheduled; - -class TestBeanWithScheduledMethod { - - Span span; - - @Scheduled(fixedDelay = 1L) - public void scheduledMethod() { - this.span = TraceContextHolder.getCurrentSpan(); - } - - public Span getSpan() { - return this.span; - } -} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TracingOnScheduledITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TracingOnScheduledITest.java index 2dd6c51e9..6b9115db0 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TracingOnScheduledITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/scheduling/TracingOnScheduledITest.java @@ -8,6 +8,11 @@ import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.SpringApplicationConfiguration; import org.springframework.cloud.sleuth.Span; +import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; +import org.springframework.cloud.sleuth.trace.TraceContextHolder; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.scheduling.annotation.Scheduled; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @RunWith(SpringJUnit4ClassRunner.class) @@ -23,7 +28,7 @@ public class TracingOnScheduledITest { @Test public void should_have_a_new_span_set_each_time_a_scheduled_method_has_been_executed() { - Span firstSpan = beanWithScheduledMethod.getSpan(); + Span firstSpan = this.beanWithScheduledMethod.getSpan(); await().until(differentSpanHasBeenSetThan(firstSpan)); } @@ -31,7 +36,7 @@ public class TracingOnScheduledITest { return new Runnable() { @Override public void run() { - Span storedSpan = beanWithScheduledMethod.getSpan(); + Span storedSpan = TracingOnScheduledITest.this.beanWithScheduledMethod.getSpan(); then(storedSpan).isNotNull(); then(storedSpan.getTraceId()).isNotNull(); } @@ -42,9 +47,33 @@ public class TracingOnScheduledITest { return new Runnable() { @Override public void run() { - then(beanWithScheduledMethod.getSpan()).isNotEqualTo(spanToCompare); + then(TracingOnScheduledITest.this.beanWithScheduledMethod.getSpan()).isNotEqualTo(spanToCompare); } }; } } + +@Configuration +@DefaultTestAutoConfiguration +class ScheduledTestConfiguration { + + @Bean TestBeanWithScheduledMethod testBeanWithScheduledMethod() { + return new TestBeanWithScheduledMethod(); + } + +} + +class TestBeanWithScheduledMethod { + + Span span; + + @Scheduled(fixedDelay = 1L) + public void scheduledMethod() { + this.span = TraceContextHolder.getCurrentSpan(); + } + + public Span getSpan() { + return this.span; + } +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java index 3a73bae31..dacbc5113 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java @@ -10,7 +10,6 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.SpringApplicationConfiguration; -import org.springframework.cloud.sleuth.MilliSpan; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.TraceManager; import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; @@ -21,7 +20,6 @@ import org.springframework.context.annotation.EnableAspectJAutoProxy; import org.springframework.scheduling.annotation.Async; import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; -import org.springframework.util.IdGenerator; import com.jayway.awaitility.Awaitility; @@ -35,8 +33,6 @@ public class TraceAsyncITest { @Autowired AsyncDelegation asyncDelegation; @Autowired - IdGenerator idGenerator; - @Autowired TraceManager traceManager; @Test @@ -49,8 +45,7 @@ public class TraceAsyncITest { } private Span givenASpanInCurrentThread() { - Span span = MilliSpan.builder().traceId(this.idGenerator.generateId().toString()) - .spanId(this.idGenerator.generateId().toString()).build(); + Span span = this.traceManager.startSpan("existing").getSpan(); this.traceManager.continueSpan(span); return span; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java index 56fdb9985..45fb1ea1f 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterITest.java @@ -2,7 +2,8 @@ package org.springframework.cloud.sleuth.instrument.web; import static org.assertj.core.api.BDDAssertions.then; -import org.junit.Ignore; +import java.util.UUID; + import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; @@ -20,10 +21,10 @@ import org.springframework.test.web.servlet.setup.DefaultMockMvcBuilder; @RunWith(SpringJUnit4ClassRunner.class) @SpringApplicationConfiguration(TraceFilterITest.class) @DefaultTestAutoConfiguration -@Ignore("Will fail cause no tracing data is set on the TraceFilter") public class TraceFilterITest extends MvcITest { - @Autowired TraceManager traceManager; + @Autowired + TraceManager traceManager; @Test public void should_create_and_return_trace_in_HTTP_header() throws Exception { @@ -33,7 +34,8 @@ public class TraceFilterITest extends MvcITest { } @Test - public void when_correlationId_is_sent_should_not_create_a_new_one_but_return_the_existing_one_instead() throws Exception { + public void when_correlationId_is_sent_should_not_create_a_new_one_but_return_the_existing_one_instead() + throws Exception { String expectedTraceId = "passedCorId"; MvcResult mvcResult = whenSentPingWithTraceId(expectedTraceId); @@ -43,20 +45,27 @@ public class TraceFilterITest extends MvcITest { @Override protected void configureMockMvcBuilder(DefaultMockMvcBuilder mockMvcBuilder) { - mockMvcBuilder.addFilters(new TraceFilter(traceManager)); + mockMvcBuilder.addFilters(new TraceFilter(this.traceManager)); } private MvcResult whenSentPingWithoutTracingData() throws Exception { - return mockMvc.perform(MockMvcRequestBuilders.get("/ping").accept(MediaType.TEXT_PLAIN)).andReturn(); + return this.mockMvc + .perform(MockMvcRequestBuilders.get("/ping").accept(MediaType.TEXT_PLAIN)) + .andReturn(); } - private MvcResult whenSentPingWithTraceId(String passedCorrelationId) throws Exception { + private MvcResult whenSentPingWithTraceId(String passedCorrelationId) + throws Exception { return sendPingWithTraceId(Trace.TRACE_ID_NAME, passedCorrelationId); } - private MvcResult sendPingWithTraceId(String headerName, String passedCorrelationId) throws Exception { - return mockMvc.perform(MockMvcRequestBuilders.get("/ping").accept(MediaType.TEXT_PLAIN) - .header(headerName, passedCorrelationId)).andReturn(); + private MvcResult sendPingWithTraceId(String headerName, String passedCorrelationId) + throws Exception { + return this.mockMvc + .perform(MockMvcRequestBuilders.get("/ping").accept(MediaType.TEXT_PLAIN) + .header(headerName, passedCorrelationId) + .header(Trace.SPAN_ID_NAME, UUID.randomUUID().toString())) + .andReturn(); } private String tracingHeaderFrom(MvcResult mvcResult) {