From 78dc17fc4564e309d0e4d64b146793c59cdf10d3 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 15 Jun 2016 13:07:42 +0200 Subject: [PATCH 01/10] Added verification for lack of response status code With this change when there is no http response status code an exception is not thrown and the tags are not set for http.status_code fixes #304 --- .../cloud/sleuth/instrument/web/TraceFilter.java | 6 +++++- .../sleuth/instrument/web/TraceFilterTests.java | 12 ++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) 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 1f88656af..cc1db1df8 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 @@ -216,6 +216,9 @@ public class TraceFilter extends GenericFilterBean { } private boolean httpStatusSuccessful(HttpServletResponse response) { + if (response.getStatus() == 0) { + return false; + } HttpStatus httpStatus = HttpStatus.valueOf(response.getStatus()); return httpStatus.is2xxSuccessful() || httpStatus.is3xxRedirection(); } @@ -308,7 +311,8 @@ public class TraceFilter extends GenericFilterBean { this.tracer.addTag(this.traceKeys.getHttp().getStatusCode(), String.valueOf(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)); } - else if ((httpStatus < 200) || (httpStatus > 399)) { + // only tag valid http statuses + else if (httpStatus >= 100 && (httpStatus < 200) || (httpStatus > 399)) { this.tracer.addTag(this.traceKeys.getHttp().getStatusCode(), String.valueOf(response.getStatus())); } 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 0add851d1..427b49424 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 @@ -131,6 +131,18 @@ public class TraceFilterTests { then(TestSpanContextHolder.getCurrentSpan()).isNull(); } + @Test + public void shouldNotStoreHttpStatusCodeWhenResponseCodeHasNotYetBeenSet() throws Exception { + TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, + this.spanExtractor, this.spanInjector, this.httpTraceKeysInjector); + this.response.setStatus(0); + filter.doFilter(this.request, this.response, this.filterChain); + + assertThat(this.span.tags()).doesNotContainKey("http.status_code"); + + then(TestSpanContextHolder.getCurrentSpan()).isNull(); + } + @Test public void startsNewTraceWithParentIdInHeaders() throws Exception { this.request = builder() From a9c1d89668c78b0cc098f3429713ed0d1151aba2 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 21 Jun 2016 23:54:53 +0200 Subject: [PATCH 02/10] Made evaluation of Tracer bean more lazy with this change Feign components resolve tracer and other beans from beanfactory when necessary fixes #307 --- .../web/client/feign/FeignEventPublisher.java | 17 ++- .../web/client/feign/SleuthFeignBuilder.java | 13 +- .../web/client/feign/TraceFeignClient.java | 23 ++-- .../TraceFeignClientAutoConfiguration.java | 9 +- .../web/client/feign/TraceFeignDecoder.java | 13 +- .../client/feign/TraceFeignErrorDecoder.java | 10 +- .../client/feign/TraceFeignObjectWrapper.java | 26 +--- .../web/client/feign/TraceFeignRetryer.java | 27 +++-- .../instrument/web/client/WebClientTests.java | 2 + .../client/feign/issues/Issue307Tests.java | 111 ++++++++++++++++++ .../MessagingApplicationTests.java | 2 + 11 files changed, 185 insertions(+), 68 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignEventPublisher.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignEventPublisher.java index 143bd7dd3..d66ccba03 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignEventPublisher.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignEventPublisher.java @@ -16,6 +16,7 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; +import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Tracer; @@ -30,18 +31,26 @@ abstract class FeignEventPublisher { private final FeignRequestContext feignRequestContext = FeignRequestContext.getInstance(); - private final Tracer tracer; + protected final BeanFactory beanFactory; + private Tracer tracer; - protected FeignEventPublisher(Tracer tracer) { - this.tracer = tracer; + protected FeignEventPublisher(BeanFactory beanFactory) { + this.beanFactory = beanFactory; } protected void finish() { Span span = this.feignRequestContext.getCurrentSpan(); if (span != null) { span.logEvent(Span.CLIENT_RECV); - this.tracer.close(span); + getTracer().close(span); this.feignRequestContext.clearContext(); } } + + Tracer getTracer() { + if (this.tracer == null) { + this.tracer = this.beanFactory.getBean(Tracer.class); + } + return this.tracer; + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java index 59fae1a2d..27c608602 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/SleuthFeignBuilder.java @@ -16,8 +16,7 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; -import org.springframework.cloud.sleuth.Tracer; -import org.springframework.cloud.sleuth.instrument.web.HttpTraceKeysInjector; +import org.springframework.beans.factory.BeanFactory; import feign.Feign; import feign.hystrix.HystrixFeign; @@ -35,11 +34,11 @@ final class SleuthFeignBuilder { private SleuthFeignBuilder() {} - static Feign.Builder builder(Tracer tracer, HttpTraceKeysInjector keysInjector) { + static Feign.Builder builder(BeanFactory beanFactory) { return HystrixFeign.builder() - .client(new TraceFeignClient(tracer, keysInjector)) - .retryer(new TraceFeignRetryer(tracer)) - .decoder(new TraceFeignDecoder(tracer)) - .errorDecoder(new TraceFeignErrorDecoder(tracer)); + .client(new TraceFeignClient(beanFactory)) + .retryer(new TraceFeignRetryer(beanFactory)) + .decoder(new TraceFeignDecoder(beanFactory)) + .errorDecoder(new TraceFeignErrorDecoder(beanFactory)); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClient.java index 0c4af84be..d0807d197 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClient.java @@ -20,7 +20,7 @@ import java.io.IOException; import java.net.URI; import java.util.Objects; -import org.springframework.cloud.sleuth.Tracer; +import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.instrument.web.HttpTraceKeysInjector; import feign.Client; @@ -39,18 +39,16 @@ import feign.RetryableException; final class TraceFeignClient extends FeignEventPublisher implements Client { private final Client delegate; - private final HttpTraceKeysInjector keysInjector; + private HttpTraceKeysInjector keysInjector; - TraceFeignClient(Tracer tracer, HttpTraceKeysInjector keysInjector) { - super(tracer); + TraceFeignClient(BeanFactory beanFactory) { + super(beanFactory); this.delegate = new Client.Default(null, null); - this.keysInjector = keysInjector; } - TraceFeignClient(Tracer tracer, Client delegate, HttpTraceKeysInjector keysInjector) { - super(tracer); + TraceFeignClient(BeanFactory beanFactory, Client delegate) { + super(beanFactory); this.delegate = delegate; - this.keysInjector = keysInjector; } @Override @@ -81,7 +79,14 @@ final class TraceFeignClient extends FeignEventPublisher implements Client { */ private void addRequestTags(Request request) { URI uri = URI.create(request.url()); - this.keysInjector.addRequestTags(uri.toString(), uri.getHost(), uri.getPath(), + getKeysInjector().addRequestTags(uri.toString(), uri.getHost(), uri.getPath(), request.method(), request.headers()); } + + HttpTraceKeysInjector getKeysInjector() { + if (this.keysInjector == null) { + this.keysInjector = this.beanFactory.getBean(HttpTraceKeysInjector.class); + } + return this.keysInjector; + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java index 93fa64c3e..74d0cb7d4 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignClientAutoConfiguration.java @@ -40,7 +40,6 @@ import org.springframework.cloud.netflix.feign.support.SpringDecoder; import org.springframework.cloud.sleuth.SpanInjector; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.instrument.hystrix.SleuthHystrixAutoConfiguration; -import org.springframework.cloud.sleuth.instrument.web.HttpTraceKeysInjector; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Primary; @@ -76,8 +75,8 @@ public class TraceFeignClientAutoConfiguration { @Scope("prototype") @ConditionalOnClass(HystrixCommand.class) @ConditionalOnProperty(name = "feign.hystrix.enabled", matchIfMissing = true) - Feign.Builder feignHystrixBuilder(Tracer tracer, HttpTraceKeysInjector keysInjector) { - return SleuthFeignBuilder.builder(tracer, keysInjector); + Feign.Builder feignHystrixBuilder(BeanFactory beanFactory) { + return SleuthFeignBuilder.builder(beanFactory); } @Configuration @@ -97,8 +96,8 @@ public class TraceFeignClientAutoConfiguration { @Bean @Primary - Decoder feignDecoder(final Tracer tracer) { - return new TraceFeignDecoder(tracer, + Decoder feignDecoder(BeanFactory beanFactory) { + return new TraceFeignDecoder(beanFactory, new ResponseEntityDecoder(new SpringDecoder(this.messageConverters)) { @Override public Object decode(Response response, Type type) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignDecoder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignDecoder.java index d1d304066..e4d130154 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignDecoder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignDecoder.java @@ -19,11 +19,10 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import java.io.IOException; import java.lang.reflect.Type; -import org.springframework.cloud.sleuth.Tracer; +import org.springframework.beans.factory.BeanFactory; import feign.FeignException; import feign.Response; -import feign.codec.DecodeException; import feign.codec.Decoder; /** @@ -37,19 +36,19 @@ final class TraceFeignDecoder extends FeignEventPublisher implements Decoder { private final Decoder delegate; - TraceFeignDecoder(Tracer tracer) { - super(tracer); + TraceFeignDecoder(BeanFactory beanFactory) { + super(beanFactory); this.delegate = new Decoder.Default(); } - TraceFeignDecoder(Tracer tracer, Decoder delegate) { - super(tracer); + TraceFeignDecoder(BeanFactory beanFactory, Decoder delegate) { + super(beanFactory); this.delegate = delegate; } @Override public Object decode(Response response, Type type) - throws IOException, DecodeException, FeignException { + throws IOException, FeignException { try { return this.delegate.decode(response, type); } finally { diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java index dc5104901..ec634fe8d 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignErrorDecoder.java @@ -16,7 +16,7 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; -import org.springframework.cloud.sleuth.Tracer; +import org.springframework.beans.factory.BeanFactory; import feign.Response; import feign.codec.ErrorDecoder; @@ -32,13 +32,13 @@ final class TraceFeignErrorDecoder extends FeignEventPublisher implements ErrorD private final ErrorDecoder delegate; - TraceFeignErrorDecoder(Tracer tracer) { - super(tracer); + TraceFeignErrorDecoder(BeanFactory beanFactory) { + super(beanFactory); this.delegate = new ErrorDecoder.Default(); } - TraceFeignErrorDecoder(Tracer tracer, ErrorDecoder delegate) { - super(tracer); + TraceFeignErrorDecoder(BeanFactory beanFactory, ErrorDecoder delegate) { + super(beanFactory); this.delegate = delegate; } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java index e4dc0022a..be7bc6a48 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java @@ -1,8 +1,6 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; import org.springframework.beans.factory.BeanFactory; -import org.springframework.cloud.sleuth.Tracer; -import org.springframework.cloud.sleuth.instrument.web.HttpTraceKeysInjector; import feign.Client; import feign.Retryer; @@ -18,8 +16,6 @@ import feign.codec.ErrorDecoder; final class TraceFeignObjectWrapper { private final BeanFactory beanFactory; - private Tracer tracer; - private HttpTraceKeysInjector keysInjector; TraceFeignObjectWrapper(BeanFactory beanFactory) { this.beanFactory = beanFactory; @@ -27,28 +23,14 @@ final class TraceFeignObjectWrapper { Object wrap(Object bean) { if (bean instanceof Decoder && !(bean instanceof TraceFeignDecoder)) { - return new TraceFeignDecoder(getTracer(), (Decoder) bean); + return new TraceFeignDecoder(this.beanFactory, (Decoder) bean); } else if (bean instanceof Retryer && !(bean instanceof TraceFeignRetryer)) { - return new TraceFeignRetryer(getTracer(), (Retryer) bean); + return new TraceFeignRetryer(this.beanFactory, (Retryer) bean); } else if (bean instanceof Client && !(bean instanceof TraceFeignClient)) { - return new TraceFeignClient(getTracer(), (Client) bean, getHttpTraceKeysInjector()); + return new TraceFeignClient(this.beanFactory, (Client) bean); } else if (bean instanceof ErrorDecoder && !(bean instanceof TraceFeignErrorDecoder)) { - return new TraceFeignErrorDecoder(getTracer(), (ErrorDecoder) bean); + return new TraceFeignErrorDecoder(this.beanFactory, (ErrorDecoder) bean); } return bean; } - - private Tracer getTracer() { - if (this.tracer == null) { - this.tracer = this.beanFactory.getBean(Tracer.class); - } - return this.tracer; - } - - private HttpTraceKeysInjector getHttpTraceKeysInjector() { - if (this.keysInjector == null) { - this.keysInjector = this.beanFactory.getBean(HttpTraceKeysInjector.class); - } - return this.keysInjector; - } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignRetryer.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignRetryer.java index b0f5b07a2..4fa8cbe4a 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignRetryer.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignRetryer.java @@ -16,6 +16,7 @@ package org.springframework.cloud.sleuth.instrument.web.client.feign; +import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.Tracer; import feign.RetryableException; @@ -33,35 +34,43 @@ import feign.Retryer; */ final class TraceFeignRetryer implements Retryer { - private final Tracer tracer; + private final BeanFactory beanFactory; + private Tracer tracer; private final FeignRequestContext feignRequestContext = FeignRequestContext .getInstance(); private final Retryer delegate; - TraceFeignRetryer(Tracer tracer) { - this(tracer, new Retryer.Default()); + TraceFeignRetryer(BeanFactory beanFactory) { + this(beanFactory, new Retryer.Default()); } - TraceFeignRetryer(Tracer tracer, Retryer delegate) { - this.tracer = tracer; + TraceFeignRetryer(BeanFactory beanFactory, Retryer delegate) { + this.beanFactory = beanFactory; this.delegate = delegate; } @Override public void continueOrPropagate(RetryableException e) { try { - this.feignRequestContext.putSpan(this.tracer.getCurrentSpan(), true); - this.tracer.getCurrentSpan().logEvent("feign.retry"); + this.feignRequestContext.putSpan(getTracer().getCurrentSpan(), true); + getTracer().getCurrentSpan().logEvent("feign.retry"); this.delegate.continueOrPropagate(e); } catch (RetryableException e2) { - this.tracer.close(this.tracer.getCurrentSpan()); + getTracer().close(getTracer().getCurrentSpan()); throw e2; } } @Override public Retryer clone() { - return new TraceFeignRetryer(this.tracer, this.delegate.clone()); + return new TraceFeignRetryer(this.beanFactory, this.delegate.clone()); + } + + Tracer getTracer() { + if (this.tracer == null) { + this.tracer = this.beanFactory.getBean(Tracer.class); + } + return this.tracer; } } \ No newline at end of file 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 32931d4eb..8222c0431 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 @@ -58,6 +58,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.HttpHeaders; import org.springframework.http.ResponseEntity; +import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.rules.SpringClassRule; import org.springframework.test.context.junit4.rules.SpringMethodRule; import org.springframework.web.bind.annotation.RequestHeader; @@ -78,6 +79,7 @@ import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @RunWith(JUnitParamsRunner.class) @SpringApplicationConfiguration(classes = { WebClientTests.TestConfiguration.class }) @WebIntegrationTest(value = { "spring.application.name=fooservice" }, randomPort = true) +@DirtiesContext public class WebClientTests { @ClassRule public static final SpringClassRule SCR = new SpringClassRule(); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java new file mode 100644 index 000000000..dd891e000 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java @@ -0,0 +1,111 @@ +/* + * Copyright 2013-2016 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.instrument.web.client.feign.issues; + +import java.util.ArrayList; +import java.util.List; + +import com.netflix.hystrix.contrib.javanica.annotation.HystrixCommand; + +import org.apache.log4j.Level; +import org.apache.log4j.Logger; +import org.junit.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.SpringApplication; +import org.springframework.boot.autoconfigure.SpringBootApplication; +import org.springframework.cloud.client.circuitbreaker.EnableCircuitBreaker; +import org.springframework.cloud.netflix.feign.EnableFeignClients; +import org.springframework.cloud.netflix.feign.FeignClient; +import org.springframework.cloud.sleuth.sampler.AlwaysSampler; +import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.annotation.Bean; +import org.springframework.stereotype.Component; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.client.RestTemplate; + +public class Issue307Tests { + + @Test + public void should_start_context() { + try (ConfigurableApplicationContext applicationContext = SpringApplication + .run(SleuthSampleApplication.class, "")) { + } + } +} + +@SpringBootApplication +@RestController +@EnableFeignClients +@EnableCircuitBreaker +class SleuthSampleApplication { + + private static final Logger LOG = Logger.getLogger(SleuthSampleApplication.class.getName()); + + @Autowired + private RestTemplate restTemplate; + + @Autowired + private ParticipantsBean participantsBean; + + @Bean + public RestTemplate getRestTemplate() { + return new RestTemplate(); + } + + @Bean + public AlwaysSampler defaultSampler() { + return new AlwaysSampler(); + } + + @RequestMapping("/") + public String home() { + LOG.log(Level.INFO, "you called home"); + return "Hello World"; + } + + @RequestMapping("/callhome") + public String callHome() { + LOG.log(Level.INFO, "calling home"); + return restTemplate.getForObject("http://localhost:8080", String.class); + } +} + +@Component +class ParticipantsBean { + @Autowired + private ParticipantsClient participantsClient; + + @HystrixCommand(fallbackMethod = "defaultParticipants") + public List getParticipants(String raceId) { + return participantsClient.getParticipants(raceId); + } + + public List defaultParticipants(String raceId) { + return new ArrayList<>(); + } +} + +@FeignClient("participants") +interface ParticipantsClient { + + @RequestMapping(method = RequestMethod.GET, value="/races/{raceId}") + List getParticipants(@PathVariable("raceId") String raceId); + +} diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java index 516299e70..a2456630e 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java @@ -31,6 +31,7 @@ import org.springframework.boot.test.WebIntegrationTest; import org.springframework.cloud.sleuth.zipkin.ZipkinSpanReporter; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.TestPropertySource; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -46,6 +47,7 @@ import static org.assertj.core.api.BDDAssertions.then; @SpringApplicationConfiguration(classes = { IntegrationSpanCollectorConfig.class, SampleMessagingApplication.class }) @WebIntegrationTest @TestPropertySource(properties="sample.zipkin.enabled=true") +@DirtiesContext public class MessagingApplicationTests extends AbstractIntegrationTest { private static int port = 3381; From b7b8b30c216cef07e997a9f55cbaa3bc7d2b7f61 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 22 Jun 2016 00:24:09 +0200 Subject: [PATCH 03/10] Fixing tests for Jenkins --- .../instrument/web/client/feign/issues/Issue307Tests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java index dd891e000..c2d491f09 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/issues/Issue307Tests.java @@ -45,7 +45,7 @@ public class Issue307Tests { @Test public void should_start_context() { try (ConfigurableApplicationContext applicationContext = SpringApplication - .run(SleuthSampleApplication.class, "")) { + .run(SleuthSampleApplication.class, "--spring.jmx.enabled=false")) { } } } From be735659b6cc630430ae7c3e9cd2a9c81bd9d001 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 22 Jun 2016 01:14:53 +0200 Subject: [PATCH 04/10] Wrong span / trace ids dont blow up the app with this change we're trying to retrieve the value of span / trace id from a header but we don't propagate the exception if that value is invalid. Instead we generate a new random value. It's better not to break the application and break the trace (if by any chance it's been corrupt). Fixed #306 --- .../messaging/MessagingSpanExtractor.java | 50 +++++++- .../web/HttpServletRequestExtractor.java | 45 +++++-- .../web/TraceWebAutoConfiguration.java | 5 +- .../MessagingSpanExtractorTests.java | 110 ++++++++++++++++++ .../web/HttpServletRequestExtractorTests.java | 92 +++++++++++++++ .../TraceFilterMockChainIntegrationTests.java | 6 +- .../instrument/web/TraceFilterTests.java | 2 +- 7 files changed, 292 insertions(+), 18 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java index b3dc9178f..a51c9cf7b 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java @@ -16,8 +16,11 @@ package org.springframework.cloud.sleuth.instrument.messaging; +import java.lang.invoke.MethodHandles; import java.util.Random; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Span.SpanBuilder; import org.springframework.cloud.sleuth.SpanExtractor; @@ -31,6 +34,8 @@ import org.springframework.messaging.Message; */ public class MessagingSpanExtractor implements SpanExtractor> { + private static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass()); + private final Random random; public MessagingSpanExtractor(Random random) { @@ -44,14 +49,13 @@ public class MessagingSpanExtractor implements SpanExtractor> { return null; // TODO: Consider throwing IllegalArgumentException; } + long traceId = getTraceIdOrSetDefault(carrier); long spanId = hasHeader(carrier, Span.SPAN_ID_NAME) - ? Span.hexToId(getHeader(carrier, Span.SPAN_ID_NAME)) + ? getSpanIdOrSetDefault(carrier) : this.random.nextLong(); - long traceId = Span.hexToId(getHeader(carrier, Span.TRACE_ID_NAME)); SpanBuilder spanBuilder = Span.builder().traceId(traceId).spanId(spanId); spanBuilder.exportable( Span.SPAN_SAMPLED.equals(getHeader(carrier, Span.SAMPLED_NAME))); - String parentId = getHeader(carrier, Span.PARENT_ID_NAME); String processId = getHeader(carrier, Span.PROCESS_ID_NAME); String spanName = getHeader(carrier, Span.SPAN_NAME_NAME); if (spanName != null) { @@ -60,9 +64,7 @@ public class MessagingSpanExtractor implements SpanExtractor> { if (processId != null) { spanBuilder.processId(processId); } - if (parentId != null) { - spanBuilder.parent(Span.hexToId(parentId)); - } + setParentIdIfApplicable(carrier, spanBuilder); spanBuilder.remote(true); return spanBuilder.build(); } @@ -78,4 +80,40 @@ public class MessagingSpanExtractor implements SpanExtractor> { boolean hasHeader(Message message, String name) { return message.getHeaders().containsKey(name); } + + private long getTraceIdOrSetDefault(Message carrier) { + try { + return Span + .hexToId(getHeader(carrier, Span.TRACE_ID_NAME)); + } catch (Exception e) { + long id = this.random.nextLong(); + log.warn("Exception occurred while trying to retrieve the trace " + + "id from headers. Will set id to value [" + + Span.idToHex(id) + "]", e); + return id; + } + } + private void setParentIdIfApplicable(Message carrier, SpanBuilder spanBuilder) { + try { + String parentId = getHeader(carrier, Span.PARENT_ID_NAME); + if (parentId != null) { + spanBuilder.parent(Span.hexToId(parentId)); + } + } catch (Exception e) { + log.warn("Exception occurred while trying to set parentId", e); + } + } + + private long getSpanIdOrSetDefault(Message carrier) { + try { + return Span + .hexToId(getHeader(carrier, Span.SPAN_ID_NAME)); + } catch (Exception e) { + long id = this.random.nextLong(); + log.warn("Exception occurred while trying to retrieve the span " + + "id from headers. Will set id to value [" + + Span.idToHex(id) + "]", e); + return id; + } + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java index f3748bae9..45527099b 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java @@ -18,6 +18,7 @@ package org.springframework.cloud.sleuth.instrument.web; import javax.servlet.http.HttpServletRequest; import java.lang.invoke.MethodHandles; +import java.util.Random; import java.util.regex.Pattern; import org.apache.commons.logging.Log; @@ -41,11 +42,13 @@ class HttpServletRequestExtractor implements SpanExtractor { private static final String HTTP_COMPONENT = "http"; private final Pattern skipPattern; + private final Random random; private UrlPathHelper urlPathHelper = new UrlPathHelper(); - public HttpServletRequestExtractor(Pattern skipPattern) { + public HttpServletRequestExtractor(Pattern skipPattern, Random random) { this.skipPattern = skipPattern; + this.random = random; } @Override @@ -57,12 +60,24 @@ class HttpServletRequestExtractor implements SpanExtractor { String uri = this.urlPathHelper.getPathWithinApplication(carrier); boolean skip = this.skipPattern.matcher(uri).matches() || Span.SPAN_NOT_SAMPLED.equals(carrier.getHeader(Span.SAMPLED_NAME)); - long traceId = Span - .hexToId(carrier.getHeader(Span.TRACE_ID_NAME)); + long traceId = getTraceIdOrSetDefault(carrier); long spanId = spanId(carrier, traceId); return buildParentSpan(carrier, uri, skip, traceId, spanId); } + private long getTraceIdOrSetDefault(HttpServletRequest carrier) { + try { + return Span + .hexToId(carrier.getHeader(Span.TRACE_ID_NAME)); + } catch (Exception e) { + long id = this.random.nextLong(); + log.warn("Exception occurred while trying to retrieve the trace " + + "id from headers. Will set id to value [" + + Span.idToHex(id) + "]", e); + return id; + } + } + private long spanId(HttpServletRequest carrier, long traceId) { String spanId = carrier.getHeader(Span.SPAN_ID_NAME); if (spanId == null) { @@ -70,7 +85,15 @@ class HttpServletRequestExtractor implements SpanExtractor { + "a root span with span id equal to trace id"); return traceId; } else { - return Span.hexToId(spanId); + try { + return Span.hexToId(spanId); + } catch (Exception e) { + long id = this.random.nextLong(); + log.warn("Exception occurred while trying to retrieve the span id " + + "from request headers. Will set id to value [" + + Span.idToHex(id) + "]", e); + return id; + } } } @@ -83,14 +106,13 @@ class HttpServletRequestExtractor implements SpanExtractor { span.name(parentName); } else { - span.name(HTTP_COMPONENT + ":" + "/parent" + uri); + span.name(HTTP_COMPONENT + ":/parent" + uri); } if (StringUtils.hasText(processId)) { span.processId(processId); } if (carrier.getHeader(Span.PARENT_ID_NAME) != null) { - span.parent(Span - .hexToId(carrier.getHeader(Span.PARENT_ID_NAME))); + setParentIdIfValid(carrier, span); } span.remote(true); if (skip) { @@ -98,4 +120,13 @@ class HttpServletRequestExtractor implements SpanExtractor { } return span.build(); } + + private void setParentIdIfValid(HttpServletRequest carrier, SpanBuilder span) { + try { + span.parent(Span + .hexToId(carrier.getHeader(Span.PARENT_ID_NAME))); + } catch (Exception e) { + log.warn("Exception occurred while trying to set parent id", e); + } + } } 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 47c5bee0d..763850d63 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 @@ -15,6 +15,7 @@ */ package org.springframework.cloud.sleuth.instrument.web; +import java.util.Random; import java.util.regex.Pattern; import javax.servlet.http.HttpServletRequest; @@ -100,8 +101,8 @@ public class TraceWebAutoConfiguration { @Bean public SpanExtractor httpServletRequestSpanExtractor( - SkipPatternProvider skipPatternProvider) { - return new HttpServletRequestExtractor(skipPatternProvider.skipPattern()); + SkipPatternProvider skipPatternProvider, Random random) { + return new HttpServletRequestExtractor(skipPatternProvider.skipPattern(), random); } @Bean diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java new file mode 100644 index 000000000..8da2be3f7 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java @@ -0,0 +1,110 @@ +/* + * Copyright 2013-2016 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.instrument.messaging; + +import java.util.HashMap; +import java.util.Map; +import java.util.Random; + +import org.junit.Test; +import org.springframework.cloud.sleuth.Span; +import org.springframework.messaging.Message; +import org.springframework.messaging.MessageHeaders; +import org.springframework.messaging.support.MessageBuilder; +import org.springframework.util.StringUtils; + +import static org.assertj.core.api.BDDAssertions.then; +import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; + +public class MessagingSpanExtractorTests { + MessagingSpanExtractor extractor = new MessagingSpanExtractor(new Random()); + + @Test + public void should_return_null_if_trace_or_span_is_missing() { + Message message = MessageBuilder.createMessage("", headers()); + then(this.extractor.joinTrace(message)).isNull(); + + message = MessageBuilder.createMessage("", headers("trace")); + then(this.extractor.joinTrace(message)).isNull(); + } + + @Test + public void should_set_random_traceid_if_header_value_is_invalid() { + Message message = MessageBuilder.createMessage("", + headers("invalid", randomId())); + + Span span = this.extractor.joinTrace(message); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + } + + @Test + public void should_set_random_spanid_if_header_value_is_invalid() { + Message message = MessageBuilder.createMessage("", + headers(randomId(), "invalid")); + + Span span = this.extractor.joinTrace(message); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + then(span.getSpanId()).isNotZero(); + } + + @Test + public void should_not_throw_exception_if_parent_id_is_invalid() { + Message message = MessageBuilder.createMessage("", + headers(randomId(), randomId(), "invalid")); + + Span span = this.extractor.joinTrace(message); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + then(span.getSpanId()).isNotZero(); + then(span.getParents()).isEmpty(); + } + + private MessageHeaders headers() { + return headers(null, null, null); + } + + private MessageHeaders headers(String traceId) { + return headers(traceId, null, null); + } + + private MessageHeaders headers(String traceId, String spanId) { + return headers(traceId, spanId, null); + } + + private MessageHeaders headers(String traceId, String spanId, String parentId) { + Map map = new HashMap<>(); + if (StringUtils.hasText(traceId)) { + map.put(Span.TRACE_ID_NAME, traceId); + } + if (StringUtils.hasText(spanId)) { + map.put(Span.SPAN_ID_NAME, spanId); + } + if (StringUtils.hasText(parentId)) { + map.put(Span.PARENT_ID_NAME, parentId); + } + return new MessageHeaders(map); + } + + private String randomId() { + return String.valueOf(new Random().nextLong()); + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java new file mode 100644 index 000000000..223e996b6 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java @@ -0,0 +1,92 @@ +/* + * Copyright 2013-2016 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.cloud.sleuth.instrument.web; + +import javax.servlet.http.HttpServletRequest; +import java.util.Random; +import java.util.regex.Pattern; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.BDDMockito; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.cloud.sleuth.Span; + +import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; + +@RunWith(MockitoJUnitRunner.class) +public class HttpServletRequestExtractorTests { + + @Mock HttpServletRequest request; + HttpServletRequestExtractor extractor = new HttpServletRequestExtractor( + Pattern.compile(""), new Random()); + + @Before + public void setup() { + BDDMockito.given(this.request.getRequestURI()).willReturn("http://foo.com"); + BDDMockito.given(this.request.getContextPath()).willReturn("/"); + } + + @Test + public void should_return_null_if_there_is_no_trace_id() { + then(extractor.joinTrace(request)).isNull(); + } + + @Test + public void should_set_random_traceid_if_header_value_is_invalid() { + BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME)) + .willReturn("invalid"); + + Span span = this.extractor.joinTrace(this.request); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + } + + @Test + public void should_set_random_spanid_if_header_value_is_invalid() { + BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME)) + .willReturn(String.valueOf(new Random().nextLong())); + BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME)) + .willReturn("invalid"); + + Span span = this.extractor.joinTrace(this.request); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + then(span.getSpanId()).isNotZero(); + } + + @Test + public void should_not_throw_exception_if_parent_id_is_invalid() { + BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME)) + .willReturn(String.valueOf(new Random().nextLong())); + BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME)) + .willReturn(String.valueOf(new Random().nextLong())); + BDDMockito.given(this.request.getHeader(Span.PARENT_ID_NAME)) + .willReturn("invalid"); + + Span span = this.extractor.joinTrace(this.request); + + then(span).isNotNull(); + then(span.getTraceId()).isNotZero(); + then(span.getSpanId()).isNotZero(); + then(span.getParents()).isEmpty(); + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java index f8dbb0134..c62a7d1ca 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java @@ -73,7 +73,8 @@ public class TraceFilterMockChainIntegrationTests { @Test public void startsNewTrace() throws Exception { TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(), - new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)), + new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN), + new Random()), new HttpServletResponseInjector(), keysInjector); filter.doFilter(this.request, this.response, this.filterChain); assertNull(TestSpanContextHolder.getCurrentSpan()); @@ -85,7 +86,8 @@ public class TraceFilterMockChainIntegrationTests { this.request = builder().header(Span.SPAN_ID_NAME, generator.nextLong()) .header(Span.TRACE_ID_NAME, generator.nextLong()).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(), - new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)), + new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN), + new Random()), new HttpServletResponseInjector(), keysInjector); filter.doFilter(this.request, this.response, this.filterChain); assertNull(TestSpanContextHolder.getCurrentSpan()); 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 427b49424..9030e5d59 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 @@ -63,7 +63,7 @@ public class TraceFilterTests { @Mock SpanLogger spanLogger; ArrayListSpanAccumulator spanReporter = new ArrayListSpanAccumulator(); SpanExtractor spanExtractor = new HttpServletRequestExtractor(Pattern - .compile(TraceFilter.DEFAULT_SKIP_PATTERN)); + .compile(TraceFilter.DEFAULT_SKIP_PATTERN), new Random()); SpanInjector spanInjector = new HttpServletResponseInjector(); private Tracer tracer; From 0d6f27f43a16e39235ca728b04aa602539c34d5a Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 22 Jun 2016 14:09:12 +0200 Subject: [PATCH 05/10] Throwing exception when tracing is malformed (#308) with this change we will throw an IllegalArgumentException when the malformed tracing data are sent. In TraceFilter we're catching it and sending back 400 response. In case of messaging the exception gets propagated fixed #306 * Changes following review * Changes following review --- .../springframework/cloud/sleuth/Span.java | 6 ++- .../messaging/MessagingSpanExtractor.java | 45 +++---------------- .../web/HttpServletRequestExtractor.java | 43 +++--------------- .../sleuth/instrument/web/TraceFilter.java | 9 +++- .../web/TraceWebAutoConfiguration.java | 5 +-- .../cloud/sleuth/SpanTests.java | 5 +++ .../MessagingSpanExtractorTests.java | 34 +++++++------- .../web/HttpServletRequestExtractorTests.java | 36 ++++++++------- .../TraceFilterMockChainIntegrationTests.java | 6 +-- .../instrument/web/TraceFilterTests.java | 15 ++++++- 10 files changed, 87 insertions(+), 117 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/Span.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/Span.java index 4fe24b6e4..5034aad0f 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/Span.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/Span.java @@ -371,7 +371,11 @@ public class Span { */ public static long hexToId(String hexString) { Assert.hasText(hexString, "Can't convert empty hex string to long"); - return new BigInteger(hexString, 16).longValue(); + try { + return new BigInteger(hexString, 16).longValue(); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Malformed id [" + hexString + "]", e); + } } @Override diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java index a51c9cf7b..92bd1a780 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java @@ -16,11 +16,8 @@ package org.springframework.cloud.sleuth.instrument.messaging; -import java.lang.invoke.MethodHandles; import java.util.Random; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Span.SpanBuilder; import org.springframework.cloud.sleuth.SpanExtractor; @@ -34,8 +31,6 @@ import org.springframework.messaging.Message; */ public class MessagingSpanExtractor implements SpanExtractor> { - private static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass()); - private final Random random; public MessagingSpanExtractor(Random random) { @@ -49,9 +44,10 @@ public class MessagingSpanExtractor implements SpanExtractor> { return null; // TODO: Consider throwing IllegalArgumentException; } - long traceId = getTraceIdOrSetDefault(carrier); + long traceId = Span + .hexToId(getHeader(carrier, Span.TRACE_ID_NAME)); long spanId = hasHeader(carrier, Span.SPAN_ID_NAME) - ? getSpanIdOrSetDefault(carrier) + ? Span.hexToId(getHeader(carrier, Span.SPAN_ID_NAME)) : this.random.nextLong(); SpanBuilder spanBuilder = Span.builder().traceId(traceId).spanId(spanId); spanBuilder.exportable( @@ -81,39 +77,10 @@ public class MessagingSpanExtractor implements SpanExtractor> { return message.getHeaders().containsKey(name); } - private long getTraceIdOrSetDefault(Message carrier) { - try { - return Span - .hexToId(getHeader(carrier, Span.TRACE_ID_NAME)); - } catch (Exception e) { - long id = this.random.nextLong(); - log.warn("Exception occurred while trying to retrieve the trace " - + "id from headers. Will set id to value [" - + Span.idToHex(id) + "]", e); - return id; - } - } private void setParentIdIfApplicable(Message carrier, SpanBuilder spanBuilder) { - try { - String parentId = getHeader(carrier, Span.PARENT_ID_NAME); - if (parentId != null) { - spanBuilder.parent(Span.hexToId(parentId)); - } - } catch (Exception e) { - log.warn("Exception occurred while trying to set parentId", e); - } - } - - private long getSpanIdOrSetDefault(Message carrier) { - try { - return Span - .hexToId(getHeader(carrier, Span.SPAN_ID_NAME)); - } catch (Exception e) { - long id = this.random.nextLong(); - log.warn("Exception occurred while trying to retrieve the span " - + "id from headers. Will set id to value [" - + Span.idToHex(id) + "]", e); - return id; + String parentId = getHeader(carrier, Span.PARENT_ID_NAME); + if (parentId != null) { + spanBuilder.parent(Span.hexToId(parentId)); } } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java index 45527099b..d0d0f9f21 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java @@ -18,7 +18,6 @@ package org.springframework.cloud.sleuth.instrument.web; import javax.servlet.http.HttpServletRequest; import java.lang.invoke.MethodHandles; -import java.util.Random; import java.util.regex.Pattern; import org.apache.commons.logging.Log; @@ -42,13 +41,11 @@ class HttpServletRequestExtractor implements SpanExtractor { private static final String HTTP_COMPONENT = "http"; private final Pattern skipPattern; - private final Random random; private UrlPathHelper urlPathHelper = new UrlPathHelper(); - public HttpServletRequestExtractor(Pattern skipPattern, Random random) { + public HttpServletRequestExtractor(Pattern skipPattern) { this.skipPattern = skipPattern; - this.random = random; } @Override @@ -60,24 +57,12 @@ class HttpServletRequestExtractor implements SpanExtractor { String uri = this.urlPathHelper.getPathWithinApplication(carrier); boolean skip = this.skipPattern.matcher(uri).matches() || Span.SPAN_NOT_SAMPLED.equals(carrier.getHeader(Span.SAMPLED_NAME)); - long traceId = getTraceIdOrSetDefault(carrier); + long traceId = Span + .hexToId(carrier.getHeader(Span.TRACE_ID_NAME)); long spanId = spanId(carrier, traceId); return buildParentSpan(carrier, uri, skip, traceId, spanId); } - private long getTraceIdOrSetDefault(HttpServletRequest carrier) { - try { - return Span - .hexToId(carrier.getHeader(Span.TRACE_ID_NAME)); - } catch (Exception e) { - long id = this.random.nextLong(); - log.warn("Exception occurred while trying to retrieve the trace " - + "id from headers. Will set id to value [" - + Span.idToHex(id) + "]", e); - return id; - } - } - private long spanId(HttpServletRequest carrier, long traceId) { String spanId = carrier.getHeader(Span.SPAN_ID_NAME); if (spanId == null) { @@ -85,15 +70,7 @@ class HttpServletRequestExtractor implements SpanExtractor { + "a root span with span id equal to trace id"); return traceId; } else { - try { - return Span.hexToId(spanId); - } catch (Exception e) { - long id = this.random.nextLong(); - log.warn("Exception occurred while trying to retrieve the span id " - + "from request headers. Will set id to value [" - + Span.idToHex(id) + "]", e); - return id; - } + return Span.hexToId(spanId); } } @@ -112,7 +89,8 @@ class HttpServletRequestExtractor implements SpanExtractor { span.processId(processId); } if (carrier.getHeader(Span.PARENT_ID_NAME) != null) { - setParentIdIfValid(carrier, span); + span.parent(Span + .hexToId(carrier.getHeader(Span.PARENT_ID_NAME))); } span.remote(true); if (skip) { @@ -120,13 +98,4 @@ class HttpServletRequestExtractor implements SpanExtractor { } return span.build(); } - - private void setParentIdIfValid(HttpServletRequest carrier, SpanBuilder span) { - try { - span.parent(Span - .hexToId(carrier.getHeader(Span.PARENT_ID_NAME))); - } catch (Exception e) { - log.warn("Exception occurred while trying to set parent id", e); - } - } } 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 cc1db1df8..d5884fc61 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 @@ -152,7 +152,14 @@ public class TraceFilter extends GenericFilterBean { } addToResponseIfNotPresent(response, Span.SAMPLED_NAME, skip ? Span.SPAN_NOT_SAMPLED : Span.SPAN_SAMPLED); String name = HTTP_COMPONENT + ":" + uri; - spanFromRequest = createSpan(request, skip, spanFromRequest, name); + try { + spanFromRequest = createSpan(request, skip, spanFromRequest, name); + } catch (IllegalArgumentException e) { + filterChain.doFilter(request, response); + response.sendError(HttpStatus.BAD_REQUEST.value(), + "Exception tracing request [" + e.getMessage() + "]"); + return; + } Throwable exception = null; try { this.spanInjector.inject(spanFromRequest, response); 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 763850d63..47c5bee0d 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 @@ -15,7 +15,6 @@ */ package org.springframework.cloud.sleuth.instrument.web; -import java.util.Random; import java.util.regex.Pattern; import javax.servlet.http.HttpServletRequest; @@ -101,8 +100,8 @@ public class TraceWebAutoConfiguration { @Bean public SpanExtractor httpServletRequestSpanExtractor( - SkipPatternProvider skipPatternProvider, Random random) { - return new HttpServletRequestExtractor(skipPatternProvider.skipPattern(), random); + SkipPatternProvider skipPatternProvider) { + return new HttpServletRequestExtractor(skipPatternProvider.skipPattern()); } @Bean diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/SpanTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/SpanTests.java index 7a09ae389..c8eb2a071 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/SpanTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/SpanTests.java @@ -109,4 +109,9 @@ public class SpanTests { then(deserialized.tags()) .isEqualTo(span.tags()); } + + @Test(expected = IllegalArgumentException.class) + public void should_throw_exception_when_converting_invalid_hex_value() { + Span.hexToId("invalid"); + } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java index 8da2be3f7..acd296951 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java @@ -27,6 +27,7 @@ import org.springframework.messaging.MessageHeaders; import org.springframework.messaging.support.MessageBuilder; import org.springframework.util.StringUtils; +import static org.assertj.core.api.Assertions.fail; import static org.assertj.core.api.BDDAssertions.then; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @@ -47,10 +48,12 @@ public class MessagingSpanExtractorTests { Message message = MessageBuilder.createMessage("", headers("invalid", randomId())); - Span span = this.extractor.joinTrace(message); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); + try { + this.extractor.joinTrace(message); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } @Test @@ -58,11 +61,12 @@ public class MessagingSpanExtractorTests { Message message = MessageBuilder.createMessage("", headers(randomId(), "invalid")); - Span span = this.extractor.joinTrace(message); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); - then(span.getSpanId()).isNotZero(); + try { + this.extractor.joinTrace(message); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } @Test @@ -70,12 +74,12 @@ public class MessagingSpanExtractorTests { Message message = MessageBuilder.createMessage("", headers(randomId(), randomId(), "invalid")); - Span span = this.extractor.joinTrace(message); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); - then(span.getSpanId()).isNotZero(); - then(span.getParents()).isEmpty(); + try { + this.extractor.joinTrace(message); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } private MessageHeaders headers() { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java index 223e996b6..fc660f2ad 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java @@ -28,6 +28,7 @@ import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.cloud.sleuth.Span; +import static org.assertj.core.api.Assertions.fail; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @RunWith(MockitoJUnitRunner.class) @@ -35,7 +36,7 @@ public class HttpServletRequestExtractorTests { @Mock HttpServletRequest request; HttpServletRequestExtractor extractor = new HttpServletRequestExtractor( - Pattern.compile(""), new Random()); + Pattern.compile("")); @Before public void setup() { @@ -53,10 +54,12 @@ public class HttpServletRequestExtractorTests { BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME)) .willReturn("invalid"); - Span span = this.extractor.joinTrace(this.request); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); + try { + this.extractor.joinTrace(this.request); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } @Test @@ -66,11 +69,12 @@ public class HttpServletRequestExtractorTests { BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME)) .willReturn("invalid"); - Span span = this.extractor.joinTrace(this.request); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); - then(span.getSpanId()).isNotZero(); + try { + this.extractor.joinTrace(this.request); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } @Test @@ -82,11 +86,11 @@ public class HttpServletRequestExtractorTests { BDDMockito.given(this.request.getHeader(Span.PARENT_ID_NAME)) .willReturn("invalid"); - Span span = this.extractor.joinTrace(this.request); - - then(span).isNotNull(); - then(span.getTraceId()).isNotZero(); - then(span.getSpanId()).isNotZero(); - then(span.getParents()).isEmpty(); + try { + this.extractor.joinTrace(this.request); + fail("should throw an exception"); + } catch (IllegalArgumentException e) { + then(e).hasMessageContaining("Malformed id"); + } } } \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java index c62a7d1ca..f8dbb0134 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java @@ -73,8 +73,7 @@ public class TraceFilterMockChainIntegrationTests { @Test public void startsNewTrace() throws Exception { TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(), - new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN), - new Random()), + new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)), new HttpServletResponseInjector(), keysInjector); filter.doFilter(this.request, this.response, this.filterChain); assertNull(TestSpanContextHolder.getCurrentSpan()); @@ -86,8 +85,7 @@ public class TraceFilterMockChainIntegrationTests { this.request = builder().header(Span.SPAN_ID_NAME, generator.nextLong()) .header(Span.TRACE_ID_NAME, generator.nextLong()).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(), - new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN), - new Random()), + new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)), new HttpServletResponseInjector(), keysInjector); filter.doFilter(this.request, this.response, this.filterChain); assertNull(TestSpanContextHolder.getCurrentSpan()); 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 9030e5d59..df9e1eac9 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 @@ -63,7 +63,7 @@ public class TraceFilterTests { @Mock SpanLogger spanLogger; ArrayListSpanAccumulator spanReporter = new ArrayListSpanAccumulator(); SpanExtractor spanExtractor = new HttpServletRequestExtractor(Pattern - .compile(TraceFilter.DEFAULT_SKIP_PATTERN), new Random()); + .compile(TraceFilter.DEFAULT_SKIP_PATTERN)); SpanInjector spanInjector = new HttpServletResponseInjector(); private Tracer tracer; @@ -317,6 +317,19 @@ public class TraceFilterTests { then(TestSpanContextHolder.getCurrentSpan()).isNull(); } + @Test + public void returns400IfSpanIsMalformed() throws Exception { + this.request = builder().header(Span.SPAN_ID_NAME, "asd") + .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); + TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, + this.spanExtractor, this.spanInjector, this.httpTraceKeysInjector); + + filter.doFilter(this.request, this.response, this.filterChain); + + then(TestSpanContextHolder.getCurrentSpan()).isNull(); + then(this.response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + } + public void verifyParentSpanHttpTags() { verifyParentSpanHttpTags(HttpStatus.OK); } From 31a0bd885fbbd8e0f11aad761431f9e891b073d5 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Sat, 11 Jun 2016 09:22:52 +0200 Subject: [PATCH 06/10] Added connection factory to rest template for tests --- .../instrument/zuul/TraceZuulIntegrationTests.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulIntegrationTests.java index 7de79b9a1..c7810c18d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulIntegrationTests.java @@ -4,6 +4,10 @@ import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.HashMap; +import com.netflix.loadbalancer.Server; +import com.netflix.loadbalancer.ServerList; +import com.netflix.zuul.context.RequestContext; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.Before; @@ -36,6 +40,7 @@ import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.http.client.ClientHttpResponse; +import org.springframework.http.client.HttpComponentsClientHttpRequestFactory; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; import org.springframework.test.context.web.WebAppConfiguration; @@ -44,10 +49,6 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.DefaultResponseErrorHandler; import org.springframework.web.client.RestTemplate; -import com.netflix.loadbalancer.Server; -import com.netflix.loadbalancer.ServerList; -import com.netflix.zuul.context.RequestContext; - import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @RunWith(SpringJUnit4ClassRunner.class) @@ -154,7 +155,9 @@ class SampleZuulProxyApplication { } @Bean RestTemplate restTemplate() { - RestTemplate restTemplate = new RestTemplate(); + HttpComponentsClientHttpRequestFactory factory = new HttpComponentsClientHttpRequestFactory(); + factory.setReadTimeout(5000); + RestTemplate restTemplate = new RestTemplate(factory); restTemplate.setErrorHandler(new DefaultResponseErrorHandler() { @Override public void handleError(ClientHttpResponse response) throws IOException { From 34cf968bffd19d77a38f53492118f095ec42d6c9 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 17 Jun 2016 10:22:32 +0200 Subject: [PATCH 07/10] Randomized port for Zipkin tests --- .../test/java/integration/ZipkinTests.java | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin/src/test/java/integration/ZipkinTests.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin/src/test/java/integration/ZipkinTests.java index c6b2c9dec..c8dc7abd3 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin/src/test/java/integration/ZipkinTests.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-zipkin/src/test/java/integration/ZipkinTests.java @@ -15,6 +15,7 @@ */ package integration; +import java.net.URI; import java.util.Random; import org.apache.commons.logging.Log; @@ -22,6 +23,7 @@ import org.apache.commons.logging.LogFactory; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.SpringApplication; import org.springframework.boot.autoconfigure.SpringBootApplication; @@ -33,8 +35,10 @@ import org.springframework.cloud.sleuth.zipkin.ZipkinProperties; import org.springframework.cloud.sleuth.zipkin.ZipkinSpanReporter; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Primary; import org.springframework.test.context.TestPropertySource; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.util.SocketUtils; import integration.ZipkinTests.WaitUntilZipkinIsUpConfig; import sample.SampleZipkinApplication; @@ -52,13 +56,22 @@ public class ZipkinTests extends AbstractIntegrationTest { @Value("${local.server.port}") private int port = 3380; private String sampleAppUrl = "http://localhost:" + this.port; + @Autowired ZipkinProperties zipkinProperties; @Before public void setup() { - ZipkinServer.main(new String[] { "--server.port=9411" }); + ZipkinServer.main(new String[] { "--server.port=" + getPortFromProps() }); await().until(zipkinQueryServerIsUp()); } + @Override protected int getZipkinServerPort() { + return getPortFromProps(); + } + + private int getPortFromProps() { + return URI.create(this.zipkinProperties.getBaseUrl()).getPort(); + } + @Test public void should_propagate_spans_to_zipkin() { long traceId = new Random().nextLong(); @@ -79,6 +92,15 @@ public class ZipkinTests extends AbstractIntegrationTest { private static final Log log = LogFactory.getLog(WaitUntilZipkinIsUpConfig.class); + @Bean + @Primary + ZipkinProperties testZipkinProperties() { + int freePort = SocketUtils.findAvailableTcpPort(); + ZipkinProperties zipkinProperties = new ZipkinProperties(); + zipkinProperties.setBaseUrl("http://localhost:" + freePort); + return zipkinProperties; + } + @Bean public ZipkinSpanReporter spanCollector(final ZipkinProperties zipkin, final SpanMetricReporter spanMetricReporter) { From 187c58aceb4214c4e6e110196dd5e0034dca7e6f Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 17 Jun 2016 10:23:47 +0200 Subject: [PATCH 08/10] Fixed the issue with Zuul and error code --- .../instrument/zuul/TracePostZuulFilter.java | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java index 08ce2eb41..3ad574f14 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java @@ -18,14 +18,15 @@ package org.springframework.cloud.sleuth.instrument.zuul; import java.lang.invoke.MethodHandles; +import com.netflix.zuul.ZuulFilter; +import com.netflix.zuul.context.RequestContext; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.TraceKeys; import org.springframework.cloud.sleuth.Tracer; - -import com.netflix.zuul.ZuulFilter; -import com.netflix.zuul.context.RequestContext; +import org.springframework.http.HttpStatus; /**8 * A post request {@link ZuulFilter} that publishes an event upon start of the filtering @@ -57,12 +58,27 @@ public class TracePostZuulFilter extends ZuulFilter { if (log.isDebugEnabled()) { log.debug("Closing current client span " + getCurrentSpan() + ""); } - this.tracer.addTag(this.traceKeys.getHttp().getStatusCode(), - String.valueOf(RequestContext.getCurrentContext().getResponse().getStatus())); + int httpStatus = RequestContext.getCurrentContext().getResponse().getStatus(); + if (httpStatus > 0) { + this.tracer.addTag(this.traceKeys.getHttp().getStatusCode(), + String.valueOf(httpStatus)); + } this.tracer.close(getCurrentSpan()); + closeParentSpanIfResponseIsNotSuccess(httpStatus); return null; } + private void closeParentSpanIfResponseIsNotSuccess(int httpStatus) { + if (httpStatus > 0 && httpStatusIsNotSuccess(httpStatus)) { + this.tracer.close(getCurrentSpan()); + } + } + + private boolean httpStatusIsNotSuccess(int httpStatus) { + return HttpStatus.valueOf(httpStatus).is4xxClientError() || + HttpStatus.valueOf(httpStatus).is5xxServerError(); + } + @Override public String filterType() { return "post"; From f92a84ed1ce91ca0f2ddb272f1a1a4a28622e4d9 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 23 Jun 2016 14:41:47 +0800 Subject: [PATCH 09/10] Updates to zipkin 1.1.5 (#309) Changes since 1.1.1 related to cassandra or json parsing. See https://github.com/openzipkin/zipkin/releases/tag/1.1.5 --- spring-cloud-sleuth-dependencies/pom.xml | 2 +- spring-cloud-sleuth-samples/pom.xml | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spring-cloud-sleuth-dependencies/pom.xml b/spring-cloud-sleuth-dependencies/pom.xml index 22fb4b8e3..8f25f6aaf 100644 --- a/spring-cloud-sleuth-dependencies/pom.xml +++ b/spring-cloud-sleuth-dependencies/pom.xml @@ -16,7 +16,7 @@ 1.1.2.RELEASE 1.8.4 - 1.1.1 + 1.1.5 diff --git a/spring-cloud-sleuth-samples/pom.xml b/spring-cloud-sleuth-samples/pom.xml index a5678b063..eba0a690d 100644 --- a/spring-cloud-sleuth-samples/pom.xml +++ b/spring-cloud-sleuth-samples/pom.xml @@ -59,12 +59,12 @@ io.zipkin.java zipkin - 1.1.1 + 1.1.5 io.zipkin.java zipkin-server - 1.1.1 + 1.1.5 From 05e221ef54eb1156fd070bdd89b998dca78b8323 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 22 Jun 2016 15:03:28 +0200 Subject: [PATCH 10/10] HTTP filter child span renamed with this change the child span of the HTTP filter span gets changed into a span coming from the Controller aspect. The name of the span becomes the name of the method. --- .../sleuth/instrument/web/TraceFilter.java | 35 +++++++++--------- .../sleuth/instrument/web/TraceWebAspect.java | 31 ++++++++++++++++ .../web/TraceFilterCustomExtractorTests.java | 22 +++++++++-- .../web/TraceFilterIntegrationTests.java | 5 ++- .../instrument/web/TraceFilterTests.java | 26 ++++++++----- .../web/client/WebClientExceptionTests.java | 16 ++++---- .../instrument/web/client/WebClientTests.java | 5 +++ .../MessagingApplicationTests.java | 37 +++++++++++-------- .../java/tools/AbstractIntegrationTest.java | 9 +++-- 9 files changed, 127 insertions(+), 59 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 d5884fc61..f50b1fea2 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 @@ -190,18 +190,9 @@ public class TraceFilter extends GenericFilterBean { if (spanFromRequest != null) { addResponseTags(response, exception); if (spanFromRequest.hasSavedSpan()) { - Span parent = spanFromRequest.getSavedSpan(); - if (parent.isRemote()) { - if (log.isDebugEnabled()) { - log.debug("Sending the parent span " + parent + " to Zipkin"); - } - parent.logEvent(Span.SERVER_SEND); - parent.stop(); - this.spanReporter.report(parent); - } - } else { - spanFromRequest.logEvent(Span.SERVER_SEND); + closeParentSpan(spanFromRequest.getSavedSpan()); } + closeParentSpan(spanFromRequest); // in case of a response with exception status will close the span when exception dispatch is handled if (httpStatusSuccessful(response)) { if (log.isDebugEnabled()) { @@ -222,6 +213,19 @@ public class TraceFilter extends GenericFilterBean { } } + private void closeParentSpan(Span parent) { + if (parent.isRemote()) { + if (log.isDebugEnabled()) { + log.debug("Sending the parent span " + parent + " to Zipkin"); + } + parent.stop(); + parent.logEvent(Span.SERVER_SEND); + this.spanReporter.report(parent); + } else { + parent.logEvent(Span.SERVER_SEND); + } + } + private boolean httpStatusSuccessful(HttpServletResponse response) { if (response.getStatus() == 0) { return false; @@ -266,10 +270,8 @@ public class TraceFilter extends GenericFilterBean { log.debug("Found a parent span " + parent + " in the request"); } addRequestTagsForParentSpan(request, parent); - spanFromRequest = this.tracer.createSpan(name, parent); - if (log.isDebugEnabled()) { - log.debug("Started a new span " + spanFromRequest + " with parent " + parent); - } + spanFromRequest = parent; + this.tracer.continueSpan(spanFromRequest); if (parent.isRemote()) { parent.logEvent(Span.SERVER_RECV); } @@ -277,8 +279,7 @@ public class TraceFilter extends GenericFilterBean { if (log.isDebugEnabled()) { log.debug("Parent span is " + parent + ""); } - } - else { + } else { if (skip) { spanFromRequest = this.tracer.createSpan(name, NeverSampler.INSTANCE); } 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 a56ac27a3..9e7569c6e 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 @@ -24,6 +24,7 @@ import org.aspectj.lang.ProceedingJoinPoint; import org.aspectj.lang.annotation.Around; import org.aspectj.lang.annotation.Aspect; import org.aspectj.lang.annotation.Pointcut; +import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanNamer; import org.springframework.cloud.sleuth.Tracer; import org.springframework.cloud.sleuth.instrument.async.TraceContinuingCallable; @@ -83,6 +84,9 @@ public class TraceWebAspect { @Pointcut("@within(org.springframework.stereotype.Controller)") private void anyControllerAnnotated() { } // NOSONAR + @Pointcut("execution(public * *(..))") + private void anyPublicMethod() { } // NOSONAR + @Pointcut("execution(public java.util.concurrent.Callable *(..))") private void anyPublicMethodReturningCallable() { } // NOSONAR @@ -95,6 +99,33 @@ public class TraceWebAspect { @Pointcut("(anyRestControllerAnnotated() || anyControllerAnnotated()) && anyPublicMethodReturningWebAsyncTask()") private void anyControllerOrRestControllerWithPublicWebAsyncTaskMethod() { } // NOSONAR + @Around("(anyRestControllerAnnotated() || anyControllerAnnotated()) && anyPublicMethod()") + @SuppressWarnings("unchecked") + public Object wrapControllerMethodWithCorrelationId(ProceedingJoinPoint pjp) throws Throwable { + String spanName = toLowerHyphen(pjp.getSignature().getName()); + Span span = this.tracer.createSpan(spanName); + log.debug("Wrapping controller method [" + spanName + "] in a span " + span); + try { + //Thread.sleep(0, 1); + return pjp.proceed(); + } finally { + this.tracer.close(span); + } + } + + static String toLowerHyphen(String name) { + StringBuilder result = new StringBuilder(); + for (int i = 0; i < name.length(); i++) { + char c = name.charAt(i); + if (c >= 'A' && c <= 'Z') { + result.append('-').append((char) (c + 'a' - 'A')); + } else { + result.append(c); + } + } + return result.toString(); + } + @Around("anyControllerOrRestControllerWithPublicAsyncMethod()") @SuppressWarnings("unchecked") public Object wrapWithCorrelationId(ProceedingJoinPoint pjp) throws Throwable { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterCustomExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterCustomExtractorTests.java index fd12cce63..400aa722c 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterCustomExtractorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterCustomExtractorTests.java @@ -24,6 +24,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Random; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; @@ -35,8 +36,9 @@ import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanExtractor; import org.springframework.cloud.sleuth.SpanInjector; +import org.springframework.cloud.sleuth.SpanReporter; 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.ApplicationListener; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -51,6 +53,7 @@ import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.RestTemplate; +import static com.jayway.awaitility.Awaitility.await; import static org.assertj.core.api.BDDAssertions.then; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @@ -63,6 +66,12 @@ public class TraceFilterCustomExtractorTests { @Autowired RestTemplate restTemplate; @Autowired Config config; @Autowired CustomRestController customRestController; + @Autowired ArrayListSpanAccumulator accumulator; + + @Before + public void setup() { + this.accumulator.getSpans().clear(); + } @Test @SuppressWarnings("unchecked") @@ -78,7 +87,9 @@ public class TraceFilterCustomExtractorTests { ResponseEntity requestHeaders = this.restTemplate.exchange(requestEntity, Map.class); - then(this.customRestController.span).hasTraceIdEqualTo(traceId); + await().until(() -> then(this.accumulator.getSpans().stream().filter( + span -> span.getSpanId() == spanId).findFirst().get()) + .hasTraceIdEqualTo(traceId)); then(requestHeaders.getBody()) .containsEntry("correlationid", Span.idToHex(traceId)) .containsEntry("myspanid", Span.idToHex(spanId)) @@ -128,6 +139,11 @@ public class TraceFilterCustomExtractorTests { Sampler alwaysSampler() { return new AlwaysSampler(); } + + @Bean + SpanReporter spanReporter() { + return new ArrayListSpanAccumulator(); + } } // tag::extractor[] @@ -161,11 +177,9 @@ public class TraceFilterCustomExtractorTests { @RestController static class CustomRestController { - Span span; @RequestMapping("/headers") public Map headers(@RequestHeader HttpHeaders headers) { - this.span = TestSpanContextHolder.getCurrentSpan(); Map map = new HashMap<>(); for (String key : headers.keySet()) { map.put(key, headers.getFirst(key)); 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 b17db68f6..62fab6cbd 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 @@ -61,7 +61,10 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { MvcResult mvcResult = whenSentPingWithoutTracingData(); then(tracingHeaderFrom(mvcResult)).isNotNull(); - then(TraceFilterIntegrationTests.span).hasLoggedAnEvent(Span.SERVER_RECV) + Span parentSpan = this.spanAccumulator.getSpans().stream().filter( + span -> span.getSpanId() == TraceFilterIntegrationTests.span.getParents().get(0)) + .findFirst().get(); + then(parentSpan).hasLoggedAnEvent(Span.SERVER_RECV) .hasLoggedAnEvent(Span.SERVER_SEND); } 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 df9e1eac9..c36e7957d 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 @@ -48,10 +48,10 @@ import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockServletContext; import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; -import static org.assertj.core.api.BDDAssertions.then; import static org.junit.Assert.assertEquals; import static org.mockito.MockitoAnnotations.initMocks; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.assertThat; +import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.entry; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; @@ -60,6 +60,9 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder */ public class TraceFilterTests { + public static final long PARENT_ID = 10L; + public static final String PARENT_ID_AS_STRING = String.valueOf(PARENT_ID); + @Mock SpanLogger spanLogger; ArrayListSpanAccumulator spanReporter = new ArrayListSpanAccumulator(); SpanExtractor spanExtractor = new HttpServletRequestExtractor(Pattern @@ -146,7 +149,7 @@ public class TraceFilterTests { @Test public void startsNewTraceWithParentIdInHeaders() throws Exception { this.request = builder() - .header(Span.SPAN_ID_NAME, Span.idToHex(1L)) + .header(Span.SPAN_ID_NAME, Span.idToHex(PARENT_ID)) .header(Span.TRACE_ID_NAME, Span.idToHex(2L)) .header(Span.PARENT_ID_NAME, Span.idToHex(3L)) .buildRequest(new MockServletContext()); @@ -156,7 +159,7 @@ public class TraceFilterTests { filter.doFilter(this.request, this.response, this.filterChain); // this creates a child span which is why we'd expect the parents to include 1L) - assertThat(this.span.getParents()).containsOnly(1L); + assertThat(this.span.getParents()).containsOnly(3L); assertThat(parentSpan()) .hasATag("http.url", "http://localhost/?foo=bar") .hasATag("http.host", "localhost") @@ -167,7 +170,9 @@ public class TraceFilterTests { private Span parentSpan() { Optional parent = this.spanReporter.getSpans().stream() - .filter(span -> span.getName().contains("parent")).findFirst(); + .filter(span -> Span.idToHex(span.getSpanId()).equals(PARENT_ID_AS_STRING) + || span.getName().equals("http:/parent/")) + .findFirst(); assertThat(parent.isPresent()).isTrue(); return parent.get(); } @@ -221,7 +226,7 @@ public class TraceFilterTests { @Test public void continuesSpanFromHeaders() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, @@ -235,7 +240,7 @@ public class TraceFilterTests { @Test public void addsAdditionalHeaders() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); this.traceKeys.getHttp().getHeaders().add("x-foo"); @@ -244,13 +249,14 @@ public class TraceFilterTests { this.request.addHeader("X-Foo", "bar"); filter.doFilter(this.request, this.response, this.filterChain); + assertThat(parentSpan().tags()).contains(entry("http.x-foo", "bar")); assertThat(parentSpan().tags()).contains(entry("http.x-foo", "bar")); then(TestSpanContextHolder.getCurrentSpan()).isNull(); } @Test public void ensuresThatParentSpanIsStoppedWhenReported() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, spanIsStoppedVeryfingReporter(), this.spanExtractor, this.spanInjector, this.httpTraceKeysInjector); @@ -264,7 +270,7 @@ public class TraceFilterTests { @Test public void additionalMultiValuedHeader() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); this.traceKeys.getHttp().getHeaders().add("x-foo"); @@ -281,7 +287,7 @@ public class TraceFilterTests { @Test public void catchesException() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, this.spanExtractor, this.spanInjector, this.httpTraceKeysInjector); @@ -306,7 +312,7 @@ public class TraceFilterTests { @Test public void detachesSpanWhenResponseStatusIsNot2xx() throws Exception { - this.request = builder().header(Span.SPAN_ID_NAME, 10L) + this.request = builder().header(Span.SPAN_ID_NAME, PARENT_ID) .header(Span.TRACE_ID_NAME, 20L).buildRequest(new MockServletContext()); TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, this.spanReporter, this.spanExtractor, this.spanInjector, this.httpTraceKeysInjector); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientExceptionTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientExceptionTests.java index a739f955d..9af225c0d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientExceptionTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientExceptionTests.java @@ -42,13 +42,13 @@ import org.springframework.cloud.netflix.ribbon.RibbonClient; import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.Tracer; -import org.springframework.cloud.sleuth.assertions.SleuthAssertions; import org.springframework.cloud.sleuth.sampler.AlwaysSampler; import org.springframework.cloud.sleuth.trace.TestSpanContextHolder; import org.springframework.cloud.sleuth.util.ExceptionUtils; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.ResponseEntity; +import org.springframework.http.client.SimpleClientHttpRequestFactory; import org.springframework.test.context.junit4.rules.SpringClassRule; import org.springframework.test.context.junit4.rules.SpringMethodRule; import org.springframework.web.bind.annotation.RequestMapping; @@ -59,9 +59,7 @@ import junitparams.JUnitParamsRunner; import junitparams.Parameters; import static junitparams.JUnitParamsRunner.$; -import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.nullValue; -import static org.junit.Assert.assertThat; +import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @RunWith(JUnitParamsRunner.class) @SpringApplicationConfiguration(classes = { @@ -106,9 +104,8 @@ public class WebClientExceptionTests { // SleuthAssertions.then(e).hasRootCauseInstanceOf(IOException.class); } - assertThat(ExceptionUtils.getLastException(), is(nullValue())); - - SleuthAssertions.then(this.tracer.getCurrentSpan()).isEqualTo(span); + then(ExceptionUtils.getLastException()).isNull(); + then(this.tracer.getCurrentSpan()).isEqualTo(span); this.tracer.close(span); } @@ -135,7 +132,10 @@ public class WebClientExceptionTests { @LoadBalanced @Bean public RestTemplate restTemplate() { - return new RestTemplate(); + SimpleClientHttpRequestFactory clientHttpRequestFactory = new SimpleClientHttpRequestFactory(); + clientHttpRequestFactory.setReadTimeout(1); + clientHttpRequestFactory.setConnectTimeout(1); + return new RestTemplate(clientHttpRequestFactory); } @Bean 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 8222c0431..5757a78de 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,6 +17,7 @@ package org.springframework.cloud.sleuth.instrument.web.client; import javax.servlet.http.HttpServletRequest; +import java.lang.invoke.MethodHandles; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -29,6 +30,7 @@ import com.netflix.loadbalancer.BaseLoadBalancer; import com.netflix.loadbalancer.ILoadBalancer; import com.netflix.loadbalancer.Server; +import org.apache.commons.logging.LogFactory; import org.junit.After; import org.junit.ClassRule; import org.junit.Rule; @@ -82,6 +84,8 @@ import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; @DirtiesContext public class WebClientTests { + private static final org.apache.commons.logging.Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass()); + @ClassRule public static final SpringClassRule SCR = new SpringClassRule(); @Rule public final SpringMethodRule springMethodRule = new SpringMethodRule(); @@ -220,6 +224,7 @@ public class WebClientTests { .forEach(span -> { int initialSize = span.logs().size(); int distinctSize = span.logs().stream().map(Log::getEvent).distinct().collect(Collectors.toList()).size(); + log.info("logs " + span.logs()); then(initialSize).as("there are no duplicate log entries").isEqualTo(distinctSize); }); then(this.testErrorController.getSpan()).isNotNull(); diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java index a2456630e..81f5174e0 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java @@ -103,7 +103,8 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { } private void thenAllSpansHaveTraceIdEqualTo(long traceId) { - then(this.integrationTestSpanCollector.hashedSpans.stream().allMatch(span -> span.traceId == traceId)).isTrue(); + then(this.integrationTestSpanCollector.hashedSpans.stream() + .allMatch(span -> span.traceId == traceId)).describedAs("All spans have same trace id").isTrue(); } private void thenTheSpansHaveProperParentStructure() { @@ -112,20 +113,17 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { Optional eventSentSpan = findSpanWithAnnotation(Constants.SERVER_SEND); Optional eventReceivedSpan = findSpanWithAnnotation(Constants.CLIENT_RECV); Optional lastHttpSpansParent = findLastHttpSpansParent(); - thenAllSpansArePresent(firstHttpSpan, eventSpans, lastHttpSpansParent, eventSentSpan, eventReceivedSpan); - // "http:/parent/" -> "http:/" -> "message:messages" -> "http:/foo" (CS + CR) -> "http:/foo" (SS) -> "http:/foo" + // "http:/parent/" -> "home" -> "message:messages" -> "http:/foo" (CS + CR) -> "http:/foo" (SS) -> "foo" Collections.sort(this.integrationTestSpanCollector.hashedSpans, (s1, s2) -> s1.timestamp.compareTo(s2.timestamp)); - then(this.integrationTestSpanCollector.hashedSpans).hasSize(6); - for (int i=0; i= 0) { - Span parent = this.integrationTestSpanCollector.hashedSpans.get(i - 1); - Span current = this.integrationTestSpanCollector.hashedSpans.get(i); - // there is a pair of spans having cs/cr and ss/sr - if (current.id != parent.id) { - then(current.parentId).isEqualTo(parent.id); - } - } - } + thenAllSpansArePresent(firstHttpSpan, eventSpans, lastHttpSpansParent, eventSentSpan, eventReceivedSpan); + then(this.integrationTestSpanCollector.hashedSpans).as("There were 6 spans").hasSize(6); + log.info("Checking the parent child structure"); + List> parentChild = this.integrationTestSpanCollector.hashedSpans.stream() + .filter(span -> span.parentId != null) + .map(span -> this.integrationTestSpanCollector.hashedSpans.stream().filter(span1 -> span1.id == span.parentId).findAny() + ).collect(Collectors.toList()); + log.info("List of parents and children " + parentChild); + then(parentChild.stream().allMatch(Optional::isPresent)).isTrue(); } private Optional findLastHttpSpansParent() { @@ -148,12 +146,21 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { private Optional findFirstHttpRequestSpan() { return this.integrationTestSpanCollector.hashedSpans.stream() - .filter(span -> "http:/".equals(span.name) && span.parentId != null).findFirst(); + // home is the name of the method + .filter(span -> "home".equals(span.name)).findFirst(); } private void thenAllSpansArePresent(Optional firstHttpSpan, List eventSpans, Optional lastHttpSpan, Optional eventSentSpan, Optional eventReceivedSpan) { + log.info("Found following spans"); + log.info("First http span " + firstHttpSpan); + log.info("Event spans " + eventSpans); + log.info("Event sent span " + eventSentSpan); + log.info("Event received span " + eventReceivedSpan); + log.info("Last http span " + lastHttpSpan); + log.info("All found spans \n" + this.integrationTestSpanCollector.hashedSpans + .stream().map(Span::toString).collect(Collectors.joining("\n"))); then(firstHttpSpan.isPresent()).isTrue(); then(eventSpans).isNotEmpty(); then(eventSentSpan.isPresent()).isTrue(); diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-test-core/src/main/java/tools/AbstractIntegrationTest.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-test-core/src/main/java/tools/AbstractIntegrationTest.java index 4f90e16e6..346a0383a 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-test-core/src/main/java/tools/AbstractIntegrationTest.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-test-core/src/main/java/tools/AbstractIntegrationTest.java @@ -15,6 +15,7 @@ */ package tools; +import java.lang.invoke.MethodHandles; import java.net.URI; import java.util.ArrayList; import java.util.Collection; @@ -23,6 +24,9 @@ import java.util.List; import java.util.Optional; import java.util.stream.Collectors; +import com.jayway.awaitility.Awaitility; +import com.jayway.awaitility.core.ConditionFactory; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.After; @@ -35,9 +39,6 @@ import org.springframework.http.RequestEntity; import org.springframework.http.ResponseEntity; import org.springframework.web.client.RestTemplate; -import com.jayway.awaitility.Awaitility; -import com.jayway.awaitility.core.ConditionFactory; - import zipkin.Codec; import zipkin.Span; @@ -49,7 +50,7 @@ import static org.assertj.core.api.BDDAssertions.then; */ public abstract class AbstractIntegrationTest { - private static final Log log = LogFactory.getLog(AbstractIntegrationTest.class); + protected static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass()); protected static final int POLL_INTERVAL = 1; protected static final int TIMEOUT = 20;