From 656c4720cbaed4f2c0514cfb081463324314ecdb Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 6 Mar 2020 10:43:54 +0800 Subject: [PATCH 1/7] Switches to a less flakey span reporter (#1579) --- .../util/BlockingQueueSpanReporter.java | 74 +++++++++++++++++++ .../util/BlockingQueueSpanReporterTests.java | 55 ++++++++++++++ .../web/TraceFilterWebIntegrationTests.java | 56 ++++++-------- 3 files changed, 151 insertions(+), 34 deletions(-) create mode 100644 spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporter.java create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporterTests.java diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporter.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporter.java new file mode 100644 index 000000000..eff2686dc --- /dev/null +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporter.java @@ -0,0 +1,74 @@ +/* + * 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. + * 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.util; + +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.TimeUnit; + +import zipkin2.Span; +import zipkin2.reporter.Reporter; + +/** + * Like {@link ArrayListSpanReporter}, except appropriate for async instrumentation. + */ +public class BlockingQueueSpanReporter implements Reporter { + + private final LinkedBlockingQueue spans = new LinkedBlockingQueue<>(); + + /** + * Blocks until a span is reported or throws an {@link AssertionError}. + * @return the first span not yet taken. + */ + public Span takeSpan() { + Span result = takeSpan(3_000); + if (result == null) { + throw new AssertionError("Span was not reported"); + } + return result; + } + + @Override + public String toString() { + return "BlockingQueueSpanReporter{spans=" + spans + '}'; + } + + @Override + public void report(Span span) { + spans.add(span); + } + + /** Use this as a post-condition to ensure all spans are accounted for. */ + public void assertEmpty() { + if (takeSpan(100) != null) { + throw new AssertionError( + "Span remaining in queue. Check for redundant reporting!"); + } + } + + private Span takeSpan(long timeout) { + Span result; + try { + result = spans.poll(timeout, TimeUnit.MILLISECONDS); + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new AssertionError(e); + } + return result; + } + +} diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporterTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporterTests.java new file mode 100644 index 000000000..190388a29 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/util/BlockingQueueSpanReporterTests.java @@ -0,0 +1,55 @@ +/* + * 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. + * 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.util; + +import org.junit.jupiter.api.Test; +import zipkin2.Span; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +public class BlockingQueueSpanReporterTests { + + Span span1 = Span.newBuilder().traceId("1").id("1").build(); + + Span span2 = Span.newBuilder().traceId("1").id("2").build(); + + BlockingQueueSpanReporter reporter = new BlockingQueueSpanReporter(); + + @Test + void takeSpan_fifo_order() { + reporter.report(span1); + reporter.report(span2); + + assertThat(reporter.takeSpan()).isSameAs(span1); + assertThat(reporter.takeSpan()).isSameAs(span2); + } + + @Test + void assertEmpty() { + reporter.assertEmpty(); + } + + @Test + void assertEmpty_fails_when_not_empty() { + reporter.report(span1); + + assertThatThrownBy(reporter::assertEmpty).isInstanceOf(AssertionError.class) + .hasMessage("Span remaining in queue. Check for redundant reporting!"); + } + +} 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 4806f8a4c..3a8afcb6f 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 @@ -28,24 +28,23 @@ import brave.http.HttpRequestParser; import brave.sampler.Sampler; import brave.sampler.SamplerFunction; import org.assertj.core.api.BDDAssertions; -import org.junit.After; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import zipkin2.Span; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.EnableAutoConfiguration; import org.springframework.boot.test.context.SpringBootTest; -import org.springframework.boot.test.system.OutputCaptureRule; -import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; +import org.springframework.boot.test.system.CapturedOutput; +import org.springframework.boot.test.system.OutputCaptureExtension; +import org.springframework.cloud.sleuth.util.BlockingQueueSpanReporter; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.env.Environment; import org.springframework.http.ResponseEntity; import org.springframework.http.client.ClientHttpResponse; -import org.springframework.test.context.junit4.SpringRunner; +import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RestController; @@ -59,17 +58,13 @@ import static org.assertj.core.api.BDDAssertions.then; /** * @author Marcin Grzejszczak */ -@RunWith(SpringRunner.class) +@ExtendWith({SpringExtension.class, OutputCaptureExtension.class}) @SpringBootTest(classes = TraceFilterWebIntegrationTests.Config.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, properties = "spring.sleuth.http.legacy.enabled=true") public class TraceFilterWebIntegrationTests { - - @Rule - public OutputCaptureRule capture = new OutputCaptureRule(); - @Autowired - ArrayListSpanReporter accumulator; + BlockingQueueSpanReporter reporter; @Autowired @HttpServerSampler @@ -78,10 +73,9 @@ public class TraceFilterWebIntegrationTests { @Autowired Environment environment; - @Before - @After + @AfterEach public void cleanup() { - this.accumulator.clear(); + this.reporter.assertEmpty(); } @Test @@ -90,12 +84,11 @@ public class TraceFilterWebIntegrationTests { String.class); then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.accumulator.getSpans()).hasSize(1); - then(this.accumulator.getSpans().get(0).tags()).containsKey("http.url"); + then(this.reporter.takeSpan().tags()).containsKey("http.url"); } @Test - public void should_not_create_a_span_for_error_controller() { + public void should_not_create_a_span_for_error_controller(CapturedOutput capture) { try { new RestTemplate().getForObject("http://localhost:" + port() + "/", String.class); @@ -105,16 +98,14 @@ public class TraceFilterWebIntegrationTests { } then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.accumulator.getSpans()).hasSize(1); - Span fromFirstTraceFilterFlow = this.accumulator.getSpans().get(0); - then(fromFirstTraceFilterFlow.tags()).containsEntry("http.status_code", "500") - .containsEntry("http.method", "GET") + Span fromFirstTraceFilterFlow = this.reporter.takeSpan(); + then(fromFirstTraceFilterFlow.tags()).containsEntry("http.method", "GET") .containsEntry("mvc.controller.class", "ExceptionThrowingController") .containsEntry("error", "Request processing failed; nested exception is java.lang.RuntimeException: Throwing exception"); // issue#714 String hex = fromFirstTraceFilterFlow.traceId(); - String[] split = this.capture.toString().split("\n"); + String[] split = capture.toString().split("\n"); List list = Arrays.stream(split) .filter(s -> s.contains("Uncaught exception thrown")) .filter(s -> s.contains(hex + "," + hex + ",true]")) @@ -133,13 +124,10 @@ public class TraceFilterWebIntegrationTests { } then(Tracing.current().tracer().currentSpan()).isNull(); - then(this.accumulator.getSpans()).hasSize(1); - then(this.accumulator.getSpans().get(0).kind().ordinal()) - .isEqualTo(Span.Kind.SERVER.ordinal()); - then(this.accumulator.getSpans().get(0).tags()).containsEntry("http.status_code", - "400"); - then(this.accumulator.getSpans().get(0).tags()).containsEntry("http.path", - "/test_bad_request"); + Span span = this.reporter.takeSpan(); + then(span.kind().ordinal()).isEqualTo(Span.Kind.SERVER.ordinal()); + then(span.tags()).containsEntry("http.status_code", "400"); + then(span.tags()).containsEntry("http.path", "/test_bad_request"); } @Test @@ -166,8 +154,8 @@ public class TraceFilterWebIntegrationTests { } @Bean - ArrayListSpanReporter reporter() { - return new ArrayListSpanReporter(); + BlockingQueueSpanReporter reporter() { + return new BlockingQueueSpanReporter(); } @Bean From dd13f098443de5634a7ed2263c7b46801c182a9a Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 6 Mar 2020 11:07:00 +0800 Subject: [PATCH 2/7] Adds first unit test for HttpClientBeanPostProcessor (#1580) --- .../HttpClientBeanPostProcessorTest.java | 72 +++++++++++++++++++ .../web/TraceFilterWebIntegrationTests.java | 3 +- 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessorTest.java diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessorTest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessorTest.java new file mode 100644 index 000000000..b7c7de7fb --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/HttpClientBeanPostProcessorTest.java @@ -0,0 +1,72 @@ +/* + * 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.client; + +import brave.propagation.TraceContext; +import io.netty.bootstrap.Bootstrap; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import reactor.core.publisher.Mono; +import reactor.netty.Connection; + +import org.springframework.cloud.sleuth.instrument.web.client.HttpClientBeanPostProcessor.PendingSpan; +import org.springframework.cloud.sleuth.instrument.web.client.HttpClientBeanPostProcessor.TracingMapConnect; + +import static org.assertj.core.api.Assertions.assertThat; + +@ExtendWith(MockitoExtension.class) +public class HttpClientBeanPostProcessorTest { + + @Mock + Connection connection; + + @Mock + Bootstrap bootstrap; + + TraceContext traceContext = TraceContext.newBuilder().traceId(1).spanId(2) + .sampled(true).build(); + + @Test + void mapConnect_should_setup_reactor_context_currentTraceContext() { + TracingMapConnect tracingMapConnect = new TracingMapConnect(() -> traceContext); + + Mono original = Mono.just(connection).handle((t, ctx) -> { + assertThat(ctx.currentContext().get(TraceContext.class)) + .isSameAs(traceContext); + assertThat(ctx.currentContext().get(PendingSpan.class)).isNotNull(); + }); + + // Wrap and run the assertions + tracingMapConnect.apply(original, bootstrap).log().subscribe(); + } + + @Test + void mapConnect_should_setup_reactor_context_no_currentTraceContext() { + TracingMapConnect tracingMapConnect = new TracingMapConnect(() -> null); + + Mono original = Mono.just(connection).handle((t, ctx) -> { + assertThat(ctx.currentContext().getOrEmpty(TraceContext.class)).isEmpty(); + assertThat(ctx.currentContext().get(PendingSpan.class)).isNotNull(); + }); + + // Wrap and run the assertions + tracingMapConnect.apply(original, bootstrap).log().subscribe(); + } + +} 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 3a8afcb6f..4831fa6bc 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 @@ -58,11 +58,12 @@ import static org.assertj.core.api.BDDAssertions.then; /** * @author Marcin Grzejszczak */ -@ExtendWith({SpringExtension.class, OutputCaptureExtension.class}) +@ExtendWith({ SpringExtension.class, OutputCaptureExtension.class }) @SpringBootTest(classes = TraceFilterWebIntegrationTests.Config.class, webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, properties = "spring.sleuth.http.legacy.enabled=true") public class TraceFilterWebIntegrationTests { + @Autowired BlockingQueueSpanReporter reporter; From e5308d711175ddaaa95af7a56e1a504ce46157ff Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Fri, 6 Mar 2020 11:45:32 +0800 Subject: [PATCH 3/7] Tracks requested but without signal interference (#1581) --- .../client/TraceWebClientBeanPostProcessor.java | 13 ++++++------- .../TraceWebClientBeanPostProcessorTest.java | 16 ++++++++-------- 2 files changed, 14 insertions(+), 15 deletions(-) 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 924976843..ce95437d0 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 @@ -18,7 +18,6 @@ package org.springframework.cloud.sleuth.instrument.web.client; import java.util.List; import java.util.concurrent.CancellationException; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Function; @@ -290,8 +289,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { } - static class TraceWebClientSubscription extends AtomicBoolean - implements Subscription { + static class TraceWebClientSubscription implements Subscription { static final Exception CANCELLED_ERROR = new CancellationException("CANCELLED") { @Override @@ -304,6 +302,8 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { final Subscription delegate; + volatile boolean requested; + TraceWebClientSubscription(Subscription delegate, AtomicReference pendingSpan) { this.delegate = delegate; @@ -312,9 +312,8 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { @Override public void request(long n) { - if (compareAndSet(false, true)) { - delegate.request(n); // Not scoping to save overhead - } + requested = true; + delegate.request(n); // Not scoping to save overhead } @Override @@ -331,7 +330,7 @@ final class TraceExchangeFilterFunction implements ExchangeFilterFunction { + span + "]"); } - if (!get()) { // Subscription.request() not called: Abandon the span. + if (!requested) { // Abandon the span. span.abandon(); } else { // Request was canceled in-flight diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessorTest.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessorTest.java index 2f67fbad8..d99b7f6c2 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessorTest.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/TraceWebClientBeanPostProcessorTest.java @@ -20,10 +20,10 @@ import java.util.concurrent.atomic.AtomicReference; import brave.Span; import org.assertj.core.api.BDDAssertions; -import org.junit.Test; -import org.junit.runner.RunWith; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.junit.jupiter.MockitoExtension; import org.reactivestreams.Subscription; import org.springframework.cloud.sleuth.instrument.web.client.TraceExchangeFilterFunction.TraceWebClientSubscription; @@ -36,7 +36,7 @@ import static org.mockito.Mockito.verify; /** * @author Marcin Grzejszczak */ -@RunWith(MockitoJUnitRunner.class) +@ExtendWith(MockitoExtension.class) public class TraceWebClientBeanPostProcessorTest { @Mock @@ -49,7 +49,7 @@ public class TraceWebClientBeanPostProcessorTest { Span span; @Test - public void should_add_filter_only_once_to_web_client() { + void should_add_filter_only_once_to_web_client() { TraceWebClientBeanPostProcessor processor = new TraceWebClientBeanPostProcessor( this.springContext); WebClient client = WebClient.create(); @@ -65,7 +65,7 @@ public class TraceWebClientBeanPostProcessorTest { } @Test - public void should_add_filter_only_once_to_web_client_via_builder() { + void should_add_filter_only_once_to_web_client_via_builder() { TraceWebClientBeanPostProcessor processor = new TraceWebClientBeanPostProcessor( this.springContext); WebClient.Builder builder = WebClient.builder(); @@ -83,7 +83,7 @@ public class TraceWebClientBeanPostProcessorTest { } @Test - public void should_close_span_on_cancel() { + void should_close_span_on_cancel() { TraceWebClientSubscription traceSubscription = new TraceWebClientSubscription( subscription, new AtomicReference<>(span)); @@ -98,7 +98,7 @@ public class TraceWebClientBeanPostProcessorTest { } @Test - public void should_not_crash_on_cancel_when_span_clear() { + void should_not_crash_on_cancel_when_span_clear() { TraceWebClientSubscription traceSubscription = new TraceWebClientSubscription( subscription, new AtomicReference<>()); From 08f853e3868afac88da9d9c66ec48bfe78889e44 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Fri, 6 Mar 2020 12:58:21 -0500 Subject: [PATCH 4/7] Updates docs branches --- docs/pom.xml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/pom.xml b/docs/pom.xml index 8484894e7..87c96433c 100644 --- a/docs/pom.xml +++ b/docs/pom.xml @@ -31,8 +31,7 @@ spring-cloud-sleuth - 1.0.x,1.1.x,1.2.x,1.3.x,2.0.x,2.1.x - + 2.1.x,2.2.x ${basedir}/.. spring.sleuth.*|spring.zipkin.* From 0d79f74df5773202d3d9d400968a1c435650e621 Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Wed, 18 Mar 2020 18:46:02 -0400 Subject: [PATCH 5/7] bumps build to 2.2.4.BUILD-SNAPSHOT --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 52def603e..73d9821d2 100644 --- a/pom.xml +++ b/pom.xml @@ -29,7 +29,7 @@ org.springframework.cloud spring-cloud-build - 2.2.3.RELEASE + 2.2.4.BUILD-SNAPSHOT From 9620ed86858a7375f0ac002ed66684f7b33029fa Mon Sep 17 00:00:00 2001 From: Spencer Gibb Date: Thu, 19 Mar 2020 12:08:31 -0400 Subject: [PATCH 6/7] bumps build to 2.3.0.BUILD-SNAPSHOT --- pom.xml | 2 +- spring-cloud-sleuth-dependencies/pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pom.xml b/pom.xml index 73d9821d2..9d03785f1 100644 --- a/pom.xml +++ b/pom.xml @@ -29,7 +29,7 @@ org.springframework.cloud spring-cloud-build - 2.2.4.BUILD-SNAPSHOT + 2.3.0.BUILD-SNAPSHOT diff --git a/spring-cloud-sleuth-dependencies/pom.xml b/spring-cloud-sleuth-dependencies/pom.xml index b99c3ba6b..a995370e0 100644 --- a/spring-cloud-sleuth-dependencies/pom.xml +++ b/spring-cloud-sleuth-dependencies/pom.xml @@ -22,7 +22,7 @@ spring-cloud-dependencies-parent org.springframework.cloud - 2.2.4.BUILD-SNAPSHOT + 2.3.0.BUILD-SNAPSHOT spring-cloud-sleuth-dependencies From 64d2355cc4c208f8890967a6fe67d9d7c5d85f7c Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Thu, 26 Mar 2020 09:46:42 +0100 Subject: [PATCH 7/7] Not tagging OK http responses with error tag; fixes gh-1590 --- .../web/SleuthHttpServerParser.java | 2 +- .../web/SleuthHttpServerParserTests.java | 95 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParserTests.java 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 417fe01a9..6f9f5b276 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 @@ -47,7 +47,7 @@ class SleuthHttpServerParser extends SleuthHttpClientParser return; // already parsed the error } - if (httpStatus == HttpServletResponse.SC_OK && response.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. span.tag(STATUS_CODE_KEY, diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParserTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParserTests.java new file mode 100644 index 000000000..6f9259ca9 --- /dev/null +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SleuthHttpServerParserTests.java @@ -0,0 +1,95 @@ +/* + * 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.Span; +import brave.Tracing; +import brave.http.HttpResponse; +import brave.sampler.Sampler; +import org.assertj.core.api.BDDAssertions; +import org.junit.Test; + +import org.springframework.cloud.sleuth.util.ArrayListSpanReporter; + +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; + +public class SleuthHttpServerParserTests { + + ArrayListSpanReporter reporter = new ArrayListSpanReporter(); + + @Test + public void should_tag_span_with_error_when_response_has_error_and_status_is_ok() { + HttpResponse response = errorResponseWithOkStatus(); + + whenSpanGetsParsed(response, newSpan()); + + thenReportedSpanContainsErrorTag(); + } + + @Test + public void should_not_tag_span_with_error_when_response_has_no_error_and_status_is_ok() { + HttpResponse response = responseWithOkStatus(); + + whenSpanGetsParsed(response, newSpan()); + + thenReportedSpanDoesNotContainErrorTag(); + } + + private void whenSpanGetsParsed(HttpResponse response, Span span) { + try { + new SleuthHttpServerParser(null).parse(response, null, span); + } + finally { + span.finish(); + } + } + + private void thenReportedSpanContainsErrorTag() { + BDDAssertions + .then(this.reporter.getSpans().stream() + .flatMap(s -> s.tags().keySet().stream())) + .contains("http.status_code"); + } + + private void thenReportedSpanDoesNotContainErrorTag() { + BDDAssertions + .then(this.reporter.getSpans().stream() + .flatMap(s -> s.tags().keySet().stream())) + .doesNotContain("http.status_code"); + } + + private HttpResponse errorResponseWithOkStatus() { + HttpResponse response = mock(HttpResponse.class); + given(response.statusCode()).willReturn(200); + given(response.error()).willReturn(new RuntimeException("hello")); + return response; + } + + private HttpResponse responseWithOkStatus() { + HttpResponse response = mock(HttpResponse.class); + given(response.statusCode()).willReturn(200); + return response; + } + + private Span newSpan() { + return Tracing.newBuilder().spanReporter(this.reporter) + .sampler(Sampler.ALWAYS_SAMPLE).build().tracer().nextSpan().name("span") + .start(); + } + +}