From e0fbcfe7c6cb0d6e0055ffdc18c2a8073293d6fd Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 10 Aug 2016 17:18:20 +0200 Subject: [PATCH] Polish --- .../springframework/cloud/sleuth/Span.java | 4 +-- .../sleuth/instrument/web/TraceFilter.java | 33 ++++++++++--------- .../sleuth/assertions/ListOfSpansAssert.java | 9 +++-- .../instrument/hystrix/TraceCommandTests.java | 4 +-- .../instrument/web/client/WebClientTests.java | 32 +++++++----------- .../stream/ConvertToZipkinSpanList.java | 2 +- .../sleuth/zipkin/ZipkinSpanListener.java | 10 +++--- 7 files changed, 41 insertions(+), 53 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 250f7a11d..3cbc67531 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 @@ -462,9 +462,7 @@ public class Span { Span other = (Span) obj; if (this.spanId != other.spanId) return false; - if (this.traceId != other.traceId) - return false; - return true; + return this.traceId == other.traceId; } public static class SpanBuilder { 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 a593922a4..d2806fdc2 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 @@ -15,18 +15,18 @@ */ package org.springframework.cloud.sleuth.instrument.web; -import javax.servlet.FilterChain; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; import java.util.regex.Pattern; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -204,30 +204,31 @@ public class TraceFilter extends GenericFilterBean { private void detachOrCloseSpans(HttpServletRequest request, HttpServletResponse response, Span spanFromRequest, Throwable exception) { - if (spanFromRequest != null) { + Span span = spanFromRequest; + if (span != null) { addResponseTags(response, exception); - if (spanFromRequest.hasSavedSpan() && requestHasAlreadyBeenHandled(request)) { - recordParentSpan(spanFromRequest.getSavedSpan()); + if (span.hasSavedSpan() && requestHasAlreadyBeenHandled(request)) { + recordParentSpan(span.getSavedSpan()); } else if (!requestHasAlreadyBeenHandled(request)) { - spanFromRequest = this.tracer.close(spanFromRequest); + span = this.tracer.close(span); } - recordParentSpan(spanFromRequest); + recordParentSpan(span); // in case of a response with exception status will close the span when exception dispatch is handled if (httpStatusSuccessful(response)) { if (log.isDebugEnabled()) { - log.debug("Closing the span " + spanFromRequest + " since the response was successful"); + log.debug("Closing the span " + span + " since the response was successful"); } - this.tracer.close(spanFromRequest); + this.tracer.close(span); } else if (errorAlreadyHandled(request)) { if (log.isDebugEnabled()) { log.debug( - "Won't detach the span " + spanFromRequest + " since error has already been handled"); + "Won't detach the span " + span + " since error has already been handled"); } } else { if (log.isDebugEnabled()) { - log.debug("Detaching the span " + spanFromRequest + " since the response was unsuccessful"); + log.debug("Detaching the span " + span + " since the response was unsuccessful"); } - this.tracer.detach(spanFromRequest); + this.tracer.detach(span); } } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/ListOfSpansAssert.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/ListOfSpansAssert.java index 9f460d24f..06fb23942 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/ListOfSpansAssert.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/assertions/ListOfSpansAssert.java @@ -20,16 +20,15 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; - -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.assertj.core.api.AbstractAssert; import org.springframework.cloud.sleuth.Span; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; + import static java.util.stream.Collectors.joining; import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; @@ -114,7 +113,7 @@ public class ListOfSpansAssert extends AbstractAssert "\nSPAN: " + span.toString() + " with name [" + span.getName() + "] " + - "\nwith tags " + span.tags() + "\nwith logs " + span.logs()).collect(Collectors.joining("\n")); + "\nwith tags " + span.tags() + "\nwith logs " + span.logs()).collect(joining("\n")); } public ListOfSpansAssert doesNotHaveASpanWithName(String name) { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/TraceCommandTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/TraceCommandTests.java index 45254cada..1ab54bf9f 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/TraceCommandTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/hystrix/TraceCommandTests.java @@ -32,7 +32,6 @@ import org.springframework.cloud.sleuth.trace.DefaultTracer; import org.springframework.cloud.sleuth.trace.TestSpanContextHolder; import com.netflix.hystrix.HystrixCommand; -import com.netflix.hystrix.HystrixCommandGroupKey; import com.netflix.hystrix.HystrixCommandKey; import com.netflix.hystrix.HystrixCommandProperties; import com.netflix.hystrix.HystrixThreadPoolProperties; @@ -103,8 +102,7 @@ public class TraceCommandTests { Tracer tracer = new DefaultTracer(new AlwaysSampler(), new Random(), new DefaultSpanNamer(), new NoOpSpanLogger(), new NoOpSpanReporter()); TraceKeys traceKeys = new TraceKeys(); - HystrixCommand.Setter setter = HystrixCommand.Setter - .withGroupKey(HystrixCommandGroupKey.Factory.asKey("group")) + HystrixCommand.Setter setter = withGroupKey(asKey("group")) .andCommandKey(HystrixCommandKey.Factory.asKey("command")); // tag::hystrix_command[] HystrixCommand hystrixCommand = new HystrixCommand(setter) { 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 5e4a02512..adef26999 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 @@ -16,7 +16,6 @@ package org.springframework.cloud.sleuth.instrument.web.client; -import javax.servlet.http.HttpServletRequest; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.Collections; @@ -25,13 +24,8 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Random; -import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; - -import com.jayway.awaitility.Awaitility; -import com.netflix.loadbalancer.BaseLoadBalancer; -import com.netflix.loadbalancer.ILoadBalancer; -import com.netflix.loadbalancer.Server; +import javax.servlet.http.HttpServletRequest; import org.apache.commons.logging.LogFactory; import org.junit.After; @@ -73,6 +67,10 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.client.HttpClientErrorException; import org.springframework.web.client.RestTemplate; +import com.netflix.loadbalancer.BaseLoadBalancer; +import com.netflix.loadbalancer.ILoadBalancer; +import com.netflix.loadbalancer.Server; + import junitparams.JUnitParamsRunner; import junitparams.Parameters; @@ -115,19 +113,13 @@ public class WebClientTests { then(getHeader(response, Span.TRACE_ID_NAME)).isNull(); then(getHeader(response, Span.SPAN_ID_NAME)).isNull(); then(this.listener.getSpans()).isNotEmpty(); - Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> { - log.info("Seraching for a notrace span in " + this.listener.getSpans()); - Optional noTraceSpan = this.listener.getSpans().stream().filter(span -> - "http:/notrace".equals(span.getName()) && !span.tags().isEmpty()).findFirst(); - then(noTraceSpan.isPresent()).isTrue(); - log.info("No trace span found"); - // TODO: matches cause there is an issue with Feign not providing the full URL at the interceptor level - then(noTraceSpan.get()).matchesATag("http.url", ".*/notrace") - .hasATag("http.path", "/notrace") - .hasATag("http.method", "GET"); - log.info("Notrace has all the necessary tags"); - }); - + Optional noTraceSpan = this.listener.getSpans().stream().filter(span -> + "http:/notrace".equals(span.getName()) && !span.tags().isEmpty()).findFirst(); + then(noTraceSpan.isPresent()).isTrue(); + // TODO: matches cause there is an issue with Feign not providing the full URL at the interceptor level + then(noTraceSpan.get()).matchesATag("http.url", ".*/notrace") + .hasATag("http.path", "/notrace") + .hasATag("http.method", "GET"); } Object[] parametersForShouldCreateANewSpanWithClientSideTagsWhenNoPreviousTracingWasPresent() { diff --git a/spring-cloud-sleuth-zipkin-stream/src/main/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanList.java b/spring-cloud-sleuth-zipkin-stream/src/main/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanList.java index 930f5a5ef..1eac99aec 100644 --- a/spring-cloud-sleuth-zipkin-stream/src/main/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanList.java +++ b/spring-cloud-sleuth-zipkin-stream/src/main/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanList.java @@ -118,7 +118,7 @@ final class ConvertToZipkinSpanList { BinaryAnnotation.create(Constants.LOCAL_COMPONENT, processId, ep)); } - private static void ensureServerAddr(Span span, zipkin.Span.Builder zipkinSpan, + private static void ensureServerAddr(Span span, Builder zipkinSpan, Endpoint ep) { String serviceName = span.tags().containsKey(Span.SPAN_PEER_SERVICE_TAG_NAME) ? span.tags().get(Span.SPAN_PEER_SERVICE_TAG_NAME) : ep.serviceName; diff --git a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java index ad63f007b..636b88a6b 100644 --- a/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java +++ b/spring-cloud-sleuth-zipkin/src/main/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListener.java @@ -136,7 +136,7 @@ public class ZipkinSpanListener implements SpanReporter { } private boolean hasClientSend(Span span) { - for (org.springframework.cloud.sleuth.Log log : span.logs()) { + for (Log log : span.logs()) { if (Constants.CLIENT_SEND.equals(log.getEvent())) { return !span.tags().containsKey(Constants.SERVER_ADDR); } @@ -183,16 +183,16 @@ public class ZipkinSpanListener implements SpanReporter { * duration (client receive - send) */ private long calculateDurationInMicros(Span span) { - org.springframework.cloud.sleuth.Log clientSend = hasLog(Span.CLIENT_SEND, span); - org.springframework.cloud.sleuth.Log clientReceived = hasLog(Span.CLIENT_RECV, span); + Log clientSend = hasLog(Span.CLIENT_SEND, span); + Log clientReceived = hasLog(Span.CLIENT_RECV, span); if (clientSend != null && clientReceived != null) { return (clientReceived.getTimestamp() - clientSend.getTimestamp()) * 1000; } return span.getAccumulatedMicros(); } - private org.springframework.cloud.sleuth.Log hasLog(String logName, Span span) { - for (org.springframework.cloud.sleuth.Log log : span.logs()) { + private Log hasLog(String logName, Span span) { + for (Log log : span.logs()) { if (logName.equals(log.getEvent())) { return log; }