From 71b7eb0fa68209b6d0e72ad4f5ae526c36f55d07 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 07:57:21 +0800 Subject: [PATCH 1/6] feign cleanup --- .../web/client/feign/TracingFeignClient.java | 104 +++++++++--------- 1 file changed, 52 insertions(+), 52 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java index 49914142b..a551829fb 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java @@ -25,16 +25,20 @@ import java.util.LinkedHashMap; import java.util.Map; import brave.Span; -import brave.Tracer; import brave.http.HttpClientHandler; +import brave.http.HttpClientRequest; +import brave.http.HttpClientResponse; import brave.http.HttpTracing; -import brave.propagation.Propagation; +import brave.propagation.CurrentTraceContext; +import brave.propagation.CurrentTraceContext.Scope; import feign.Client; import feign.Request; import feign.Response; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.lang.Nullable; + /** * Feign client wrapper. * @@ -45,37 +49,14 @@ final class TracingFeignClient implements Client { private static final Log log = LogFactory.getLog(TracingFeignClient.class); - static final Propagation.Setter>, String> SETTER = new Propagation.Setter>, String>() { - @Override - public void put(Map> carrier, String key, - String value) { - if (!carrier.containsKey(key)) { - carrier.put(key, Collections.singletonList(value)); - if (log.isTraceEnabled()) { - log.trace("Added key [" + key + "] and header value [" + value + "]"); - } - } - else { - if (log.isTraceEnabled()) { - log.trace("Key [" + key + "] already there in the headers"); - } - } - } - - @Override - public String toString() { - return "Map::set"; - } - }; - - final Tracer tracer; + final CurrentTraceContext currentTraceContext; final Client delegate; - final HttpClientHandler handler; + final HttpClientHandler handler; TracingFeignClient(HttpTracing httpTracing, Client delegate) { - this.tracer = httpTracing.tracing().tracer(); + this.currentTraceContext = httpTracing.tracing().currentTraceContext(); this.handler = HttpClientHandler.create(httpTracing); this.delegate = delegate; } @@ -86,29 +67,27 @@ final class TracingFeignClient implements Client { @Override public Response execute(Request req, Request.Options options) throws IOException { - HttpClientRequest request = new HttpClientRequest(req); + RequestWrapper request = new RequestWrapper(req); Span span = this.handler.handleSend(request); if (log.isDebugEnabled()) { log.debug("Handled send of " + span); } - HttpClientResponse response = null; + Response res = null; Throwable error = null; - try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span)) { - Response res = this.delegate.execute(request.build(), options); - if (res != null) { - response = new HttpClientResponse(res); - } - else { // possibly null on bad implementation or mocks - response = new HttpClientResponse( - Response.builder().request(req).build()); + try (Scope ws = this.currentTraceContext.newScope(span.context())) { + res = this.delegate.execute(request.build(), options); + if (res == null) { // possibly null on bad implementation or mocks + res = Response.builder().request(req).build(); } return res; } - catch (IOException | RuntimeException | Error e) { + catch (Throwable e) { error = e; throw e; } finally { + ResponseWrapper response = res != null + ? new ResponseWrapper(request, res, error) : null; this.handler.handleReceive(response, error, span); if (log.isDebugEnabled()) { @@ -117,20 +96,22 @@ final class TracingFeignClient implements Client { } } - void handleSendAndReceive(Span span, Request request, Response response, - Throwable error) { - this.handler.handleSend(new HttpClientRequest(request), span); - this.handler.handleReceive( - response != null ? new HttpClientResponse(response) : null, error, span); + void handleSendAndReceive(Span span, Request req, @Nullable Response res, + @Nullable Throwable error) { + RequestWrapper request = new RequestWrapper(req); + this.handler.handleSend(request, span); + ResponseWrapper response = res != null ? new ResponseWrapper(request, res, error) + : null; + this.handler.handleReceive(response, error, span); } - static final class HttpClientRequest extends brave.http.HttpClientRequest { + static final class RequestWrapper extends HttpClientRequest { final Request delegate; Map> headers; - HttpClientRequest(Request delegate) { + RequestWrapper(Request delegate) { this.delegate = delegate; } @@ -198,22 +179,41 @@ final class TracingFeignClient implements Client { } - static final class HttpClientResponse extends brave.http.HttpClientResponse { + static final class ResponseWrapper extends HttpClientResponse { - final Response delegate; + final RequestWrapper request; - HttpClientResponse(Response delegate) { - this.delegate = delegate; + final Response response; + + @Nullable + final Throwable error; + + ResponseWrapper(RequestWrapper request, Response response, + @Nullable Throwable error) { + this.request = request; + this.response = response; + this.error = error; } @Override public Object unwrap() { - return delegate; + return response; + } + + @Override + public RequestWrapper request() { + return request; + } + + @Override + @Nullable + public Throwable error() { + return error; } @Override public int statusCode() { - return delegate.status(); + return response.status(); } } From 2ea203076baf1c383aae26656dbcfd7bd3fea2fe Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 07:53:04 +0800 Subject: [PATCH 2/6] Fixes WebClient which started a client span prior to a subscription (#1576) In looking at underlying HttpClient mechanics, I noticed the reactor call doesn't happen until subscribe. Before this change, we started the client span at the ExchangeFilterFunction, not at subscribe time. --- .../client/HttpClientBeanPostProcessor.java | 10 ++---- .../TraceWebClientBeanPostProcessor.java | 32 ++++++++----------- 2 files changed, 16 insertions(+), 26 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java index 7e45a9b2b..888377a63 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java @@ -25,7 +25,6 @@ import brave.Span; import brave.http.HttpClientHandler; import brave.http.HttpTracing; import brave.propagation.CurrentTraceContext; -import brave.propagation.CurrentTraceContext.Scope; import brave.propagation.TraceContext; import io.netty.bootstrap.Bootstrap; import reactor.core.publisher.Mono; @@ -155,12 +154,9 @@ class HttpClientBeanPostProcessor implements BeanPostProcessor { null); WrappedHttpClientRequest request = new WrappedHttpClientRequest(req); - // Simplify after openzipkin/brave#1082 - try (Scope ws = currentTraceContext().maybeScope(parent)) { - clientSpan = handler().handleSend(request); - parseConnectionAddress(connection, clientSpan); - ref.set(clientSpan); - } + clientSpan = handler().handleSendWithParent(request, parent); + parseConnectionAddress(connection, clientSpan); + ref.set(clientSpan); } static void parseConnectionAddress(Connection connection, Span span) { diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java index 86d1c6f65..97faff492 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java @@ -138,14 +138,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { @Override public Mono filter(ClientRequest request, ExchangeFunction next) { - HttpClientRequest wrapper = new HttpClientRequest(request); - TraceContext parent = currentTraceContext().get(); - Span clientSpan = handler().handleSend(wrapper); - if (log.isDebugEnabled()) { - log.debug("HttpClientHandler::handleSend: " + clientSpan); - } - return new MonoWebClientTrace(next, wrapper.buildRequest(), this, parent, - clientSpan); + return new MonoWebClientTrace(next, request, this); } CurrentTraceContext currentTraceContext() { @@ -177,18 +170,14 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { @Nullable final TraceContext parent; - private final Span span; - MonoWebClientTrace(ExchangeFunction next, ClientRequest request, - TraceExchangeFilterFunction filterFunction, @Nullable TraceContext parent, - Span span) { + TraceExchangeFilterFunction filterFunction) { this.next = next; this.request = request; this.handler = filterFunction.handler(); this.currentTraceContext = filterFunction.currentTraceContext(); this.scopePassingTransformer = filterFunction.scopePassingTransformer; - this.parent = parent; - this.span = span; + this.parent = currentTraceContext.get(); } @Override @@ -196,8 +185,14 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { Context context = subscriber.currentContext(); - this.next.exchange(request).subscribe(new WebClientTracerSubscriber( - subscriber, context, parent, span, this)); + HttpClientRequest wrapper = new HttpClientRequest(request); + Span span = handler.handleSendWithParent(wrapper, parent); + if (log.isDebugEnabled()) { + log.debug("HttpClientHandler::handleSend: " + span); + } + + this.next.exchange(wrapper.buildRequest()).subscribe( + new WebClientTracerSubscriber(subscriber, context, span, this)); } } @@ -224,10 +219,9 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { boolean done; WebClientTracerSubscriber(CoreSubscriber actual, - Context ctx, @Nullable final TraceContext parent, Span clientSpan, - MonoWebClientTrace mono) { + Context ctx, Span clientSpan, MonoWebClientTrace mono) { this.actual = actual; - this.parent = parent; + this.parent = mono.parent; this.clientSpan = clientSpan; this.handler = mono.handler; this.currentTraceContext = mono.currentTraceContext; From eec55b9a6c153a84b1b3f19a7fd1cbe7edd0de55 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 09:12:47 +0800 Subject: [PATCH 3/6] Adds wiring for HttpRequestParser and HttpResponseParser (#1575) --- .../main/asciidoc/spring-cloud-sleuth.adoc | 38 +- .../web/HttpClientRequestParser.java | 51 ++ .../web/HttpClientResponseParser.java | 51 ++ .../web/HttpServerRequestParser.java | 51 ++ .../web/HttpServerResponseParser.java | 51 ++ .../web/SleuthHttpClientParser.java | 45 +- .../web/SleuthHttpServerParser.java | 61 +-- .../web/TraceHttpAutoConfiguration.java | 69 ++- .../web/SleuthHttpClientParserTests.java | 85 ++- .../web/SleuthHttpParserAccessor.java | 41 -- .../instrument/web/TraceFilterTests.java | 495 ------------------ .../web/TraceHttpAutoConfigurationTests.java | 224 ++++++++ .../TraceRestTemplateInterceptorTests.java | 6 +- .../web/client/feign/FeignRetriesTests.java | 4 +- .../client/feign/TraceFeignAspectTests.java | 4 +- .../client/feign/TracingFeignClientTests.java | 21 +- .../zuul/TracePostZuulFilterTests.java | 6 +- .../sample/SampleWebsocketApplication.java | 4 +- .../web/TraceFilterWebIntegrationTests.java | 44 +- 19 files changed, 665 insertions(+), 686 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientRequestParser.java create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientResponseParser.java create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerRequestParser.java create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerResponseParser.java delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpParserAccessor.java delete mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java diff --git a/docs/src/main/asciidoc/spring-cloud-sleuth.adoc b/docs/src/main/asciidoc/spring-cloud-sleuth.adoc index 40e8d9c50..de4957584 100644 --- a/docs/src/main/asciidoc/spring-cloud-sleuth.adoc +++ b/docs/src/main/asciidoc/spring-cloud-sleuth.adoc @@ -838,12 +838,38 @@ Sleuth will search for beans of those types and automatically apply customizatio === HTTP -If a customization of client / server parsing of the HTTP related spans is -required, just register a bean of type `brave.http.HttpClientParser` or -`brave.http.HttpServerParser`. If client /server sampling is required, just -register a bean of type `brave.sampler.SamplerFunction` and name -the bean `sleuthHttpClientSampler` for client sampler and -`sleuthHttpServerSampler` for server sampler. +==== Data Policy + +The default span data policy for HTTP requests is described in Brave: +https://github.com/openzipkin/brave/tree/master/instrumentation/http#span-data-policy + +To add different data to the span, you need to register a bean of type +`brave.http.HttpRequestParser` or `brave.http.HttpResponseParser` based on when +the data is collected. + +The bean names correspond to the request or response side, and whether it is +a client or server. For example, `sleuthHttpClientRequestParser` changes what +is collected before a client request is sent to the server. + +For your convenience `@HttpClientRequestParser`, `@HttpClientResponseParser` +and corresponding server annotations can be used to inject the proper beans +or to reference the bean names via their static String `NAME` fields. + +Here's an example adding the HTTP url in addition to defaults: +[source,java] +---- +@Configuration +class Config { +include::{project-root}/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java[tags=custom_parser,indent=2] +} +---- + +==== Sampling + +If client /server sampling is required, just register a bean of type +`brave.sampler.SamplerFunction` and name the bean +`sleuthHttpClientSampler` for client sampler and `sleuthHttpServerSampler` +for server sampler. For your convenience the `@HttpClientSampler` and `@HttpServerSampler` annotations can be used to inject the proper beans or to reference the bean diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientRequestParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientRequestParser.java new file mode 100644 index 000000000..f79639f58 --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientRequestParser.java @@ -0,0 +1,51 @@ +/* + * Copyright 2013-2019 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 + * + * https://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.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import brave.http.HttpRequestParser; +import brave.http.HttpTracing; + +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Annotate a client {@link HttpRequestParser} that should be injected to + * {@link HttpTracing.Builder#clientRequestParser(HttpRequestParser)}. + * + * @see Qualifier + * @since 2.2.2 + */ +@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, + ElementType.ANNOTATION_TYPE }) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +@Documented +@Qualifier(HttpClientRequestParser.NAME) +public @interface HttpClientRequestParser { + + /** + * Default name for Sleuth HTTP client request parser. + */ + String NAME = "sleuthHttpClientRequestParser"; + +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientResponseParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientResponseParser.java new file mode 100644 index 000000000..a5ecc499c --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpClientResponseParser.java @@ -0,0 +1,51 @@ +/* + * Copyright 2013-2019 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 + * + * https://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.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import brave.http.HttpResponseParser; +import brave.http.HttpTracing; + +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Annotate a client {@link HttpResponseParser} that should be injected to + * {@link HttpTracing.Builder#clientResponseParser(HttpResponseParser)}. + * + * @see Qualifier + * @since 2.2.2 + */ +@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, + ElementType.ANNOTATION_TYPE }) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +@Documented +@Qualifier(HttpClientResponseParser.NAME) +public @interface HttpClientResponseParser { + + /** + * Default name for Sleuth HTTP client response parser. + */ + String NAME = "sleuthHttpClientResponseParser"; + +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerRequestParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerRequestParser.java new file mode 100644 index 000000000..7ae20954f --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerRequestParser.java @@ -0,0 +1,51 @@ +/* + * Copyright 2013-2019 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 + * + * https://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.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import brave.http.HttpRequestParser; +import brave.http.HttpTracing; + +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Annotate a server {@link HttpRequestParser} that should be injected to + * {@link HttpTracing.Builder#serverRequestParser(HttpRequestParser)}. + * + * @see Qualifier + * @since 2.2.2 + */ +@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, + ElementType.ANNOTATION_TYPE }) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +@Documented +@Qualifier(HttpServerRequestParser.NAME) +public @interface HttpServerRequestParser { + + /** + * Default name for Sleuth HTTP server request parser. + */ + String NAME = "sleuthHttpServerRequestParser"; + +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerResponseParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerResponseParser.java new file mode 100644 index 000000000..1e19082cd --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/HttpServerResponseParser.java @@ -0,0 +1,51 @@ +/* + * Copyright 2013-2019 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 + * + * https://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.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import brave.http.HttpResponseParser; +import brave.http.HttpTracing; + +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Annotate a server {@link HttpResponseParser} that should be injected to + * {@link HttpTracing.Builder#serverResponseParser(HttpResponseParser)}. + * + * @see Qualifier + * @since 2.2.2 + */ +@Target({ ElementType.FIELD, ElementType.METHOD, ElementType.PARAMETER, ElementType.TYPE, + ElementType.ANNOTATION_TYPE }) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +@Documented +@Qualifier(HttpServerResponseParser.NAME) +public @interface HttpServerResponseParser { + + /** + * Default name for Sleuth HTTP server response parser. + */ + String NAME = "sleuthHttpServerResponseParser"; + +} diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParser.java index 8c77eacfd..7b894c34e 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParser.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParser.java @@ -19,18 +19,19 @@ package org.springframework.cloud.sleuth.instrument.web; import java.net.URI; import brave.SpanCustomizer; -import brave.http.HttpAdapter; -import brave.http.HttpClientParser; +import brave.http.HttpRequest; +import brave.http.HttpRequestParser; +import brave.propagation.TraceContext; import org.springframework.cloud.sleuth.util.SpanNameUtil; /** - * An {@link HttpClientParser} that behaves like Sleuth in versions 1.x. + * An {@link HttpRequestParser} for clients that behaves like Sleuth in versions 1.x. * * @author Marcin Grzejszczak * @since 2.0.0 */ -class SleuthHttpClientParser extends HttpClientParser { +class SleuthHttpClientParser implements HttpRequestParser { private static final String HOST_KEY = "http.host"; @@ -47,22 +48,20 @@ class SleuthHttpClientParser extends HttpClientParser { } @Override - protected String spanName(HttpAdapter adapter, Req req) { - return getName(URI.create(adapter.url(req))); - } + public void parse(HttpRequest request, TraceContext context, SpanCustomizer span) { + HttpRequestParser.DEFAULT.parse(request, context, span); + + String url = request.url(); + if (url != null) { + URI uri = URI.create(url); + span.name(getName(uri)); + addRequestTags(span, url, uri.getHost(), uri.getPath(), request.method()); + } - @Override - public void request(HttpAdapter adapter, Req req, - SpanCustomizer customizer) { - super.request(adapter, req, customizer); - String url = adapter.url(req); - URI uri = URI.create(url); - addRequestTags(customizer, url, uri.getHost(), uri.getPath(), - adapter.method(req)); for (String header : this.traceKeys.getHttp().getHeaders()) { - String headerValue = adapter.requestHeader(req, header); + String headerValue = request.header(header); if (headerValue != null) { - customizer.tag(key(header), headerValue); + span.tag(key(header), headerValue); } } } @@ -81,14 +80,14 @@ class SleuthHttpClientParser extends HttpClientParser { return uri.getScheme() == null ? "http" : uri.getScheme(); } - private void addRequestTags(SpanCustomizer customizer, String url, String host, - String path, String method) { - customizer.tag(URL_KEY, url); + private void addRequestTags(SpanCustomizer span, String url, String host, String path, + String method) { + span.tag(URL_KEY, url); if (host != null) { - customizer.tag(HOST_KEY, host); + span.tag(HOST_KEY, host); } - customizer.tag(PATH_KEY, path); - customizer.tag(METHOD_KEY, method); + span.tag(PATH_KEY, path); + span.tag(METHOD_KEY, method); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParser.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParser.java index b35dfa0b9..417fe01a9 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParser.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParser.java @@ -18,70 +18,45 @@ package org.springframework.cloud.sleuth.instrument.web; import javax.servlet.http.HttpServletResponse; -import brave.ErrorParser; import brave.SpanCustomizer; -import brave.http.HttpAdapter; -import brave.http.HttpClientParser; -import brave.http.HttpServerParser; +import brave.http.HttpRequestParser; +import brave.http.HttpResponse; +import brave.http.HttpResponseParser; +import brave.propagation.TraceContext; /** - * An {@link HttpClientParser} that behaves like Sleuth in versions 1.x. + * An {@link HttpRequestParser} and {@link HttpResponseParser} for servers that behaves + * like Sleuth in versions 1.x. * * @author Marcin Grzejszczak * @since 2.0.0 */ -class SleuthHttpServerParser extends HttpServerParser { +class SleuthHttpServerParser extends SleuthHttpClientParser + implements HttpResponseParser { private static final String STATUS_CODE_KEY = "http.status_code"; - private final SleuthHttpClientParser clientParser; - - private final ErrorParser errorParser; - - SleuthHttpServerParser(TraceKeys traceKeys, ErrorParser errorParser) { - this.clientParser = new SleuthHttpClientParser(traceKeys); - this.errorParser = errorParser; + SleuthHttpServerParser(TraceKeys traceKeys) { + super(traceKeys); } @Override - protected ErrorParser errorParser() { - return this.errorParser; - } - - @Override - protected String spanName(HttpAdapter adapter, Req req) { - return this.clientParser.spanName(adapter, req); - } - - @Override - public void request(HttpAdapter adapter, Req req, - SpanCustomizer customizer) { - this.clientParser.request(adapter, req, customizer); - } - - @Override - public void response(HttpAdapter adapter, Resp res, Throwable error, - SpanCustomizer customizer) { - if (res == null) { - error(null, error, customizer); - return; + public void parse(HttpResponse response, TraceContext context, SpanCustomizer span) { + int httpStatus = response.statusCode(); + if (httpStatus == 0) { + return; // already parsed the error } - Integer httpStatus = adapter.statusCode(res); - if (httpStatus == null) { - error(httpStatus, error, customizer); - return; - } - if (httpStatus == HttpServletResponse.SC_OK && error != null) { + + if (httpStatus == HttpServletResponse.SC_OK && response.error() == null) { // Filter chain threw exception but the response status may not have been set // yet, so we have to guess. - customizer.tag(STATUS_CODE_KEY, + span.tag(STATUS_CODE_KEY, String.valueOf(HttpServletResponse.SC_INTERNAL_SERVER_ERROR)); } // only tag valid http statuses else if (httpStatus >= 100 && (httpStatus < 200) || (httpStatus > 399)) { - customizer.tag(STATUS_CODE_KEY, String.valueOf(httpStatus)); + span.tag(STATUS_CODE_KEY, String.valueOf(httpStatus)); } - error(httpStatus, error, customizer); } } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfiguration.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfiguration.java index 3d85b8cfe..f0f72b59f 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfiguration.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfiguration.java @@ -19,12 +19,11 @@ package org.springframework.cloud.sleuth.instrument.web; import java.util.ArrayList; import java.util.List; -import brave.ErrorParser; import brave.Tracing; -import brave.http.HttpClientParser; import brave.http.HttpRequest; +import brave.http.HttpRequestParser; +import brave.http.HttpResponseParser; import brave.http.HttpSampler; -import brave.http.HttpServerParser; import brave.http.HttpTracing; import brave.http.HttpTracingCustomizer; import brave.sampler.SamplerFunction; @@ -64,7 +63,12 @@ public class TraceHttpAutoConfiguration { @ConditionalOnMissingBean // NOTE: stable bean name as might be used outside sleuth HttpTracing httpTracing(Tracing tracing, SkipPatternProvider provider, - HttpClientParser clientParser, HttpServerParser serverParser, + @Nullable @HttpClientRequestParser HttpRequestParser httpClientRequestParser, + @Nullable @HttpClientResponseParser HttpResponseParser httpClientResponseParser, + @Nullable brave.http.HttpClientParser clientParser, + @Nullable @HttpServerRequestParser HttpRequestParser httpServerRequestParser, + @Nullable @HttpServerResponseParser HttpResponseParser httpServerResponseParser, + @Nullable brave.http.HttpServerParser serverParser, @HttpClientSampler SamplerFunction httpClientSampler, @Nullable @ServerSampler HttpSampler serverSampler, @Nullable @HttpServerSampler SamplerFunction httpServerSampler) { @@ -74,11 +78,36 @@ public class TraceHttpAutoConfiguration { SamplerFunction combinedSampler = combineUserProvidedSamplerWithSkipPatternSampler( httpServerSampler, provider); HttpTracing.Builder builder = HttpTracing.newBuilder(tracing) - .clientParser(clientParser).serverParser(serverParser) .clientSampler(httpClientSampler).serverSampler(combinedSampler); + + if (httpClientRequestParser != null || httpClientResponseParser != null) { + if (httpClientRequestParser != null) { + builder.clientRequestParser(httpClientRequestParser); + } + if (httpClientResponseParser != null) { + builder.clientResponseParser(httpClientResponseParser); + } + } + else if (clientParser != null) { // consider deprecated last + builder.clientParser(clientParser); + } + + if (httpServerRequestParser != null || httpServerResponseParser != null) { + if (httpServerRequestParser != null) { + builder.serverRequestParser(httpServerRequestParser); + } + if (httpServerResponseParser != null) { + builder.serverResponseParser(httpServerResponseParser); + } + } + else if (serverParser != null) { // consider deprecated last + builder.serverParser(serverParser); + } + for (HttpTracingCustomizer customizer : this.httpTracingCustomizers) { customizer.customize(builder); } + return builder.build(); } @@ -94,39 +123,27 @@ public class TraceHttpAutoConfiguration { } @Bean + @ConditionalOnMissingBean(name = HttpClientRequestParser.NAME) @ConditionalOnProperty(name = "spring.sleuth.http.legacy.enabled", havingValue = "true") - HttpClientParser sleuthHttpClientParser(TraceKeys traceKeys) { + HttpRequestParser sleuthHttpClientRequestParser(TraceKeys traceKeys) { return new SleuthHttpClientParser(traceKeys); } @Bean - @ConditionalOnProperty(name = "spring.sleuth.http.legacy.enabled", - havingValue = "false", matchIfMissing = true) - @ConditionalOnMissingBean - HttpClientParser httpClientParser(ErrorParser errorParser) { - return new HttpClientParser() { - @Override - protected ErrorParser errorParser() { - return errorParser; - } - }; - } - - @Bean + @ConditionalOnMissingBean(name = HttpServerRequestParser.NAME) @ConditionalOnProperty(name = "spring.sleuth.http.legacy.enabled", havingValue = "true") - HttpServerParser sleuthHttpServerParser(TraceKeys traceKeys, - ErrorParser errorParser) { - return new SleuthHttpServerParser(traceKeys, errorParser); + HttpRequestParser sleuthHttpServerRequestParser(TraceKeys traceKeys) { + return new SleuthHttpServerParser(traceKeys); } @Bean + @ConditionalOnMissingBean(name = HttpServerResponseParser.NAME) @ConditionalOnProperty(name = "spring.sleuth.http.legacy.enabled", - havingValue = "false", matchIfMissing = true) - @ConditionalOnMissingBean - HttpServerParser defaultHttpServerParser() { - return new HttpServerParser(); + havingValue = "true") + HttpResponseParser sleuthHttpServerResponseParser(TraceKeys traceKeys) { + return new SleuthHttpServerParser(traceKeys); } @Bean diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParserTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParserTests.java index 479d8c541..76fadb6e1 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParserTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpClientParserTests.java @@ -22,44 +22,98 @@ import java.util.HashMap; import java.util.Map; import brave.SpanCustomizer; -import brave.http.HttpClientAdapter; +import brave.http.HttpClientRequest; +import brave.http.HttpRequest; import org.junit.Test; import static org.assertj.core.api.BDDAssertions.then; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; /** - * Test case for HttpTraceKeysInjector. + * Test case for Sleuth 1.0 tagging implementation. * * @author Sven Zethelius */ +@Deprecated public class SleuthHttpClientParserTests { private TraceKeys traceKeys = new TraceKeys(); - private TestSpanCustomizer customizer = new TestSpanCustomizer(); + private TestSpan span = new TestSpan(); private SleuthHttpClientParser parser = new SleuthHttpClientParser(this.traceKeys); + @Test + public void should_shorten_the_span_name() { + HttpRequest request = mock(HttpRequest.class); + when(request.method()).thenReturn("GET"); + when(request.url()).thenReturn("https://foo/" + bigName()); + + parser.parse(request, null, span); + + then(this.span.name).hasSize(50); + } + + private String bigName() { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 60; i++) { + sb.append("a"); + } + return sb.toString(); + } + + @Test + public void recordsMethodHostAndUrl() { + HttpRequest request = mock(HttpRequest.class); + when(request.method()).thenReturn("POST"); + when(request.url()).thenReturn("http://localhost/?foo=bar"); + when(request.header("host")).thenReturn("localhost"); + + parser.parse(request, null, span); + + then(this.span.tags).containsEntry("http.url", "http://localhost/?foo=bar") + .containsEntry("http.host", "localhost").containsEntry("http.path", "/") + .containsEntry("http.method", "POST"); + } + + @Test + public void addsAdditionalHeaders() { + this.traceKeys.getHttp().getHeaders().add("x-foo"); + + HttpRequest request = mock(HttpRequest.class); + when(request.header("x-foo")).thenReturn("bar"); + + parser.parse(request, null, span); + + then(this.span.tags).containsEntry("http.x-foo", "bar"); + } + @Test public void should_set_tags_on_span_with_proper_header_values() throws Exception { this.traceKeys.getHttp() .setHeaders(Arrays.asList("Accept", "User-Agent", "Content-Type")); - this.parser.request(new HttpClientAdapter() { + this.parser.parse(new HttpClientRequest() { private final URL url = new URL("http://localhost:8080/"); @Override - public String method(Object request) { + public String method() { return "GET"; } @Override - public String url(Object request) { + public String path() { + return null; + } + + @Override + public String url() { return this.url.toString(); } @Override - public String requestHeader(Object request, String name) { + public String header(String name) { if (name.equals("Accept")) { return "'text/plain','text/xml'"; } @@ -70,24 +124,31 @@ public class SleuthHttpClientParserTests { } @Override - public Integer statusCode(Object response) { - return 200; + public Object unwrap() { + return null; } - }, null, this.customizer); - then(this.customizer.tags).containsEntry("http.user-agent", "Test") + @Override + public void header(String name, String value) { + } + }, null, this.span); + + then(this.span.tags).containsEntry("http.user-agent", "Test") .containsEntry("http.accept", "'text/plain','text/xml'") .doesNotContainKey("http.content-type"); } } -class TestSpanCustomizer implements SpanCustomizer { +class TestSpan implements SpanCustomizer { + + String name; Map tags = new HashMap<>(); @Override public SpanCustomizer name(String name) { + this.name = name; return this; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpParserAccessor.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpParserAccessor.java deleted file mode 100644 index a8a6a15b0..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpParserAccessor.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2013-2019 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 - * - * https://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 brave.ErrorParser; -import brave.http.HttpClientParser; -import brave.http.HttpServerParser; - -/** - * @author Marcin Grzejszczak - * @since - */ -public final class SleuthHttpParserAccessor { - - private SleuthHttpParserAccessor() { - throw new IllegalStateException("Can't instantiate a utility class"); - } - - public static HttpClientParser getClient() { - return new SleuthHttpClientParser(new TraceKeys()); - } - - public static HttpServerParser getServer(ErrorParser errorParser) { - return new SleuthHttpServerParser(new TraceKeys(), errorParser); - } - -} 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 deleted file mode 100644 index 09498b586..000000000 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterTests.java +++ /dev/null @@ -1,495 +0,0 @@ -/* - * Copyright 2013-2019 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 - * - * https://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.util.concurrent.atomic.AtomicReference; -import java.util.regex.Pattern; - -import javax.servlet.Filter; - -import brave.ErrorParser; -import brave.Span; -import brave.Tracer; -import brave.Tracing; -import brave.http.HttpTracing; -import brave.propagation.StrictScopeDecorator; -import brave.propagation.ThreadLocalCurrentTraceContext; -import brave.sampler.Sampler; -import brave.servlet.TracingFilter; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; - -import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; -import org.springframework.cloud.sleuth.util.SpanUtil; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.mock.web.MockFilterChain; -import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.mock.web.MockHttpServletResponse; -import org.springframework.mock.web.MockServletContext; -import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.BDDAssertions.then; -import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; - -/** - * @author Spencer Gibb - */ -public class TraceFilterTests { - - static final String PARENT_ID = SpanUtil.idToHex(10L); - static final String TRACE_ID_NAME = "X-B3-TraceId"; - static final String SPAN_ID_NAME = "X-B3-SpanId"; - static final String PARENT_SPAN_ID_NAME = "X-B3-ParentSpanId"; - static final String SAMPLED_ID_NAME = "X-B3-Sampled"; - static final String SPAN_FLAGS = "X-B3-Flags"; - - ArrayListSpanReporter reporter = new ArrayListSpanReporter(); - - Tracing tracing = Tracing.newBuilder() - .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() - .addScopeDecorator(StrictScopeDecorator.create()).build()) - .spanReporter(this.reporter).build(); - - Tracer tracer = this.tracing.tracer(); - - TraceKeys traceKeys = new TraceKeys(); - - HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing) - .clientParser(new SleuthHttpClientParser(this.traceKeys)) - .serverParser(new SleuthHttpServerParser(this.traceKeys, new ErrorParser())) - .serverSampler(new SkipPatternHttpServerSampler(() -> Pattern.compile(""))) - .build(); - - Filter filter = TracingFilter.create(this.httpTracing); - - MockHttpServletRequest request; - - MockHttpServletResponse response; - - MockFilterChain filterChain; - - @Before - public void init() { - this.request = builder().buildRequest(new MockServletContext()); - this.response = new MockHttpServletResponse(); - this.response.setContentType(MediaType.APPLICATION_JSON_VALUE); - this.filterChain = new MockFilterChain(); - } - - public MockHttpServletRequestBuilder builder() { - return get("/?foo=bar").accept(MediaType.APPLICATION_JSON).header("User-Agent", - "MockMvc"); - } - - @After - public void cleanup() { - Tracing.current().close(); - } - - @Test - public void notTraced() throws Exception { - this.request = get("/favicon.ico").accept(MediaType.ALL) - .buildRequest(new MockServletContext()); - - neverSampleFilter().doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isEmpty(); - } - - private Filter neverSampleFilter() { - Tracing tracing = Tracing.newBuilder() - .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() - .addScopeDecorator(StrictScopeDecorator.create()).build()) - .spanReporter(this.reporter).sampler(Sampler.NEVER_SAMPLE) - .supportsJoin(false).build(); - HttpTracing httpTracing = HttpTracing.newBuilder(tracing) - .clientParser(new SleuthHttpClientParser(this.traceKeys)) - .serverParser( - new SleuthHttpServerParser(this.traceKeys, new ErrorParser())) - .serverSampler( - new SkipPatternHttpServerSampler(() -> Pattern.compile(""))) - .build(); - return TracingFilter.create(httpTracing); - } - - @Test - public void startsNewTrace() throws Exception { - this.filter.doFilter(this.request, this.response, this.filterChain); - - 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 { - this.response.setStatus(0); - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()) - .doesNotContainKey("http.status_code"); - } - - @Test - public void startsNewTraceWithParentIdInHeaders() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(2L)) - .header(PARENT_SPAN_ID_NAME, SpanUtil.idToHex(3L)) - .buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).id()).isEqualTo(PARENT_ID); - 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()); - } - - @Test - public void continuesATraceWhenSpanNotSampled() throws Exception { - AtomicReference span = new AtomicReference<>(); - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(2L)) - .header(PARENT_SPAN_ID_NAME, SpanUtil.idToHex(3L)) - .header(SAMPLED_ID_NAME, 0).buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, (req, resp) -> { - this.filterChain.doFilter(req, resp); - span.set(this.tracing.tracer().currentSpan()); - }); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(span.get().context().traceIdString()).isEqualTo(SpanUtil.idToHex(2L)); - } - - @Test - public void continuesSpanInRequestAttr() throws Exception { - Span span = this.tracer.nextSpan().name("http:foo"); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - } - - @Test - public void closesSpanInRequestAttrIfStatusCodeNotSuccessful() throws Exception { - Span span = this.tracer.nextSpan().name("http:foo"); - this.response.setStatus(404); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - } - - @Test - public void doesntDetachASpanIfStatusCodeNotSuccessfulAndRequestWasProcessed() - throws Exception { - Span span = this.tracer.nextSpan().name("http:foo"); - this.response.setStatus(404); - - then(Tracing.current().tracer().currentSpan()).isNull(); - this.filter.doFilter(this.request, this.response, this.filterChain); - } - - @Test - public void continuesSpanFromHeaders() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - verifyParentSpanHttpTags(); - } - - @Test - public void createsChildFromHeadersWhenJoinUnsupported() throws Exception { - Tracing tracing = Tracing.newBuilder() - .currentTraceContext(ThreadLocalCurrentTraceContext.newBuilder() - .addScopeDecorator(StrictScopeDecorator.create()).build()) - .spanReporter(this.reporter).supportsJoin(false).build(); - HttpTracing httpTracing = HttpTracing.create(tracing); - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - TracingFilter.create(httpTracing).doFilter(this.request, this.response, - this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).parentId()).isEqualTo(PARENT_ID); - } - - @Test - public void addsAdditionalHeaders() throws Exception { - 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"); - this.request.addHeader("X-Foo", "bar"); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()).containsEntry("http.x-foo", "bar"); - } - - @Test - public void additionalMultiValuedHeader() throws Exception { - 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"); - this.request.addHeader("X-Foo", "bar"); - this.request.addHeader("X-Foo", "spam"); - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - // We no longer support multi value headers - then(this.reporter.getSpans().get(0).tags()).containsEntry("http.x-foo", "bar"); - } - - @Test - public void shouldAnnotateSpanWithErrorWhenExceptionIsThrown() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - this.filterChain = new MockFilterChain() { - @Override - public void doFilter(javax.servlet.ServletRequest request, - javax.servlet.ServletResponse response) - throws java.io.IOException, javax.servlet.ServletException { - throw new RuntimeException("Planned"); - } - }; - try { - this.filter.doFilter(this.request, this.response, this.filterChain); - } - catch (RuntimeException e) { - assertThat(e.getMessage()).isEqualTo("Planned"); - } - - then(Tracing.current().tracer().currentSpan()).isNull(); - verifyParentSpanHttpTags(HttpStatus.INTERNAL_SERVER_ERROR); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()).containsEntry("error", "Planned"); - } - - @Test - public void detachesSpanWhenResponseStatusIsNot2xx() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - this.response.setStatus(404); - - then(Tracing.current().tracer().currentSpan()).isNull(); - this.filter.doFilter(this.request, this.response, this.filterChain); - } - - @Test - public void closesSpanWhenResponseStatusIs2xx() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - this.response.setStatus(200); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - } - - @Test - public void closesSpanWhenResponseStatusIs3xx() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - this.response.setStatus(302); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - } - - @Test - public void returns400IfSpanIsMalformedAndCreatesANewSpan() throws Exception { - this.request = builder().header(SPAN_ID_NAME, "asd") - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isNotEmpty(); - then(this.response.getStatus()).isEqualTo(HttpStatus.OK.value()); - } - - @Test - public void returns200IfSpanParentIsMalformedAndCreatesANewSpan() throws Exception { - this.request = builder().header(SPAN_ID_NAME, PARENT_ID) - .header(PARENT_SPAN_ID_NAME, "-") - .header(TRACE_ID_NAME, SpanUtil.idToHex(20L)) - .buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isNotEmpty(); - then(this.response.getStatus()).isEqualTo(HttpStatus.OK.value()); - } - - @Test - public void samplesASpanRegardlessOfTheSamplerWhenXB3FlagsIsPresentAndSetTo1() - throws Exception { - this.request = builder().header(SPAN_FLAGS, 1) - .buildRequest(new MockServletContext()); - - neverSampleFilter().doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isNotEmpty(); - } - - @Test - public void doesNotOverrideTheSampledFlagWhenXB3FlagIsSetToOtherValueThan1() - throws Exception { - this.request = builder().header(SPAN_FLAGS, 0) - .buildRequest(new MockServletContext()); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isNotEmpty(); - } - - @SuppressWarnings("Duplicates") - @Test - public void samplesWhenDebugFlagIsSetTo1AndOnlySpanIdIsSet() throws Exception { - this.request = builder().header(SPAN_FLAGS, 1) - .header(SPAN_ID_NAME, SpanUtil.idToHex(10L)) - .buildRequest(new MockServletContext()); - - 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 - then(this.reporter.getSpans()).hasSize(1).extracting("id") - .isNotEqualTo(SpanUtil.idToHex(10L)); - } - - @SuppressWarnings("Duplicates") - @Test - public void usesSamplingMechanismWhenIncomingTraceIsMalformed() throws Exception { - this.request = builder().header(SPAN_FLAGS, 1) - .header(TRACE_ID_NAME, SpanUtil.idToHex(10L)) - .buildRequest(new MockServletContext()); - - neverSampleFilter().doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).isEmpty(); - } - - // #668 - @Test - public void shouldSetTraceKeysForAnUntracedRequest() throws Exception { - this.request = builder().param("foo", "bar") - .buildRequest(new MockServletContext()); - this.response.setStatus(295); - - this.filter.doFilter(this.request, this.response, this.filterChain); - - 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", "295") - } - - @Test - public void samplesASpanDebugFlagWithInterceptor() throws Exception { - this.request = builder().header(SPAN_FLAGS, 1) - .buildRequest(new MockServletContext()); - - neverSampleFilter().doFilter(this.request, this.response, this.filterChain); - - then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).name()).isEqualTo("http:/"); - } - - public void verifyParentSpanHttpTags() { - verifyParentSpanHttpTags(HttpStatus.OK); - } - - /** - * Shows the expansion of {@link import - * org.springframework.cloud.sleuth.instrument.TraceKeys}. - * @param status http status - */ - public void verifyParentSpanHttpTags(HttpStatus status) { - then(this.reporter.getSpans().size()).isGreaterThan(0); - 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()); - verifyCurrentSpanStatusCodeForAContinuedSpan(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()) { - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()) - .doesNotContainKey("http.status_code"); - } - else { - then(this.reporter.getSpans()).hasSize(1); - then(this.reporter.getSpans().get(0).tags()).containsEntry("http.status_code", - "500"); - } - } - -} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfigurationTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfigurationTests.java index a5206db89..b74b1aa8e 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfigurationTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfigurationTests.java @@ -17,8 +17,12 @@ package org.springframework.cloud.sleuth.instrument.web; import brave.http.HttpAdapter; +import brave.http.HttpClientParser; import brave.http.HttpRequest; +import brave.http.HttpRequestParser; +import brave.http.HttpResponseParser; import brave.http.HttpSampler; +import brave.http.HttpServerParser; import brave.http.HttpTracing; import brave.sampler.SamplerFunction; import org.junit.Test; @@ -122,6 +126,142 @@ public class TraceHttpAutoConfigurationTests { }; } + @Test + public void defaultHttpClientParser() { + contextRunner().run((context) -> { + HttpRequestParser clientRequestParser = context.getBean(HttpTracing.class) + .clientRequestParser(); + HttpResponseParser clientResponseParser = context.getBean(HttpTracing.class) + .clientResponseParser(); + + then(clientRequestParser).isInstanceOf(HttpRequestParser.Default.class); + then(clientResponseParser).isInstanceOf(HttpResponseParser.Default.class); + }); + } + + @Test + public void configuresUserProvidedDeprecatedClientParser() { + contextRunner().withUserConfiguration(DeprecatedHttpClientParserConfig.class) + .run((context) -> { + HttpClientParser clientParser = context.getBean(HttpTracing.class) + .clientParser(); + + then(clientParser) + .isSameAs(DeprecatedHttpClientParserConfig.INSTANCE); + }); + } + + @Test + public void configuresUserProvidedHttpClientParser() { + contextRunner().withUserConfiguration(HttpClientParserConfig.class) + .run((context) -> { + HttpRequestParser clientRequestParser = context + .getBean(HttpTracing.class).clientRequestParser(); + HttpResponseParser clientResponseParser = context + .getBean(HttpTracing.class).clientResponseParser(); + + then(clientRequestParser) + .isSameAs(HttpClientParserConfig.REQUEST_PARSER); + then(clientResponseParser) + .isSameAs(HttpClientParserConfig.RESPONSE_PARSER); + }); + } + + @Test + public void prefersUserProvidedHttpClientParser() { + contextRunner().withUserConfiguration(DeprecatedHttpClientParserConfig.class) + .withUserConfiguration(HttpClientParserConfig.class).run((context) -> { + HttpRequestParser clientRequestParser = context + .getBean(HttpTracing.class).clientRequestParser(); + HttpResponseParser clientResponseParser = context + .getBean(HttpTracing.class).clientResponseParser(); + + then(clientRequestParser) + .isSameAs(HttpClientParserConfig.REQUEST_PARSER); + then(clientResponseParser) + .isSameAs(HttpClientParserConfig.RESPONSE_PARSER); + }); + } + + @Test + public void defaultHttpServerParser() { + contextRunner().run((context) -> { + HttpRequestParser serverRequestParser = context.getBean(HttpTracing.class) + .serverRequestParser(); + HttpResponseParser serverResponseParser = context.getBean(HttpTracing.class) + .serverResponseParser(); + + then(serverRequestParser).isInstanceOf(HttpRequestParser.Default.class); + then(serverResponseParser).isInstanceOf(HttpResponseParser.Default.class); + }); + } + + @Test + public void configuresUserProvidedDeprecatedServerParser() { + contextRunner().withUserConfiguration(DeprecatedHttpServerParserConfig.class) + .run((context) -> { + HttpServerParser serverParser = context.getBean(HttpTracing.class) + .serverParser(); + + then(serverParser) + .isSameAs(DeprecatedHttpServerParserConfig.INSTANCE); + }); + } + + @Test + public void configuresUserProvidedHttpServerParser() { + contextRunner().withUserConfiguration(HttpServerParserConfig.class) + .run((context) -> { + HttpRequestParser serverRequestParser = context + .getBean(HttpTracing.class).serverRequestParser(); + HttpResponseParser serverResponseParser = context + .getBean(HttpTracing.class).serverResponseParser(); + + then(serverRequestParser) + .isSameAs(HttpServerParserConfig.REQUEST_PARSER); + then(serverResponseParser) + .isSameAs(HttpServerParserConfig.RESPONSE_PARSER); + }); + } + + @Test + public void prefersUserProvidedHttpServerParser() { + contextRunner().withUserConfiguration(DeprecatedHttpServerParserConfig.class) + .withUserConfiguration(HttpServerParserConfig.class).run((context) -> { + HttpRequestParser serverRequestParser = context + .getBean(HttpTracing.class).serverRequestParser(); + HttpResponseParser serverResponseParser = context + .getBean(HttpTracing.class).serverResponseParser(); + + then(serverRequestParser) + .isSameAs(HttpServerParserConfig.REQUEST_PARSER); + then(serverResponseParser) + .isSameAs(HttpServerParserConfig.RESPONSE_PARSER); + }); + } + + /** + * Shows bean aliases work to configure the same instance for both client and server + */ + @Test + public void configuresUserProvidedHttpClientAndServerParser() { + contextRunner().withUserConfiguration(HttpParserConfig.class).run((context) -> { + HttpRequestParser serverRequestParser = context.getBean(HttpTracing.class) + .serverRequestParser(); + HttpResponseParser serverResponseParser = context.getBean(HttpTracing.class) + .serverResponseParser(); + HttpRequestParser clientRequestParser = context.getBean(HttpTracing.class) + .clientRequestParser(); + HttpResponseParser clientResponseParser = context.getBean(HttpTracing.class) + .clientResponseParser(); + + then(clientRequestParser).isSameAs(HttpParserConfig.REQUEST_PARSER); + then(clientResponseParser).isSameAs(HttpParserConfig.RESPONSE_PARSER); + then(serverRequestParser).isSameAs(HttpParserConfig.REQUEST_PARSER); + then(serverResponseParser).isSameAs(HttpParserConfig.RESPONSE_PARSER); + }); + } + private ApplicationContextRunner contextRunner(String... propertyValues) { return new ApplicationContextRunner().withPropertyValues(propertyValues) .withConfiguration(AutoConfigurations.of(TraceAutoConfiguration.class, @@ -188,3 +328,87 @@ class DeprecatedServerSamplerConfig { } } + +@Configuration +class HttpClientParserConfig { + + static final HttpRequestParser REQUEST_PARSER = (r, c, s) -> { + }; + static final HttpResponseParser RESPONSE_PARSER = (r, c, s) -> { + }; + + @Bean(HttpClientRequestParser.NAME) + HttpRequestParser sleuthHttpClientRequestParser() { + return REQUEST_PARSER; + } + + @Bean(HttpClientResponseParser.NAME) + HttpResponseParser sleuthHttpClientResponseParser() { + return RESPONSE_PARSER; + } + +} + +@Configuration +class DeprecatedHttpClientParserConfig { + + static final HttpClientParser INSTANCE = new HttpClientParser(); + + @Bean + HttpClientParser clientParser() { + return INSTANCE; + } + +} + +@Configuration +class HttpServerParserConfig { + + static final HttpRequestParser REQUEST_PARSER = (r, c, s) -> { + }; + static final HttpResponseParser RESPONSE_PARSER = (r, c, s) -> { + }; + + @Bean(HttpServerRequestParser.NAME) + HttpRequestParser sleuthHttpServerRequestParser() { + return REQUEST_PARSER; + } + + @Bean(HttpServerResponseParser.NAME) + HttpResponseParser sleuthHttpServerResponseParser() { + return RESPONSE_PARSER; + } + +} + +@Configuration +class DeprecatedHttpServerParserConfig { + + static final HttpServerParser INSTANCE = new HttpServerParser(); + + @Bean + HttpServerParser serverParser() { + return INSTANCE; + } + +} + +@Configuration +class HttpParserConfig { + + static final HttpRequestParser REQUEST_PARSER = (r, c, s) -> { + }; + static final HttpResponseParser RESPONSE_PARSER = (r, c, s) -> { + }; + + @Bean(name = { HttpClientRequestParser.NAME, HttpServerRequestParser.NAME }) + HttpRequestParser sleuthHttpServerRequestParser() { + return REQUEST_PARSER; + } + + @Bean(name = { HttpClientResponseParser.NAME, HttpServerResponseParser.NAME }) + HttpResponseParser sleuthHttpServerResponseParser() { + return RESPONSE_PARSER; + } + +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceRestTemplateInterceptorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceRestTemplateInterceptorTests.java index 55e0e6fe6..5cf5da04e 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceRestTemplateInterceptorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceRestTemplateInterceptorTests.java @@ -123,7 +123,7 @@ public class TraceRestTemplateInterceptorTests { @Test public void requestHeadersAddedWhenTracing() { setInterceptors(HttpTracing.newBuilder(this.tracing) - .clientParser(new SleuthHttpClientParser(this.traceKeys)).build()); + .clientRequestParser(new SleuthHttpClientParser(this.traceKeys)).build()); Span span = this.tracer.nextSpan().name("new trace"); try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span.start())) { @@ -183,7 +183,7 @@ public class TraceRestTemplateInterceptorTests { @Test public void createdSpanNameHasOnlyPrintableAsciiCharactersForNonEncodedURIWithNonAsciiChars() { setInterceptors(HttpTracing.newBuilder(this.tracing) - .clientParser(new SleuthHttpClientParser(this.traceKeys)).build()); + .clientRequestParser(new SleuthHttpClientParser(this.traceKeys)).build()); Span span = this.tracer.nextSpan().name("new trace"); try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span.start())) { @@ -206,7 +206,7 @@ public class TraceRestTemplateInterceptorTests { @Test public void willShortenTheNameOfTheSpan() { setInterceptors(HttpTracing.newBuilder(this.tracing) - .clientParser(new SleuthHttpClientParser(this.traceKeys)).build()); + .clientRequestParser(new SleuthHttpClientParser(this.traceKeys)).build()); Span span = this.tracer.nextSpan().name("new trace"); try (Tracer.SpanInScope ws = this.tracer.withSpanInScope(span.start())) { diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignRetriesTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignRetriesTests.java index 684460977..9d1d366ea 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignRetriesTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignRetriesTests.java @@ -44,7 +44,6 @@ import org.mockito.junit.MockitoJUnitRunner; import zipkin2.Span; import org.springframework.beans.factory.BeanFactory; -import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown; @@ -69,8 +68,7 @@ public class FeignRetriesTests { .addScopeDecorator(StrictScopeDecorator.create()).build()) .spanReporter(this.reporter).build(); - HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing) - .clientParser(SleuthHttpParserAccessor.getClient()).build(); + HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing).build(); @Before @After diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspectTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspectTests.java index bd193ea76..a5291a6c2 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspectTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspectTests.java @@ -31,7 +31,6 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.beans.factory.BeanFactory; -import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.never; @@ -60,8 +59,7 @@ public class TraceFeignAspectTests { .addScopeDecorator(StrictScopeDecorator.create()).build()) .build(); - HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing) - .clientParser(SleuthHttpParserAccessor.getClient()).build(); + HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing).build(); TraceFeignAspect traceFeignAspect; diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java index 53dc877cd..39a59c040 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClientTests.java @@ -37,8 +37,6 @@ import org.mockito.BDDMockito; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; -import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; - import static org.assertj.core.api.BDDAssertions.then; /** @@ -60,8 +58,7 @@ public class TracingFeignClientTests { Tracer tracer = this.tracing.tracer(); - HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing) - .clientParser(SleuthHttpParserAccessor.getClient()).build(); + HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing).build(); @Mock Client client; @@ -109,20 +106,4 @@ public class TracingFeignClientTests { then(this.spans.get(0).tags()).containsEntry("error", "exception has occurred"); } - @Test - public void should_shorten_the_span_name() throws IOException { - this.traceFeignClient.execute(Request.create("GET", "https://foo/" + bigName(), - new HashMap<>(), null, null), this.options); - - then(this.spans.get(0).name()).hasSize(50); - } - - private String bigName() { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < 60; i++) { - sb.append("a"); - } - return sb.toString(); - } - } 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 af6a3ec83..c7b2a033f 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 @@ -21,7 +21,6 @@ import java.util.List; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import brave.ErrorParser; import brave.Span; import brave.Tracer; import brave.Tracing; @@ -39,7 +38,6 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import org.springframework.cloud.netflix.zuul.metrics.EmptyTracerFactory; -import org.springframework.cloud.sleuth.instrument.web.SleuthHttpParserAccessor; import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; import static org.assertj.core.api.BDDAssertions.then; @@ -64,9 +62,7 @@ public class TracePostZuulFilterTests { .addScopeDecorator(StrictScopeDecorator.create()).build()) .spanReporter(this.reporter).build(); - HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing) - .clientParser(SleuthHttpParserAccessor.getClient()) - .serverParser(SleuthHttpParserAccessor.getServer(new ErrorParser())).build(); + HttpTracing httpTracing = HttpTracing.newBuilder(this.tracing).build(); RequestContext requestContext = new RequestContext(); diff --git a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-websocket/src/main/java/sample/SampleWebsocketApplication.java b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-websocket/src/main/java/sample/SampleWebsocketApplication.java index e671eb9f7..37f92fe0f 100644 --- a/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-websocket/src/main/java/sample/SampleWebsocketApplication.java +++ b/spring-cloud-sleuth-samples/spring-cloud-sleuth-sample-websocket/src/main/java/sample/SampleWebsocketApplication.java @@ -19,13 +19,13 @@ package sample; import org.springframework.boot.SpringApplication; import org.springframework.boot.autoconfigure.SpringBootApplication; import org.springframework.messaging.simp.config.MessageBrokerRegistry; -import org.springframework.web.socket.config.annotation.AbstractWebSocketMessageBrokerConfigurer; import org.springframework.web.socket.config.annotation.EnableWebSocketMessageBroker; import org.springframework.web.socket.config.annotation.StompEndpointRegistry; +import org.springframework.web.socket.config.annotation.WebSocketMessageBrokerConfigurer; @SpringBootApplication @EnableWebSocketMessageBroker -public class SampleWebsocketApplication extends AbstractWebSocketMessageBrokerConfigurer { +public class SampleWebsocketApplication implements WebSocketMessageBrokerConfigurer { public static void main(String[] args) { SpringApplication.run(SampleWebsocketApplication.class, args); diff --git a/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java b/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java index 3548b8215..4806f8a4c 100644 --- a/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-mvc-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceFilterWebIntegrationTests.java @@ -24,6 +24,7 @@ import java.util.stream.Collectors; import brave.Tracing; import brave.http.HttpRequest; +import brave.http.HttpRequestParser; import brave.sampler.Sampler; import brave.sampler.SamplerFunction; import org.assertj.core.api.BDDAssertions; @@ -67,9 +68,6 @@ public class TraceFilterWebIntegrationTests { @Rule public OutputCaptureRule capture = new OutputCaptureRule(); - @Autowired - Tracing tracer; - @Autowired ArrayListSpanReporter accumulator; @@ -86,6 +84,16 @@ public class TraceFilterWebIntegrationTests { this.accumulator.clear(); } + @Test + public void should_tag_url() { + new RestTemplate().getForObject("http://localhost:" + port() + "/good", + String.class); + + then(Tracing.current().tracer().currentSpan()).isNull(); + then(this.accumulator.getSpans()).hasSize(1); + then(this.accumulator.getSpans().get(0).tags()).containsKey("http.url"); + } + @Test public void should_not_create_a_span_for_error_controller() { try { @@ -148,7 +156,12 @@ public class TraceFilterWebIntegrationTests { public static class Config { @Bean - ExceptionThrowingController controller() { + GoodController goodController() { + return new GoodController(); + } + + @Bean + ExceptionThrowingController badController() { return new ExceptionThrowingController(); } @@ -162,6 +175,19 @@ public class TraceFilterWebIntegrationTests { return Sampler.ALWAYS_SAMPLE; } + // tag::custom_parser[] + @Bean(name = { HttpClientRequestParser.NAME, HttpServerRequestParser.NAME }) + HttpRequestParser sleuthHttpServerRequestParser() { + return (req, context, span) -> { + HttpRequestParser.DEFAULT.parse(req, context, span); + String url = req.url(); + if (url != null) { + span.tag("http.url", url); + } + }; + } + // end::custom_parser[] + // tag::custom_server_sampler[] @Bean(name = HttpServerSampler.NAME) SamplerFunction myHttpSampler(SkipPatternProvider provider) { @@ -190,6 +216,16 @@ public class TraceFilterWebIntegrationTests { } + @RestController + public static class GoodController { + + @RequestMapping("/good") + public String beGood() { + return "good"; + } + + } + @RestController public static class ExceptionThrowingController { From 27fe606c83f109db593c00bf921adb98ca2f0cf1 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 16:24:59 +0800 Subject: [PATCH 4/6] polish --- .../client/HttpClientBeanPostProcessor.java | 14 ++++++------ .../TraceWebClientBeanPostProcessor.java | 22 ++++++++++--------- ...FilterFunctionHttpClientResponseTests.java | 6 ++--- .../web/client/WebClientBraveTests.java | 11 +++++++--- 4 files changed, 30 insertions(+), 23 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java index 888377a63..45312ffa5 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessor.java @@ -152,7 +152,7 @@ class HttpClientBeanPostProcessor implements BeanPostProcessor { // Start a new client span with the appropriate parent TraceContext parent = req.currentContext().getOrDefault(TraceContext.class, null); - WrappedHttpClientRequest request = new WrappedHttpClientRequest(req); + HttpClientRequestWrapper request = new HttpClientRequestWrapper(req); clientSpan = handler().handleSendWithParent(request, parent); parseConnectionAddress(connection, clientSpan); @@ -240,18 +240,18 @@ class HttpClientBeanPostProcessor implements BeanPostProcessor { if (clientSpan == null) { return; // Unexpected. In the handle method, without a span to finish! } - WrappedHttpClientResponse response = resp != null - ? new WrappedHttpClientResponse(resp) : null; + HttpClientResponseWrapper response = resp != null + ? new HttpClientResponseWrapper(resp) : null; handler().handleReceive(response, error, clientSpan); } } - static final class WrappedHttpClientRequest extends brave.http.HttpClientRequest { + static final class HttpClientRequestWrapper extends brave.http.HttpClientRequest { final HttpClientRequest delegate; - WrappedHttpClientRequest(HttpClientRequest delegate) { + HttpClientRequestWrapper(HttpClientRequest delegate) { this.delegate = delegate; } @@ -287,11 +287,11 @@ class HttpClientBeanPostProcessor implements BeanPostProcessor { } - static final class WrappedHttpClientResponse extends brave.http.HttpClientResponse { + static final class HttpClientResponseWrapper extends brave.http.HttpClientResponse { final HttpClientResponse delegate; - WrappedHttpClientResponse(HttpClientResponse delegate) { + HttpClientResponseWrapper(HttpClientResponse delegate) { this.delegate = delegate; } diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java index 97faff492..600c35fe5 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessor.java @@ -23,6 +23,8 @@ import java.util.function.Function; import brave.Span; import brave.http.HttpClientHandler; +import brave.http.HttpClientRequest; +import brave.http.HttpClientResponse; import brave.http.HttpTracing; import brave.propagation.CurrentTraceContext; import brave.propagation.CurrentTraceContext.Scope; @@ -122,7 +124,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { final Function, ? extends Publisher> scopePassingTransformer; // Lazy initialized fields - HttpClientHandler handler; + HttpClientHandler handler; CurrentTraceContext currentTraceContext; @@ -148,7 +150,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { return this.currentTraceContext; } - HttpClientHandler handler() { + HttpClientHandler handler() { if (this.handler == null) { this.handler = HttpClientHandler.create(this.httpTracing.get()); } @@ -161,7 +163,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { final ClientRequest request; - final HttpClientHandler handler; + final HttpClientHandler handler; final CurrentTraceContext currentTraceContext; @@ -185,7 +187,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { Context context = subscriber.currentContext(); - HttpClientRequest wrapper = new HttpClientRequest(request); + ClientRequestWrapper wrapper = new ClientRequestWrapper(request); Span span = handler.handleSendWithParent(wrapper, parent); if (log.isDebugEnabled()) { log.debug("HttpClientHandler::handleSend: " + span); @@ -209,7 +211,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { final Span clientSpan; - final HttpClientHandler handler; + final HttpClientHandler handler; final Function, ? extends Publisher> scopePassingTransformer; @@ -308,20 +310,20 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { } void handleReceive(@Nullable ClientResponse res, @Nullable Throwable error) { - HttpClientResponse response = res != null ? new HttpClientResponse(res) + ClientResponseWrapper response = res != null ? new ClientResponseWrapper(res) : null; this.handler.handleReceive(response, error, clientSpan); } } - private static final class HttpClientRequest extends brave.http.HttpClientRequest { + private static final class ClientRequestWrapper extends HttpClientRequest { final ClientRequest delegate; final ClientRequest.Builder builder; - HttpClientRequest(ClientRequest delegate) { + ClientRequestWrapper(ClientRequest delegate) { this.delegate = delegate; this.builder = ClientRequest.from(delegate); } @@ -362,11 +364,11 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { } - static final class HttpClientResponse extends brave.http.HttpClientResponse { + static final class ClientResponseWrapper extends HttpClientResponse { final ClientResponse delegate; - HttpClientResponse(ClientResponse delegate) { + ClientResponseWrapper(ClientResponse delegate) { this.delegate = delegate; } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java index b194cf6ab..e615af73d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceExchangeFilterFunctionHttpClientResponseTests.java @@ -20,7 +20,7 @@ import org.assertj.core.api.BDDAssertions; import org.junit.Test; import org.mockito.BDDMockito; -import org.springframework.cloud.sleuth.instrument.web.client.TraceExchangeFilterFunction.HttpClientResponse; +import org.springframework.cloud.sleuth.instrument.web.client.TraceExchangeFilterFunction.ClientResponseWrapper; import org.springframework.web.reactive.function.client.ClientResponse; public class TraceExchangeFilterFunctionHttpClientResponseTests { @@ -29,7 +29,7 @@ public class TraceExchangeFilterFunctionHttpClientResponseTests { public void should_return_0_when_invalid_status_code_is_returned() { ClientResponse clientResponse = BDDMockito.mock(ClientResponse.class); BDDMockito.given(clientResponse.rawStatusCode()).willReturn(-1); - HttpClientResponse response = new HttpClientResponse(clientResponse); + ClientResponseWrapper response = new ClientResponseWrapper(clientResponse); Integer statusCode = response.statusCode(); @@ -40,7 +40,7 @@ public class TraceExchangeFilterFunctionHttpClientResponseTests { public void should_return_status_code_when_valid_status_code_is_returned() { ClientResponse clientResponse = BDDMockito.mock(ClientResponse.class); BDDMockito.given(clientResponse.rawStatusCode()).willReturn(200); - HttpClientResponse response = new HttpClientResponse(clientResponse); + ClientResponseWrapper response = new ClientResponseWrapper(clientResponse); Integer statusCode = response.statusCode(); diff --git a/tests/spring-cloud-sleuth-instrumentation-reactor-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientBraveTests.java b/tests/spring-cloud-sleuth-instrumentation-reactor-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientBraveTests.java index d5859a3d5..e2b6833ed 100644 --- a/tests/spring-cloud-sleuth-instrumentation-reactor-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientBraveTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-reactor-tests/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/WebClientBraveTests.java @@ -28,6 +28,7 @@ import org.springframework.beans.factory.config.BeanPostProcessor; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.http.client.reactive.ClientHttpConnector; import org.springframework.http.client.reactive.ReactorClientHttpConnector; import org.springframework.web.reactive.function.BodyInserters; import org.springframework.web.reactive.function.client.ClientResponse; @@ -118,9 +119,13 @@ public class WebClientBraveTests } @Bean - WebClient.Builder webClientBuilder(HttpClient httpClient) { - return WebClient.builder() - .clientConnector(new ReactorClientHttpConnector(httpClient)); + ClientHttpConnector clientHttpConnector(HttpClient httpClient) { + return new ReactorClientHttpConnector(httpClient); + } + + @Bean + WebClient.Builder webClientBuilder(ClientHttpConnector clientHttpConnector) { + return WebClient.builder().clientConnector(clientHttpConnector); } } From 7d053beed91398627584253cb6e93ca10de6c204 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 18:30:26 +0800 Subject: [PATCH 5/6] Attempts to align feign code with master (#1577) This tries to pick in the relevant changes made in 3813cf9dd47f98db0e075412abbffbd9d6742974 Notably, there's one glitch, `TraceFeignAspect` still passes bean, not wrappedBean. The latter trips out `ManuallyCreatedLoadBalancerFeignClientTests` as the nonexistenturl raises a hard error in Ribbon. I *think* this is a bug and tests need to just adjust for that, but need a second opinion. Also, I'm not entirely sure the intent of using the broken url then asserting against a success result.. --- .../instrument/web/client/feign/TraceFeignAspect.java | 2 ++ .../web/client/feign/TraceFeignObjectWrapper.java | 5 +++-- .../instrument/web/client/feign/TracingFeignClient.java | 7 +++++-- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspect.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspect.java index ba1044917..5f3467a02 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspect.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspect.java @@ -53,6 +53,8 @@ class TraceFeignAspect { log.debug("Executing feign client via TraceFeignAspect"); } if (bean != wrappedBean) { + // NOTE: in master(3813cf9dd47f98db0e075412abbffbd9d6742974), + // this is executeTraceFeignClient(wrappedBean, pjp) return executeTraceFeignClient(bean, pjp); } return pjp.proceed(); diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java index a5c3c15f1..11a54e745 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignObjectWrapper.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2019 the original author or authors. + * Copyright 2013-2020 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. @@ -29,6 +29,7 @@ import org.springframework.cloud.netflix.ribbon.SpringClientFactory; import org.springframework.cloud.openfeign.loadbalancer.FeignBlockingLoadBalancerClient; import org.springframework.cloud.openfeign.ribbon.CachingSpringLoadBalancerFactory; import org.springframework.cloud.openfeign.ribbon.LoadBalancerFeignClient; +import org.springframework.cloud.util.ProxyUtils; import org.springframework.util.ClassUtils; /** @@ -100,7 +101,7 @@ final class TraceFeignObjectWrapper { private Object instrumentedFeignLoadBalancerClient(Object bean) { if (AopUtils.getTargetClass(bean).equals(FeignBlockingLoadBalancerClient.class)) { - FeignBlockingLoadBalancerClient client = ((FeignBlockingLoadBalancerClient) bean); + FeignBlockingLoadBalancerClient client = ProxyUtils.getTargetObject(bean); return new TraceFeignBlockingLoadBalancerClient( (Client) new TraceFeignObjectWrapper(this.beanFactory) .wrap(client.getDelegate()), diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java index a551829fb..ac6d69442 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TracingFeignClient.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2019 the original author or authors. + * Copyright 2013-2020 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. @@ -37,6 +37,7 @@ import feign.Response; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.springframework.cloud.util.ProxyUtils; import org.springframework.lang.Nullable; /** @@ -58,7 +59,9 @@ final class TracingFeignClient implements Client { TracingFeignClient(HttpTracing httpTracing, Client delegate) { this.currentTraceContext = httpTracing.tracing().currentTraceContext(); this.handler = HttpClientHandler.create(httpTracing); - this.delegate = delegate; + Client delegateTarget = ProxyUtils.getTargetObject(delegate); + this.delegate = delegateTarget instanceof TracingFeignClient + ? ((TracingFeignClient) delegateTarget).delegate : delegateTarget; } static Client create(HttpTracing httpTracing, Client delegate) { From 2024e2893738de5b591d462f262418092c128e64 Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Tue, 3 Mar 2020 16:22:07 +0100 Subject: [PATCH 6/6] Added example with Stream Bridge --- pom.xml | 2 +- .../pom.xml | 12 ++ ...extPropagationChannelInterceptorTests.java | 10 +- .../TraceStreamChannelInterceptorTests.java | 123 ++++++++++++++++++ .../issue_943/HelloSpringIntegration.java | 4 +- 5 files changed, 144 insertions(+), 7 deletions(-) create mode 100644 tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceStreamChannelInterceptorTests.java diff --git a/pom.xml b/pom.xml index 40875eaf2..bf45ea181 100644 --- a/pom.xml +++ b/pom.xml @@ -254,7 +254,7 @@ 2.2.2.BUILD-SNAPSHOT 2.2.2.BUILD-SNAPSHOT 1.0.1.BUILD-SNAPSHOT - Horsham.SR1 + Horsham.BUILD-SNAPSHOT 2.2.2.BUILD-SNAPSHOT 2.2.2.BUILD-SNAPSHOT 5.10.1 diff --git a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/pom.xml b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/pom.xml index 7560caebb..9133fe704 100644 --- a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/pom.xml +++ b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/pom.xml @@ -60,6 +60,18 @@ https://www.w3.org/2001/XMLSchema-instance "> org.springframework.cloud spring-cloud-starter-sleuth + + org.springframework.cloud + spring-cloud-stream + jar + + + org.springframework.cloud + spring-cloud-stream + test-jar + test + test-binder + org.springframework.boot spring-boot-starter-test diff --git a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceContextPropagationChannelInterceptorTests.java b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceContextPropagationChannelInterceptorTests.java index ad823b785..52e85490d 100644 --- a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceContextPropagationChannelInterceptorTests.java +++ b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceContextPropagationChannelInterceptorTests.java @@ -37,14 +37,14 @@ import org.springframework.integration.support.MessageBuilder; import org.springframework.messaging.Message; import org.springframework.messaging.PollableChannel; import org.springframework.test.annotation.DirtiesContext; -import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.test.context.junit4.SpringRunner; import static org.assertj.core.api.Assertions.assertThat; /** * @author Spencer Gibb */ -@RunWith(SpringJUnit4ClassRunner.class) +@RunWith(SpringRunner.class) @SpringBootTest(classes = TraceContextPropagationChannelInterceptorTests.App.class) @DirtiesContext public class TraceContextPropagationChannelInterceptorTests { @@ -68,6 +68,7 @@ public class TraceContextPropagationChannelInterceptorTests { public void testSpanPropagation() { Span span = this.tracing.tracer().nextSpan().name("http:testSendMessage").start(); String expectedSpanId = SpanUtil.idToHex(span.context().spanId()); + try (Tracer.SpanInScope ws = this.tracing.tracer().withSpanInScope(span)) { this.channel.send(MessageBuilder.withPayload("hi").build()); } @@ -75,6 +76,10 @@ public class TraceContextPropagationChannelInterceptorTests { span.finish(); } + assertThatNewSpanIdWasSetOnMessage(expectedSpanId); + } + + private void assertThatNewSpanIdWasSetOnMessage(String expectedSpanId) { Message message = this.channel.receive(0); assertThat(message).as("message was null").isNotNull(); @@ -91,7 +96,6 @@ public class TraceContextPropagationChannelInterceptorTests { String.class); assertThat(parentId).as("parentId was not equal to parent's id") .isEqualTo(this.reporter.getSpans().get(0).id()); - } @Configuration diff --git a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceStreamChannelInterceptorTests.java b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceStreamChannelInterceptorTests.java new file mode 100644 index 000000000..dc126fe9f --- /dev/null +++ b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/TraceStreamChannelInterceptorTests.java @@ -0,0 +1,123 @@ +/* + * Copyright 2013-2019 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 + * + * https://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 brave.Span; +import brave.Tracer; +import brave.Tracing; +import brave.sampler.Sampler; +import org.junit.After; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.autoconfigure.ImportAutoConfiguration; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.cloud.sleuth.instrument.util.SpanUtil; +import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; +import org.springframework.cloud.stream.binder.test.OutputDestination; +import org.springframework.cloud.stream.binder.test.TestChannelBinderConfiguration; +import org.springframework.cloud.stream.function.StreamBridge; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.messaging.Message; +import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.junit4.SpringRunner; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Spencer Gibb + */ +@RunWith(SpringRunner.class) +@SpringBootTest(classes = TraceStreamChannelInterceptorTests.App.class, + properties = "spring.cloud.stream.source=testSupplier") +@DirtiesContext +public class TraceStreamChannelInterceptorTests { + + @Autowired + private OutputDestination channel; + + @Autowired + private Tracing tracing; + + @Autowired + private StreamBridge streamBridge; + + @Autowired + private ArrayListSpanReporter reporter; + + @After + public void close() { + this.reporter.clear(); + } + + @Test + public void testSpanPropagationViaBridge() { + Span span = this.tracing.tracer().nextSpan().name("http:testSendMessage").start(); + String expectedSpanId = SpanUtil.idToHex(span.context().spanId()); + + try (Tracer.SpanInScope ws = this.tracing.tracer().withSpanInScope(span)) { + this.streamBridge.send("testSupplier-out-0", "hi"); + } + finally { + span.finish(); + } + + assertThatNewSpanIdWasSetOnMessage(expectedSpanId); + } + + private void assertThatNewSpanIdWasSetOnMessage(String expectedSpanId) { + Message message = this.channel.receive(0); + assertThat(message).as("message was null").isNotNull(); + + String spanId = message.getHeaders().get(TraceMessageHeaders.SPAN_ID_NAME, + String.class); + assertThat(spanId).as("spanId was equal to parent's id") + .isNotEqualTo(expectedSpanId); + + String traceId = message.getHeaders().get(TraceMessageHeaders.TRACE_ID_NAME, + String.class); + assertThat(traceId).as("traceId was null").isNotNull(); + + String parentId = message.getHeaders().get(TraceMessageHeaders.PARENT_ID_NAME, + String.class); + // [0] - producer + // [1] - http:testsendmessage + assertThat(parentId).as("parentId was not equal to parent's id") + .isEqualTo(this.reporter.getSpans().get(1).id()); + } + + @Configuration + @EnableAutoConfiguration + @ImportAutoConfiguration(TestChannelBinderConfiguration.class) + static class App { + + @Bean + Sampler testSampler() { + return Sampler.ALWAYS_SAMPLE; + } + + @Bean + ArrayListSpanReporter reporter() { + return new ArrayListSpanReporter(); + } + + } + +} diff --git a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/issues/issue_943/HelloSpringIntegration.java b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/issues/issue_943/HelloSpringIntegration.java index 20fef6839..32b80db76 100644 --- a/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/issues/issue_943/HelloSpringIntegration.java +++ b/tests/spring-cloud-sleuth-instrumentation-messaging-tests/src/test/java/org/springframework/cloud/sleuth/instrument/messaging/issues/issue_943/HelloSpringIntegration.java @@ -19,7 +19,6 @@ package org.springframework.cloud.sleuth.instrument.messaging.issues.issue_943; import brave.sampler.Sampler; import org.springframework.boot.SpringApplication; -import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.autoconfigure.SpringBootApplication; import org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration; import org.springframework.boot.autoconfigure.orm.jpa.HibernateJpaAutoConfiguration; @@ -30,8 +29,7 @@ import org.springframework.integration.config.EnableIntegration; import org.springframework.scheduling.annotation.EnableAsync; import org.springframework.web.client.RestTemplate; -@SpringBootApplication -@EnableAutoConfiguration(exclude = { DataSourceAutoConfiguration.class, +@SpringBootApplication(exclude = { DataSourceAutoConfiguration.class, HibernateJpaAutoConfiguration.class }) @ImportResource("classpath:beans/applicationContext.xml") @EnableIntegration