From be735659b6cc630430ae7c3e9cd2a9c81bd9d001 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Wed, 22 Jun 2016 01:14:53 +0200 Subject: [PATCH] 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 --- .../messaging/MessagingSpanExtractor.java | 50 +++++++- .../web/HttpServletRequestExtractor.java | 45 +++++-- .../web/TraceWebAutoConfiguration.java | 5 +- .../MessagingSpanExtractorTests.java | 110 ++++++++++++++++++ .../web/HttpServletRequestExtractorTests.java | 92 +++++++++++++++ .../TraceFilterMockChainIntegrationTests.java | 6 +- .../instrument/web/TraceFilterTests.java | 2 +- 7 files changed, 292 insertions(+), 18 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java index b3dc9178f..a51c9cf7b 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractor.java +++ b/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> { + 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> { 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> { 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> { 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; + } + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java index f3748bae9..45527099b 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractor.java +++ b/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 { 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 { 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 { + "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 { 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 { } 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); + } + } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java index 47c5bee0d..763850d63 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java +++ b/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 httpServletRequestSpanExtractor( - SkipPatternProvider skipPatternProvider) { - return new HttpServletRequestExtractor(skipPatternProvider.skipPattern()); + SkipPatternProvider skipPatternProvider, Random random) { + return new HttpServletRequestExtractor(skipPatternProvider.skipPattern(), random); } @Bean diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/MessagingSpanExtractorTests.java new file mode 100644 index 000000000..8da2be3f7 --- /dev/null +++ b/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 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()); + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/HttpServletRequestExtractorTests.java new file mode 100644 index 000000000..223e996b6 --- /dev/null +++ b/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(); + } +} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java index f8dbb0134..c62a7d1ca 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterMockChainIntegrationTests.java +++ b/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()); diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java index 427b49424..9030e5d59 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java +++ b/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 spanExtractor = new HttpServletRequestExtractor(Pattern - .compile(TraceFilter.DEFAULT_SKIP_PATTERN)); + .compile(TraceFilter.DEFAULT_SKIP_PATTERN), new Random()); SpanInjector spanInjector = new HttpServletResponseInjector(); private Tracer tracer;