Restores special case that ignores sampling properties (#1619)

Sleuth is unlike most tracing configuration libraries, as it has a
legacy from 1.x as being primarily log correlation. In short, when
Zipkin is not installed, it ignored the sampling properties. This
behavior was managed implicitly through an untested combination of
configuration conventions between the core and zipkin modules.

16fb8e3 broke this and this change puts it back, in a way not tightly
coupled to Zipkin and neither requires the more simple, but confusing
"import SamplerAutoConfiguration" approach. It also backfills tests
that should have broken earlier.

In practice, a site that only uses logging is likely uniform in that, so
whether or not the sampled bit is set is of no consequence. However,
there's a chance that someone might rely on this historical behavior.

We should follow-up on 3.0 and remove this as it is very unintuitive to
intentionally ignore sampling properties. For now, this restores the old
behavior based on heuristics of bean definitions.

docs/src/main/asciidoc/spring-cloud-sleuth.adoc

@@ -312,16 +312,15 @@ Span nextSpan(final Request input) {
 
 === Sampling in Spring Cloud Sleuth
 
-By default Spring Cloud Sleuth sets all spans to non-exportable.
-That means that traces appear in logs but not in any remote store.
-For testing the default is often enough, and it probably is all you need if you use only the logs (for example, with an ELK aggregator).
-If you export span data to Zipkin, there is also an `Sampler.ALWAYS_SAMPLE` setting that exports everything and a `ProbabilityBasedSampler` setting that samples a fixed fraction of spans.
+Sampling only applies to tracing backends, such as Zipkin. Trace IDs appear in logs regardless of
+sample rate. Sampling is a way to prevent overloading the system, by consistently tracing some, but
+not all requests.
 
-NOTE: The `ProbabilityBasedSampler` is the default if you use `spring-cloud-sleuth-zipkin`.
-You can configure the exports by setting `spring.sleuth.sampler.probability`.
-The passed value needs to be a double from `0.0` to `1.0`.
+The default to sample 10% of traces is controlled by the `spring.sleuth.sampler.probability`
+property and applies when we know Sleuth is used for reasons besides logging. The passed
+value needs to be a float from `0.0` to `1.0`.
 
-A sampler can be installed by creating a bean definition, as shown in the following example:
+The sampler can be set by Java Config also, as shown in the following example:
 
 [source,java]
 ----
@@ -329,9 +328,7 @@ include::../../../../spring-cloud-sleuth-core/src/test/java/org/springframework/
 ----
 
 TIP: You can set the HTTP header `X-B3-Flags` to `1`, or, when doing messaging, you can set the `spanFlags` header to `1`.
-Doing so forces the current span to be exportable regardless of the sampling decision.
-
-In order to use the rate-limited sampler set the `spring.sleuth.sampler.rate` property to choose an amount of traces to accept on a per-second interval. The minimum number is 0 and the max is 2,147,483,647 (max int).
+Doing so forces the corresponding trace to be sampled regardless of the sampling configuration.
 
 == Propagation
 

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/SamplerAutoConfiguration.java

@@ -23,12 +23,14 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.cloud.context.config.annotation.RefreshScope;
 import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Conditional;
 import org.springframework.context.annotation.Configuration;
 
 /**
  * {@linkplain Configuration configuration} for {@link Sampler}.
  *
  * @author Marcin Grzejszczak
+ * @see SamplerCondition
  * @since 2.1.0
  */
 @Configuration
@@ -44,14 +46,17 @@ public class SamplerAutoConfiguration {
 		return Sampler.NEVER_SAMPLE;
 	}
 
+	// NOTE: Brave's default samplers return Sampler.NEVER_SAMPLE if the config implies
+	// that
 	static Sampler samplerFromProps(SamplerProperties config) {
 		if (config.getRate() != null) {
-			return new RateLimitingSampler(config);
+			return brave.sampler.RateLimitingSampler.create(config.getRate());
 		}
-		return new ProbabilityBasedSampler(config);
+		return brave.sampler.CountingSampler.create(config.getProbability());
 	}
 
 	@Configuration
+	@Conditional(SamplerCondition.class)
 	@ConditionalOnBean(type = "org.springframework.cloud.context.scope.refresh.RefreshScope")
 	protected static class RefreshScopedSamplerConfiguration {
 
@@ -59,12 +64,18 @@ public class SamplerAutoConfiguration {
 		@RefreshScope
 		@ConditionalOnMissingBean
 		public Sampler defaultTraceSampler(SamplerProperties config) {
-			return samplerFromProps(config);
+			// TODO: Rewrite: refresh should replace the sampler, not change its state
+			// internally
+			if (config.getRate() != null) {
+				return new RateLimitingSampler(config);
+			}
+			return new ProbabilityBasedSampler(config);
 		}
 
 	}
 
 	@Configuration
+	@Conditional(SamplerCondition.class)
 	@ConditionalOnMissingBean(type = "org.springframework.cloud.context.scope.refresh.RefreshScope")
 	protected static class NonRefreshScopeSamplerConfiguration {
 

spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/SamplerCondition.java

@@ -0,0 +1,82 @@
+/*
+ * 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.sampler;
+
+import brave.TracingCustomizer;
+import brave.handler.FinishedSpanHandler;
+import brave.sampler.Sampler;
+
+import org.springframework.boot.autoconfigure.condition.AnyNestedCondition;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnBean;
+
+/**
+ * Sleuth 1.x optimized for log-correlation only. Unless "spring-cloud-sleuth-zipkin" was
+ * present, the sampler defaulted to {@link Sampler#NEVER_SAMPLE}. This was to ensure the
+ * log only nodes never set the sampled flag.
+ *
+ * <p>
+ * "spring-cloud-sleuth-zipkin" obviated the {@link Sampler#NEVER_SAMPLE} default by
+ * importing {@link SamplerAutoConfiguration}. Nothing else did, so sampling properties
+ * were effectively ignored unless "spring-cloud-sleuth-zipkin" was in use, or something
+ * else similarly imported {@link SamplerAutoConfiguration}.
+ *
+ * <p>
+ * During a review of Wavefront integration, it was considered not correct to have other
+ * code import {@link SamplerAutoConfiguration}. To avoid that, retain the old behaviour
+ * about log only nodes, and also not pin configuration to Zipkin involves a more complex
+ * condition.
+ *
+ * <p>
+ * This condition looks for signs of non-default setup which likely requires sampling
+ * configuration. It passes on one of the following beans exist:
+ *
+ * <p>
+ * <ul>
+ * <li>{@code zipkin2.reporter.Reporter} - what's used by Zipkin or others like
+ * Stackdriver</li>
+ * <li>{@link FinishedSpanHandler} - only accepts sampled data</li>
+ * <li>{@link TracingCustomizer} - can configure one of the above</li>
+ * </ul>
+ *
+ * <p>
+ * An integrated test that shows {@link Sampler#NEVER_SAMPLE} is default on fail exists in
+ * {@code TraceAutoConfigurationTests} intentionally, as users now needn't import
+ * {@link SamplerAutoConfiguration} directly.
+ */
+final class SamplerCondition extends AnyNestedCondition {
+
+	SamplerCondition() {
+		super(ConfigurationPhase.REGISTER_BEAN);
+	}
+
+	// zipkin2.reporter.Reporter is in the classpath here, but technically it is optional
+	@ConditionalOnBean(type = "zipkin2.reporter.Reporter")
+	static final class ReporterAvailable {
+
+	}
+
+	@ConditionalOnBean(FinishedSpanHandler.class)
+	static final class FinishedSpanHandlerAvailable {
+
+	}
+
+	@ConditionalOnBean(TracingCustomizer.class)
+	static final class TracingCustomizerAvailable {
+
+	}
+
+}

spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/sampler/SamplerAutoConfigurationTests.java

@@ -16,33 +16,159 @@
 
 package org.springframework.cloud.sleuth.sampler;
 
+import brave.Tracing;
+import brave.TracingCustomizer;
+import brave.handler.FinishedSpanHandler;
+import brave.handler.MutableSpan;
+import brave.propagation.TraceContext;
+import brave.sampler.CountingSampler;
 import brave.sampler.Sampler;
 import org.assertj.core.api.BDDAssertions;
 import org.junit.Test;
+import zipkin2.Span;
+import zipkin2.reporter.Reporter;
+
+import org.springframework.boot.autoconfigure.AutoConfigurations;
+import org.springframework.boot.test.context.runner.ApplicationContextRunner;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
 
 /**
  * @author Marcin Grzejszczak
  * @since
  */
+// TODO: missing spring cloud context tests
 public class SamplerAutoConfigurationTests {
 
+	private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
+			.withConfiguration(AutoConfigurations.of(SamplerAutoConfiguration.class));
+
 	@Test
-	public void should_use_rate_limit_sampler_when_property_set() {
-		SamplerProperties properties = new SamplerProperties();
-		properties.setRate(10);
-
-		Sampler sampler = SamplerAutoConfiguration.samplerFromProps(properties);
-
-		BDDAssertions.then(sampler).isInstanceOf(RateLimitingSampler.class);
+	public void should_use_NEVER_SAMPLER_when_only_logging() {
+		this.contextRunner.run((context -> {
+			final Sampler bean = context.getBean(Sampler.class);
+			BDDAssertions.then(bean).isSameAs(Sampler.NEVER_SAMPLE);
+		}));
 	}
 
 	@Test
-	public void should_use_probability_sampler_when_rate_limiting_not_set() {
+	public void should_use_CountingSampler_withFinishedSpanHandler() {
+		this.contextRunner.withUserConfiguration(WithFinishedSpanHandler.class)
+				.run((context -> {
+					final Sampler bean = context.getBean(Sampler.class);
+					BDDAssertions.then(bean).isInstanceOf(CountingSampler.class);
+				}));
+	}
+
+	@Test
+	public void should_use_CountingSampler_withReporter() {
+		this.contextRunner.withUserConfiguration(WithReporter.class).run((context -> {
+			final Sampler bean = context.getBean(Sampler.class);
+			BDDAssertions.then(bean).isInstanceOf(CountingSampler.class);
+		}));
+	}
+
+	@Test
+	public void should_use_CountingSampler_withTracingCustomizer() {
+		this.contextRunner.withUserConfiguration(WithTracingCustomizer.class)
+				.run((context -> {
+					final Sampler bean = context.getBean(Sampler.class);
+					BDDAssertions.then(bean).isInstanceOf(CountingSampler.class);
+				}));
+	}
+
+	@Test
+	public void should_override_sampler() {
+		this.contextRunner.withUserConfiguration(WithReporter.class, WithSampler.class)
+				.run((context -> {
+					final Sampler bean = context.getBean(Sampler.class);
+					BDDAssertions.then(bean).isSameAs(Sampler.ALWAYS_SAMPLE);
+				}));
+	}
+
+	@Test
+	public void samplerFromProps_probability() {
+		SamplerProperties properties = new SamplerProperties();
+		properties.setProbability(0.01f);
+
+		Sampler sampler = SamplerAutoConfiguration.samplerFromProps(properties);
+
+		BDDAssertions.then(sampler).isInstanceOf(brave.sampler.CountingSampler.class);
+	}
+
+	@Test
+	public void samplerFromProps_rateLimit() {
 		SamplerProperties properties = new SamplerProperties();
 
 		Sampler sampler = SamplerAutoConfiguration.samplerFromProps(properties);
 
-		BDDAssertions.then(sampler).isInstanceOf(ProbabilityBasedSampler.class);
+		BDDAssertions.then(sampler).isInstanceOf(brave.sampler.CountingSampler.class);
+	}
+
+	@Test
+	public void samplerFromProps_rateLimitZero() {
+		SamplerProperties properties = new SamplerProperties();
+		properties.setRate(0);
+
+		Sampler sampler = SamplerAutoConfiguration.samplerFromProps(properties);
+
+		BDDAssertions.then(sampler).isSameAs(Sampler.NEVER_SAMPLE);
+	}
+
+	@Test
+	public void samplerFromProps_prefersRate() {
+		SamplerProperties properties = new SamplerProperties();
+		properties.setProbability(0.01f);
+		properties.setRate(20);
+
+		Sampler sampler = SamplerAutoConfiguration.samplerFromProps(properties);
+
+		BDDAssertions.then(sampler).isInstanceOf(brave.sampler.RateLimitingSampler.class);
+	}
+
+	@Configuration
+	static class WithFinishedSpanHandler {
+
+		@Bean
+		FinishedSpanHandler finishedSpanHandler() {
+			return new FinishedSpanHandler() {
+				@Override
+				public boolean handle(TraceContext context, MutableSpan span) {
+					return true;
+				}
+			};
+		}
+
+	}
+
+	@Configuration
+	static class WithReporter {
+
+		@Bean
+		Reporter<Span> spanReporter() {
+			return zipkin2.Span::toString;
+		}
+
+	}
+
+	@Configuration
+	static class WithSampler {
+
+		@Bean
+		Sampler alwaysSampler() {
+			return Sampler.ALWAYS_SAMPLE;
+		}
+
+	}
+
+	@Configuration
+	static class WithTracingCustomizer {
+
+		@Bean
+		TracingCustomizer tracingCustomizer() {
+			return Tracing.Builder::toString;
+		}
+
 	}
 
 }