From aa20124088d0bc693c60146b571cc7521fa9ecf1 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 18 Aug 2017 16:24:28 +0200 Subject: [PATCH] TraceFilter skipPattern and extra span when X-B3-Flags header is present without this change if there's a debug flag we set traceid & spanid for the first request inside the zipkin http extractor. we don't set the parent span flag so an additional span is created. with this change we set the trace and span id ONLY when the debug flag is set AND the request is malformed. That means - only when the span id is set and there is no trace id. fixes #665 --- .../web/ZipkinHttpSpanExtractor.java | 28 +++++++++++++------ .../instrument/web/TraceFilterTests.java | 26 +++++++++++++++-- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ZipkinHttpSpanExtractor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ZipkinHttpSpanExtractor.java index 5cfe870ea..91906e711 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ZipkinHttpSpanExtractor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ZipkinHttpSpanExtractor.java @@ -1,16 +1,16 @@ package org.springframework.cloud.sleuth.instrument.web; +import java.lang.invoke.MethodHandles; +import java.util.Map; +import java.util.Random; +import java.util.regex.Pattern; + import org.apache.commons.logging.LogFactory; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanTextMap; import org.springframework.cloud.sleuth.util.TextMapUtil; import org.springframework.util.StringUtils; -import java.lang.invoke.MethodHandles; -import java.util.Map; -import java.util.Random; -import java.util.regex.Pattern; - /** * Default implementation, compatible with Zipkin propagation. * @@ -35,11 +35,11 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor { public Span joinTrace(SpanTextMap textMap) { Map carrier = TextMapUtil.asMap(textMap); boolean debug = Span.SPAN_SAMPLED.equals(carrier.get(Span.SPAN_FLAGS)); - if (debug) { + if (debug && onlySpanIdIsPresent(carrier)) { // we're only generating Trace ID since if there's no Span ID will assume - // that it's equal to Trace ID + // that it's equal to Trace ID - we're trying to fix a malformed request generateIdIfMissing(carrier, Span.TRACE_ID_NAME); - } else if (carrier.get(Span.TRACE_ID_NAME) == null) { + } else if (traceIdIsMissing(carrier)) { // can't build a Span without trace id return null; } @@ -55,6 +55,18 @@ public class ZipkinHttpSpanExtractor implements HttpSpanExtractor { } } + private boolean onlySpanIdIsPresent(Map carrier) { + return traceIdIsMissing(carrier) && spanIdIsPresent(carrier); + } + + private boolean traceIdIsMissing(Map carrier) { + return carrier.get(Span.TRACE_ID_NAME) == null; + } + + private boolean spanIdIsPresent(Map carrier) { + return carrier.get(Span.SPAN_ID_NAME) != null; + } + private void generateIdIfMissing(Map carrier, String key) { if (!carrier.containsKey(key)) { carrier.put(key, Span.idToHex(new Random().nextLong())); 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 e7f903ecc..2b1812cf5 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 @@ -31,7 +31,6 @@ import org.springframework.beans.factory.BeanFactory; import org.springframework.cloud.sleuth.DefaultSpanNamer; import org.springframework.cloud.sleuth.ErrorParser; import org.springframework.cloud.sleuth.ExceptionMessageErrorParser; -import org.springframework.cloud.sleuth.NoOpSpanReporter; import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanReporter; @@ -471,6 +470,29 @@ public class TraceFilterTests { then(ExceptionUtils.getLastException()).isNull(); } + @Test + public void samplesASpanDebugFlagWithInterceptor() throws Exception { + this.request = builder() + .header(Span.SPAN_FLAGS, 1) + .buildRequest(new MockServletContext()); + this.sampler = new NeverSampler(); + TraceFilter filter = new TraceFilter(beanFactory()); + + filter.doFilter(this.request, this.response, (req, res) -> { + // Simulate the TraceHandlerInterceptor + req.setAttribute(TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR, span); + this.filterChain.doFilter(req, res); + }); + + then(new ListOfSpans(this.spanReporter.getSpans())) + .doesNotHaveASpanWithName("http:/parent/") + .hasASpanWithName("http:/") + .hasSize(1) + .allSpansAreExportable(); + then(TestSpanContextHolder.getCurrentSpan()).isNull(); + then(ExceptionUtils.getLastException()).isNull(); + } + public void verifyParentSpanHttpTags() { verifyParentSpanHttpTags(HttpStatus.OK); } @@ -510,7 +532,7 @@ public class TraceFilterTests { BDDMockito.given(beanFactory.getBean(Tracer.class)).willReturn(this.tracer); BDDMockito.given(beanFactory.getBean(TraceKeys.class)).willReturn(this.traceKeys); BDDMockito.given(beanFactory.getBean(HttpSpanExtractor.class)).willReturn(this.spanExtractor); - BDDMockito.given(beanFactory.getBean(SpanReporter.class)).willReturn(new NoOpSpanReporter()); + BDDMockito.given(beanFactory.getBean(SpanReporter.class)).willReturn(this.spanReporter); BDDMockito.given(beanFactory.getBean(HttpTraceKeysInjector.class)).willReturn(this.httpTraceKeysInjector); BDDMockito.given(beanFactory.getBean(ErrorParser.class)).willReturn(new ExceptionMessageErrorParser()); return beanFactory;