From eec55b9a6c153a84b1b3f19a7fd1cbe7edd0de55 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Mon, 2 Mar 2020 09:12:47 +0800 Subject: [PATCH] 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 {