diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ExceptionLoggingFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ExceptionLoggingFilter.java new file mode 100644 index 000000000..f5617a6cb --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/ExceptionLoggingFilter.java @@ -0,0 +1,60 @@ +/* + * Copyright 2013-2018 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 java.io.IOException; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * Filter running after {@link brave.servlet.TracingFilter} + * that logs uncaught exceptions + * + * @author Marcin Grzejszczak + * @since 2.0.0 + */ +class ExceptionLoggingFilter implements Filter { + + private static final Log log = LogFactory.getLog(ExceptionLoggingFilter.class); + + @Override public void init(FilterConfig filterConfig) throws ServletException { + + } + + @Override public void doFilter(ServletRequest request, ServletResponse response, + FilterChain chain) throws IOException, ServletException { + try { + chain.doFilter(request, response); + } catch (Exception e) { + if (log.isErrorEnabled()) { + log.error("Uncaught exception thrown", e); + } + throw e; + } + } + + @Override public void destroy() { + + } +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthTraceHandlerInterceptor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthTraceHandlerInterceptor.java new file mode 100644 index 000000000..713f9d607 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthTraceHandlerInterceptor.java @@ -0,0 +1,127 @@ +/* + * Copyright 2013-2018 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 javax.servlet.http.HttpServletResponse; + +import brave.Span; +import brave.http.HttpTracing; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.springframework.beans.factory.BeanFactory; +import org.springframework.cloud.sleuth.ErrorParser; +import org.springframework.cloud.sleuth.TraceKeys; +import org.springframework.web.method.HandlerMethod; +import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; + +/** + * {@link org.springframework.web.servlet.HandlerInterceptor} that wraps handling of a + * adds tags related to the class and method name. + * + * @author Marcin Grzejszczak + * @since 1.0.3 + */ +class SleuthTraceHandlerInterceptor extends HandlerInterceptorAdapter { + + private static final Log log = LogFactory.getLog(SleuthTraceHandlerInterceptor.class); + + private final BeanFactory beanFactory; + private HttpTracing tracing; + private TraceKeys traceKeys; + private ErrorParser errorParser; + + public SleuthTraceHandlerInterceptor(BeanFactory beanFactory) { + this.beanFactory = beanFactory; + } + + @Override + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, + Object handler) { + Span span = httpTracing().tracing() + .tracer().currentSpan(); + if (span == null) { + return true; + } + if (log.isDebugEnabled()) { + log.debug("Adding tags to span " + span); + } + addClassMethodTag(handler, span); + addClassNameTag(handler, span); + return true; + } + + private void addClassMethodTag(Object handler, Span span) { + if (handler instanceof HandlerMethod) { + String methodName = ((HandlerMethod) handler).getMethod().getName(); + span.tag(traceKeys().getMvc().getControllerMethod(), methodName); + if (log.isDebugEnabled()) { + log.debug("Adding a method tag with value [" + methodName + "] to a span " + span); + } + } + } + + private void addClassNameTag(Object handler, Span span) { + String className; + if (handler instanceof HandlerMethod) { + className = ((HandlerMethod) handler).getBeanType().getSimpleName(); + } else { + className = handler.getClass().getSimpleName(); + } + if (log.isDebugEnabled()) { + log.debug("Adding a class tag with value [" + className + "] to a span " + span); + } + span.tag(traceKeys().getMvc().getControllerClass(), className); + } + + @Override + public void afterConcurrentHandlingStarted(HttpServletRequest request, + HttpServletResponse response, Object handler) { + + } + + @Override + public void afterCompletion(HttpServletRequest request, HttpServletResponse response, + Object handler, Exception ex) { + Span span = httpTracing().tracing().tracer().currentSpan(); + if (ex != null && span != null) { + errorParser().parseErrorTags(span, ex); + } + } + + private HttpTracing httpTracing() { + if (this.tracing == null) { + this.tracing = this.beanFactory.getBean(HttpTracing.class); + } + return this.tracing; + } + + private TraceKeys traceKeys() { + if (this.traceKeys == null) { + this.traceKeys = this.beanFactory.getBean(TraceKeys.class); + } + return this.traceKeys; + } + + private ErrorParser errorParser() { + if (this.errorParser == null) { + this.errorParser = this.beanFactory.getBean(ErrorParser.class); + } + return this.errorParser; + } + +} 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 deleted file mode 100644 index 202fa089e..000000000 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceFilter.java +++ /dev/null @@ -1,366 +0,0 @@ -/* - * Copyright 2013-2018 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 java.io.IOException; -import javax.servlet.FilterChain; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -import brave.Span; -import brave.Tracer; -import brave.http.HttpServerHandler; -import brave.http.HttpTracing; -import brave.servlet.HttpServletAdapter; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; -import org.springframework.beans.factory.BeanFactory; -import org.springframework.beans.factory.NoSuchBeanDefinitionException; -import org.springframework.boot.web.servlet.error.ErrorController; -import org.springframework.cloud.sleuth.TraceKeys; -import org.springframework.core.Ordered; -import org.springframework.core.annotation.Order; -import org.springframework.http.HttpStatus; -import org.springframework.web.context.request.async.WebAsyncUtils; -import org.springframework.web.filter.GenericFilterBean; -import org.springframework.web.util.UrlPathHelper; - -/** - * Filter that takes the value of the headers from either request and uses them to - * create a new span. - * - *

- * In order to keep the size of spans manageable, this only add tags defined in - * {@link TraceKeys}. - * - * @author Jakub Nabrdalik, 4financeIT - * @author Tomasz Nurkiewicz, 4financeIT - * @author Marcin Grzejszczak - * @author Spencer Gibb - * @author Dave Syer - * @since 1.0.0 - * - * @see Tracer - * @see TraceKeys - * @see TraceWebServletAutoConfiguration#traceFilter - */ -@Order(TraceFilter.ORDER) -public class TraceFilter extends GenericFilterBean { - - private static final Log log = LogFactory.getLog(TraceFilter.class); - - private static final String HTTP_COMPONENT = "http"; - - /** - * If you register your filter before the {@link TraceFilter} then you will not - * have the tracing context passed for you out of the box. That means that e.g. your - * logs will not get correlated. - */ - public static final int ORDER = Ordered.HIGHEST_PRECEDENCE + 5; - - protected static final String TRACE_REQUEST_ATTR = TraceFilter.class.getName() - + ".TRACE"; - - protected static final String TRACE_ERROR_HANDLED_REQUEST_ATTR = TraceFilter.class.getName() - + ".ERROR_HANDLED"; - - 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 static final String TRACE_EXCEPTION_REQUEST_ATTR = TraceFilter.class.getName() - + ".EXCEPTION"; - - private HttpTracing tracing; - private TraceKeys traceKeys; - private final BeanFactory beanFactory; - private HttpServerHandler handler; - private Boolean hasErrorController; - - private final UrlPathHelper urlPathHelper = new UrlPathHelper(); - - public TraceFilter(BeanFactory beanFactory) { - this.beanFactory = beanFactory; - } - @Override - public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, - FilterChain filterChain) throws IOException, ServletException { - if (!(servletRequest instanceof HttpServletRequest) || - !(servletResponse instanceof HttpServletResponse)) { - throw new ServletException("Filter just supports HTTP requests"); - } - HttpServletRequest request = (HttpServletRequest) servletRequest; - HttpServletResponse response = (HttpServletResponse) servletResponse; - String uri = this.urlPathHelper.getPathWithinApplication(request); - Span spanFromRequest = getSpanFromAttribute(request); - Tracer.SpanInScope ws = null; - if (spanFromRequest != null) { - ws = continueSpan(request, spanFromRequest); - } - if (log.isDebugEnabled()) { - log.debug("Received a request to uri [" + uri + "]"); - } - // in case of a response with exception status a exception controller will close the span - if (!httpStatusSuccessful(response) && isSpanContinued(request)) { - processErrorRequest(filterChain, request, response, spanFromRequest, ws); - return; - } - String name = HTTP_COMPONENT + ":" + uri; - SpanAndScope spanAndScope = new SpanAndScope(); - Throwable exception = null; - try { - spanAndScope = createSpan(request, spanFromRequest, name, ws); - filterChain.doFilter(request, response); - } catch (Throwable e) { - exception = e; - if (log.isErrorEnabled()) { - log.error("Uncaught exception thrown", e); - } - request.setAttribute(TRACE_EXCEPTION_REQUEST_ATTR, e); - throw e; - } finally { - if (isAsyncStarted(request) || request.isAsyncStarted()) { - if (log.isDebugEnabled()) { - log.debug("The span " + spanFromRequest + " was created for async"); - } - // TODO: how to deal with response annotations and async? - } else { - detachOrCloseSpans(request, response, spanAndScope, exception); - } - if (spanAndScope.scope != null) { - spanAndScope.scope.close(); - } - } - } - - private void processErrorRequest(FilterChain filterChain, HttpServletRequest request, - HttpServletResponse response, Span spanFromRequest, Tracer.SpanInScope ws) - throws IOException, ServletException { - if (log.isDebugEnabled()) { - log.debug("The span " + spanFromRequest + " was already detached once and we're processing an error"); - } - try { - filterChain.doFilter(request, response); - } finally { - request.setAttribute(TRACE_ERROR_HANDLED_REQUEST_ATTR, true); - if (request.getAttribute(TraceRequestAttributes.ERROR_HANDLED_SPAN_REQUEST_ATTR) == null) { - handler().handleSend(response, - (Throwable) request.getAttribute(TRACE_EXCEPTION_REQUEST_ATTR), spanFromRequest); - request.setAttribute(TRACE_EXCEPTION_REQUEST_ATTR, null); - } - if (ws != null) { - ws.close(); - } - } - } - - private Tracer.SpanInScope continueSpan(HttpServletRequest request, Span spanFromRequest) { - request.setAttribute(TraceRequestAttributes.SPAN_CONTINUED_REQUEST_ATTR, "true"); - if (log.isDebugEnabled()) { - log.debug("There has already been a span in the request " + spanFromRequest); - } - return httpTracing().tracing().tracer().withSpanInScope(spanFromRequest); - } - - private boolean requestHasAlreadyBeenHandled(HttpServletRequest request) { - return request.getAttribute(TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR) != null; - } - - private void detachOrCloseSpans(HttpServletRequest request, - HttpServletResponse response, SpanAndScope spanFromRequest, Throwable exception) { - Span span = spanFromRequest.span; - if (span != null) { - addResponseTagsForSpanWithoutParent(exception, request, response, span); - // in case of a response with exception status will close the span when exception dispatch is handled - // checking if tracing is in progress due to async / different order of view controller processing - if (httpStatusSuccessful(response) && !requestHasAlreadyBeenHandled(request)) { - if (log.isDebugEnabled()) { - log.debug("Closing the span " + span + " since the response was successful"); - } - if (exception == null || !hasErrorController()) { - clearTraceAttribute(request); - handler().handleSend(response, exception, span); - } - } else if (errorAlreadyHandled(request) && !shouldCloseSpan(request)) { - if (log.isDebugEnabled()) { - log.debug( - "Won't detach the span " + span + " since error has already been handled"); - } - } else if ((shouldCloseSpan(request) || isRootSpan(span)) && stillTracingCurrentSpan(span)) { - if (log.isDebugEnabled()) { - log.debug("Will handle sent for span " + span); - } - handler().handleSend(response, exception, span); - if (shouldCloseSpan(request)) { - clearTraceAttribute(request); - } - } else if (span != null || requestHasAlreadyBeenHandled(request)) { - if (log.isDebugEnabled()) { - log.debug("Detaching the span " + span + " since the response was unsuccessful"); - } - if (!hasErrorController()) { - clearTraceAttribute(request); - } - if (exception == null || !hasErrorController()) { - handler().handleSend(response, exception, span); - } else { - abandonSpan(span); - } - } - } - } - - // visible for tests - void abandonSpan(Span span) { - span.abandon(); - } - - private void addResponseTagsForSpanWithoutParent(Throwable exception, - HttpServletRequest request, HttpServletResponse response, Span span) { - if (exception == null && spanWithoutParent(request) && response.getStatus() >= 100) { - span.tag(traceKeys().getHttp().getStatusCode(), - String.valueOf(response.getStatus())); - } - } - - private boolean spanWithoutParent(HttpServletRequest request) { - return request.getAttribute(TRACE_SPAN_WITHOUT_PARENT) != null; - } - - private boolean isRootSpan(Span span) { - return span.context().traceId() == span.context().spanId(); - } - - private boolean stillTracingCurrentSpan(Span span) { - Span currentSpan = httpTracing().tracing().tracer().currentSpan(); - return currentSpan != null && currentSpan.equals(span); - } - - private boolean httpStatusSuccessful(HttpServletResponse response) { - if (response.getStatus() == 0) { - return false; - } - HttpStatus.Series httpStatusSeries = HttpStatus.Series.valueOf(response.getStatus()); - return httpStatusSeries == HttpStatus.Series.SUCCESSFUL || httpStatusSeries == HttpStatus.Series.REDIRECTION; - } - - private Span getSpanFromAttribute(HttpServletRequest request) { - return (Span) request.getAttribute(TRACE_REQUEST_ATTR); - } - - private void clearTraceAttribute(HttpServletRequest request) { - request.setAttribute(TRACE_REQUEST_ATTR, null); - } - - private boolean errorAlreadyHandled(HttpServletRequest request) { - return Boolean.valueOf( - String.valueOf(request.getAttribute(TRACE_ERROR_HANDLED_REQUEST_ATTR))); - } - - private boolean shouldCloseSpan(HttpServletRequest request) { - return Boolean.valueOf( - String.valueOf(request.getAttribute(TRACE_CLOSE_SPAN_REQUEST_ATTR))); - } - - private boolean isSpanContinued(HttpServletRequest request) { - return getSpanFromAttribute(request) != null; - } - - /** - * Creates a span and appends it as the current request's attribute - */ - private SpanAndScope createSpan(HttpServletRequest request, - Span spanFromRequest, String name, Tracer.SpanInScope ws) { - if (spanFromRequest != null) { - if (log.isDebugEnabled()) { - log.debug("Span has already been created - continuing with the previous one"); - } - return new SpanAndScope(spanFromRequest, ws); - } - spanFromRequest = handler().handleReceive(httpTracing().tracing() - .propagation().extractor(HttpServletRequest::getHeader), request); - if (log.isDebugEnabled()) { - log.debug("Found a parent span " + spanFromRequest.context() + " in the request"); - } - request.setAttribute(TRACE_REQUEST_ATTR, spanFromRequest); - if (log.isDebugEnabled()) { - log.debug("Parent span is " + spanFromRequest + ""); - } - return new SpanAndScope(spanFromRequest, httpTracing().tracing() - .tracer().withSpanInScope(spanFromRequest)); - } - - class SpanAndScope { - - final Span span; - final Tracer.SpanInScope scope; - - SpanAndScope(Span span, Tracer.SpanInScope scope) { - this.span = span; - this.scope = scope; - } - - SpanAndScope() { - this.span = null; - this.scope = null; - } - } - - protected boolean isAsyncStarted(HttpServletRequest request) { - return WebAsyncUtils.getAsyncManager(request).isConcurrentHandlingStarted(); - } - - @SuppressWarnings("unchecked") - HttpServerHandler handler() { - if (this.handler == null) { - this.handler = HttpServerHandler.create(this.beanFactory.getBean(HttpTracing.class), - new HttpServletAdapter()); - } - return this.handler; - } - - TraceKeys traceKeys() { - if (this.traceKeys == null) { - this.traceKeys = this.beanFactory.getBean(TraceKeys.class); - } - return this.traceKeys; - } - - HttpTracing httpTracing() { - if (this.tracing == null) { - this.tracing = this.beanFactory.getBean(HttpTracing.class); - } - return this.tracing; - } - - // null check is only for tests - private boolean hasErrorController() { - if (this.hasErrorController == null) { - try { - this.hasErrorController = this.beanFactory.getBean(ErrorController.class) != null; - } catch (NoSuchBeanDefinitionException e) { - this.hasErrorController = false; - } - } - return this.hasErrorController; - } -} - diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptor.java deleted file mode 100644 index 09ea317ad..000000000 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptor.java +++ /dev/null @@ -1,227 +0,0 @@ -/* - * Copyright 2013-2018 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 javax.servlet.http.HttpServletResponse; -import java.util.concurrent.atomic.AtomicReference; - -import brave.Span; -import brave.Tracer; -import brave.http.HttpServerHandler; -import brave.http.HttpTracing; -import brave.servlet.HttpServletAdapter; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; -import org.springframework.beans.factory.BeanFactory; -import org.springframework.beans.factory.NoSuchBeanDefinitionException; -import org.springframework.boot.web.servlet.error.ErrorController; -import org.springframework.cloud.sleuth.ErrorParser; -import org.springframework.cloud.sleuth.TraceKeys; -import org.springframework.cloud.sleuth.util.SpanNameUtil; -import org.springframework.web.method.HandlerMethod; -import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; - -/** - * {@link org.springframework.web.servlet.HandlerInterceptor} that wraps handling of a - * request in a Span. Adds tags related to the class and method name. - * - * The interceptor will not create spans for error controller related paths. - * - * It's important to note that this implementation will set the request attribute - * {@link TraceRequestAttributes#HANDLED_SPAN_REQUEST_ATTR} when the request is processed. - * That way the {@link TraceFilter} will not create the "fallback" span. - * - * @author Marcin Grzejszczak - * @since 1.0.3 - */ -public class TraceHandlerInterceptor extends HandlerInterceptorAdapter { - - private static final Log log = LogFactory.getLog(TraceHandlerInterceptor.class); - - private final BeanFactory beanFactory; - - private HttpTracing tracing; - private TraceKeys traceKeys; - private ErrorParser errorParser; - private AtomicReference errorController; - private HttpServerHandler handler; - - public TraceHandlerInterceptor(BeanFactory beanFactory) { - this.beanFactory = beanFactory; - } - - @Override - public boolean preHandle(HttpServletRequest request, HttpServletResponse response, - Object handler) throws Exception { - String spanName = spanName(handler); - boolean continueSpan = getRootSpanFromAttribute(request) != null; - Span span = continueSpan ? getRootSpanFromAttribute(request) : - httpTracing().tracing().tracer().nextSpan().name(spanName).start(); - try (Tracer.SpanInScope ws = httpTracing().tracing().tracer().withSpanInScope(span)) { - if (log.isDebugEnabled()) { - log.debug("Handling span " + span); - } - addClassMethodTag(handler, span); - addClassNameTag(handler, span); - setSpanInAttribute(request, span); - if (!continueSpan) { - setNewSpanCreatedAttribute(request, span); - } - } - return true; - } - - private boolean isErrorControllerRelated(HttpServletRequest request) { - return errorController() != null && errorController().getErrorPath() - .equals(request.getRequestURI()); - } - - private void addClassMethodTag(Object handler, Span span) { - if (handler instanceof HandlerMethod) { - String methodName = ((HandlerMethod) handler).getMethod().getName(); - span.tag(traceKeys().getMvc().getControllerMethod(), methodName); - if (log.isDebugEnabled()) { - log.debug("Adding a method tag with value [" + methodName + "] to a span " + span); - } - } - } - - private void addClassNameTag(Object handler, Span span) { - String className; - if (handler instanceof HandlerMethod) { - className = ((HandlerMethod) handler).getBeanType().getSimpleName(); - } else { - className = handler.getClass().getSimpleName(); - } - if (log.isDebugEnabled()) { - log.debug("Adding a class tag with value [" + className + "] to a span " + span); - } - span.tag(traceKeys().getMvc().getControllerClass(), className); - } - - private String spanName(Object handler) { - if (handler instanceof HandlerMethod) { - return SpanNameUtil.toLowerHyphen(((HandlerMethod) handler).getMethod().getName()); - } - return SpanNameUtil.toLowerHyphen(handler.getClass().getSimpleName()); - } - - @Override - public void afterConcurrentHandlingStarted(HttpServletRequest request, - HttpServletResponse response, Object handler) throws Exception { - Span spanFromRequest = getNewSpanFromAttribute(request); - if (spanFromRequest != null) { - try (Tracer.SpanInScope ws = httpTracing().tracing().tracer().withSpanInScope(spanFromRequest)) { - if (log.isDebugEnabled()) { - log.debug("Closing the span " + spanFromRequest); - } - } finally { - spanFromRequest.finish(); - } - } - } - - @Override - public void afterCompletion(HttpServletRequest request, HttpServletResponse response, - Object handler, Exception ex) throws Exception { - if (isErrorControllerRelated(request)) { - if (log.isDebugEnabled()) { - log.debug("Skipping closing of a span for error controller processing"); - } - return; - } - Span span = getRootSpanFromAttribute(request); - if (ex != null) { - errorParser().parseErrorTags(span, ex); - } - if (getNewSpanFromAttribute(request) != null) { - if (log.isDebugEnabled()) { - log.debug("Closing span " + span); - } - Span newSpan = getNewSpanFromAttribute(request); - handler().handleSend(response, ex, newSpan); - clearNewSpanCreatedAttribute(request); - } - } - - private Span getNewSpanFromAttribute(HttpServletRequest request) { - return (Span) request.getAttribute(TraceRequestAttributes.NEW_SPAN_REQUEST_ATTR); - } - - private Span getRootSpanFromAttribute(HttpServletRequest request) { - return (Span) request.getAttribute(TraceFilter.TRACE_REQUEST_ATTR); - } - - private void setSpanInAttribute(HttpServletRequest request, Span span) { - request.setAttribute(TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR, span); - } - - private void setNewSpanCreatedAttribute(HttpServletRequest request, Span span) { - request.setAttribute(TraceRequestAttributes.NEW_SPAN_REQUEST_ATTR, span); - } - - private void clearNewSpanCreatedAttribute(HttpServletRequest request) { - request.removeAttribute(TraceRequestAttributes.NEW_SPAN_REQUEST_ATTR); - } - - private HttpTracing httpTracing() { - if (this.tracing == null) { - this.tracing = this.beanFactory.getBean(HttpTracing.class); - } - return this.tracing; - } - - private TraceKeys traceKeys() { - if (this.traceKeys == null) { - this.traceKeys = this.beanFactory.getBean(TraceKeys.class); - } - return this.traceKeys; - } - - @SuppressWarnings("unchecked") - HttpServerHandler handler() { - if (this.handler == null) { - this.handler = HttpServerHandler.create(this.beanFactory.getBean(HttpTracing.class), - new HttpServletAdapter()); - } - return this.handler; - } - - private ErrorParser errorParser() { - if (this.errorParser == null) { - this.errorParser = this.beanFactory.getBean(ErrorParser.class); - } - return this.errorParser; - } - - ErrorController errorController() { - if (this.errorController == null) { - try { - ErrorController errorController = this.beanFactory.getBean(ErrorController.class); - this.errorController = new AtomicReference<>(errorController); - } catch (NoSuchBeanDefinitionException e) { - if (log.isTraceEnabled()) { - log.trace("ErrorController bean not found"); - } - this.errorController = new AtomicReference<>(); - } - } - return this.errorController.get(); - } - -} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceSpringDataBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceSpringDataBeanPostProcessor.java index 804eb20f1..0d364aa30 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceSpringDataBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceSpringDataBeanPostProcessor.java @@ -16,14 +16,15 @@ package org.springframework.cloud.sleuth.instrument.web; -import javax.servlet.http.HttpServletRequest; import java.util.Collections; +import javax.servlet.http.HttpServletRequest; +import brave.spring.webmvc.TracingHandlerInterceptor; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.BeansException; -import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.config.BeanPostProcessor; +import org.springframework.context.ApplicationContext; import org.springframework.data.rest.webmvc.support.DelegatingHandlerMapping; import org.springframework.web.servlet.HandlerExecutionChain; import org.springframework.web.servlet.HandlerMapping; @@ -38,10 +39,10 @@ class TraceSpringDataBeanPostProcessor implements BeanPostProcessor { private static final Log log = LogFactory.getLog(TraceSpringDataBeanPostProcessor.class); - private final BeanFactory beanFactory; + private final ApplicationContext applicationContext; - public TraceSpringDataBeanPostProcessor(BeanFactory beanFactory) { - this.beanFactory = beanFactory; + public TraceSpringDataBeanPostProcessor(ApplicationContext applicationContext) { + this.applicationContext = applicationContext; } @Override @@ -53,7 +54,7 @@ class TraceSpringDataBeanPostProcessor implements BeanPostProcessor { "] in its trace representation"); } return new TraceDelegatingHandlerMapping((DelegatingHandlerMapping) bean, - this.beanFactory); + this.applicationContext); } return bean; } @@ -67,13 +68,13 @@ class TraceSpringDataBeanPostProcessor implements BeanPostProcessor { private static class TraceDelegatingHandlerMapping extends DelegatingHandlerMapping { private final DelegatingHandlerMapping delegate; - private final BeanFactory beanFactory; + private final ApplicationContext applicationContext; public TraceDelegatingHandlerMapping(DelegatingHandlerMapping delegate, - BeanFactory beanFactory) { + ApplicationContext beanFactory) { super(Collections.emptyList()); this.delegate = delegate; - this.beanFactory = beanFactory; + this.applicationContext = beanFactory; } @Override @@ -88,7 +89,12 @@ class TraceSpringDataBeanPostProcessor implements BeanPostProcessor { if (handlerExecutionChain == null) { return null; } - handlerExecutionChain.addInterceptor(new TraceHandlerInterceptor(this.beanFactory)); + handlerExecutionChain.addInterceptor(this.applicationContext.getBean(TracingHandlerInterceptor.class)); + String legacyEnabled = this.applicationContext.getEnvironment() + .getProperty("spring.sleuth.http.legacy.enabled", "false"); + if (Boolean.parseBoolean(legacyEnabled)) { + handlerExecutionChain.addInterceptor(this.applicationContext.getBean(SleuthTraceHandlerInterceptor.class)); + } return handlerExecutionChain; } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebMvcConfigurer.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebMvcConfigurer.java index b0b96dcb6..682589104 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebMvcConfigurer.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebMvcConfigurer.java @@ -16,15 +16,19 @@ package org.springframework.cloud.sleuth.instrument.web; +import brave.http.HttpTracing; +import brave.spring.webmvc.TracingHandlerInterceptor; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.web.servlet.config.annotation.InterceptorRegistry; import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; /** - * MVC Adapter that adds the {@link TraceHandlerInterceptor} + * MVC Adapter that adds the {@link TracingHandlerInterceptor} * * @author Marcin Grzejszczak * @@ -32,15 +36,26 @@ import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; */ @Configuration class TraceWebMvcConfigurer implements WebMvcConfigurer { - @Autowired BeanFactory beanFactory; + @Autowired ApplicationContext applicationContext; @Bean - public TraceHandlerInterceptor traceHandlerInterceptor(BeanFactory beanFactory) { - return new TraceHandlerInterceptor(beanFactory); + public TracingHandlerInterceptor tracingHandlerInterceptor(HttpTracing tracing) { + return (TracingHandlerInterceptor) TracingHandlerInterceptor.create(tracing); + } + + @Bean + @ConditionalOnProperty("spring.sleuth.http.legacy.enabled") + public SleuthTraceHandlerInterceptor legacySleuthTraceHandlerInterceptor(BeanFactory beanFactory) { + return new SleuthTraceHandlerInterceptor(beanFactory); } @Override public void addInterceptors(InterceptorRegistry registry) { - registry.addInterceptor(this.beanFactory.getBean(TraceHandlerInterceptor.class)); + registry.addInterceptor(this.applicationContext.getBean(TracingHandlerInterceptor.class)); + String legacyEnabled = this.applicationContext.getEnvironment() + .getProperty("spring.sleuth.http.legacy.enabled", "false"); + if (Boolean.parseBoolean(legacyEnabled)) { + registry.addInterceptor(this.applicationContext.getBean(SleuthTraceHandlerInterceptor.class)); + } } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebServletAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebServletAutoConfiguration.java index 5528f80ab..29e50be80 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebServletAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebServletAutoConfiguration.java @@ -18,7 +18,7 @@ package org.springframework.cloud.sleuth.instrument.web; import brave.Tracer; import brave.http.HttpTracing; -import org.springframework.beans.factory.BeanFactory; +import brave.servlet.TracingFilter; import org.springframework.boot.autoconfigure.AutoConfigureAfter; import org.springframework.boot.autoconfigure.condition.ConditionalOnBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnClass; @@ -28,9 +28,11 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnWebApplicat import org.springframework.boot.web.servlet.FilterRegistrationBean; import org.springframework.cloud.sleuth.ErrorParser; import org.springframework.cloud.sleuth.SpanNamer; +import org.springframework.context.ApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; +import org.springframework.core.Ordered; import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; import static javax.servlet.DispatcherType.ASYNC; @@ -54,6 +56,8 @@ import static javax.servlet.DispatcherType.REQUEST; @AutoConfigureAfter(TraceHttpAutoConfiguration.class) public class TraceWebServletAutoConfiguration { + public static final int TRACING_FILTER_ORDER = Ordered.HIGHEST_PRECEDENCE + 5; + /** * Nested config that configures Web MVC if it's present (without adding a runtime * dependency to it) @@ -72,22 +76,30 @@ public class TraceWebServletAutoConfiguration { @Bean @ConditionalOnClass(name = "org.springframework.data.rest.webmvc.support.DelegatingHandlerMapping") public static TraceSpringDataBeanPostProcessor traceSpringDataBeanPostProcessor( - BeanFactory beanFactory) { - return new TraceSpringDataBeanPostProcessor(beanFactory); + ApplicationContext applicationContext) { + return new TraceSpringDataBeanPostProcessor(applicationContext); } @Bean public FilterRegistrationBean traceWebFilter( - TraceFilter traceFilter) { - FilterRegistrationBean filterRegistrationBean = new FilterRegistrationBean(traceFilter); + TracingFilter tracingFilter) { + FilterRegistrationBean filterRegistrationBean = new FilterRegistrationBean(tracingFilter); filterRegistrationBean.setDispatcherTypes(ASYNC, ERROR, FORWARD, INCLUDE, REQUEST); - filterRegistrationBean.setOrder(TraceFilter.ORDER); + filterRegistrationBean.setOrder(TraceWebServletAutoConfiguration.TRACING_FILTER_ORDER); + return filterRegistrationBean; + } + + @Bean + public FilterRegistrationBean exceptionThrowingFilter() { + FilterRegistrationBean filterRegistrationBean = new FilterRegistrationBean(new ExceptionLoggingFilter()); + filterRegistrationBean.setDispatcherTypes(ASYNC, ERROR, FORWARD, INCLUDE, REQUEST); + filterRegistrationBean.setOrder(TraceWebServletAutoConfiguration.TRACING_FILTER_ORDER + 1); return filterRegistrationBean; } @Bean @ConditionalOnMissingBean - public TraceFilter traceFilter(BeanFactory beanFactory) { - return new TraceFilter(beanFactory); + public TracingFilter tracingFilter(HttpTracing tracing) { + return (TracingFilter) TracingFilter.create(tracing); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java index 0897e8d91..adb5f6d57 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilter.java @@ -26,12 +26,10 @@ import com.netflix.zuul.ZuulFilter; import com.netflix.zuul.context.RequestContext; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.cloud.sleuth.instrument.web.TraceFilter; -import org.springframework.cloud.sleuth.instrument.web.TraceRequestAttributes; import org.springframework.http.HttpStatus; /** - * A post request {@link ZuulFilter} that marks a span for closing in {@link TraceFilter} + * A post request {@link ZuulFilter} * * @author Dave Syer * @since 1.0.0 @@ -67,9 +65,6 @@ class TracePostZuulFilter extends ZuulFilter { if (log.isDebugEnabled()) { log.debug("Marking current span as handled"); } - RequestContext.getCurrentContext() - .getRequest().setAttribute( - TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR, "true"); HttpServletResponse response = RequestContext.getCurrentContext().getResponse(); Throwable exception = RequestContext.getCurrentContext().getThrowable(); this.handler.handleSend(response, exception, this.tracer.currentSpan()); diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulHandlerMappingBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulHandlerMappingBeanPostProcessor.java index baf328ae0..735f52720 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulHandlerMappingBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/zuul/TraceZuulHandlerMappingBeanPostProcessor.java @@ -18,13 +18,13 @@ package org.springframework.cloud.sleuth.instrument.zuul; import java.lang.invoke.MethodHandles; +import brave.spring.webmvc.TracingHandlerInterceptor; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.cloud.netflix.zuul.web.ZuulHandlerMapping; -import org.springframework.cloud.sleuth.instrument.web.TraceHandlerInterceptor; /** * Bean post processor that wraps {@link ZuulHandlerMapping} in its @@ -58,7 +58,7 @@ class TraceZuulHandlerMappingBeanPostProcessor implements BeanPostProcessor { } ZuulHandlerMapping zuulHandlerMapping = (ZuulHandlerMapping) bean; zuulHandlerMapping.setInterceptors( - new TraceHandlerInterceptor(this.beanFactory)); + this.beanFactory.getBean(TracingHandlerInterceptor.class)); } return bean; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java index bd13bb07f..561a34eba 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterIntegrationTests.java @@ -30,6 +30,7 @@ import java.util.concurrent.CompletableFuture; import brave.Span; import brave.Tracer; import brave.sampler.Sampler; +import brave.servlet.TracingFilter; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.junit.After; @@ -40,7 +41,6 @@ import org.slf4j.MDC; import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.actuate.autoconfigure.web.server.ManagementServerProperties; -import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.cloud.sleuth.TraceKeys; import org.springframework.cloud.sleuth.instrument.DefaultTestAutoConfiguration; @@ -69,7 +69,8 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilder import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @RunWith(SpringRunner.class) -@SpringBootTest(classes = TraceFilterIntegrationTests.Config.class) +@SpringBootTest(classes = TraceFilterIntegrationTests.Config.class, +properties = "spring.sleuth.http.legacy.enabled=true") public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { static final String TRACE_ID_NAME = "X-B3-TraceId"; static final String SPAN_ID_NAME = "X-B3-SpanId"; @@ -78,7 +79,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { private static Log logger = LogFactory.getLog( TraceFilterIntegrationTests.class); - @Autowired TraceFilter traceFilter; + @Autowired TracingFilter traceFilter; @Autowired MyFilter myFilter; @Autowired ArrayListSpanReporter reporter; @Autowired Tracer tracer; @@ -95,7 +96,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { public void should_create_a_trace() throws Exception { whenSentPingWithoutTracingData(); - then(this.reporter.getSpans()).hasSize(1); + then(this.reporter.getSpans()).hasSize(2); zipkin2.Span span = this.reporter.getSpans().get(0); then(span.tags()) .containsKey(new TraceKeys().getMvc().getControllerClass()) @@ -173,7 +174,15 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { whenSentToNonExistentEndpointWithTraceId(expectedTraceId); - then(this.reporter.getSpans()).hasSize(1); + // it's a span with the same ids + then(this.reporter.getSpans()).hasSize(2); + zipkin2.Span serverSpan = this.reporter.getSpans().get(0); + then(serverSpan.tags()) + .containsEntry("custom", "tag") + .containsEntry("http.status_code", "404"); + zipkin2.Span handlerSpan = this.reporter.getSpans().get(0); + then(handlerSpan.tags()) + .containsEntry("http.status_code", "404"); then(this.tracer.currentSpan()).isNull(); } @@ -190,8 +199,13 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { // we need to dump the span cause it's not in TraceFilter since TF // has also error dispatch and the ErrorController would report the span - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()).containsKey("error"); + then(this.reporter.getSpans()).hasSize(2); + // server + then(this.reporter.getSpans().get(0).tags()) + .containsEntry("error", "java.lang.RuntimeException"); + // handler + then(this.reporter.getSpans().get(1).tags()) + .containsEntry("error", "Request processing failed; nested exception is java.lang.RuntimeException"); } @Test @@ -357,17 +371,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { } @Bean - TraceFilter myTraceFilter(BeanFactory beanFactory) { - return new TraceFilter(beanFactory) { - @Override void abandonSpan(Span span) { - log.info("Simulating Error Controller"); - span.finish(); - } - }; - } - - @Bean - @Order(TraceFilter.ORDER + 1) + @Order(TraceWebServletAutoConfiguration.TRACING_FILTER_ORDER + 1) Filter myFilter(Tracer tracer) { return new MyFilter(tracer); } @@ -376,7 +380,7 @@ public class TraceFilterIntegrationTests extends AbstractMvcIntegrationTest { //tag::response_headers[] @Component -@Order(TraceFilter.ORDER + 1) +@Order(TraceWebServletAutoConfiguration.TRACING_FILTER_ORDER + 1) class MyFilter extends GenericFilterBean { private final Tracer tracer; @@ -388,7 +392,9 @@ class MyFilter extends GenericFilterBean { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { Span currentSpan = this.tracer.currentSpan(); - then(currentSpan).isNotNull(); + if (currentSpan == null) { + return; + } // for readability we're returning trace id in a hex form ((HttpServletResponse) response) .addHeader("ZIPKIN-TRACE-ID", 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 afffead58..bbe3de3f4 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 @@ -18,6 +18,7 @@ package org.springframework.cloud.sleuth.instrument.web; import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Pattern; +import javax.servlet.Filter; import brave.Span; import brave.Tracer; @@ -25,17 +26,12 @@ import brave.Tracing; import brave.http.HttpTracing; import brave.propagation.CurrentTraceContext; import brave.sampler.Sampler; +import brave.servlet.TracingFilter; import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.mockito.BDDMockito; -import org.mockito.Mockito; -import org.springframework.beans.factory.BeanFactory; -import org.springframework.beans.factory.NoSuchBeanDefinitionException; -import org.springframework.cloud.sleuth.ErrorParser; import org.springframework.cloud.sleuth.ExceptionMessageErrorParser; import org.springframework.cloud.sleuth.TraceKeys; -import org.springframework.cloud.sleuth.autoconfig.SleuthProperties; import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; import org.springframework.cloud.sleuth.util.SpanUtil; import org.springframework.http.HttpMethod; @@ -76,12 +72,11 @@ public class TraceFilterTests { new ExceptionMessageErrorParser())) .serverSampler(new SleuthHttpSampler(() -> Pattern.compile(""))) .build(); - SleuthProperties properties = new SleuthProperties(); + Filter filter = TracingFilter.create(this.httpTracing); MockHttpServletRequest request; MockHttpServletResponse response; MockFilterChain filterChain; - BeanFactory beanFactory = Mockito.mock(BeanFactory.class); @Before public void init() { @@ -103,19 +98,16 @@ public class TraceFilterTests { @Test public void notTraced() throws Exception { - BeanFactory beanFactory = neverSampleTracing(); - TraceFilter filter = new TraceFilter(beanFactory); - this.request = get("/favicon.ico").accept(MediaType.ALL) .buildRequest(new MockServletContext()); - filter.doFilter(this.request, this.response, this.filterChain); + neverSampleFilter().doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); then(this.reporter.getSpans()).isEmpty(); } - private BeanFactory neverSampleTracing() { + private Filter neverSampleFilter() { Tracing tracing = Tracing.newBuilder() .currentTraceContext(CurrentTraceContext.Default.create()) .spanReporter(this.reporter) @@ -128,14 +120,11 @@ public class TraceFilterTests { new ExceptionMessageErrorParser())) .serverSampler(new SleuthHttpSampler(() -> Pattern.compile(""))) .build(); - BeanFactory beanFactory = beanFactory(); - BDDMockito.given(beanFactory.getBean(HttpTracing.class)).willReturn(httpTracing); - return beanFactory; + return TracingFilter.create(httpTracing); } @Test public void startsNewTrace() throws Exception { - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); then(this.reporter.getSpans()) @@ -149,32 +138,8 @@ public class TraceFilterTests { //.containsEntry("http.status_code", "200") } - @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, - tracing.tracer().currentSpan()); - }); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()) - .hasSize(1); - then(this.reporter.getSpans().get(0).tags()) - .containsEntry("http.url", "http://localhost/?foo=bar") - .containsEntry("http.host", "localhost") - .containsEntry("http.path", "/") - .containsEntry("http.method", HttpMethod.GET.toString()); - // we don't check for status_code anymore cause Brave doesn't support it oob - //.containsEntry("http.status_code", "200") - } - @Test public void shouldNotStoreHttpStatusCodeWhenResponseCodeHasNotYetBeenSet() throws Exception { - TraceFilter filter = new TraceFilter(beanFactory()); this.response.setStatus(0); filter.doFilter(this.request, this.response, this.filterChain); @@ -192,9 +157,7 @@ public class TraceFilterTests { .header(TRACE_ID_NAME, SpanUtil.idToHex(2L)) .header(PARENT_SPAN_ID_NAME, SpanUtil.idToHex(3L)) .buildRequest(new MockServletContext()); - BeanFactory beanFactory = beanFactory(); - - TraceFilter filter = new TraceFilter(beanFactory); + filter.doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); @@ -217,9 +180,7 @@ public class TraceFilterTests { .header(PARENT_SPAN_ID_NAME, SpanUtil.idToHex(3L)) .header(SAMPLED_ID_NAME, 0) .buildRequest(new MockServletContext()); - BeanFactory beanFactory = beanFactory(); - - TraceFilter filter = new TraceFilter(beanFactory); + filter.doFilter(this.request, this.response, (req, resp) -> { this.filterChain.doFilter(req, resp); span.set(this.tracing.tracer().currentSpan()); @@ -233,26 +194,20 @@ public class TraceFilterTests { @Test public void continuesSpanInRequestAttr() throws Exception { Span span = this.tracer.nextSpan().name("http:foo"); - this.request.setAttribute(TraceFilter.TRACE_REQUEST_ATTR, span); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.request.getAttribute(TraceFilter.TRACE_ERROR_HANDLED_REQUEST_ATTR)).isNull(); } @Test public void closesSpanInRequestAttrIfStatusCodeNotSuccessful() throws Exception { Span span = this.tracer.nextSpan().name("http:foo"); - this.request.setAttribute(TraceFilter.TRACE_REQUEST_ATTR, span); this.response.setStatus(404); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.request.getAttribute(TraceFilter.TRACE_ERROR_HANDLED_REQUEST_ATTR)).isNotNull(); then(this.reporter.getSpans()) .hasSize(1); } @@ -260,12 +215,8 @@ public class TraceFilterTests { @Test public void doesntDetachASpanIfStatusCodeNotSuccessfulAndRequestWasProcessed() throws Exception { Span span = this.tracer.nextSpan().name("http:foo"); - this.request.setAttribute(TraceFilter.TRACE_REQUEST_ATTR, span); - this.request.setAttribute(TraceFilter.TRACE_ERROR_HANDLED_REQUEST_ATTR, true); this.response.setStatus(404); - TraceFilter filter = new TraceFilter(beanFactory()); - then(Tracing.current().tracer().currentSpan()).isNull(); filter.doFilter(this.request, this.response, this.filterChain); } @@ -275,8 +226,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - BeanFactory beanFactory = beanFactory(); - TraceFilter filter = new TraceFilter(beanFactory); filter.doFilter(this.request, this.response, this.filterChain); @@ -295,11 +244,8 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - BeanFactory beanFactory = beanFactory(); - BDDMockito.given(beanFactory.getBean(HttpTracing.class)).willReturn(httpTracing); - TraceFilter filter = new TraceFilter(beanFactory); - filter.doFilter(this.request, this.response, this.filterChain); + TracingFilter.create(httpTracing).doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); then(this.reporter.getSpans()) @@ -314,9 +260,7 @@ public class TraceFilterTests { .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); this.traceKeys.getHttp().getHeaders().add("x-foo"); - BeanFactory beanFactory = beanFactory(); - TraceFilter filter = new TraceFilter(beanFactory); - this.request.addHeader("X-Foo", "bar"); + this.request.addHeader("X-Foo", "bar"); filter.doFilter(this.request, this.response, this.filterChain); @@ -332,8 +276,7 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - this.traceKeys.getHttp().getHeaders().add("x-foo");BeanFactory beanFactory = beanFactory(); - TraceFilter filter = new TraceFilter(beanFactory); + this.traceKeys.getHttp().getHeaders().add("x-foo"); this.request.addHeader("X-Foo", "bar"); this.request.addHeader("X-Foo", "spam"); filter.doFilter(this.request, this.response, this.filterChain); @@ -351,8 +294,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - BeanFactory beanFactory = beanFactory(); - TraceFilter filter = new TraceFilter(beanFactory); this.filterChain = new MockFilterChain() { @Override @@ -382,7 +323,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); this.response.setStatus(404); @@ -395,7 +335,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); this.response.setStatus(200); filter.doFilter(this.request, this.response, this.filterChain); @@ -410,7 +349,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, PARENT_ID) .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); this.response.setStatus(302); filter.doFilter(this.request, this.response, this.filterChain); @@ -425,7 +363,6 @@ public class TraceFilterTests { this.request = builder().header(SPAN_ID_NAME, "asd") .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); @@ -440,7 +377,6 @@ public class TraceFilterTests { .header(PARENT_SPAN_ID_NAME, "-") .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); @@ -454,9 +390,8 @@ public class TraceFilterTests { this.request = builder() .header(SPAN_FLAGS, 1) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(neverSampleTracing()); - filter.doFilter(this.request, this.response, this.filterChain); + neverSampleFilter().doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); then(this.reporter.getSpans()).isNotEmpty(); @@ -467,7 +402,6 @@ public class TraceFilterTests { this.request = builder() .header(SPAN_FLAGS, 0) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); @@ -483,8 +417,7 @@ public class TraceFilterTests { .header(SPAN_ID_NAME, SpanUtil.idToHex(10L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(neverSampleTracing()); - filter.doFilter(this.request, this.response, this.filterChain); + neverSampleFilter().doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); // It is ok to go without a trace ID, if sampling or debug is set @@ -500,9 +433,8 @@ public class TraceFilterTests { .header(SPAN_FLAGS, 1) .header(TRACE_ID_NAME, SpanUtil.idToHex(10L)) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(neverSampleTracing()); - filter.doFilter(this.request, this.response, this.filterChain); + neverSampleFilter().doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); then(this.reporter.getSpans()).isEmpty(); @@ -515,7 +447,6 @@ public class TraceFilterTests { .param("foo", "bar") .buildRequest(new MockServletContext()); this.response.setStatus(295); - TraceFilter filter = new TraceFilter(beanFactory()); filter.doFilter(this.request, this.response, this.filterChain); @@ -536,9 +467,8 @@ public class TraceFilterTests { this.request = builder() .header(SPAN_FLAGS, 1) .buildRequest(new MockServletContext()); - TraceFilter filter = new TraceFilter(neverSampleTracing()); - filter.doFilter(this.request, this.response, this.filterChain); + neverSampleFilter().doFilter(this.request, this.response, this.filterChain); then(Tracing.current().tracer().currentSpan()).isNull(); then(this.reporter.getSpans()) @@ -581,18 +511,4 @@ public class TraceFilterTests { .containsEntry("http.status_code", status.toString()); } } - - private BeanFactory beanFactory() { - BDDMockito.given(beanFactory.getBean(SkipPatternProvider.class)) - .willThrow(new NoSuchBeanDefinitionException("foo")); - BDDMockito.given(beanFactory.getBean(SleuthProperties.class)) - .willReturn(this.properties); - BDDMockito.given(beanFactory.getBean(HttpTracing.class)) - .willReturn(this.httpTracing); - BDDMockito.given(beanFactory.getBean(TraceKeys.class)) - .willReturn(this.traceKeys); - BDDMockito.given(beanFactory.getBean(ErrorParser.class)) - .willReturn(new ExceptionMessageErrorParser()); - return beanFactory; - } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java index 635649ddf..7aca0177a 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java @@ -85,13 +85,12 @@ public class TraceFilterWebIntegrationTests { then(fromFirstTraceFilterFlow.tags()) .containsEntry("http.status_code", "500") .containsEntry("http.method", "GET") - .containsEntry("error", "Request processing failed; nested exception is java.lang.RuntimeException: Throwing exception") + .containsEntry("error", "Throwing exception") .containsEntry("mvc.controller.class", "ExceptionThrowingController"); Span fromErrorController = this.accumulator.getSpans().get(1); then(fromErrorController.tags()) .containsEntry("http.status_code", "500") - .containsEntry("error", "Request processing failed; nested exception is java.lang.RuntimeException: Throwing exception") - .containsEntry("mvc.controller.class", "BasicErrorController"); + .containsEntry("error", "Request processing failed; nested exception is java.lang.RuntimeException: Throwing exception"); // issue#714 String hex = fromErrorController.traceId(); String[] split = capture.toString().split("\n"); @@ -110,10 +109,10 @@ public class TraceFilterWebIntegrationTests { } catch (HttpClientErrorException e) { } - //TODO: Check if it should be 1 or 2 spans then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.accumulator.getSpans()).hasSize(1); + then(this.accumulator.getSpans()).hasSize(2).as("spans with same id, one from server, one from handler"); then(this.accumulator.getSpans().get(0).kind().ordinal()).isEqualTo(Span.Kind.SERVER.ordinal()); + then(this.accumulator.getSpans().get(0).tags()).containsEntry("http.status_code", "400"); } private int port() { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptorTests.java deleted file mode 100644 index ad03401f9..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHandlerInterceptorTests.java +++ /dev/null @@ -1,60 +0,0 @@ -/* - * Copyright 2013-2018 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 org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.BDDMockito; -import org.mockito.InjectMocks; -import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; -import org.springframework.beans.factory.BeanFactory; -import org.springframework.beans.factory.NoSuchBeanDefinitionException; -import org.springframework.boot.web.servlet.error.ErrorController; - -import static org.assertj.core.api.BDDAssertions.then; -import static org.mockito.BDDMockito.given; -import static org.mockito.Mockito.only; - -/** - * @author Marcin Grzejszczak - */ -@RunWith(MockitoJUnitRunner.class) -public class TraceHandlerInterceptorTests { - - @Mock BeanFactory beanFactory; - @InjectMocks TraceHandlerInterceptor traceHandlerInterceptor; - - @Test - public void should_cache_the_retrieved_bean_when_exception_took_place() throws Exception { - given(this.beanFactory.getBean(ErrorController.class)).willThrow(new NoSuchBeanDefinitionException("errorController")); - - then(this.traceHandlerInterceptor.errorController()).isNull(); - then(this.traceHandlerInterceptor.errorController()).isNull(); - BDDMockito.then(this.beanFactory).should(only()).getBean(ErrorController.class); - } - - @Test - public void should_cache_the_retrieved_bean_when_no_exception_took_place() throws Exception { - given(this.beanFactory.getBean(ErrorController.class)).willReturn(() -> null); - - then(this.traceHandlerInterceptor.errorController()).isNotNull(); - then(this.traceHandlerInterceptor.errorController()).isNotNull(); - BDDMockito.then(this.beanFactory).should(only()).getBean(ErrorController.class); - } - -} \ No newline at end of file diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilterTests.java index 6baf0d523..27fdc673d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilterTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/zuul/TracePostZuulFilterTests.java @@ -38,7 +38,6 @@ import org.springframework.cloud.netflix.zuul.metrics.EmptyTracerFactory; import org.springframework.cloud.sleuth.ExceptionMessageErrorParser; import org.springframework.cloud.sleuth.TraceKeys; import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; -import org.springframework.cloud.sleuth.instrument.web.TraceRequestAttributes; import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; import static org.assertj.core.api.BDDAssertions.then; @@ -114,7 +113,5 @@ public class TracePostZuulFilterTests { .containsEntry("http.status_code", "456"); then(spans.get(0).name()).isEqualTo("http:start"); then(this.tracing.tracer().currentSpan()).isNull(); - BDDMockito.then(this.httpServletRequest).should().setAttribute( - TraceRequestAttributes.HANDLED_SPAN_REQUEST_ATTR, "true"); } } \ No newline at end of file diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java index b966a104b..1fe53a086 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-messaging/src/test/java/integration/MessagingApplicationTests.java @@ -44,7 +44,8 @@ import static org.assertj.core.api.BDDAssertions.then; @RunWith(SpringJUnit4ClassRunner.class) @SpringBootTest(classes = { IntegrationSpanCollectorConfig.class, SampleMessagingApplication.class }, webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT) -@TestPropertySource(properties = { "sample.zipkin.enabled=true" }) +@TestPropertySource(properties = { "sample.zipkin.enabled=true", + "spring.sleuth.http.legacy.enabled=true" }) @DirtiesContext public class MessagingApplicationTests extends AbstractIntegrationTest { @@ -76,7 +77,7 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { long spanId = new Random().nextLong(); await().atMost(15, SECONDS).untilAsserted(() -> - httpMessageWithTraceIdInHeadersIsSuccessfullySent(sampleAppUrl + "/", traceId, spanId).run() + httpMessageWithTraceIdInHeadersIsSuccessfullySent(sampleAppUrl + "/", traceId, spanId).run() ); await().atMost(15, SECONDS).untilAsserted(() -> { @@ -140,7 +141,7 @@ public class MessagingApplicationTests extends AbstractIntegrationTest { private Optional findLastHttpSpansParent() { return this.integrationTestSpanCollector.hashedSpans.stream() - .filter(span -> "get".equals(span.name()) && span.kind() != null).findFirst(); + .filter(span -> "http:/".equals(span.name()) && span.kind() != null).findFirst(); } private Optional findSpanWithKind(Span.Kind kind) {