Fixes bugs in skip pattern code and Feign tests (#1598)

Before, the client sampler would only skip the path named ""! This was
the path used in one of the failing Feign tests, which made
troubleshooting like a murder mystery.

The "" is a regression in spring-cloud-openfeign noted below:
https://github.com/spring-cloud/spring-cloud-openfeign/pull/245/files#r40342437

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternSampler.java

@@ -27,18 +27,12 @@ import brave.sampler.SamplerFunction;
  * @author Marcin Grzejszczak
  * @since 2.0.0
  */
-class SkipPatternHttpServerSampler implements SamplerFunction<HttpRequest> {
-
-	private final SkipPatternProvider provider;
+abstract class SkipPatternSampler implements SamplerFunction<HttpRequest> {
 
 	private Pattern pattern;
 
-	SkipPatternHttpServerSampler(SkipPatternProvider provider) {
-		this.provider = provider;
-	}
-
 	@Override
-	public Boolean trySample(HttpRequest request) {
+	public final Boolean trySample(HttpRequest request) {
 		String url = request.path();
 		boolean shouldSkip = pattern().matcher(url).matches();
 		if (shouldSkip) {
@@ -47,9 +41,11 @@ class SkipPatternHttpServerSampler implements SamplerFunction<HttpRequest> {
 		return null;
 	}
 
+	abstract Pattern getPattern();
+
 	private Pattern pattern() {
 		if (this.pattern == null) {
-			this.pattern = this.provider.skipPattern();
+			this.pattern = getPattern();
 		}
 		return this.pattern;
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/SleuthWebProperties.java

@@ -96,7 +96,7 @@ public class SleuthWebProperties {
 	}
 
 	public void setSkipPattern(String skipPattern) {
-		this.skipPattern = skipPattern;
+		this.skipPattern = emptyToNull(skipPattern);
 	}
 
 	public String getAdditionalSkipPattern() {
@@ -104,7 +104,7 @@ public class SleuthWebProperties {
 	}
 
 	public void setAdditionalSkipPattern(String additionalSkipPattern) {
-		this.additionalSkipPattern = additionalSkipPattern;
+		this.additionalSkipPattern = emptyToNull(additionalSkipPattern);
 	}
 
 	public int getFilterOrder() {
@@ -149,6 +149,13 @@ public class SleuthWebProperties {
 		this.client = client;
 	}
 
+	static String emptyToNull(String skipPattern) {
+		if (skipPattern != null && skipPattern.isEmpty()) {
+			skipPattern = null; // otherwise this would skip paths named ""!
+		}
+		return skipPattern;
+	}
+
 	/**
 	 * Web client properties.
 	 *
@@ -159,7 +166,7 @@ public class SleuthWebProperties {
 		/**
 		 * Pattern for URLs that should be skipped in client side tracing.
 		 */
-		private String skipPattern = "";
+		private String skipPattern;
 
 		/**
 		 * Enable interceptor injecting into
@@ -180,7 +187,7 @@ public class SleuthWebProperties {
 		}
 
 		public void setSkipPattern(String skipPattern) {
-			this.skipPattern = skipPattern;
+			this.skipPattern = emptyToNull(skipPattern);
 		}
 
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfiguration.java

@@ -18,6 +18,7 @@ package org.springframework.cloud.sleuth.instrument.web;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.regex.Pattern;
 
 import brave.Tracing;
 import brave.http.HttpRequest;
@@ -27,6 +28,7 @@ import brave.http.HttpSampler;
 import brave.http.HttpTracing;
 import brave.http.HttpTracingCustomizer;
 import brave.sampler.SamplerFunction;
+import brave.sampler.SamplerFunctions;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.AutoConfigureAfter;
@@ -62,7 +64,7 @@ public class TraceHttpAutoConfiguration {
 	@Bean
 	@ConditionalOnMissingBean
 	// NOTE: stable bean name as might be used outside sleuth
-	HttpTracing httpTracing(Tracing tracing, SkipPatternProvider provider,
+	HttpTracing httpTracing(Tracing tracing, @Nullable SkipPatternProvider provider,
 			@Nullable @HttpClientRequestParser HttpRequestParser httpClientRequestParser,
 			@Nullable @HttpClientResponseParser HttpResponseParser httpClientResponseParser,
 			@Nullable brave.http.HttpClientParser clientParser,
@@ -113,12 +115,18 @@ public class TraceHttpAutoConfiguration {
 
 	private SamplerFunction<HttpRequest> combineUserProvidedSamplerWithSkipPatternSampler(
 			@Nullable SamplerFunction<HttpRequest> serverSampler,
-			SkipPatternProvider provider) {
-		SkipPatternHttpServerSampler skipPatternSampler = new SkipPatternHttpServerSampler(
-				provider);
-		if (serverSampler == null) {
+			@Nullable SkipPatternProvider provider) {
+		SamplerFunction<HttpRequest> skipPatternSampler = provider != null
+				? new SkipPatternHttpServerSampler(provider) : null;
+		if (serverSampler == null && skipPatternSampler == null) {
+			return SamplerFunctions.deferDecision();
+		}
+		else if (serverSampler == null) {
 			return skipPatternSampler;
 		}
+		else if (skipPatternSampler == null) {
+			return serverSampler;
+		}
 		return new CompositeHttpSampler(skipPatternSampler, serverSampler);
 	}
 
@@ -154,7 +162,13 @@ public class TraceHttpAutoConfiguration {
 		if (sleuthClientSampler != null) {
 			return sleuthClientSampler;
 		}
-		return new SkipPatternHttpClientSampler(sleuthWebProperties);
+
+		String skipPattern = sleuthWebProperties.getClient().getSkipPattern();
+		if (skipPattern == null) {
+			return SamplerFunctions.deferDecision();
+		}
+
+		return new SkipPatternHttpClientSampler(Pattern.compile(skipPattern));
 	}
 
 }
@@ -164,7 +178,7 @@ public class TraceHttpAutoConfiguration {
  *
  * @author Adrian Cole
  */
-class CompositeHttpSampler implements SamplerFunction<HttpRequest> {
+final class CompositeHttpSampler implements SamplerFunction<HttpRequest> {
 
 	final SamplerFunction<HttpRequest> left;
 
@@ -205,21 +219,32 @@ class CompositeHttpSampler implements SamplerFunction<HttpRequest> {
  *
  * @author Marcin Grzejszczak
  */
-class SkipPatternHttpClientSampler implements SamplerFunction<HttpRequest> {
+final class SkipPatternHttpServerSampler extends SkipPatternSampler {
 
-	private final SleuthWebProperties properties;
+	private final SkipPatternProvider provider;
 
-	SkipPatternHttpClientSampler(SleuthWebProperties properties) {
-		this.properties = properties;
+	SkipPatternHttpServerSampler(SkipPatternProvider provider) {
+		this.provider = provider;
 	}
 
 	@Override
-	public Boolean trySample(HttpRequest request) {
-		String path = request.path();
-		if (path == null) {
-			return null;
-		}
-		return path.matches(this.properties.getClient().getSkipPattern()) ? false : null;
+	Pattern getPattern() {
+		return this.provider.skipPattern();
+	}
+
+}
+
+final class SkipPatternHttpClientSampler extends SkipPatternSampler {
+
+	private final Pattern skipPattern;
+
+	SkipPatternHttpClientSampler(Pattern skipPattern) {
+		this.skipPattern = skipPattern;
+	}
+
+	@Override
+	Pattern getPattern() {
+		return skipPattern;
 	}
 
 }

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/TraceWebAutoConfiguration.java

@@ -22,6 +22,7 @@ import java.util.List;
 import java.util.Optional;
 import java.util.StringJoiner;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import brave.Tracing;
 
@@ -66,18 +67,26 @@ public class TraceWebAutoConfiguration {
 	@Bean
 	@ConditionalOnMissingBean
 	SkipPatternProvider sleuthSkipPatternProvider() {
-		return () -> {
-			StringJoiner joiner = new StringJoiner("|");
-			for (SingleSkipPattern pattern : this.patterns) {
-				Optional<Pattern> skipPattern = pattern.skipPattern();
-				if (skipPattern.isPresent()) {
-					Pattern pattern1 = skipPattern.get();
-					String s = pattern1.pattern();
-					joiner.add(s);
-				}
-			}
-			return Pattern.compile(joiner.toString());
-		};
+		if (this.patterns == null) {
+			return null;
+		}
+		List<Pattern> presentPatterns = this.patterns.stream()
+				.map(SingleSkipPattern::skipPattern).filter(Optional::isPresent)
+				.map(Optional::get).collect(Collectors.toList());
+		if (presentPatterns.isEmpty()) {
+			return null;
+		}
+		if (presentPatterns.size() == 1) {
+			Pattern pattern = presentPatterns.get(0);
+			return () -> pattern;
+		}
+		StringJoiner joiner = new StringJoiner("|");
+		for (Pattern pattern : presentPatterns) {
+			String s = pattern.pattern();
+			joiner.add(s);
+		}
+		Pattern pattern = Pattern.compile(joiner.toString());
+		return () -> pattern;
 	}
 
 	@Configuration(proxyBeanMethods = false)
@@ -200,24 +209,25 @@ public class TraceWebAutoConfiguration {
 	@Configuration(proxyBeanMethods = false)
 	static class DefaultSkipPatternConfig {
 
-		private static String combinedPattern(String skipPattern,
-				String additionalSkipPattern) {
-			String pattern = skipPattern;
-			if (!StringUtils.hasText(skipPattern)) {
-				pattern = SleuthWebProperties.DEFAULT_SKIP_PATTERN;
-			}
-			if (StringUtils.hasText(additionalSkipPattern)) {
-				return pattern + "|" + additionalSkipPattern;
-			}
-			return pattern;
-		}
-
 		@Bean
 		SingleSkipPattern defaultSkipPatternBean(
 				SleuthWebProperties sleuthWebProperties) {
-			return () -> Optional.of(
-					Pattern.compile(combinedPattern(sleuthWebProperties.getSkipPattern(),
-							sleuthWebProperties.getAdditionalSkipPattern())));
+			Pattern pattern = combinePatterns(sleuthWebProperties.getSkipPattern(),
+					sleuthWebProperties.getAdditionalSkipPattern());
+			return () -> Optional.ofNullable(pattern);
+		}
+
+		private static Pattern combinePatterns(String left, String right) {
+			if (left == null && right == null) {
+				return null;
+			}
+			else if (left == null) {
+				return Pattern.compile(right);
+			}
+			else if (right == null) {
+				return Pattern.compile(left);
+			}
+			return Pattern.compile(left + "|" + right);
 		}
 
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignAspect.java

@@ -53,9 +53,7 @@ class TraceFeignAspect {
 			log.debug("Executing feign client via TraceFeignAspect");
 		}
 		if (bean != wrappedBean) {
-			// NOTE: in master(3813cf9dd47f98db0e075412abbffbd9d6742974),
-			// this is executeTraceFeignClient(wrappedBean, pjp)
-			return executeTraceFeignClient(bean, pjp);
+			return executeTraceFeignClient(wrappedBean, pjp);
 		}
 		return pjp.proceed();
 	}

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/instrument/web/client/feign/TraceFeignContext.java

@@ -44,7 +44,10 @@ class TraceFeignContext extends FeignContext {
 	@SuppressWarnings("unchecked")
 	public <T> T getInstance(String name, Class<T> type) {
 		T object = this.delegate.getInstance(name, type);
-		return (T) this.traceFeignObjectWrapper.wrap(object);
+		if (object != null) {
+			return (T) this.traceFeignObjectWrapper.wrap(object);
+		}
+		return null;
 	}
 
 	@Override

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternProviderConfigTest.java

@@ -59,6 +59,13 @@ public class SkipPatternProviderConfigTest {
 					EndpointAutoConfiguration.class, WebEndpointAutoConfiguration.class,
 					TraceAutoConfiguration.class, TraceWebAutoConfiguration.class));
 
+	@Test
+	public void should_return_null_when_cleared() throws Exception {
+		contextRunner.withPropertyValues("spring.sleuth.web.skip-pattern")
+				.run(context -> then(context.getBean("sleuthSkipPatternProvider"))
+						.hasToString("null"));
+	}
+
 	@Test
 	public void should_pick_skip_pattern_from_sleuth_properties() throws Exception {
 		contextRunner.withPropertyValues("spring.sleuth.web.skip-pattern=foo.*|bar.*")

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/SkipPatternSamplerTests.java

@@ -31,27 +31,59 @@ import static org.assertj.core.api.BDDAssertions.then;
  * @author Marcin Grzejszczak
  */
 @RunWith(MockitoJUnitRunner.class)
-public class SkipPatternHttpServerSamplerTests {
+public class SkipPatternSamplerTests {
 
 	@Mock
 	HttpRequest request;
 
 	@Test
 	public void should_delegate_sampling_decision_if_pattern_is_not_matched() {
-		SkipPatternProvider provider = () -> Pattern.compile("foo");
 		BDDMockito.given(this.request.path()).willReturn("url");
-		SkipPatternHttpServerSampler sampler = new SkipPatternHttpServerSampler(provider);
+		SkipPatternSampler sampler = new SkipPatternSampler() {
+			@Override
+			Pattern getPattern() {
+				return Pattern.compile("foo");
+			}
+		};
 
 		then(sampler.trySample(this.request)).isNull();
 	}
 
 	@Test
 	public void should_not_sample_if_pattern_is_matched() {
-		SkipPatternProvider provider = () -> Pattern.compile(".*");
 		BDDMockito.given(this.request.path()).willReturn("url");
-		SkipPatternHttpServerSampler sampler = new SkipPatternHttpServerSampler(provider);
+		SkipPatternSampler sampler = new SkipPatternSampler() {
+			@Override
+			Pattern getPattern() {
+				return Pattern.compile(".*");
+			}
+		};
 
 		then(sampler.trySample(this.request)).isFalse();
 	}
 
+	@Test
+	public void should_not_get_pattern_twice() {
+		BDDMockito.given(this.request.path()).willReturn("url");
+		SkipPatternSampler sampler = new SkipPatternSampler() {
+			boolean provisioned;
+
+			@Override
+			Pattern getPattern() {
+				if (provisioned) {
+					throw new AssertionError("called twice!");
+				}
+				try {
+					return Pattern.compile(".*");
+				}
+				finally {
+					provisioned = true;
+				}
+			}
+		};
+
+		then(sampler.trySample(this.request)).isFalse();
+		then(sampler.trySample(this.request)).isFalse();
+	}
+
 }

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/TraceHttpAutoConfigurationTests.java

@@ -25,6 +25,7 @@ import brave.http.HttpSampler;
 import brave.http.HttpServerParser;
 import brave.http.HttpTracing;
 import brave.sampler.SamplerFunction;
+import brave.sampler.SamplerFunctions;
 import org.junit.Test;
 
 import org.springframework.boot.autoconfigure.AutoConfigurations;
@@ -40,15 +41,27 @@ import static org.assertj.core.api.BDDAssertions.then;
 public class TraceHttpAutoConfigurationTests {
 
 	@Test
-	public void defaultsToSkipPatternHttpClientSampler() {
+	public void defaultsClientSamplerToDefer() {
 		contextRunner().run((context) -> {
 			SamplerFunction<HttpRequest> clientSampler = context
 					.getBean(HttpTracing.class).clientRequestSampler();
 
-			then(clientSampler).isInstanceOf(SkipPatternHttpClientSampler.class);
+			then(clientSampler).isSameAs(SamplerFunctions.deferDecision());
 		});
 	}
 
+	@Test
+	public void configuresClientSkipPattern() throws Exception {
+		contextRunner()
+				.withPropertyValues("spring.sleuth.web.client.skip-pattern=foo.*|bar.*")
+				.run((context) -> {
+					SamplerFunction<HttpRequest> clientSampler = context
+							.getBean(HttpTracing.class).clientRequestSampler();
+
+					then(clientSampler).isInstanceOf(SkipPatternHttpClientSampler.class);
+				});
+	}
+
 	@Test
 	public void configuresUserProvidedDeprecatedClientSampler() {
 		contextRunner().withUserConfiguration(DeprecatedClientSamplerConfig.class)
@@ -83,7 +96,7 @@ public class TraceHttpAutoConfigurationTests {
 	}
 
 	@Test
-	public void defaultsToSkipPatternHttpServerSampler() {
+	public void defaultsServerSamplerToSkipPattern() {
 		contextRunner().run((context) -> {
 			SamplerFunction<HttpRequest> serverSampler = context
 					.getBean(HttpTracing.class).serverRequestSampler();
@@ -92,6 +105,17 @@ public class TraceHttpAutoConfigurationTests {
 		});
 	}
 
+	@Test
+	public void defaultsServerSamplerToDeferWhenSkipPatternCleared() {
+		contextRunner().withPropertyValues("spring.sleuth.web.skip-pattern")
+				.run((context) -> {
+					SamplerFunction<HttpRequest> clientSampler = context
+							.getBean(HttpTracing.class).serverRequestSampler();
+
+					then(clientSampler).isSameAs(SamplerFunctions.deferDecision());
+				});
+	}
+
 	@Test
 	public void wrapsUserProvidedDeprecatedServerSampler() {
 		contextRunner().withUserConfiguration(DeprecatedServerSamplerConfig.class).run(

tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125/ManuallyCreatedLoadBalancerFeignClientTests.java

@@ -160,7 +160,7 @@ class MyDelegateClient implements Client {
 
 }
 
-@FeignClient(name = "foo", url = "https://non.existing.url")
+@FeignClient(name = "foo", url = "http://foo")
 interface MyNameRemote {
 
 	@RequestMapping(value = "/", method = RequestMethod.GET)

tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue1125delegates/ManuallyCreatedDelegateLoadBalancerFeignClientTests.java

@@ -21,7 +21,6 @@ import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.List;
 
-import brave.Tracing;
 import brave.sampler.Sampler;
 import feign.Client;
 import feign.Contract;
@@ -77,9 +76,6 @@ public class ManuallyCreatedDelegateLoadBalancerFeignClientTests {
 	@Autowired
 	ArrayListSpanReporter reporter;
 
-	@Autowired
-	Tracing tracer;
-
 	@Before
 	public void open() {
 		this.reporter.clear();
@@ -92,7 +88,6 @@ public class ManuallyCreatedDelegateLoadBalancerFeignClientTests {
 		then(this.myClient.wasCalled()).isTrue();
 		then(this.myDelegateClient.wasCalled()).isTrue();
 		then(response).isEqualTo("foo");
-		System.out.println("this.myclient.wascalled: " + this.myClient.wasCalled());
 		List<Span> spans = this.reporter.getSpans();
 		// retries
 		then(spans).hasSize(1);
@@ -138,8 +133,8 @@ class Application {
 	public MyNameRemote myNameRemote(Client client, Decoder decoder, Encoder encoder,
 			Contract contract) {
 		return Feign.builder().client(client).encoder(encoder).decoder(decoder)
-				.contract(contract).target(new HardCodedTarget<>(MyNameRemote.class,
-						"foo", "https://non.existing.url"));
+				.contract(contract)
+				.target(new HardCodedTarget<>(MyNameRemote.class, "foo", "http://foo"));
 	}
 
 	@Bean
@@ -194,7 +189,7 @@ class MyDelegateClient implements Client {
 
 }
 
-@FeignClient(name = "foo", url = "https://non.existing.url")
+@FeignClient(name = "foo", url = "http://foo")
 interface MyNameRemote {
 
 	@RequestMapping(value = "/", method = RequestMethod.GET)

tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/java/org/springframework/cloud/sleuth/instrument/feign/issues/issue502/Issue502Tests.java

@@ -45,7 +45,7 @@ import org.springframework.web.bind.annotation.RequestMethod;
 
 import static org.assertj.core.api.BDDAssertions.then;
 
-@FeignClient(name = "foo", url = "https://non.existing.url")
+@FeignClient(name = "foo", url = "http://foo")
 interface MyNameRemote {
 
 	@RequestMapping(value = "/", method = RequestMethod.GET)
@@ -85,7 +85,7 @@ public class Issue502Tests {
 		List<Span> spans = this.reporter.getSpans();
 		// retries
 		then(spans).hasSize(1);
-		then(spans.get(0).tags().get("http.path")).isEqualTo("/");
+		then(spans.get(0).tags().get("http.path")).isEqualTo("");
 	}
 
 }

tests/spring-cloud-sleuth-instrumentation-feign-tests/src/test/resources/application.yml

@@ -2,6 +2,12 @@ hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds: 5000
 ribbon:
   ConnectTimeout: 3000
   ReadTimeout: 5000
+  eager-load:
+    enabled: true
+    clients: foo
+foo:
+  ribbon:
+    listOfServers: non.existing.url
 
 exceptionService.ribbon:
   MaxAutoRetries: 3