diff --git a/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpanIterator.java b/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpan2Iterator.java similarity index 61% rename from zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpanIterator.java rename to zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpan2Iterator.java index b759aa64fa4..6736a8eafc0 100644 --- a/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpanIterator.java +++ b/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/DependencyLinkSpan2Iterator.java @@ -16,28 +16,32 @@ import java.util.Iterator; import org.jooq.Record; import org.jooq.TableField; -import zipkin.internal.DependencyLinkSpan; +import zipkin.Endpoint; import zipkin.internal.Nullable; import zipkin.internal.PeekingIterator; +import zipkin.internal.Span2; import zipkin.storage.mysql.internal.generated.tables.ZipkinSpans; import static zipkin.Constants.CLIENT_ADDR; import static zipkin.Constants.CLIENT_SEND; import static zipkin.Constants.SERVER_ADDR; import static zipkin.Constants.SERVER_RECV; +import static zipkin.internal.Util.equal; import static zipkin.storage.mysql.internal.generated.tables.ZipkinAnnotations.ZIPKIN_ANNOTATIONS; /** - * Convenience that lazy converts rows into {@linkplain DependencyLinkSpan} objects. + * Lazy converts rows into {@linkplain Span2} objects suitable for dependency links. This takes + * short-cuts to require less data. For example, it folds shared RPC spans into one, and doesn't + * include tags, non-core annotations or time units. * *

Out-of-date schemas may be missing the trace_id_high field. When present, this becomes {@link - * DependencyLinkSpan.TraceId#hi} used as the left-most 16 characters of the traceId in logging + * Span2#traceIdHigh()} used as the left-most 16 characters of the traceId in logging * statements. */ -final class DependencyLinkSpanIterator implements Iterator { +final class DependencyLinkSpan2Iterator implements Iterator { /** Assumes the input records are sorted by trace id, span id */ - static final class ByTraceId implements Iterator> { + static final class ByTraceId implements Iterator> { final PeekingIterator delegate; final boolean hasTraceIdHigh; @@ -53,10 +57,10 @@ static final class ByTraceId implements Iterator> { return delegate.hasNext(); } - @Override public Iterator next() { + @Override public Iterator next() { currentTraceIdHi = hasTraceIdHigh ? traceIdHigh(delegate) : null; currentTraceIdLo = delegate.peek().getValue(ZipkinSpans.ZIPKIN_SPANS.TRACE_ID); - return new DependencyLinkSpanIterator(delegate, currentTraceIdHi, currentTraceIdLo); + return new DependencyLinkSpan2Iterator(delegate, currentTraceIdHi, currentTraceIdLo); } @Override public void remove() { @@ -68,7 +72,7 @@ static final class ByTraceId implements Iterator> { @Nullable final Long traceIdHi; final long traceIdLo; - DependencyLinkSpanIterator(PeekingIterator delegate, Long traceIdHi, long traceIdLo) { + DependencyLinkSpan2Iterator(PeekingIterator delegate, Long traceIdHi, long traceIdLo) { this.delegate = delegate; this.traceIdHi = traceIdHi; this.traceIdLo = traceIdLo; @@ -83,17 +87,11 @@ public boolean hasNext() { } @Override - public DependencyLinkSpan next() { + public Span2 next() { Record row = delegate.peek(); long spanId = row.getValue(ZipkinSpans.ZIPKIN_SPANS.ID); - DependencyLinkSpan.Builder result = DependencyLinkSpan.builder( - traceIdHi != null ? traceIdHi : 0L, - traceIdLo, - row.getValue(ZipkinSpans.ZIPKIN_SPANS.PARENT_ID), - spanId - ); - + String srService = null, csService = null, caService = null, saService = null; while (hasNext()) { // there are more values for this trace if (spanId != delegate.peek().getValue(ZipkinSpans.ZIPKIN_SPANS.ID)) { break; // if we are in a new span @@ -105,18 +103,48 @@ public DependencyLinkSpan next() { if (key == null || value == null) continue; // neither client nor server switch (key) { case CLIENT_ADDR: - result.caService(value); + caService = value; break; case CLIENT_SEND: - result.csService(value); + csService = value; break; case SERVER_ADDR: - result.saService(value); + saService = value; break; case SERVER_RECV: - result.srService(value); + srService = value; } } + + // The client address is more authoritative than the client send owner. + if (caService == null) caService = csService; + + // Finagle labels two sides of the same socket ("ca", "sa") with the same name. + // Skip the client side, so it isn't mistaken for a loopback request + if (equal(saService, caService)) caService = null; + + Span2.Builder result = Span2.builder() + .traceIdHigh(traceIdHi != null ? traceIdHi : 0L) + .traceId(traceIdLo) + .parentId(row.getValue(ZipkinSpans.ZIPKIN_SPANS.PARENT_ID)) + .id(spanId); + + if (srService != null) { + return result.kind(Span2.Kind.SERVER) + .localEndpoint(ep(srService)) + .remoteEndpoint(ep(caService)) + .build(); + } else if (saService != null) { + return result + .kind(csService != null ? Span2.Kind.CLIENT : null) + .localEndpoint(ep(caService)) + .remoteEndpoint(ep(saService)) + .build(); + } else if (csService != null) { + return result.kind(Span2.Kind.SERVER) + .localEndpoint(ep(caService)) + .build(); + } return result.build(); } @@ -129,8 +157,12 @@ static long traceIdHigh(PeekingIterator delegate) { return delegate.peek().getValue(ZipkinSpans.ZIPKIN_SPANS.TRACE_ID_HIGH); } - static String emptyToNull(Record next, TableField field) { + static @Nullable String emptyToNull(Record next, TableField field) { String result = next.getValue(field); return result != null && !"".equals(result) ? result : null; } + + static Endpoint ep(@Nullable String serviceName) { + return serviceName != null ? Endpoint.builder().serviceName(serviceName).build() : null; + } } diff --git a/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/MySQLSpanStore.java b/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/MySQLSpanStore.java index 75c5480b657..f3c8b321e3b 100644 --- a/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/MySQLSpanStore.java +++ b/zipkin-storage/mysql/src/main/java/zipkin/storage/mysql/MySQLSpanStore.java @@ -40,11 +40,11 @@ import zipkin.DependencyLink; import zipkin.Endpoint; import zipkin.Span; -import zipkin.internal.DependencyLinkSpan; import zipkin.internal.DependencyLinker; import zipkin.internal.GroupByTraceId; import zipkin.internal.Nullable; import zipkin.internal.Pair; +import zipkin.internal.Span2; import zipkin.storage.QueryRequest; import zipkin.storage.SpanStore; import zipkin.storage.mysql.internal.generated.tables.ZipkinAnnotations; @@ -324,8 +324,8 @@ List aggregateDependencies(long endTs, @Nullable Long lookback, // Grouping so that later code knows when a span or trace is finished. .groupBy(schema.dependencyLinkGroupByFields).fetchLazy(); - Iterator> traces = - new DependencyLinkSpanIterator.ByTraceId(cursor.iterator(), schema.hasTraceIdHigh); + Iterator> traces = + new DependencyLinkSpan2Iterator.ByTraceId(cursor.iterator(), schema.hasTraceIdHigh); if (!traces.hasNext()) return Collections.emptyList(); diff --git a/zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpanIteratorTest.java b/zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpan2IteratorTest.java similarity index 57% rename from zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpanIteratorTest.java rename to zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpan2IteratorTest.java index 7bc7f8b2812..21f2d9cc544 100644 --- a/zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpanIteratorTest.java +++ b/zipkin-storage/mysql/src/test/java/zipkin/storage/mysql/DependencyLinkSpan2IteratorTest.java @@ -19,16 +19,15 @@ import org.jooq.impl.DSL; import org.junit.Test; import zipkin.Constants; -import zipkin.internal.DependencyLinkSpan; import zipkin.internal.PeekingIterator; +import zipkin.internal.Span2; import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; import static zipkin.storage.mysql.internal.generated.tables.ZipkinAnnotations.ZIPKIN_ANNOTATIONS; import static zipkin.storage.mysql.internal.generated.tables.ZipkinSpans.ZIPKIN_SPANS; -// TODO: this class temporarily uses reflection until zipkin2 span replaces DependencyLinkSpan -public class DependencyLinkSpanIteratorTest { +public class DependencyLinkSpan2IteratorTest { Long traceIdHigh = null; long traceId = 1L; Long parentId = null; @@ -36,50 +35,50 @@ public class DependencyLinkSpanIteratorTest { /** You cannot make a dependency link unless you know the the local or peer endpoint. */ @Test public void whenNoServiceLabelsExist_kindIsUnknown() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", null) ); - DependencyLinkSpan span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("UNKNOWN"); - assertThat(span).extracting("service").containsNull(); - assertThat(span).extracting("peerService").containsNull(); + Span2 span = iterator.next(); + assertThat(span.kind()).isNull(); + assertThat(span.localEndpoint()).isNull(); + assertThat(span.remoteEndpoint()).isNull(); } - @Test public void whenOnlyAddressLabelsExist_kindIsClient() { - DependencyLinkSpanIterator iterator = iterator( + @Test public void whenOnlyAddressLabelsExist_kindIsNull() { + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service1"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sa", "service2") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("CLIENT"); - assertThat(span).extracting("service").containsOnly("service1"); - assertThat(span).extracting("peerService").containsOnly("service2"); + assertThat(span.kind()).isNull(); + assertThat(span.localEndpoint().serviceName).isEqualTo("service1"); + assertThat(span.remoteEndpoint().serviceName).isEqualTo("service2"); } /** The linker is biased towards server spans, or client spans that know the peer localEndpoint(). */ @Test public void whenServerLabelsAreMissing_kindIsUnknownAndLabelsAreCleared() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service1") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("UNKNOWN"); - assertThat(span).extracting("service").containsNull(); - assertThat(span).extracting("peerService").containsNull(); + assertThat(span.kind()).isNull(); + assertThat(span.localEndpoint()).isNull(); + assertThat(span.remoteEndpoint()).isNull(); } /** {@link Constants#SERVER_RECV} is only applied when the local span is acting as a server */ @Test public void whenSrServiceExists_kindIsServer() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "service") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service"); - assertThat(span).extracting("peerService").containsNull(); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service"); + assertThat(span.remoteEndpoint()).isNull(); } /** @@ -87,15 +86,15 @@ public class DependencyLinkSpanIteratorTest { * span */ @Test public void whenSrAndCaServiceExists_caIsThePeer() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service1"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "service2") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service2"); - assertThat(span).extracting("peerService").containsOnly("service1"); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service2"); + assertThat(span.remoteEndpoint().serviceName).isEqualTo("service1"); } /** @@ -103,57 +102,58 @@ public class DependencyLinkSpanIteratorTest { * span */ @Test public void whenSrAndCsServiceExists_caIsThePeer() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", "service1"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "service2") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service2"); - assertThat(span).extracting("peerService").containsOnly("service1"); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service2"); + assertThat(span.remoteEndpoint().serviceName).isEqualTo("service1"); } /** {@link Constants#CLIENT_ADDR} is more authoritative than {@link Constants#CLIENT_SEND} */ @Test public void whenCrAndCaServiceExists_caIsThePeer() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", "foo"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service1"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "service2") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service2"); - assertThat(span).extracting("peerService").containsOnly("service1"); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service2"); + assertThat(span.remoteEndpoint().serviceName).isEqualTo("service1"); } /** Finagle labels two sides of the same socket "ca", "sa" with the local endpoint name */ @Test public void specialCasesFinagleLocalSocketLabeling_client() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( + newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", "service"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sa", "service") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); // When there's no "sr" annotation, we assume it is a client. - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("CLIENT"); - assertThat(span).extracting("service").containsNull(); - assertThat(span).extracting("peerService").containsOnly("service"); + assertThat(span.kind()).isEqualTo(Span2.Kind.CLIENT); + assertThat(span.localEndpoint()).isNull(); + assertThat(span.remoteEndpoint().serviceName).isEqualTo("service"); } @Test public void specialCasesFinagleLocalSocketLabeling_server() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", "service"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sa", "service"), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "service") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); // When there is an "sr" annotation, we know it is a server - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service"); - assertThat(span).extracting("peerService").containsNull(); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service"); + assertThat(span.remoteEndpoint()).isNull(); } /** @@ -161,33 +161,33 @@ public class DependencyLinkSpanIteratorTest { * caller, than a client span lacking its receiver. */ @Test public void csWithoutSaIsServer() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", "service1") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("SERVER"); - assertThat(span).extracting("service").containsOnly("service1"); - assertThat(span).extracting("peerService").containsNull(); + assertThat(span.kind()).isEqualTo(Span2.Kind.SERVER); + assertThat(span.localEndpoint().serviceName).isEqualTo("service1"); + assertThat(span.remoteEndpoint()).isNull(); } /** Service links to empty string are confusing and offer no value. */ @Test public void emptyToNull() { - DependencyLinkSpanIterator iterator = iterator( + DependencyLinkSpan2Iterator iterator = iterator( newRecord().values(traceIdHigh, traceId, parentId, spanId, "ca", ""), newRecord().values(traceIdHigh, traceId, parentId, spanId, "cs", ""), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sa", ""), newRecord().values(traceIdHigh, traceId, parentId, spanId, "sr", "") ); - DependencyLinkSpan span = iterator.next(); + Span2 span = iterator.next(); - assertThat(span).extracting("kind").extracting(Object::toString).containsOnly("UNKNOWN"); - assertThat(span).extracting("service").containsNull(); - assertThat(span).extracting("peerService").containsNull(); + assertThat(span.kind()).isNull(); + assertThat(span.localEndpoint()).isNull(); + assertThat(span.remoteEndpoint()).isNull(); } - static DependencyLinkSpanIterator iterator(Record... records) { - return new DependencyLinkSpanIterator( + static DependencyLinkSpan2Iterator iterator(Record... records) { + return new DependencyLinkSpan2Iterator( new PeekingIterator<>(asList(records).iterator()), records[0].get(ZIPKIN_SPANS.TRACE_ID_HIGH), records[0].get(ZIPKIN_SPANS.TRACE_ID) ); diff --git a/zipkin/src/main/java/zipkin/internal/DependencyLinkSpan.java b/zipkin/src/main/java/zipkin/internal/DependencyLinkSpan.java deleted file mode 100644 index 1fc6bc887d2..00000000000 --- a/zipkin/src/main/java/zipkin/internal/DependencyLinkSpan.java +++ /dev/null @@ -1,245 +0,0 @@ -/** - * Copyright 2015-2017 The OpenZipkin 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 zipkin.internal; - -import zipkin.Annotation; -import zipkin.BinaryAnnotation; -import zipkin.Constants; -import zipkin.Span; - -import static zipkin.internal.Util.checkNotNull; -import static zipkin.internal.Util.equal; -import static zipkin.internal.Util.writeHexLong; - -/** - * Internal type used by {@link DependencyLinker linker} that holds the minimum state needed to - * aggregate {@link zipkin.DependencyLink dependency links}. - */ -// fields not exposed as public to further discourage use as a general type -public final class DependencyLinkSpan { - - /** Unique 8 or 16-byte identifier for a trace, set on all spans within it. */ - static final class TraceId { - - /** 0 may imply 8-byte identifiers are in use */ - final long hi; - final long lo; - - TraceId(long hi, long lo) { - this.hi = hi; - this.lo = lo; - } - - @Override - public boolean equals(Object o) { - if (o == this) return true; - if (o instanceof TraceId) { - TraceId that = (TraceId) o; - return (this.hi == that.hi) - && (this.lo == that.lo); - } - return false; - } - - @Override - public int hashCode() { - int h = 1; - h *= 1000003; - h ^= (hi >>> 32) ^ hi; - h *= 1000003; - h ^= (lo >>> 32) ^ lo; - return h; - } - - /** Returns the hex representation of the span's trace ID */ - @Override public String toString() { - if (hi != 0) { - char[] result = new char[32]; - writeHexLong(result, 0, hi); - writeHexLong(result, 16, lo); - return new String(result); - } - char[] result = new char[16]; - writeHexLong(result, 0, lo); - return new String(result); - } - } - - /** - * Indicates the primary span type. - */ - enum Kind { - CLIENT, - /** The span includes a {@link zipkin.Constants#SERVER_RECV}. */ - SERVER, - UNKNOWN - } - - final TraceId traceId; - @Nullable - final Long parentId; - final long id; - final Kind kind; - @Nullable - final String service; - @Nullable - final String peerService; - - DependencyLinkSpan(TraceId traceId, Long parentId, long id, Kind kind, String service, - String peerService) { - this.traceId = traceId; - this.parentId = parentId; - this.id = id; - this.kind = checkNotNull(kind, "kind"); - this.service = service; - this.peerService = peerService; - } - - @Override public String toString() { - StringBuilder json = new StringBuilder(); - json.append("{\"traceId\": \"").append(Util.toLowerHex(traceId.hi, traceId.lo)).append('\"'); - if (parentId != null) { - json.append(", \"parentId\": \"").append(Util.toLowerHex(parentId)).append('\"'); - } - json.append(", \"id\": \"").append(Util.toLowerHex(id)).append('\"'); - json.append(", \"kind\": \"").append(kind).append('\"'); - if (service != null) json.append(", \"service\": \"").append(service).append('\"'); - if (peerService != null) json.append(", \"peerService\": \"").append(peerService).append('\"'); - return json.append("}").toString(); - } - - /** Only considers ID fields, as these spans are not expected to repeat */ - @Override - public boolean equals(Object o) { - if (o == this) return true; - if (o instanceof DependencyLinkSpan) { - DependencyLinkSpan that = (DependencyLinkSpan) o; - return equal(this.traceId, that.traceId) - && equal(this.parentId, that.parentId) - && (this.id == that.id); - } - return false; - } - - /** Only considers ID fields, as these spans are not expected to repeat */ - @Override - public int hashCode() { - int h = 1; - h *= 1000003; - h ^= traceId.hashCode(); - h *= 1000003; - h ^= (parentId == null) ? 0 : parentId.hashCode(); - h *= 1000003; - h ^= (id >>> 32) ^ id; - return h; - } - - public static Builder builder(long traceIdHigh, long traceIdLow, Long parentId, long spanId) { - return new Builder(new TraceId(traceIdHigh, traceIdLow), parentId, spanId); - } - - public static DependencyLinkSpan from(Span s) { - TraceId traceId = new TraceId(s.traceIdHigh, s.traceId); - DependencyLinkSpan.Builder linkSpan = new DependencyLinkSpan.Builder(traceId, s.parentId, s.id); - for (BinaryAnnotation a : s.binaryAnnotations) { - if (a.key.equals(Constants.CLIENT_ADDR) && a.endpoint != null) { - linkSpan.caService(a.endpoint.serviceName); - } else if (a.key.equals(Constants.SERVER_ADDR) && a.endpoint != null) { - linkSpan.saService(a.endpoint.serviceName); - } - } - for (Annotation a : s.annotations) { - if (a.value.equals(Constants.SERVER_RECV) && a.endpoint != null) { - linkSpan.srService(a.endpoint.serviceName); - } else if (a.value.equals(Constants.CLIENT_SEND) && a.endpoint != null) { - linkSpan.csService(a.endpoint.serviceName); - } - } - return linkSpan.build(); - } - - public static final class Builder { - final TraceId traceId; - final Long parentId; - final long spanId; - String srService; - String csService; - String caService; - String saService; - - Builder(TraceId traceId, Long parentId, long spanId) { - this.traceId = traceId; - this.spanId = spanId; - this.parentId = parentId; - } - - /** - * {@link zipkin.Constants#SERVER_RECV} is the preferred name of server, and this is a - * traditional span. - */ - public Builder srService(String srService) { - if ("".equals(srService)) srService = null; - this.srService = srService; - return this; - } - - /** - * {@link zipkin.Constants#CLIENT_SEND} is read to see calls into the root span from - * instrumented clients. - */ - public Builder csService(String csService) { - if ("".equals(csService)) csService = null; - this.csService = csService; - return this; - } - - /** - * {@link zipkin.Constants#CLIENT_ADDR} is read to see calls into the root span from - * uninstrumented clients. - */ - public Builder caService(String caService) { - if ("".equals(caService)) caService = null; - this.caService = caService; - return this; - } - - /** - * {@link zipkin.Constants#SERVER_ADDR} is only read at the leaf, when a client calls an - * un-instrumented server. - */ - public Builder saService(String saService) { - if ("".equals(saService)) saService = null; - this.saService = saService; - return this; - } - - public DependencyLinkSpan build() { - // The client address is more authoritative than the client send owner. - if (caService == null) caService = csService; - - // Finagle labels two sides of the same socket ("ca", "sa") with the same name. - // Skip the client side, so it isn't mistaken for a loopback request - if (equal(saService, caService)) caService = null; - - if (srService != null) { - return new DependencyLinkSpan(traceId, parentId, spanId, Kind.SERVER, srService, caService); - } else if (saService != null) { - return new DependencyLinkSpan(traceId, parentId, spanId, Kind.CLIENT, caService, saService); - } else if (csService != null) { - return new DependencyLinkSpan(traceId, parentId, spanId, Kind.SERVER, caService, null); - } - return new DependencyLinkSpan(traceId, parentId, spanId, Kind.UNKNOWN, null, null); - } - } -} diff --git a/zipkin/src/main/java/zipkin/internal/DependencyLinker.java b/zipkin/src/main/java/zipkin/internal/DependencyLinker.java index 3748065ce07..abd9f067d7b 100644 --- a/zipkin/src/main/java/zipkin/internal/DependencyLinker.java +++ b/zipkin/src/main/java/zipkin/internal/DependencyLinker.java @@ -23,17 +23,18 @@ import java.util.logging.Logger; import zipkin.DependencyLink; import zipkin.Span; +import zipkin.internal.Span2.Kind; import static java.util.logging.Level.FINE; -import static zipkin.internal.Util.checkNotNull; /** * This parses a span tree into dependency links used by Web UI. Ex. http://zipkin/dependency * - *

This implementation traverses the tree, and only creates links between {@link - * DependencyLinkSpan.Kind#SERVER server} spans. One exception is at the bottom of the trace tree. - * {@link DependencyLinkSpan.Kind#CLIENT client} spans that record their {@link - * DependencyLinkSpan#peerService peer} are included, as this accounts for uninstrumented services. + *

This implementation traverses the tree, and only creates links between {@link Kind#SERVER + * server} spans. One exception is at the bottom of the trace tree. {@link Kind#CLIENT client} spans + * that record their {@link Span2#remoteEndpoint()} are included, as this accounts for + * uninstrumented services. Spans with {@link Span2#kind()} unset, but {@link + * Span2#remoteEndpoint()} set are treated the same as client spans. */ public final class DependencyLinker { private final Logger logger; @@ -53,34 +54,52 @@ public DependencyLinker() { public DependencyLinker putTrace(Collection spans) { if (spans.isEmpty()) return this; - List linkSpans = new LinkedList<>(); + List linkSpans = new LinkedList<>(); for (Span s : MergeById.apply(spans)) { - linkSpans.add(DependencyLinkSpan.from(s)); + linkSpans.addAll(Span2Converter.fromSpan(s)); } return putTrace(linkSpans.iterator()); } + static final Node.MergeFunction MERGE_RPC = new Node.MergeFunction() { + @Override public Span2 merge(Span2 existing, Span2 update) { + if (existing == null) return update; + if (update == null) return existing; + if (existing.kind() == null) return update; + if (update.kind() == null) return existing; + Span2 server = existing.kind() == Kind.SERVER ? existing : update; + Span2 client = existing == server ? update : existing; + if (server.remoteEndpoint() != null && !"".equals(server.remoteEndpoint().serviceName)) { + return server; + } + return server.toBuilder().remoteEndpoint(client.localEndpoint()).build(); + } + + @Override public String toString() { + return "MergeRpc"; + } + }; + /** * @param spans spans where all spans have the same trace id */ - public DependencyLinker putTrace(Iterator spans) { + public DependencyLinker putTrace(Iterator spans) { if (!spans.hasNext()) return this; - DependencyLinkSpan first = spans.next(); - Node.TreeBuilder builder = - new Node.TreeBuilder<>(logger, first.traceId.toString()); - builder.addNode(first.parentId, first.id, first); - + Span2 first = spans.next(); + Node.TreeBuilder builder = + new Node.TreeBuilder<>(logger, MERGE_RPC, first.traceIdString()); + builder.addNode(first.parentId(), first.id(), first); while (spans.hasNext()) { - DependencyLinkSpan next = spans.next(); - builder.addNode(next.parentId, next.id, next); + Span2 next = spans.next(); + builder.addNode(next.parentId(), next.id(), next); } - Node tree = builder.build(); + Node tree = builder.build(); if (logger.isLoggable(FINE)) logger.fine("traversing trace tree, breadth-first"); - for (Iterator> i = tree.traverse(); i.hasNext(); ) { - Node current = i.next(); - DependencyLinkSpan currentSpan = current.value(); + for (Iterator> i = tree.traverse(); i.hasNext(); ) { + Node current = i.next(); + Span2 currentSpan = current.value(); if (logger.isLoggable(FINE)) { logger.fine("processing " + currentSpan); } @@ -88,12 +107,31 @@ public DependencyLinker putTrace(Iterator spans) { logger.fine("skipping synthetic node for broken span tree"); continue; } + + Kind kind = currentSpan.kind(); + if (Kind.CLIENT.equals(kind) && !current.children().isEmpty()) { + logger.fine("deferring link to rpc child span"); + continue; + } + + String serviceName = serviceName(currentSpan); + String remoteServiceName = remoteServiceName(currentSpan); + if (kind == null) { + // Treat unknown type of span as a client span if we know both sides + if (serviceName != null && remoteServiceName != null) { + kind = Kind.CLIENT; + } else { + logger.fine("non-rpc span; skipping"); + continue; + } + } + String child; String parent; - switch (currentSpan.kind) { + switch (kind) { case SERVER: - child = currentSpan.service; - parent = currentSpan.peerService; + child = serviceName; + parent = remoteServiceName; if (current == tree) { // we are the root-most span. if (parent == null) { logger.fine("root's peer is unknown; skipping"); @@ -102,11 +140,11 @@ public DependencyLinker putTrace(Iterator spans) { } break; case CLIENT: - child = currentSpan.peerService; - parent = currentSpan.service; + parent = serviceName; + child = remoteServiceName; break; default: - logger.fine("non-rpc span; skipping"); + logger.fine("unknown kind; skipping"); continue; } @@ -114,39 +152,60 @@ public DependencyLinker putTrace(Iterator spans) { logger.fine("cannot determine parent, looking for first server ancestor"); } - // Local spans may be between the current node and its remote ancestor - // Look up the stack until we see a service name, and assume that's the client - Node ancestor = current.parent(); - while (ancestor != null && parent == null) { - if (logger.isLoggable(FINE)) { - logger.fine("processing ancestor " + ancestor.value()); - } - DependencyLinkSpan ancestorLink = ancestor.value(); - if (!ancestor.isSyntheticRootForPartialTree() && - ancestorLink.kind == DependencyLinkSpan.Kind.SERVER) { - parent = ancestorLink.service; - break; + String rpcAncestor = findRpcAncestor(current); + if (rpcAncestor != null) { + + // Local spans may be between the current node and its remote parent + if (parent == null) parent = rpcAncestor; + + // Some users accidentally put the remote service name on client annotations. + // Check for this and backfill a link from the nearest remote to that service as necessary. + if (Kind.CLIENT.equals(kind) && serviceName != null && !rpcAncestor.equals(serviceName)) { + logger.fine("detected missing link to client span"); + addLink(rpcAncestor, serviceName); + continue; } - ancestor = ancestor.parent(); } if (parent == null || child == null) { logger.fine("cannot find server ancestor; skipping"); continue; - } else if (logger.isLoggable(FINE)) { - logger.fine("incrementing link " + parent + " -> " + child); } - Pair key = Pair.create(parent, child); - if (linkMap.containsKey(key)) { - linkMap.put(key, linkMap.get(key) + 1); - } else { - linkMap.put(key, 1L); - } + addLink(parent, child); } return this; } + String findRpcAncestor(Node current) { + Node ancestor = current.parent(); + while (ancestor != null) { + if (logger.isLoggable(FINE)) { + logger.fine("processing ancestor " + ancestor.value()); + } + if (!ancestor.isSyntheticRootForPartialTree()) { + Span2 maybeRemote = ancestor.value(); + if (maybeRemote.kind() != null) { + return serviceName(maybeRemote); + } + } + ancestor = ancestor.parent(); + } + return null; + } + + void addLink(String parent, String child) { + if (logger.isLoggable(FINE)) { + logger.fine("incrementing link " + parent + " -> " + child); + } + Pair key = Pair.create(parent, child); + if (linkMap.containsKey(key)) { + linkMap.put(key, linkMap.get(key) + 1); + } else { + linkMap.put(key, 1L); + } + } + public List link() { // links are merged by mapping to parent/child and summing corresponding links List result = new ArrayList<>(linkMap.size()); @@ -173,4 +232,16 @@ public static List merge(Iterable in) { } return result; } + + static String serviceName(Span2 span) { + return span.localEndpoint() != null && !"".equals(span.localEndpoint().serviceName) + ? span.localEndpoint().serviceName + : null; + } + + static String remoteServiceName(Span2 span) { + return span.remoteEndpoint() != null && !"".equals(span.remoteEndpoint().serviceName) + ? span.remoteEndpoint().serviceName + : null; + } } diff --git a/zipkin/src/main/java/zipkin/internal/Node.java b/zipkin/src/main/java/zipkin/internal/Node.java index 5b8b4b97eb0..48ef740e5a6 100644 --- a/zipkin/src/main/java/zipkin/internal/Node.java +++ b/zipkin/src/main/java/zipkin/internal/Node.java @@ -109,6 +109,16 @@ public void remove() { } } + interface MergeFunction { + V merge(@Nullable V existing, @Nullable V update); + } + + static final MergeFunction FIRST_NOT_NULL = new MergeFunction() { + @Override public Object merge(Object existing, Object update) { + return existing != null ? existing : update; + } + }; + /** * Some operations do not require the entire span object. This creates a tree given (parent id, * id) pairs. @@ -117,16 +127,21 @@ public void remove() { */ static final class TreeBuilder { final Logger logger; + final MergeFunction mergeFunction; final String traceId; TreeBuilder(Logger logger, String traceId) { + this(logger, FIRST_NOT_NULL, traceId); + } + + TreeBuilder(Logger logger, MergeFunction mergeFunction, String traceId) { this.logger = logger; + this.mergeFunction = mergeFunction; this.traceId = traceId; } - Node rootNode = null; Long rootId = null; - + Node rootNode = null; // Nodes representing the trace tree Map> idToNode = new LinkedHashMap<>(); // Collect the parent-child relationships between all spans. @@ -158,8 +173,11 @@ public boolean addNode(@Nullable Long parentId, long id, V value) { if (parentId == null && rootNode == null) { rootNode = node; rootId = id; + } else if (parentId == null && rootId == id) { + rootNode.value(mergeFunction.merge(rootNode.value, node.value)); } else { - idToNode.put(id, node); + Node previous = idToNode.put(id, node); + if (previous != null) node.value(mergeFunction.merge(previous.value, node.value)); idToParent.put(id, parentId); } return true; diff --git a/zipkin/src/test/java/zipkin/internal/DependencyLinkSpanTest.java b/zipkin/src/test/java/zipkin/internal/DependencyLinkSpanTest.java deleted file mode 100644 index d8a1f702055..00000000000 --- a/zipkin/src/test/java/zipkin/internal/DependencyLinkSpanTest.java +++ /dev/null @@ -1,221 +0,0 @@ -/** - * Copyright 2015-2017 The OpenZipkin 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 zipkin.internal; - -import org.junit.Test; -import zipkin.Constants; -import zipkin.internal.DependencyLinkSpan.Kind; - -import static org.assertj.core.api.Assertions.assertThat; - -public class DependencyLinkSpanTest { - - @Test - public void testToString() { - assertThat(DependencyLinkSpan.builder(0L, 1L, null, 1L).build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"id\": \"0000000000000001\", \"kind\": \"UNKNOWN\"}"); - - assertThat(DependencyLinkSpan.builder(0L, 1L, 1L, 2L).build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"parentId\": \"0000000000000001\", \"id\": \"0000000000000002\", \"kind\": \"UNKNOWN\"}"); - - assertThat(DependencyLinkSpan.builder(0L, 1L, 1L, 2L) - .srService("processor") - .caService("kinesis").build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"parentId\": \"0000000000000001\", \"id\": \"0000000000000002\", \"kind\": \"SERVER\", \"service\": \"processor\", \"peerService\": \"kinesis\"}"); - - // It is invalid to log "ca" without "sr", so marked as unknown - assertThat(DependencyLinkSpan.builder(0L, 1L, 1L, 2L) - .caService("kinesis").build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"parentId\": \"0000000000000001\", \"id\": \"0000000000000002\", \"kind\": \"UNKNOWN\"}"); - - assertThat(DependencyLinkSpan.builder(0L, 1L, 1L, 2L) - .saService("mysql").build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"parentId\": \"0000000000000001\", \"id\": \"0000000000000002\", \"kind\": \"CLIENT\", \"peerService\": \"mysql\"}"); - - // arbitrary 2-sided span - assertThat(DependencyLinkSpan.builder(0L, 1L, 1L, 2L) - .caService("shell-script") - .saService("mysql").build()) - .hasToString( - "{\"traceId\": \"0000000000000001\", \"parentId\": \"0000000000000001\", \"id\": \"0000000000000002\", \"kind\": \"CLIENT\", \"service\": \"shell-script\", \"peerService\": \"mysql\"}"); - - // 128-bit trace ID - assertThat(DependencyLinkSpan.builder(3L, 1L, null, 1L).build()) - .hasToString( - "{\"traceId\": \"00000000000000030000000000000001\", \"id\": \"0000000000000001\", \"kind\": \"UNKNOWN\"}"); - } - - @Test - public void parentAndChildApply() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L).build(); - assertThat(span.parentId).isNull(); - assertThat(span.id).isEqualTo(1L); - - span = DependencyLinkSpan.builder(0L, 1L, 1L, 2L).build(); - assertThat(span.parentId).isEqualTo(1L); - assertThat(span.id).isEqualTo(2L); - } - - /** You cannot make a dependency link unless you know the the local or peer service. */ - @Test - public void whenNoServiceLabelsExist_kindIsUnknown() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L).build(); - - assertThat(span.kind).isEqualTo(Kind.UNKNOWN); - assertThat(span.peerService).isNull(); - assertThat(span.service).isNull(); - } - - @Test - public void whenOnlyAddressLabelsExist_kindIsClient() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .caService("service1") - .saService("service2") - .build(); - - assertThat(span.kind).isEqualTo(Kind.CLIENT); - assertThat(span.service).isEqualTo("service1"); - assertThat(span.peerService).isEqualTo("service2"); - } - - /** The linker is biased towards server spans, or client spans that know the peer service. */ - @Test - public void whenServerLabelsAreMissing_kindIsUnknownAndLabelsAreCleared() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .caService("service1") - .build(); - - assertThat(span.kind).isEqualTo(Kind.UNKNOWN); - assertThat(span.service).isNull(); - assertThat(span.peerService).isNull(); - } - - /** {@link Constants#SERVER_RECV} is only applied when the local span is acting as a server */ - @Test - public void whenSrServiceExists_kindIsServer() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .srService("service") - .build(); - - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service"); - assertThat(span.peerService).isNull(); - } - - /** - * {@link Constants#CLIENT_ADDR} indicates the peer, which is a client in the case of a server - * span - */ - @Test - public void whenSrAndCaServiceExists_caIsThePeer() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .caService("service1") - .srService("service2") - .build(); - - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service2"); - assertThat(span.peerService).isEqualTo("service1"); - } - - /** - * {@link Constants#CLIENT_SEND} indicates the peer, which is a client in the case of a server - * span - */ - @Test - public void whenSrAndCsServiceExists_caIsThePeer() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .csService("service1") - .srService("service2") - .build(); - - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service2"); - assertThat(span.peerService).isEqualTo("service1"); - } - - /** {@link Constants#CLIENT_ADDR} is more authoritative than {@link Constants#CLIENT_SEND} */ - @Test - public void whenCrAndCaServiceExists_caIsThePeer() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .csService("foo") - .caService("service1") - .srService("service2") - .build(); - - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service2"); - assertThat(span.peerService).isEqualTo("service1"); - } - - @Test - public void specialCasesFinagleLocalSocketLabeling() { - // Finagle labels two sides of the same socket ("ca", "sa") with the local service name. - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .caService("service") - .saService("service") - .build(); - - // When there's no "sr" annotation, we assume it is a client. - assertThat(span.kind).isEqualTo(Kind.CLIENT); - assertThat(span.service).isNull(); - assertThat(span.peerService).isEqualTo("service"); - - span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .srService("service") - .caService("service") - .saService("service") - .build(); - - // When there is an "sr" annotation, we know it is a server - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service"); - assertThat(span.peerService).isNull(); - } - - /** - *

Dependency linker works backwards: it is easier to treat a "cs" as a server span lacking its - * caller, than a client span lacking its receiver. - */ - @Test - public void csWithoutSaIsServer() { - DependencyLinkSpan span = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .csService("service1") - .build(); - - assertThat(span.kind).isEqualTo(Kind.SERVER); - assertThat(span.service).isEqualTo("service1"); - assertThat(span.peerService).isNull(); - } - - /** Service links to empty string are confusing and offer no value. */ - @Test - public void emptyToNull() { - DependencyLinkSpan.Builder builder = DependencyLinkSpan.builder(0L, 1L, null, 1L) - .caService("") - .csService("") - .saService("") - .srService(""); - - assertThat(builder.caService).isNull(); - assertThat(builder.csService).isNull(); - assertThat(builder.saService).isNull(); - assertThat(builder.srService).isNull(); - } -} diff --git a/zipkin/src/test/java/zipkin/internal/DependencyLinkerTest.java b/zipkin/src/test/java/zipkin/internal/DependencyLinkerTest.java index 362d6a63bda..c38181aa406 100644 --- a/zipkin/src/test/java/zipkin/internal/DependencyLinkerTest.java +++ b/zipkin/src/test/java/zipkin/internal/DependencyLinkerTest.java @@ -20,10 +20,10 @@ import java.util.stream.Collectors; import org.junit.Test; import zipkin.DependencyLink; +import zipkin.Endpoint; import zipkin.Span; import zipkin.TestObjects; -import zipkin.internal.DependencyLinkSpan.Kind; -import zipkin.internal.DependencyLinkSpan.TraceId; +import zipkin.internal.Span2.Kind; import static java.util.Arrays.asList; import static org.assertj.core.api.Assertions.assertThat; @@ -50,8 +50,8 @@ public void baseCase() { @Test public void linksSpans() { assertThat(new DependencyLinker().putTrace(TestObjects.TRACE).link()).containsExactly( - DependencyLink.create("web", "app", 1L), - DependencyLink.create("app", "db", 1L) + DependencyLink.create("web", "app", 1L), + DependencyLink.create("app", "db", 1L) ); } @@ -69,71 +69,48 @@ public void dropsSelfReferencingSpans() { ); } - /** - * The linker links a directed graph, if the span kind is unknown, we don't know the direction to - * link. - */ - @Test - public void doesntLinkUnknownRootSpans() { - List unknownRootSpans = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.UNKNOWN, null, null), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.UNKNOWN, "server", "client"), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.UNKNOWN, "client", "server") - ); - - for (DependencyLinkSpan span : unknownRootSpans) { - assertThat(new DependencyLinker(logger) - .putTrace(asList(span).iterator()).link()) - .isEmpty(); - } - - assertThat(messages).contains( - "non-rpc span; skipping" - ); - } - /** * A root span can be a client-originated trace or a server receipt which knows its peer. In these * cases, the peer is known and kind establishes the direction. */ @Test public void linksSpansDirectedByKind() { - List validRootSpans = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "server", "client"), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.CLIENT, "client", "server") + List validRootSpans = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "server", "client"), + span2(0L, 1L, null, 1L, Kind.CLIENT, "client", "server") ); - for (DependencyLinkSpan span : validRootSpans) { + for (Span2 span : validRootSpans) { assertThat(new DependencyLinker() - .putTrace(asList(span).iterator()).link()) - .containsOnly(DependencyLink.create("client", "server", 1L)); + .putTrace(asList(span).iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 1L)); } } @Test public void callsAgainstTheSameLinkIncreasesCallCount_span() { - List trace = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "client", null), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 2L, Kind.CLIENT, null, "server"), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 3L, Kind.CLIENT, null, "server") + List trace = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "client", null), + span2(0L, 1L, 1L, 2L, Kind.CLIENT, null, "server"), + span2(0L, 1L, 1L, 3L, Kind.CLIENT, null, "server") ); assertThat(new DependencyLinker() - .putTrace(trace.iterator()).link()) - .containsOnly(DependencyLink.create("client", "server", 2L)); + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 2L)); } @Test public void callsAgainstTheSameLinkIncreasesCallCount_trace() { - List trace = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "client", null), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 2L, Kind.CLIENT, null, "server") + List trace = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "client", null), + span2(0L, 1L, 1L, 2L, Kind.CLIENT, null, "server") ); assertThat(new DependencyLinker() - .putTrace(trace.iterator()) - .putTrace(trace.iterator()).link()) - .containsOnly(DependencyLink.create("client", "server", 2L)); + .putTrace(trace.iterator()) + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 2L)); } /** @@ -142,22 +119,57 @@ public void callsAgainstTheSameLinkIncreasesCallCount_trace() { */ @Test public void singleHostSpansResultInASingleCallCount() { - List> singleLinks = asList( - asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.CLIENT, "client", "server"), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 2L, Kind.SERVER, "server", null) - ), - asList( - new DependencyLinkSpan(new TraceId(0L, 3L), null, 3L, Kind.SERVER, "client", null), - new DependencyLinkSpan(new TraceId(0L, 3L), 3L, 4L, Kind.CLIENT, "client", "server") - ) + List trace = asList( + span2(0L, 3L, null, 3L, Kind.CLIENT, "client", null), + span2(0L, 3L, 3L, 4L, Kind.SERVER, "server", "client") ); - for (List trace : singleLinks) { - assertThat(new DependencyLinker() - .putTrace(trace.iterator()).link()) - .containsOnly(DependencyLink.create("client", "server", 1L)); - } + assertThat(new DependencyLinker() + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 1L)); + } + + @Test + public void singleHostSpansResultInASingleCallCount_defersNameToServer() { + List trace = asList( + span2(0L, 1L, null, 1L, Kind.CLIENT, "client", "server"), + span2(0L, 1L, 1L, 2L, Kind.SERVER, "server", null) + ); + + assertThat(new DependencyLinker(logger) + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 1L)); + + assertThat(messages).contains("deferring link to rpc child span"); + messages.clear(); + } + + @Test + public void singleHostSpans_multipleChildren() { + List trace = asList( + span2(0L, 4L, null, 4L, Kind.CLIENT, "client", null), + span2(0L, 4L, 4L, 5L, Kind.SERVER, "server", "client"), + span2(0L, 4L, 4L, 6L, Kind.SERVER, "server", "client") + ); + + assertThat(new DependencyLinker() + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 2L)); + } + + @Test + public void singleHostSpans_multipleChildren_defersNameToServer() { + List trace = asList( + span2(0L, 1L, null, 1L, Kind.CLIENT, "client", "server"), + span2(0L, 1L, 1L, 2L, Kind.SERVER, "server", null), + span2(0L, 1L, 1L, 3L, Kind.SERVER, "server", null) + ); + + assertThat(new DependencyLinker(logger) + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 2L)); + + assertThat(messages).contains("deferring link to rpc child span"); } /** @@ -166,67 +178,82 @@ public void singleHostSpansResultInASingleCallCount() { */ @Test public void intermediatedClientSpansMissingLocalServiceNameLinkToNearestServer() { - List trace = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "client", null), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 2L, Kind.UNKNOWN, null, null), - // possibly a local fan-out span - new DependencyLinkSpan(new TraceId(0L, 1L), 2L, 3L, Kind.CLIENT, null, "server"), - new DependencyLinkSpan(new TraceId(0L, 1L), 2L, 4L, Kind.CLIENT, null, "server") + List trace = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "client", null), + span2(0L, 1L, 1L, 2L, null, null, null), + // possibly a local fan-out span + span2(0L, 1L, 2L, 3L, Kind.CLIENT, null, "server"), + span2(0L, 1L, 2L, 4L, Kind.CLIENT, null, "server") ); assertThat(new DependencyLinker() - .putTrace(trace.iterator()).link()) - .containsOnly(DependencyLink.create("client", "server", 2L)); + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("client", "server", 2L)); } /** A loopback span is direction-agnostic, so can be linked properly regardless of kind. */ @Test public void linksLoopbackSpans() { - List validRootSpans = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "service", "service"), - new DependencyLinkSpan(new TraceId(0L, 2L), null, 2L, Kind.CLIENT, "service", "service") + List validRootSpans = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "service", "service"), + span2(0L, 2L, null, 2L, Kind.CLIENT, "service", "service") ); - for (DependencyLinkSpan span : validRootSpans) { + for (Span2 span : validRootSpans) { assertThat(new DependencyLinker() - .putTrace(asList(span).iterator()).link()) - .containsOnly(DependencyLink.create("service", "service", 1L)); + .putTrace(asList(span).iterator()).link()) + .containsOnly(DependencyLink.create("service", "service", 1L)); } } + @Test + public void noSpanKindTreatedSameAsClient() { + List trace = asList( + span2(0L, 1L, null, 1L, null, "some-client", "web"), + span2(0L, 1L, 1L, 2L, null, "web", "app"), + span2(0L, 1L, 2L, 3L, null, "app", "db") + ); + + assertThat(new DependencyLinker().putTrace(trace.iterator()).link()).containsOnly( + DependencyLink.create("some-client", "web", 1L), + DependencyLink.create("web", "app", 1L), + DependencyLink.create("app", "db", 1L) + ); + } + /** * A dependency link is between two services. Given only one span, we cannot link if we don't know * both service names. */ @Test public void cannotLinkSingleSpanWithoutBothServiceNames() { - List incompleteRootSpans = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, null, null), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "server", null), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, null, "client"), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.CLIENT, null, null), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.CLIENT, "client", null), - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.CLIENT, null, "server") + List incompleteRootSpans = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, null, null), + span2(0L, 1L, null, 1L, Kind.SERVER, "server", null), + span2(0L, 1L, null, 1L, Kind.SERVER, null, "client"), + span2(0L, 1L, null, 1L, Kind.CLIENT, null, null), + span2(0L, 1L, null, 1L, Kind.CLIENT, "client", null), + span2(0L, 1L, null, 1L, Kind.CLIENT, null, "server") ); - for (DependencyLinkSpan span : incompleteRootSpans) { + for (Span2 span : incompleteRootSpans) { assertThat(new DependencyLinker(logger) - .putTrace(asList(span).iterator()).link()) - .isEmpty(); + .putTrace(asList(span).iterator()).link()) + .isEmpty(); } } @Test public void doesntLinkUnrelatedSpansWhenMissingRootSpan() { long missingParentId = 1; - List trace = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), missingParentId, 2L, Kind.SERVER, "service1", null), - new DependencyLinkSpan(new TraceId(0L, 1L), missingParentId, 3L, Kind.SERVER, "service2", null) + List trace = asList( + span2(0L, 1L, missingParentId, 2L, Kind.SERVER, "service1", null), + span2(0L, 1L, missingParentId, 3L, Kind.SERVER, "service2", null) ); assertThat(new DependencyLinker(logger) - .putTrace(trace.iterator()).link()) - .isEmpty(); + .putTrace(trace.iterator()).link()) + .isEmpty(); assertThat(messages).contains( "skipping synthetic node for broken span tree" @@ -236,14 +263,14 @@ public void doesntLinkUnrelatedSpansWhenMissingRootSpan() { @Test public void linksRelatedSpansWhenMissingRootSpan() { long missingParentId = 1; - List trace = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), missingParentId, 2L, Kind.SERVER, "service1", null), - new DependencyLinkSpan(new TraceId(0L, 1L), 2L, 3L, Kind.SERVER, "service2", null) + List trace = asList( + span2(0L, 1L, missingParentId, 2L, Kind.SERVER, "service1", null), + span2(0L, 1L, 2L, 3L, Kind.SERVER, "service2", null) ); assertThat(new DependencyLinker(logger) - .putTrace(trace.iterator()).link()) - .containsOnly(DependencyLink.create("service1", "service2", 1L)); + .putTrace(trace.iterator()).link()) + .containsOnly(DependencyLink.create("service1", "service2", 1L)); assertThat(messages).contains( "skipping synthetic node for broken span tree" @@ -253,9 +280,9 @@ public void linksRelatedSpansWhenMissingRootSpan() { /** Client+Server spans that don't share IDs are treated as server spans missing their peer */ @Test public void linksSingleHostSpans() { - List singleHostSpans = asList( - new DependencyLinkSpan(new TraceId(0L, 1L), null, 1L, Kind.SERVER, "web", null), - new DependencyLinkSpan(new TraceId(0L, 1L), 1L, 2L, Kind.SERVER, "app", null) + List singleHostSpans = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "web", null), + span2(0L, 1L, 1L, 2L, Kind.SERVER, "app", null) ); assertThat(new DependencyLinker() @@ -263,17 +290,45 @@ public void linksSingleHostSpans() { .containsOnly(DependencyLink.create("web", "app", 1L)); } + /** Creates a link when there's a span missing, in this case 2L which is an RPC from web to app */ + @Test + public void missingSpan() { + List singleHostSpans = asList( + span2(0L, 1L, null, 1L, Kind.SERVER, "web", null), + span2(0L, 1L, 1L, 2L, Kind.CLIENT, "app", null) + ); + + assertThat(new DependencyLinker(logger) + .putTrace(singleHostSpans.iterator()).link()) + .containsOnly(DependencyLink.create("web", "app", 1L)); + + assertThat(messages).contains( + "detected missing link to client span" + ); + } + @Test public void merge() { List links = asList( - DependencyLink.create("client", "server", 2L), - DependencyLink.create("client", "server", 2L), - DependencyLink.create("client", "client", 1L) + DependencyLink.create("client", "server", 2L), + DependencyLink.create("client", "server", 2L), + DependencyLink.create("client", "client", 1L) ); assertThat(DependencyLinker.merge(links)).containsExactly( - DependencyLink.create("client", "server", 4L), - DependencyLink.create("client", "client", 1L) + DependencyLink.create("client", "server", 4L), + DependencyLink.create("client", "client", 1L) ); } + + static Span2 span2(long traceIdHigh, long traceId, @Nullable Long parentId, long id, + @Nullable Kind kind, + @Nullable String local, @Nullable String remote) { + Span2.Builder result = Span2.builder(); + result.traceIdHigh(traceIdHigh).traceId(traceId).parentId(parentId).id(id); + result.kind(kind); + if (local != null) result.localEndpoint(Endpoint.builder().serviceName(local).build()); + if (remote != null) result.remoteEndpoint(Endpoint.builder().serviceName(remote).build()); + return result.build(); + } }