SpringCloud/spring-cloud-sleuth
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Removed setting of the "fallback span"

Browse Source 
without this change we created a "fallback span" in the case where a Spring component didn't handle the span. By handling it means that it got processed by anyway (a tag, log was added etc.). Why did we do that? That's because if no component was involved in creation of additional spans then in the Zipkin UI the service that had only the span that has the SR, SS logs wouldn't show up. In other words, if service A called service B and service B had only a single span, then service B wouldn't show up in the UI (there would be no blue bar). All the data would show up however in the client side of that span.

with this change we're removing this behaviour. That's because it introduces confusion for our users, that use non Spring related components. Also it seems that the Zipkin UI got fixed and this error no longer is present. If however it turns out that it's still there, it clearly seems that the place to fix this error should be Zipkin and not Sleuth.

fixes #708
This commit is contained in:
Marcin Grzejszczak
2017-09-22 13:21:46 +02:00
parent 4bf4484dcd
commit 1b1a1698ca
2 changed files with 39 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java
View File

@@ -171,7 +171,6 @@ public class TraceFilter extends GenericFilterBean {
// TODO: how to deal with response annotations and async?
return;
}
spanFromRequest = createSpanIfRequestNotHandled(request, spanFromRequest, name, skip);
detachOrCloseSpans(request, response, spanFromRequest, exception);
}
}
@@ -211,22 +210,6 @@ public class TraceFilter extends GenericFilterBean {
}
}
// This method is a fallback in case if handler interceptors didn't catch the request.
// In that case we are creating an artificial span so that it can be visible in Zipkin.
private Span createSpanIfRequestNotHandled(HttpServletRequest request,
Span spanFromRequest, String name, boolean skip) {
if (!requestHasAlreadyBeenHandled(request)) {
spanFromRequest = tracer().createSpan(name);
request.setAttribute(TRACE_REQUEST_ATTR, spanFromRequest);
if (log.isDebugEnabled() && !skip) {
log.debug("The request with uri [" + request.getRequestURI() + "] hasn't been handled by any of Sleuth's components. "
+ "That means that most likely you're using custom HandlerMappings and didn't add Sleuth's TraceHandlerInterceptor. "
+ "Sleuth will create a span to ensure that the graph of calls remains valid in Zipkin");
}
}
return spanFromRequest;
}
private boolean requestHasAlreadyBeenHandled(HttpServletRequest request) {
return request.getAttribute(TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR) != null;
}
@@ -239,8 +222,6 @@ public class TraceFilter extends GenericFilterBean {
addResponseTagsForSpanWithoutParent(request, response);
if (span.hasSavedSpan() && requestHasAlreadyBeenHandled(request)) {
recordParentSpan(span.getSavedSpan());
} else if (!requestHasAlreadyBeenHandled(request)) {
span = tracer().close(span);
}
recordParentSpan(span);
// in case of a response with exception status will close the span when exception dispatch is handled

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

@@ -45,6 +45,7 @@ import org.springframework.cloud.sleuth.trace.DefaultTracer;
import org.springframework.cloud.sleuth.trace.TestSpanContextHolder;
import org.springframework.cloud.sleuth.util.ArrayListSpanAccumulator;
import org.springframework.cloud.sleuth.util.ExceptionUtils;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.mock.web.MockFilterChain;
@@ -135,6 +136,35 @@ public class TraceFilterTests {
assertThat(this.span.tags()).containsEntry("http.status_code", HttpStatus.OK.toString());
then(TestSpanContextHolder.getCurrentSpan()).isNull();
then(new ListOfSpans(this.spanReporter.getSpans()))
.hasSize(1)
.hasASpanWithTagEqualTo("http.url", "http://localhost/?foo=bar")
.hasASpanWithTagEqualTo("http.host", "localhost")
.hasASpanWithTagEqualTo("http.path", "/")
.hasASpanWithTagEqualTo("http.method", HttpMethod.GET.toString())
.hasASpanWithTagEqualTo("http.status_code", HttpStatus.OK.toString())
.allSpansAreExportable();
}
@Test
public void startsNewTraceWithTraceHandlerInterceptor() throws Exception {
final BeanFactory beanFactory = beanFactory();
TraceFilter filter = new TraceFilter(beanFactory);
filter.doFilter(this.request, this.response, (req, resp) -> {
this.filterChain.doFilter(req, resp);
// Simulate execution of the TraceHandlerInterceptor
request.setAttribute(TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR, tracer.getCurrentSpan());
});
then(TestSpanContextHolder.getCurrentSpan()).isNull();
then(new ListOfSpans(this.spanReporter.getSpans()))
.hasSize(1)
.hasASpanWithTagEqualTo("http.url", "http://localhost/?foo=bar")
.hasASpanWithTagEqualTo("http.host", "localhost")
.hasASpanWithTagEqualTo("http.path", "/")
.hasASpanWithTagEqualTo("http.method", HttpMethod.GET.toString())
.hasASpanWithTagEqualTo("http.status_code", HttpStatus.OK.toString())
.allSpansAreExportable();
}
@Test
@@ -160,16 +190,14 @@ public class TraceFilterTests {
TraceFilter filter = new TraceFilter(beanFactory);
filter.doFilter(this.request, this.response, this.filterChain);
// this creates a child span which is why we'd expect the parents to include the parent id
// especially important if no handler interceptors have been used.
// We add a child span on the server side to show which controller serviced the request
assertThat(this.span.getParents()).containsOnly(PARENT_ID);
assertThat(parentSpan())
assertThat(this.span.getSpanId()).isEqualTo(PARENT_ID);
assertThat(this.span)
.hasATag("http.url", "http://localhost/?foo=bar")
.hasATag("http.host", "localhost")
.hasATag("http.path", "/")
.hasATag("http.method", "GET");
then(TestSpanContextHolder.getCurrentSpan()).isNull();
then(ExceptionUtils.getLastException()).isNull();
}
private Span parentSpan() {
@@ -252,8 +280,8 @@ public class TraceFilterTests {
this.request.addHeader("X-Foo", "bar");
filter.doFilter(this.request, this.response, this.filterChain);
assertThat(parentSpan().tags()).contains(entry("http.x-foo", "bar"));
assertThat(parentSpan().tags()).contains(entry("http.x-foo", "bar"));
assertThat(this.span.tags()).contains(entry("http.x-foo", "bar"));
assertThat(this.span.tags()).contains(entry("http.x-foo", "bar"));
then(TestSpanContextHolder.getCurrentSpan()).isNull();
}
@@ -283,7 +311,7 @@ public class TraceFilterTests {
this.request.addHeader("X-Foo", "spam");
filter.doFilter(this.request, this.response, this.filterChain);
assertThat(parentSpan().tags()).contains(entry("http.x-foo", "'bar','spam'"));
assertThat(this.span.tags()).contains(entry("http.x-foo", "'bar','spam'"));
then(TestSpanContextHolder.getCurrentSpan()).isNull();
}
@@ -426,7 +454,7 @@ public class TraceFilterTests {
filter.doFilter(this.request, this.response, this.filterChain);
then(new ListOfSpans(this.spanReporter.getSpans()))
.allSpansAreExportable().hasSize(2).hasASpanWithSpanId(Span.hexToId("10"));
.allSpansAreExportable().hasSize(1).hasASpanWithSpanId(Span.hexToId("10"));
then(TestSpanContextHolder.getCurrentSpan()).isNull();
then(ExceptionUtils.getLastException()).isNull();
}
@@ -479,11 +507,7 @@ public class TraceFilterTests {
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);
});
filter.doFilter(this.request, this.response, this.filterChain);
then(new ListOfSpans(this.spanReporter.getSpans()))
.doesNotHaveASpanWithName("http:/parent/")
@@ -503,7 +527,7 @@ public class TraceFilterTests {
* org.springframework.cloud.sleuth.instrument.TraceKeys}.
*/
public void verifyParentSpanHttpTags(HttpStatus status) {
assertThat(parentSpan().tags()).contains(entry("http.host", "localhost"),
assertThat(this.span.tags()).contains(entry("http.host", "localhost"),
entry("http.url", "http://localhost/?foo=bar"), entry("http.path", "/"),
entry("http.method", "GET"));
verifyCurrentSpanStatusCodeForAContinuedSpan(status);