From 7e8a48314973eeba26d95a2af4787ac80a2a477d Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 24 Dec 2015 10:17:14 +0100 Subject: [PATCH] [#39] Refactored code according to the code review --- .../sleuth/assertions/SleuthAssertions.java | 4 + .../cloud/sleuth/assertions/SpanAssert.java | 2 +- .../instrument/hystrix/JavanicaITest.java | 124 ------------------ ...nPassingForHystrixViaAnnotationsITest.java | 106 +++++++++++++++ .../instrument/web/TraceAsyncITest.java | 62 +++------ 5 files changed, 132 insertions(+), 166 deletions(-) delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/JavanicaITest.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/SpanPassingForHystrixViaAnnotationsITest.java diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SleuthAssertions.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SleuthAssertions.java index 6a73c118b..bc1369f36 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SleuthAssertions.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SleuthAssertions.java @@ -6,6 +6,10 @@ import org.springframework.cloud.sleuth.Span; public class SleuthAssertions extends BDDAssertions { public static SpanAssert then(Span actual) { + return assertThat(actual); + } + + public static SpanAssert assertThat(Span actual) { return new SpanAssert(actual); } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SpanAssert.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SpanAssert.java index 56420aa86..296340dde 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SpanAssert.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/SpanAssert.java @@ -15,7 +15,7 @@ public class SpanAssert extends AbstractAssert { return new SpanAssert(actual); } - public SpanAssert hasTraceId(String traceId) { + public SpanAssert hasTraceIdEqualTo(String traceId) { isNotNull(); if (!Objects.equals(actual.getTraceId(), traceId)) { failWithMessage("Expected span's traceId to be <%s> but was <%s>", traceId, actual.getTraceId()); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/JavanicaITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/JavanicaITest.java deleted file mode 100644 index df065914c..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/JavanicaITest.java +++ /dev/null @@ -1,124 +0,0 @@ -package org.springframework.cloud.sleuth.instrument.hystrix; - -import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; - -import java.util.concurrent.atomic.AtomicReference; - -import org.junit.After; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.test.SpringApplicationConfiguration; -import org.springframework.cloud.netflix.hystrix.EnableHystrix; -import org.springframework.cloud.sleuth.Span; -import org.springframework.cloud.sleuth.TraceManager; -import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; -import org.springframework.cloud.sleuth.trace.TraceContextHolder; -import org.springframework.context.annotation.Bean; -import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.EnableAspectJAutoProxy; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; - -import com.jayway.awaitility.Awaitility; -import com.netflix.hystrix.contrib.javanica.annotation.HystrixCommand; - -@RunWith(SpringJUnit4ClassRunner.class) -@SpringApplicationConfiguration(classes = { - JavanicaITest.JavanicaITestConfiguration.class }) -public class JavanicaITest { - - @Autowired JavanicaClass javanicaClass; - @Autowired JavanicaDelegation javanicaDelegation; - @Autowired TraceManager traceManager; - - @Test - public void should_set_span_on_an_hystrix_command_annotated_method() { - final Span span = givenASpanInCurrentThread(); - - whenHystrixCommandGetsExecutedViaJavanica(); - - thenSpanPutInTheAsyncThreadIsSameAs(span); - } - - private Span givenASpanInCurrentThread() { - Span span = this.traceManager.startSpan("existing").getSpan(); - this.traceManager.continueSpan(span); - return span; - } - - private void whenHystrixCommandGetsExecutedViaJavanica() { - this.javanicaDelegation.doSthThatDelegatesToJavanica(); - } - - private void thenSpanPutInTheAsyncThreadIsSameAs(final Span span) { - Awaitility.await().until(new Runnable() { - @Override - public void run() { - then(span) - .hasTraceId(javanicaClass.getTraceId()) - .hasNameNotEqualTo(javanicaClass.getSpanName()); - } - }); - } - - @After - public void cleanTrace() { - TraceContextHolder.removeCurrentTrace(); - } - - @DefaultTestAutoConfiguration - @EnableHystrix - @EnableAspectJAutoProxy(proxyTargetClass = true) - @Configuration - public static class JavanicaITestConfiguration { - - @Bean - JavanicaClass javanicaClass() { - return new JavanicaClass(); - } - - @Bean - JavanicaDelegation javanicaDelegation() { - return new JavanicaDelegation(javanicaClass()); - } - } - - public static class JavanicaDelegation { - - private final JavanicaClass javanicaClass; - - public JavanicaDelegation(JavanicaClass javanicaClass) { - this.javanicaClass = javanicaClass; - } - - public void doSthThatDelegatesToJavanica() { - this.javanicaClass.doSth(); - } - } - - public static class JavanicaClass { - - AtomicReference span; - - @HystrixCommand - public void doSth() { - this.span = new AtomicReference<>(TraceContextHolder.getCurrentSpan()); - } - - public String getTraceId() { - if (this.span == null || this.span.get() == null || (this.span.get() != null - && this.span.get().getTraceId() == null)) { - return null; - } - return this.span.get().getTraceId(); - } - - public String getSpanName() { - if (this.span == null - || (this.span.get() != null && this.span.get().getName() == null)) { - return null; - } - return this.span.get().getName(); - } - } -} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/SpanPassingForHystrixViaAnnotationsITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/SpanPassingForHystrixViaAnnotationsITest.java new file mode 100644 index 000000000..407c48f3a --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/SpanPassingForHystrixViaAnnotationsITest.java @@ -0,0 +1,106 @@ +package org.springframework.cloud.sleuth.instrument.hystrix; + +import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then; + +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.After; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.SpringApplicationConfiguration; +import org.springframework.cloud.netflix.hystrix.EnableHystrix; +import org.springframework.cloud.sleuth.Span; +import org.springframework.cloud.sleuth.TraceManager; +import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; +import org.springframework.cloud.sleuth.trace.TraceContextHolder; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; + +import com.jayway.awaitility.Awaitility; +import com.netflix.hystrix.contrib.javanica.annotation.HystrixCommand; + +@RunWith(SpringJUnit4ClassRunner.class) +@SpringApplicationConfiguration(classes = { + SpanPassingForHystrixViaAnnotationsITest.TestConfig.class }) +public class SpanPassingForHystrixViaAnnotationsITest { + + @Autowired HystrixCommandInvocationSpanCatcher hystrixCommandInvocationSpanCatcher; + @Autowired TraceManager traceManager; + + @Test + public void should_set_span_on_an_hystrix_command_annotated_method() { + Span span = givenASpanInCurrentThread(); + + whenHystrixCommandAnnotatedMethodGetsExecuted(); + + thenTraceIdIsPassedFromTheCurrentThreadToTheHystrixOne(span); + } + + private Span givenASpanInCurrentThread() { + Span span = traceManager.startSpan("existing").getSpan(); + traceManager.continueSpan(span); + return span; + } + + private void whenHystrixCommandAnnotatedMethodGetsExecuted() { + hystrixCommandInvocationSpanCatcher.invokeLogicWrappedInHystrixCommand(); + } + + private void thenTraceIdIsPassedFromTheCurrentThreadToTheHystrixOne(final Span span) { + Awaitility.await().until(new Runnable() { + @Override + public void run() { + then(span) + .hasTraceIdEqualTo(hystrixCommandInvocationSpanCatcher.getTraceId()) + .hasNameNotEqualTo(hystrixCommandInvocationSpanCatcher.getSpanName()); + } + }); + } + + @After + public void cleanTrace() { + TraceContextHolder.removeCurrentTrace(); + } + + @DefaultTestAutoConfiguration + @EnableHystrix + @Configuration + static class TestConfig { + + @Bean HystrixCommandInvocationSpanCatcher spanCatcher() { + return new HystrixCommandInvocationSpanCatcher(); + } + + } + + static class HystrixCommandInvocationSpanCatcher { + + AtomicReference spanCaughtFromHystrixThread; + + @HystrixCommand + public void invokeLogicWrappedInHystrixCommand() { + spanCaughtFromHystrixThread = new AtomicReference<>(TraceContextHolder.getCurrentSpan()); + } + + public String getTraceId() { + if (spanCaughtFromHystrixThread == null || + spanCaughtFromHystrixThread.get() == null || + (spanCaughtFromHystrixThread.get() != null && + spanCaughtFromHystrixThread.get().getTraceId() == null)) { + return null; + } + return spanCaughtFromHystrixThread.get().getTraceId(); + } + + public String getSpanName() { + if (spanCaughtFromHystrixThread == null || + (spanCaughtFromHystrixThread.get() != null && + spanCaughtFromHystrixThread.get().getName() == null)) { + return null; + } + return spanCaughtFromHystrixThread.get().getName(); + } + } +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java index 745a05e88..896c2854e 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceAsyncITest.java @@ -16,7 +16,6 @@ import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; import org.springframework.cloud.sleuth.trace.TraceContextHolder; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.EnableAspectJAutoProxy; import org.springframework.scheduling.annotation.Async; import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -28,36 +27,35 @@ import com.jayway.awaitility.Awaitility; TraceAsyncITest.TraceAsyncITestConfiguration.class }) public class TraceAsyncITest { - @Autowired AsyncClass asyncClass; - @Autowired AsyncDelegation asyncDelegation; + @Autowired ClassPerformingAsyncLogic classPerformingAsyncLogic; @Autowired TraceManager traceManager; @Test public void should_set_span_on_an_async_annotated_method() { - final Span span = givenASpanInCurrentThread(); + Span span = givenASpanInCurrentThread(); whenAsyncProcessingTakesPlace(); - thenSpanPutInTheAsyncThreadIsSameAs(span); + thenTraceIdIsPassedFromTheCurrentThreadToTheAsyncOne(span); } private Span givenASpanInCurrentThread() { - Span span = this.traceManager.startSpan("existing").getSpan(); - this.traceManager.continueSpan(span); + Span span = traceManager.startSpan("existing").getSpan(); + traceManager.continueSpan(span); return span; } private void whenAsyncProcessingTakesPlace() { - this.asyncDelegation.doSthThatDelegatesToAsync(); + classPerformingAsyncLogic.invokeAsynchronousLogic(); } - private void thenSpanPutInTheAsyncThreadIsSameAs(final Span span) { + private void thenTraceIdIsPassedFromTheCurrentThreadToTheAsyncOne(final Span span) { Awaitility.await().until(new Runnable() { @Override public void run() { then(span) - .hasTraceId(asyncClass.getTraceId()) - .hasNameNotEqualTo(asyncClass.getSpanName()); + .hasTraceIdEqualTo(classPerformingAsyncLogic.getTraceId()) + .hasNameNotEqualTo(classPerformingAsyncLogic.getSpanName()); } }); } @@ -69,57 +67,39 @@ public class TraceAsyncITest { @DefaultTestAutoConfiguration @EnableAsync - @EnableAspectJAutoProxy(proxyTargetClass = true) @Configuration - public static class TraceAsyncITestConfiguration { + static class TraceAsyncITestConfiguration { @Bean - AsyncClass asyncClass() { - return new AsyncClass(); + ClassPerformingAsyncLogic asyncClass() { + return new ClassPerformingAsyncLogic(); } - @Bean - AsyncDelegation asyncDelegation() { - return new AsyncDelegation(asyncClass()); - } } - public static class AsyncDelegation { - - private final AsyncClass asyncClass; - - public AsyncDelegation(AsyncClass asyncClass) { - this.asyncClass = asyncClass; - } - - public void doSthThatDelegatesToAsync() { - this.asyncClass.doSth(); - } - } - - public static class AsyncClass { + static class ClassPerformingAsyncLogic { AtomicReference span; @Async - public void doSth() { - this.span = new AtomicReference<>(TraceContextHolder.getCurrentSpan()); + public void invokeAsynchronousLogic() { + span = new AtomicReference<>(TraceContextHolder.getCurrentSpan()); } public String getTraceId() { - if (this.span == null || (this.span.get() != null - && this.span.get().getTraceId() == null)) { + if (span == null || (span.get() != null + && span.get().getTraceId() == null)) { return null; } - return this.span.get().getTraceId(); + return span.get().getTraceId(); } public String getSpanName() { - if (this.span == null - || (this.span.get() != null && this.span.get().getName() == null)) { + if (span == null + || (span.get() != null && span.get().getName() == null)) { return null; } - return this.span.get().getName(); + return span.get().getName(); } } }