From ad700ea66421da47b037be5602188e1497f069b3 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Mon, 19 Dec 2016 13:26:04 +0100 Subject: [PATCH] Adding SA only if peer tag is present without this change Zipkin doesn't properly visualize the span on the server side. with this change we're setting SA only if peer tag is set. fixes #481 --- .../zipkin/stream/ConvertToZipkinSpanList.java | 11 ++++++----- .../stream/ConvertToZipkinSpanListTests.java | 18 +++++++++++++++++- .../sleuth/zipkin/ZipkinSpanListener.java | 9 +++++---- .../sleuth/zipkin/ZipkinSpanListenerTests.java | 16 ++++++++++++++-- 4 files changed, 42 insertions(+), 12 deletions(-) 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 7bd83c15b..96b1809ad 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 @@ -130,11 +130,12 @@ final class ConvertToZipkinSpanList { 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; - Endpoint endpoint = ep.toBuilder().serviceName(serviceName).build(); - zipkinSpan.addBinaryAnnotation( - BinaryAnnotation.address(Constants.SERVER_ADDR, endpoint)); + if (span.tags().containsKey(Span.SPAN_PEER_SERVICE_TAG_NAME)) { + Endpoint endpoint = ep.toBuilder().serviceName(span.tags().get( + Span.SPAN_PEER_SERVICE_TAG_NAME)).build(); + zipkinSpan.addBinaryAnnotation( + BinaryAnnotation.address(Constants.SERVER_ADDR, endpoint)); + } } private static boolean notClientOrServer(Span span) { diff --git a/spring-cloud-sleuth-zipkin-stream/src/test/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanListTests.java b/spring-cloud-sleuth-zipkin-stream/src/test/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanListTests.java index 26161e080..6c5700e57 100644 --- a/spring-cloud-sleuth-zipkin-stream/src/test/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanListTests.java +++ b/spring-cloud-sleuth-zipkin-stream/src/test/java/org/springframework/cloud/sleuth/zipkin/stream/ConvertToZipkinSpanListTests.java @@ -74,9 +74,10 @@ public class ConvertToZipkinSpanListTests { } @Test - public void appendsServerAddressTagIfClientLogIsPresent() { + public void appendServerAddressTagIfClientLogIsPresentWhenPeerServiceIsPresent() { Span span = span("foo"); span.logEvent(Constants.CLIENT_SEND); + span.tag(Span.SPAN_PEER_SERVICE_TAG_NAME, "myservice"); Spans spans = new Spans(this.host, Collections.singletonList(span)); List result = ConvertToZipkinSpanList.convert(spans); @@ -89,6 +90,21 @@ public class ConvertToZipkinSpanListTests { .contains("myservice"); } + @Test + public void doesNotAppendServerAddressTagIfClientLogIsPresent() { + Span span = span("foo"); + span.logEvent(Constants.CLIENT_SEND); + Spans spans = new Spans(this.host, Collections.singletonList(span)); + + List result = ConvertToZipkinSpanList.convert(spans); + + assertThat(result) + .hasSize(1) + .flatExtracting(input1 -> input1.binaryAnnotations) + .filteredOn("key", Constants.SERVER_ADDR) + .isEmpty(); + } + @Test public void shouldReuseServerAddressTag() { Span span = span("foo"); 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 46a34f5b5..e10ec4936 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 @@ -127,10 +127,11 @@ public class ZipkinSpanListener implements SpanReporter { } private void ensureServerAddr(Span span, zipkin.Span.Builder zipkinSpan) { - String serviceName = span.tags().containsKey(Span.SPAN_PEER_SERVICE_TAG_NAME) ? - span.tags().get(Span.SPAN_PEER_SERVICE_TAG_NAME) : this.endpointLocator.local().serviceName; - zipkinSpan.addBinaryAnnotation(BinaryAnnotation.address(Constants.SERVER_ADDR, - this.endpointLocator.local().toBuilder().serviceName(serviceName).build())); + if (span.tags().containsKey(Span.SPAN_PEER_SERVICE_TAG_NAME)) { + zipkinSpan.addBinaryAnnotation(BinaryAnnotation.address(Constants.SERVER_ADDR, + this.endpointLocator.local().toBuilder().serviceName( + span.tags().get(Span.SPAN_PEER_SERVICE_TAG_NAME)).build())); + } } private boolean notClientOrServer(Span span) { diff --git a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListenerTests.java b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListenerTests.java index 239754a49..c38cc37c5 100644 --- a/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListenerTests.java +++ b/spring-cloud-sleuth-zipkin/src/test/java/org/springframework/cloud/sleuth/zipkin/ZipkinSpanListenerTests.java @@ -17,7 +17,6 @@ package org.springframework.cloud.sleuth.zipkin; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import javax.annotation.PostConstruct; @@ -204,8 +203,9 @@ public class ZipkinSpanListenerTests { } @Test - public void appendsServerAddressTagIfClientLogIsPresent() { + public void appendServerAddressTagIfClientLogIsPresentWhenPeerServiceIsPresent() { this.parent.logEvent(Constants.CLIENT_SEND); + this.parent.tag(Span.SPAN_PEER_SERVICE_TAG_NAME, "fooservice"); this.parent.stop(); zipkin.Span result = this.spanReporter.convert(this.parent); @@ -215,6 +215,18 @@ public class ZipkinSpanListenerTests { .isNotEmpty(); } + @Test + public void doesNotAppendServerAddressTagIfClientLogIsPresent() { + this.parent.logEvent(Constants.CLIENT_SEND); + this.parent.stop(); + + zipkin.Span result = this.spanReporter.convert(this.parent); + + assertThat(result.binaryAnnotations) + .filteredOn("key", Constants.SERVER_ADDR) + .isEmpty(); + } + @Test public void converts128BitTraceId() { Span span = Span.builder().traceIdHigh(1L).traceId(2L).spanId(3L).name("foo").build();