Wrong span / trace ids dont blow up the app

with this change we're trying to retrieve the value of span / trace id from a header but we don't propagate the exception if that value is invalid. Instead we generate a new random value. It's better not to break the application and break the trace (if by any chance it's been corrupt).

Fixed #306

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java

@@ -16,8 +16,11 @@
 
 package org.springframework.cloud.sleuth.instrument.messaging;
 
+import java.lang.invoke.MethodHandles;
 import java.util.Random;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.cloud.sleuth.Span;
 import org.springframework.cloud.sleuth.Span.SpanBuilder;
 import org.springframework.cloud.sleuth.SpanExtractor;
@@ -31,6 +34,8 @@ import org.springframework.messaging.Message;
  */
 public class MessagingSpanExtractor implements SpanExtractor<Message<?>> {
 
+	private static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass());
+
 	private final Random random;
 
 	public MessagingSpanExtractor(Random random) {
@@ -44,14 +49,13 @@ public class MessagingSpanExtractor implements SpanExtractor<Message<?>> {
 			return null;
 			// TODO: Consider throwing IllegalArgumentException;
 		}
+		long traceId = getTraceIdOrSetDefault(carrier);
 		long spanId = hasHeader(carrier, Span.SPAN_ID_NAME)
-				? Span.hexToId(getHeader(carrier, Span.SPAN_ID_NAME))
+				? getSpanIdOrSetDefault(carrier)
 				: this.random.nextLong();
-		long traceId = Span.hexToId(getHeader(carrier, Span.TRACE_ID_NAME));
 		SpanBuilder spanBuilder = Span.builder().traceId(traceId).spanId(spanId);
 		spanBuilder.exportable(
 				Span.SPAN_SAMPLED.equals(getHeader(carrier, Span.SAMPLED_NAME)));
-		String parentId = getHeader(carrier, Span.PARENT_ID_NAME);
 		String processId = getHeader(carrier, Span.PROCESS_ID_NAME);
 		String spanName = getHeader(carrier, Span.SPAN_NAME_NAME);
 		if (spanName != null) {
@@ -60,9 +64,7 @@ public class MessagingSpanExtractor implements SpanExtractor<Message<?>> {
 		if (processId != null) {
 			spanBuilder.processId(processId);
 		}
-		if (parentId != null) {
-			spanBuilder.parent(Span.hexToId(parentId));
-		}
+		setParentIdIfApplicable(carrier, spanBuilder);
 		spanBuilder.remote(true);
 		return spanBuilder.build();
 	}
@@ -78,4 +80,40 @@ public class MessagingSpanExtractor implements SpanExtractor<Message<?>> {
 	boolean hasHeader(Message<?> message, String name) {
 		return message.getHeaders().containsKey(name);
 	}
+
+	private long getTraceIdOrSetDefault(Message<?> carrier) {
+		try {
+			return Span
+					.hexToId(getHeader(carrier, Span.TRACE_ID_NAME));
+		} catch (Exception e) {
+			long id = this.random.nextLong();
+			log.warn("Exception occurred while trying to retrieve the trace "
+					+ "id from headers. Will set id to value ["
+					+ Span.idToHex(id) + "]", e);
+			return id;
+		}
+	}
+	private void setParentIdIfApplicable(Message<?> carrier, SpanBuilder spanBuilder) {
+		try {
+			String parentId = getHeader(carrier, Span.PARENT_ID_NAME);
+			if (parentId != null) {
+				spanBuilder.parent(Span.hexToId(parentId));
+			}
+		} catch (Exception e) {
+			log.warn("Exception occurred while trying to set parentId", e);
+		}
+	}
+
+	private long getSpanIdOrSetDefault(Message<?> carrier) {
+		try {
+			return Span
+					.hexToId(getHeader(carrier, Span.SPAN_ID_NAME));
+		} catch (Exception e) {
+			long id = this.random.nextLong();
+			log.warn("Exception occurred while trying to retrieve the span "
+					+ "id from headers. Will set id to value ["
+					+ Span.idToHex(id) + "]", e);
+			return id;
+		}
+	}
 }

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java

@@ -18,6 +18,7 @@ package org.springframework.cloud.sleuth.instrument.web;
 
 import javax.servlet.http.HttpServletRequest;
 import java.lang.invoke.MethodHandles;
+import java.util.Random;
 import java.util.regex.Pattern;
 
 import org.apache.commons.logging.Log;
@@ -41,11 +42,13 @@ class HttpServletRequestExtractor implements SpanExtractor<HttpServletRequest> {
 	private static final String HTTP_COMPONENT = "http";
 
 	private final Pattern skipPattern;
+	private final Random random;
 
 	private UrlPathHelper urlPathHelper = new UrlPathHelper();
 
-	public HttpServletRequestExtractor(Pattern skipPattern) {
+	public HttpServletRequestExtractor(Pattern skipPattern, Random random) {
 		this.skipPattern = skipPattern;
+		this.random = random;
 	}
 
 	@Override
@@ -57,12 +60,24 @@ class HttpServletRequestExtractor implements SpanExtractor<HttpServletRequest> {
 		String uri = this.urlPathHelper.getPathWithinApplication(carrier);
 		boolean skip = this.skipPattern.matcher(uri).matches()
 				|| Span.SPAN_NOT_SAMPLED.equals(carrier.getHeader(Span.SAMPLED_NAME));
-		long traceId = Span
-				.hexToId(carrier.getHeader(Span.TRACE_ID_NAME));
+		long traceId = getTraceIdOrSetDefault(carrier);
 		long spanId = spanId(carrier, traceId);
 		return buildParentSpan(carrier, uri, skip, traceId, spanId);
 	}
 
+	private long getTraceIdOrSetDefault(HttpServletRequest carrier) {
+		try {
+			return Span
+					.hexToId(carrier.getHeader(Span.TRACE_ID_NAME));
+		} catch (Exception e) {
+			long id = this.random.nextLong();
+			log.warn("Exception occurred while trying to retrieve the trace "
+					+ "id from headers. Will set id to value ["
+					+ Span.idToHex(id) + "]", e);
+			return id;
+		}
+	}
+
 	private long spanId(HttpServletRequest carrier, long traceId) {
 		String spanId = carrier.getHeader(Span.SPAN_ID_NAME);
 		if (spanId == null) {
@@ -70,7 +85,15 @@ class HttpServletRequestExtractor implements SpanExtractor<HttpServletRequest> {
 					+ "a root span with span id equal to trace id");
 			return traceId;
 		} else {
-			return Span.hexToId(spanId);
+			try {
+				return Span.hexToId(spanId);
+			} catch (Exception e) {
+				long id = this.random.nextLong();
+				log.warn("Exception occurred while trying to retrieve the span id "
+						+ "from request headers. Will set id to value ["
+						+ Span.idToHex(id) + "]", e);
+				return id;
+			}
 		}
 	}
 
@@ -83,14 +106,13 @@ class HttpServletRequestExtractor implements SpanExtractor<HttpServletRequest> {
 			span.name(parentName);
 		}
 		else {
-			span.name(HTTP_COMPONENT + ":" + "/parent" + uri);
+			span.name(HTTP_COMPONENT + ":/parent" + uri);
 		}
 		if (StringUtils.hasText(processId)) {
 			span.processId(processId);
 		}
 		if (carrier.getHeader(Span.PARENT_ID_NAME) != null) {
-			span.parent(Span
-					.hexToId(carrier.getHeader(Span.PARENT_ID_NAME)));
+			setParentIdIfValid(carrier, span);
 		}
 		span.remote(true);
 		if (skip) {
@@ -98,4 +120,13 @@ class HttpServletRequestExtractor implements SpanExtractor<HttpServletRequest> {
 		}
 		return span.build();
 	}
+
+	private void setParentIdIfValid(HttpServletRequest carrier, SpanBuilder span) {
+		try {
+			span.parent(Span
+					.hexToId(carrier.getHeader(Span.PARENT_ID_NAME)));
+		} catch (Exception e) {
+			log.warn("Exception occurred while trying to set parent id", e);
+		}
+	}
 }

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java

@@ -15,6 +15,7 @@
  */
 package org.springframework.cloud.sleuth.instrument.web;
 
+import java.util.Random;
 import java.util.regex.Pattern;
 
 import javax.servlet.http.HttpServletRequest;
@@ -100,8 +101,8 @@ public class TraceWebAutoConfiguration {
 
 	@Bean
 	public SpanExtractor<HttpServletRequest> httpServletRequestSpanExtractor(
-			SkipPatternProvider skipPatternProvider) {
-		return new HttpServletRequestExtractor(skipPatternProvider.skipPattern());
+			SkipPatternProvider skipPatternProvider, Random random) {
+		return new HttpServletRequestExtractor(skipPatternProvider.skipPattern(), random);
 	}
 
 	@Bean

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright 2013-2016 the original author or 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 org.springframework.cloud.sleuth.instrument.messaging;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Random;
+
+import org.junit.Test;
+import org.springframework.cloud.sleuth.Span;
+import org.springframework.messaging.Message;
+import org.springframework.messaging.MessageHeaders;
+import org.springframework.messaging.support.MessageBuilder;
+import org.springframework.util.StringUtils;
+
+import static org.assertj.core.api.BDDAssertions.then;
+import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.then;
+
+public class MessagingSpanExtractorTests {
+	MessagingSpanExtractor extractor = new MessagingSpanExtractor(new Random());
+
+	@Test
+	public void should_return_null_if_trace_or_span_is_missing() {
+		Message message = MessageBuilder.createMessage("", headers());
+		then(this.extractor.joinTrace(message)).isNull();
+
+		message = MessageBuilder.createMessage("", headers("trace"));
+		then(this.extractor.joinTrace(message)).isNull();
+	}
+
+	@Test
+	public void should_set_random_traceid_if_header_value_is_invalid() {
+		Message message = MessageBuilder.createMessage("", 
+				headers("invalid", randomId()));
+
+		Span span = this.extractor.joinTrace(message);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+	}
+
+	@Test
+	public void should_set_random_spanid_if_header_value_is_invalid() {
+		Message message = MessageBuilder.createMessage("",
+				headers(randomId(), "invalid"));
+
+		Span span = this.extractor.joinTrace(message);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+		then(span.getSpanId()).isNotZero();
+	}
+
+	@Test
+	public void should_not_throw_exception_if_parent_id_is_invalid() {
+		Message message = MessageBuilder.createMessage("",
+				headers(randomId(), randomId(), "invalid"));
+
+		Span span = this.extractor.joinTrace(message);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+		then(span.getSpanId()).isNotZero();
+		then(span.getParents()).isEmpty();
+	}
+
+	private MessageHeaders headers() {
+		return headers(null, null, null);
+	}
+
+	private MessageHeaders headers(String traceId) {
+		return headers(traceId, null, null);
+	}
+
+	private MessageHeaders headers(String traceId, String spanId) {
+		return headers(traceId, spanId, null);
+	}
+	
+	private MessageHeaders headers(String traceId, String spanId, String parentId) {
+		Map<String, Object> map = new HashMap<>();
+		if (StringUtils.hasText(traceId)) {
+			map.put(Span.TRACE_ID_NAME, traceId);
+		}
+		if (StringUtils.hasText(spanId)) {
+			map.put(Span.SPAN_ID_NAME, spanId);
+		}
+		if (StringUtils.hasText(parentId)) {
+			map.put(Span.PARENT_ID_NAME, parentId);
+		}
+		return new MessageHeaders(map);
+	}
+	
+	private String randomId() {
+		return String.valueOf(new Random().nextLong());
+	}
+}

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright 2013-2016 the original author or 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 org.springframework.cloud.sleuth.instrument.web;
+
+import javax.servlet.http.HttpServletRequest;
+import java.util.Random;
+import java.util.regex.Pattern;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.BDDMockito;
+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 {
+
+	@Mock HttpServletRequest request;
+	HttpServletRequestExtractor extractor = new HttpServletRequestExtractor(
+			Pattern.compile(""), new Random());
+
+	@Before
+	public void setup() {
+		BDDMockito.given(this.request.getRequestURI()).willReturn("http://foo.com");
+		BDDMockito.given(this.request.getContextPath()).willReturn("/");
+	}
+
+	@Test
+	public void should_return_null_if_there_is_no_trace_id() {
+		then(extractor.joinTrace(request)).isNull();
+	}
+
+	@Test
+	public void should_set_random_traceid_if_header_value_is_invalid() {
+		BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME))
+				.willReturn("invalid");
+
+		Span span = this.extractor.joinTrace(this.request);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+	}
+
+	@Test
+	public void should_set_random_spanid_if_header_value_is_invalid() {
+		BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME))
+				.willReturn(String.valueOf(new Random().nextLong()));
+		BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME))
+				.willReturn("invalid");
+
+		Span span = this.extractor.joinTrace(this.request);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+		then(span.getSpanId()).isNotZero();
+	}
+
+	@Test
+	public void should_not_throw_exception_if_parent_id_is_invalid() {
+		BDDMockito.given(this.request.getHeader(Span.TRACE_ID_NAME))
+				.willReturn(String.valueOf(new Random().nextLong()));
+		BDDMockito.given(this.request.getHeader(Span.SPAN_ID_NAME))
+				.willReturn(String.valueOf(new Random().nextLong()));
+		BDDMockito.given(this.request.getHeader(Span.PARENT_ID_NAME))
+				.willReturn("invalid");
+
+		Span span = this.extractor.joinTrace(this.request);
+
+		then(span).isNotNull();
+		then(span.getTraceId()).isNotZero();
+		then(span.getSpanId()).isNotZero();
+		then(span.getParents()).isEmpty();
+	}
+}

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java

@@ -73,7 +73,8 @@ public class TraceFilterMockChainIntegrationTests {
 	@Test
 	public void startsNewTrace() throws Exception {
 		TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(),
-				new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)),
+				new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN),
+						new Random()),
 				new HttpServletResponseInjector(), keysInjector);
 		filter.doFilter(this.request, this.response, this.filterChain);
 		assertNull(TestSpanContextHolder.getCurrentSpan());
@@ -85,7 +86,8 @@ public class TraceFilterMockChainIntegrationTests {
 		this.request = builder().header(Span.SPAN_ID_NAME, generator.nextLong())
 				.header(Span.TRACE_ID_NAME, generator.nextLong()).buildRequest(new MockServletContext());
 		TraceFilter filter = new TraceFilter(this.tracer, this.traceKeys, new NoOpSpanReporter(),
-				new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN)),
+				new HttpServletRequestExtractor(Pattern.compile(TraceFilter.DEFAULT_SKIP_PATTERN),
+						new Random()),
 				new HttpServletResponseInjector(), keysInjector);
 		filter.doFilter(this.request, this.response, this.filterChain);
 		assertNull(TestSpanContextHolder.getCurrentSpan());

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java

@@ -63,7 +63,7 @@ public class TraceFilterTests {
 	@Mock SpanLogger spanLogger;
 	ArrayListSpanAccumulator spanReporter = new ArrayListSpanAccumulator();
 	SpanExtractor<HttpServletRequest> spanExtractor = new HttpServletRequestExtractor(Pattern
-			.compile(TraceFilter.DEFAULT_SKIP_PATTERN));
+			.compile(TraceFilter.DEFAULT_SKIP_PATTERN), new Random());
 	SpanInjector<HttpServletResponse> spanInjector = new HttpServletResponseInjector();
 
 	private Tracer tracer;