From 4c53d65039c895cf98c3a52e32c4ece5430e7711 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 17 Aug 2017 17:51:50 +0200 Subject: [PATCH] Added http keys for a root span without this, the root span doesn't have any HTTP trace keys set with this, the root span has both the request and response trace keys set fixes #668 --- .../sleuth/instrument/web/TraceFilter.java | 18 +++++++++ .../instrument/web/TraceFilterTests.java | 39 +++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java index a8e1565f2..46b70fb50 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java @@ -90,6 +90,9 @@ public class TraceFilter extends GenericFilterBean { protected static final String TRACE_CLOSE_SPAN_REQUEST_ATTR = TraceFilter.class.getName() + ".CLOSE_SPAN"; + private static final String TRACE_SPAN_WITHOUT_PARENT = TraceFilter.class.getName() + + ".SPAN_WITH_NO_PARENT"; + private Tracer tracer; private TraceKeys traceKeys; private Pattern skipPattern; @@ -216,6 +219,7 @@ public class TraceFilter extends GenericFilterBean { Span span = spanFromRequest; if (span != null) { addResponseTags(response, exception); + addResponseTagsForSpanWithoutParent(request, response); if (span.hasSavedSpan() && requestHasAlreadyBeenHandled(request)) { recordParentSpan(span.getSavedSpan()); } else if (!requestHasAlreadyBeenHandled(request)) { @@ -252,6 +256,18 @@ public class TraceFilter extends GenericFilterBean { } } + private void addResponseTagsForSpanWithoutParent(HttpServletRequest request, + HttpServletResponse response) { + if (spanWithoutParent(request) && response.getStatus() >= 100) { + tracer().addTag(traceKeys().getHttp().getStatusCode(), + String.valueOf(response.getStatus())); + } + } + + private boolean spanWithoutParent(HttpServletRequest request) { + return request.getAttribute(TRACE_SPAN_WITHOUT_PARENT) != null; + } + private boolean stillTracingCurrentSapn(Span span) { return tracer().getCurrentSpan().equals(span); } @@ -351,6 +367,8 @@ public class TraceFilter extends GenericFilterBean { } else { spanFromRequest = tracer().createSpan(name); } + addRequestTags(spanFromRequest, request); + request.setAttribute(TRACE_SPAN_WITHOUT_PARENT, spanFromRequest); } spanFromRequest.logEvent(Span.SERVER_RECV); request.setAttribute(TRACE_REQUEST_ATTR, spanFromRequest); 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 59375449c..e7f903ecc 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 @@ -16,6 +16,11 @@ package org.springframework.cloud.sleuth.instrument.web; +import java.util.ArrayList; +import java.util.Optional; +import java.util.Random; +import java.util.regex.Pattern; + import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -48,11 +53,6 @@ import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockServletContext; import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; -import java.util.ArrayList; -import java.util.Optional; -import java.util.Random; -import java.util.regex.Pattern; - import static org.junit.Assert.assertEquals; import static org.mockito.MockitoAnnotations.initMocks; import static org.springframework.cloud.sleuth.assertions.SleuthAssertions.assertThat; @@ -132,7 +132,7 @@ public class TraceFilterTests { TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); - verifyCurrentSpanStatusCode(HttpStatus.OK); + assertThat(this.span.tags()).containsEntry("http.status_code", HttpStatus.OK.toString()); then(TestSpanContextHolder.getCurrentSpan()).isNull(); } @@ -448,6 +448,29 @@ public class TraceFilterTests { then(ExceptionUtils.getLastException()).isNull(); } + // #668 + @Test + public void shouldSetTraceKeysForAnUntracedRequest() throws Exception { + this.request = builder() + .param("foo", "bar") + .buildRequest(new MockServletContext()); + this.response.setStatus(295); + TraceFilter filter = new TraceFilter(beanFactory()); + + filter.doFilter(this.request, this.response, this.filterChain); + + then(new ListOfSpans(this.spanReporter.getSpans())) + .hasASpanWithName("http:/") + .hasASpanWithTagEqualTo("http.url", "http://localhost/?foo=bar") + .hasASpanWithTagEqualTo("http.host", "localhost") + .hasASpanWithTagEqualTo("http.path", "/") + .hasASpanWithTagEqualTo("http.method", "GET") + .hasASpanWithTagEqualTo("http.status_code", "295") + .allSpansAreExportable(); + then(TestSpanContextHolder.getCurrentSpan()).isNull(); + then(ExceptionUtils.getLastException()).isNull(); + } + public void verifyParentSpanHttpTags() { verifyParentSpanHttpTags(HttpStatus.OK); } @@ -460,11 +483,11 @@ public class TraceFilterTests { assertThat(parentSpan().tags()).contains(entry("http.host", "localhost"), entry("http.url", "http://localhost/?foo=bar"), entry("http.path", "/"), entry("http.method", "GET")); - verifyCurrentSpanStatusCode(status); + verifyCurrentSpanStatusCodeForAContinuedSpan(status); } - private void verifyCurrentSpanStatusCode(HttpStatus status) { + private void verifyCurrentSpanStatusCodeForAContinuedSpan(HttpStatus status) { // Status is only interesting in non-success case. Omitting it saves at least // 20bytes per span. if (status.is2xxSuccessful()) {