Introduced a "shared" field in Span (#698)

whenever:

- a span or trace id get generated when a request / message arrives
- child span is created

we set the field to false

whenever we find ids in the incoming request / message we set the field to true

fixes #696
This commit is contained in:
Marcin Grzejszczak
2017-09-14 17:14:09 +02:00
committed by GitHub
parent 5bb444f042
commit cfd3a619dd
11 changed files with 240 additions and 64 deletions

View File

@@ -16,12 +16,6 @@
package org.springframework.cloud.sleuth;
import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
@@ -34,6 +28,13 @@ import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
import com.fasterxml.jackson.annotation.JsonAutoDetect;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
/**
* Class for gathering and reporting statistics about a block of execution.
* <p>
@@ -160,6 +161,13 @@ public class Span implements SpanContext {
@JsonIgnore
private final Long startNanos;
private Long durationMicros; // serialized in json so micros precision isn't lost
/*
Using B3 propagation, it is most typical to share the same span ID across client and
the server. This has backend implications like who owns the timestamp (hint the
client does). When a SpanReporter receives a completed span, it should know if it
is shared or not.
*/
private final boolean shared;
@SuppressWarnings("unused")
private Span() {
@@ -191,17 +199,18 @@ public class Span implements SpanContext {
this.durationMicros = current.durationMicros;
this.baggage = current.baggage;
this.savedSpan = savedSpan;
this.shared = current.shared;
}
Span(long begin, long end, String name, long traceId, List<Long> parents,
long spanId, boolean remote, boolean exportable, String processId) {
this(begin, end, name, traceId, parents, spanId, remote, exportable, processId,
null);
null, false);
}
Span(long begin, long end, String name, long traceId, List<Long> parents,
long spanId, boolean remote, boolean exportable, String processId,
Span savedSpan) {
Span savedSpan, boolean shared) {
this(new SpanBuilder()
.begin(begin)
.end(end)
@@ -212,7 +221,8 @@ public class Span implements SpanContext {
.remote(remote)
.exportable(exportable)
.processId(processId)
.savedSpan(savedSpan));
.savedSpan(savedSpan)
.shared(shared));
}
Span(SpanBuilder builder) {
@@ -242,6 +252,7 @@ public class Span implements SpanContext {
this.logs.addAll(builder.logs);
this.baggage = new ConcurrentHashMap<>();
this.baggage.putAll(builder.baggage);
this.shared = builder.shared;
}
public static SpanBuilder builder() {
@@ -499,6 +510,16 @@ public class Span implements SpanContext {
return this.exportable;
}
/**
* Span and trace id got extracted from a carrier?
* We are adding data to the same span created by a remote client2
*
* @since 1.3.0
*/
public boolean isShared() {
return this.shared;
}
/**
* Returns the 16 or 32 character hex representation of the span's trace ID
*
@@ -644,6 +665,7 @@ public class Span implements SpanContext {
private final List<Log> logs = new ArrayList<>();
private final Map<String, String> tags = new LinkedHashMap<>();
private final Map<String, String> baggage = new LinkedHashMap<>();
private boolean shared;
SpanBuilder() {
}
@@ -748,6 +770,11 @@ public class Span implements SpanContext {
return this;
}
public Span.SpanBuilder shared(boolean shared) {
this.shared = shared;
return this;
}
/**
* Creates a {@link Span.SpanBuilder} from the {@link Span}.
*/

View File

@@ -1,12 +1,12 @@
package org.springframework.cloud.sleuth.instrument.messaging;
import java.util.Map;
import java.util.Random;
import org.springframework.cloud.sleuth.Span;
import org.springframework.cloud.sleuth.SpanTextMap;
import org.springframework.cloud.sleuth.util.TextMapUtil;
import java.util.Map;
import java.util.Random;
/**
* Default implementation for messaging
*
@@ -18,9 +18,11 @@ public class HeaderBasedMessagingExtractor implements MessagingSpanTextMapExtrac
@Override
public Span joinTrace(SpanTextMap textMap) {
Map<String, String> carrier = TextMapUtil.asMap(textMap);
boolean spanIdMissing = !hasHeader(carrier, TraceMessageHeaders.SPAN_ID_NAME);
boolean traceIdMissing = !hasHeader(carrier, TraceMessageHeaders.TRACE_ID_NAME);
if (Span.SPAN_SAMPLED.equals(carrier.get(TraceMessageHeaders.SPAN_FLAGS_NAME))) {
String traceId = generateTraceIdIfMissing(carrier);
if (!carrier.containsKey(TraceMessageHeaders.SPAN_ID_NAME)) {
String traceId = generateTraceIdIfMissing(carrier, traceIdMissing);
if (spanIdMissing) {
carrier.put(TraceMessageHeaders.SPAN_ID_NAME, traceId);
}
} else if (!hasHeader(carrier, TraceMessageHeaders.SPAN_ID_NAME)
@@ -28,28 +30,32 @@ public class HeaderBasedMessagingExtractor implements MessagingSpanTextMapExtrac
return null;
// TODO: Consider throwing IllegalArgumentException;
}
return extractSpanFromHeaders(carrier, Span.builder());
boolean idMissing = spanIdMissing || traceIdMissing;
return extractSpanFromHeaders(carrier, Span.builder(), idMissing);
}
private String generateTraceIdIfMissing(Map<String, String> carrier) {
if (!hasHeader(carrier, TraceMessageHeaders.TRACE_ID_NAME)) {
private String generateTraceIdIfMissing(Map<String, String> carrier,
boolean traceIdMissing) {
if (traceIdMissing) {
carrier.put(TraceMessageHeaders.TRACE_ID_NAME, Span.idToHex(new Random().nextLong()));
}
return carrier.get(TraceMessageHeaders.TRACE_ID_NAME);
}
private Span extractSpanFromHeaders(Map<String, String> carrier, Span.SpanBuilder spanBuilder) {
private Span extractSpanFromHeaders(Map<String, String> carrier,
Span.SpanBuilder spanBuilder, boolean idMissing) {
String traceId = carrier.get(TraceMessageHeaders.TRACE_ID_NAME);
spanBuilder = spanBuilder
.traceIdHigh(traceId.length() == 32 ? Span.hexToId(traceId, 0) : 0)
.traceId(Span.hexToId(traceId))
.spanId(Span.hexToId(carrier.get(TraceMessageHeaders.SPAN_ID_NAME)));
String flags = carrier.get(TraceMessageHeaders.SPAN_FLAGS_NAME);
if (Span.SPAN_SAMPLED.equals(flags)) {
boolean debug = Span.SPAN_SAMPLED.equals(flags);
boolean spanSampled = Span.SPAN_SAMPLED.equals(carrier.get(TraceMessageHeaders.SAMPLED_NAME));
if (debug) {
spanBuilder.exportable(true);
} else {
spanBuilder.exportable(
Span.SPAN_SAMPLED.equals(carrier.get(TraceMessageHeaders.SAMPLED_NAME)));
spanBuilder.exportable(spanSampled);
}
String processId = carrier.get(TraceMessageHeaders.PROCESS_ID_NAME);
String spanName = carrier.get(TraceMessageHeaders.SPAN_NAME_NAME);
@@ -61,6 +67,7 @@ public class HeaderBasedMessagingExtractor implements MessagingSpanTextMapExtrac
}
setParentIdIfApplicable(carrier, spanBuilder, TraceMessageHeaders.PARENT_ID_NAME);
spanBuilder.remote(true);
spanBuilder.shared((debug || spanSampled) && !idMissing);
for (Map.Entry<String, String> entry : carrier.entrySet()) {
if (entry.getKey().toLowerCase().startsWith(Span.SPAN_BAGGAGE_HEADER_PREFIX + TraceMessageHeaders.HEADER_DELIMITER)) {
spanBuilder.baggage(unprefixedKey(entry.getKey()), entry.getValue());

View File

@@ -35,7 +35,8 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor {
public Span joinTrace(SpanTextMap textMap) {
Map<String, String> carrier = TextMapUtil.asMap(textMap);
boolean debug = Span.SPAN_SAMPLED.equals(carrier.get(Span.SPAN_FLAGS));
if (debug && onlySpanIdIsPresent(carrier)) {
boolean idToBeGenerated = debug && onlySpanIdIsPresent(carrier);
if (idToBeGenerated) {
// we're only generating Trace ID since if there's no Span ID will assume
// that it's equal to Trace ID - we're trying to fix a malformed request
generateIdIfMissing(carrier, Span.TRACE_ID_NAME);
@@ -48,7 +49,7 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor {
boolean skip = this.skipPattern.matcher(uri).matches()
|| Span.SPAN_NOT_SAMPLED.equals(carrier.get(Span.SAMPLED_NAME));
long spanId = spanId(carrier);
return buildParentSpan(carrier, uri, skip, spanId);
return buildParentSpan(carrier, uri, skip, spanId, idToBeGenerated);
} catch (Exception e) {
log.error("Exception occurred while trying to extract span from carrier", e);
return null;
@@ -86,7 +87,8 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor {
}
}
private Span buildParentSpan(Map<String, String> carrier, String uri, boolean skip, long spanId) {
private Span buildParentSpan(Map<String, String> carrier, String uri, boolean skip,
long spanId, boolean idToBeGenerated) {
String traceId = carrier.get(Span.TRACE_ID_NAME);
Span.SpanBuilder span = Span.builder()
.traceIdHigh(traceId.length() == 32 ? Span.hexToId(traceId, 0) : 0)
@@ -106,6 +108,8 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor {
span.parent(Span.hexToId(carrier.get(Span.PARENT_ID_NAME)));
}
span.remote(true);
// trace, span id were retrieved from the headers and span is sampled
span.shared(!(skip || idToBeGenerated));
boolean debug = Span.SPAN_SAMPLED.equals(carrier.get(Span.SPAN_FLAGS));
if (debug) {
span.exportable(true);

View File

@@ -16,15 +16,16 @@
package org.springframework.cloud.sleuth;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.Test;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.assertThat;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicLong;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.assertThat;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
import org.junit.Test;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
/**
* @author Marcin Grzejszczak
@@ -286,6 +287,7 @@ public class SpanTests {
private Span.SpanBuilder builder() {
return Span.builder().name("http:name").traceId(1L).spanId(2L).parent(3L)
.begin(1L).end(2L).traceId(3L).exportable(true).parent(4L)
.baggage("foo", "bar").remote(true).tag("tag", "tag").log(new Log(System.currentTimeMillis(), "log"));
.baggage("foo", "bar")
.remote(true).shared(true).tag("tag", "tag").log(new Log(System.currentTimeMillis(), "log"));
}
}

View File

@@ -190,6 +190,26 @@ public class SpanAssert extends AbstractAssert<SpanAssert, Span> {
return this;
}
public SpanAssert isShared() {
isNotNull();
if (!this.actual.isShared()) {
String message = "The span is supposed to be shared but it's not!";
log.error(message);
failWithMessage(message);
}
return this;
}
public SpanAssert isNotShared() {
isNotNull();
if (this.actual.isShared()) {
String message = "The span is NOT supposed to be shared but it is!";
log.error(message);
failWithMessage(message);
}
return this;
}
public SpanAssert isNotExportable() {
isNotNull();
if (this.actual.isExportable()) {

View File

@@ -16,6 +16,8 @@
package org.springframework.cloud.sleuth.instrument.messaging;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
@@ -24,8 +26,6 @@ import org.junit.Test;
import org.springframework.cloud.sleuth.Span;
import org.springframework.cloud.sleuth.SpanTextMap;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
/**
* @author Marcin Grzejszczak
*/
@@ -42,7 +42,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isShared();
}
@Test
@@ -56,7 +56,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isShared();
}
@Test
@@ -69,7 +69,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isShared();
}
@Test
@@ -82,7 +82,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isNotExportable();
then(span).isNotExportable().isNotShared();
}
@Test
@@ -93,7 +93,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isNotShared();
then(span.traceIdString()).isNotEmpty();
then(span.getSpanId()).isNotNull();
}
@@ -107,7 +107,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isNotShared();
then(span.traceIdString()).isNotEmpty();
then(span.getSpanId()).isEqualTo(10L);
}
@@ -121,7 +121,7 @@ public class HeaderBasedMessagingExtractorTests {
Span span = extractor.joinTrace(spanTextMap);
then(span).isExportable();
then(span).isExportable().isNotShared();
then(span.getTraceId()).isEqualTo(10L);
then(span.getSpanId()).isEqualTo(10L);
}

View File

@@ -16,6 +16,8 @@
package org.springframework.cloud.sleuth.instrument.web;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
import javax.servlet.http.HttpServletRequest;
import java.util.Arrays;
import java.util.Random;
@@ -30,8 +32,6 @@ import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;
import org.springframework.cloud.sleuth.Span;
import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
@RunWith(MockitoJUnitRunner.class)
public class HttpServletRequestExtractorTests {
@@ -82,18 +82,60 @@ public class HttpServletRequestExtractorTests {
@Test
public void should_accept_128bit_trace_id() {
String hex128Bits = "463ac35c9f6413ad48485a3953bb6124";
String lower64Bits = "48485a3953bb6124";
String hex128Bits = spanInHeaders();
BDDMockito.given(this.request.getHeaderNames())
.willReturn(new Vector<>(Arrays.asList(Span.TRACE_ID_NAME, Span.SPAN_ID_NAME)).elements());
BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME))
.willReturn(hex128Bits);
BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME))
.willReturn(lower64Bits);
Span span = this.extractor.joinTrace(new HttpServletRequestTextMap(this.request));
then(span.traceIdString()).isEqualTo(hex128Bits);
}
@Test
public void should_set_shared_flag_for_sampled_span_in_headers() {
spanInHeaders();
Span span = this.extractor.joinTrace(new HttpServletRequestTextMap(this.request));
then(span.isShared()).isTrue();
}
@Test
public void should_not_set_shared_flag_for_non_sampled_span_in_headers() {
spanInHeaders();
BDDMockito.given(this.request.getHeader(Span.SAMPLED_NAME))
.willReturn(Span.SPAN_NOT_SAMPLED);
Span span = this.extractor.joinTrace(new HttpServletRequestTextMap(this.request));
then(span.isShared()).isFalse();
}
@Test
public void should_not_set_shared_flag_for_sampled_span_in_headers_without_span_trace_id() {
BDDMockito.given(this.request.getHeaderNames())
.willReturn(new Vector<>(Arrays.asList(Span.SPAN_FLAGS, Span.SPAN_ID_NAME)).elements());
BDDMockito.given(this.request.getHeader(Span.SPAN_FLAGS))
.willReturn("1");
BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME))
.willReturn("48485a3953bb6124");
Span span = this.extractor.joinTrace(new HttpServletRequestTextMap(this.request));
then(span.isShared()).isFalse();
}
private String spanInHeaders() {
String hex128Bits = "463ac35c9f6413ad48485a3953bb6124";
String lower64Bits = "48485a3953bb6124";
BDDMockito.given(this.request.getHeaderNames())
.willReturn(new Vector<>(Arrays.asList(Span.TRACE_ID_NAME, Span.SPAN_ID_NAME, Span.SAMPLED_NAME)).elements());
BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME))
.willReturn(hex128Bits);
BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME))
.willReturn(lower64Bits);
BDDMockito.given(this.request.getHeader(Span.SAMPLED_NAME))
.willReturn(Span.SPAN_SAMPLED);
return hex128Bits;
}
}

View File

@@ -15,6 +15,10 @@
*/
package org.springframework.cloud.sleuth.zipkin.stream;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import org.apache.commons.logging.Log;
import org.springframework.cloud.sleuth.Span;
import org.springframework.cloud.sleuth.stream.Host;
@@ -26,10 +30,6 @@ import zipkin.Constants;
import zipkin.Endpoint;
import zipkin.Span.Builder;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
/**
* This converts sleuth spans to zipkin ones, skipping invalid or unsampled.
*
@@ -96,9 +96,14 @@ final class ConvertToZipkinSpanList {
// rather let the client do that. Worst case we were propagated an unreported ID and
// Zipkin backfills timestamp and duration.
if (!span.isRemote()) {
zipkinSpan.timestamp(span.getBegin() * 1000);
if (!span.isRunning()) { // duration is authoritative, only write when the span stopped
zipkinSpan.duration(calculateDurationInMicros(span));
if (Boolean.TRUE.equals(span.isShared())) {
// don't report server-side timestamp on shared spans
zipkinSpan.timestamp(null).duration(null);
} else {
zipkinSpan.timestamp(span.getBegin() * 1000);
if (!span.isRunning()) { // duration is authoritative, only write when the span stopped
zipkinSpan.duration(calculateDurationInMicros(span));
}
}
}
zipkinSpan.traceIdHigh(span.getTraceIdHigh());

View File

@@ -15,6 +15,11 @@
*/
package org.springframework.cloud.sleuth.zipkin.stream;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Random;
import org.assertj.core.api.Condition;
import org.junit.Test;
import org.springframework.cloud.sleuth.Span;
@@ -23,11 +28,6 @@ import org.springframework.cloud.sleuth.stream.Spans;
import zipkin.Constants;
import zipkin.Endpoint;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Random;
import static org.assertj.core.api.Assertions.assertThat;
public class ConvertToZipkinSpanListTests {
@@ -217,6 +217,39 @@ public class ConvertToZipkinSpanListTests {
assertThat(result.traceId).isEqualTo(span.getTraceId());
}
@Test
public void shouldRemoveTimestampAndDurationForNonRemoteSharedSpan() {
Span span = Span.builder()
.name("foo")
.exportable(false)
.remote(false)
.shared(true)
.build();
Spans spans = new Spans(this.host, Collections.singletonList(span));
zipkin.Span result = ConvertToZipkinSpanList.convert(spans).get(0);
assertThat(result.duration).isNull();
assertThat(result.timestamp).isNull();
}
@Test
public void shouldNotRemoveTimestampAndDurationForNonRemoteNonSharedSpan() {
Span span = Span.builder()
.name("foo")
.exportable(false)
.remote(false)
.shared(false)
.build();
span.stop();
Spans spans = new Spans(this.host, Collections.singletonList(span));
zipkin.Span result = ConvertToZipkinSpanList.convert(spans).get(0);
assertThat(result.duration).isNotNull();
assertThat(result.timestamp).isNotNull();
}
Span span(String name) {
return span(name, false);
}

View File

@@ -99,9 +99,14 @@ public class ZipkinSpanListener implements SpanReporter {
// rather let the client do that. Worst case we were propagated an unreported ID and
// Zipkin backfills timestamp and duration.
if (!convertedSpan.isRemote()) {
zipkinSpan.timestamp(convertedSpan.getBegin() * 1000L);
if (!convertedSpan.isRunning()) { // duration is authoritative, only write when the span stopped
zipkinSpan.duration(calculateDurationInMicros(convertedSpan));
// don't report server-side timestamp on shared spans
if (Boolean.TRUE.equals(convertedSpan.isShared())) {
zipkinSpan.timestamp(null).duration(null);
} else {
zipkinSpan.timestamp(convertedSpan.getBegin() * 1000L);
if (!convertedSpan.isRunning()) { // duration is authoritative, only write when the span stopped
zipkinSpan.duration(calculateDurationInMicros(convertedSpan));
}
}
}
zipkinSpan.traceIdHigh(convertedSpan.getTraceIdHigh());

View File

@@ -319,6 +319,37 @@ public class ZipkinSpanListenerTests {
assertThat(result.name).isEqualTo("foo");
}
@Test
public void shouldRemoveTimestampAndDurationForNonRemoteSharedSpan() {
Span span = Span.builder()
.name("foo")
.exportable(false)
.remote(false)
.shared(true)
.build();
zipkin.Span result = this.spanListener.convert(span);
assertThat(result.duration).isNull();
assertThat(result.timestamp).isNull();
}
@Test
public void shouldNotRemoveTimestampAndDurationForNonRemoteNonSharedSpan() {
Span span = Span.builder()
.name("foo")
.exportable(false)
.remote(false)
.shared(false)
.build();
span.stop();
zipkin.Span result = this.spanListener.convert(span);
assertThat(result.duration).isNotNull();
assertThat(result.timestamp).isNotNull();
}
@Configuration
@EnableAutoConfiguration
protected static class TestConfiguration {