From db0e3097d200f17ef575a8aab8f34c124e08b93d Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 29 Apr 2016 13:18:43 +0200 Subject: [PATCH] I've reused the algorithm from Zipkin CalculatingSampler#fixes #260* Changed the algo to counting one* Added comments* Made tests intolerant to errors* Fixed checkstyle I've reused the algorithm from Zipkin CountingTraceIdSampler fixes #260 --- .../sampler/PercentageBasedSampler.java | 44 +++-- .../feign/FeignClientServerErrorTests.java | 177 +++++++++--------- .../sampler/PercentageBasedSamplerTests.java | 33 +++- 3 files changed, 134 insertions(+), 120 deletions(-) diff --git a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSampler.java b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSampler.java index 1fc3a0a88..5c879cd1c 100644 --- a/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSampler.java +++ b/spring-cloud-sleuth-core/src/main/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSampler.java @@ -4,20 +4,16 @@ import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; /** - * Sampler that based on the given percentage rate will allow sampling. - *

+ * This sampler is appropriate for low-traffic instrumentation (ex servers that each receive <100K + * requests), or those who do not provision random trace ids. It not appropriate for collectors as + * the sampling decision isn't idempotent (consistent based on trace id). * - * A couple of assumptions have to take place in order for the algorithm to work properly: - *

+ *

Implementation

* - * + *

Taken from Zipkin project

* - * The value provided from sampler configuration in terms of percentage is an estimation. It might occur that amount - * of data sampled differs from the provided percentage. + *

This counts to see how many out of 100 traces should be retained. This means that it is + * accurate in units of 100 traces. * * @author Marcin Grzejszczak * @author Adrian Cole @@ -25,21 +21,33 @@ import org.springframework.cloud.sleuth.Span; */ public class PercentageBasedSampler implements Sampler { - private final SamplerProperties configuration; + private final int outOf100; + + private int i = 0; // guarded by this + private boolean skipping = false; // guarded by this public PercentageBasedSampler(SamplerProperties configuration) { - this.configuration = configuration; + this.outOf100 = (int) (configuration.getPercentage() * 100.0f);; } @Override public boolean isSampled(Span currentSpan) { - long threshold = Math.abs(Long.MAX_VALUE * (int) (this.configuration.getPercentage() * 100)); // drops fractional percentage. - if (currentSpan == null || threshold == 0L) { + if (this.outOf100 == 0 || currentSpan == null) { return false; + } else if (this.outOf100 == 100) { + return true; + } + synchronized (this) { + boolean result = !this.skipping; + this.i = this.i + 1; + if (this.i == this.outOf100) { + this.skipping = true; + } else if (this.i == 100) { + this.i = 0; + this.skipping = false; + } + return result; } - long traceId = currentSpan.getTraceId(); - Long mod = Math.abs(traceId % 100); - return mod.compareTo(threshold) <= 0; } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java index 9d927a42c..0ebb47a6d 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/instrument/web/client/feign/FeignClientServerErrorTests.java @@ -37,7 +37,6 @@ import org.springframework.cloud.netflix.ribbon.RibbonClient; import org.springframework.cloud.sleuth.Span; import org.springframework.cloud.sleuth.SpanReporter; import org.springframework.cloud.sleuth.Tracer; -import org.springframework.cloud.sleuth.assertions.SleuthAssertions; import org.springframework.cloud.sleuth.util.ExceptionUtils; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -60,126 +59,118 @@ import static org.assertj.core.api.BDDAssertions.then; /** * Related to https://github.com/spring-cloud/spring-cloud-sleuth/issues/257 + * * @author ryarabori */ -@RunWith(SpringJUnit4ClassRunner.class) -@SpringApplicationConfiguration(classes = { FeignClientServerErrorTests.TestConfiguration.class }) +@RunWith(SpringJUnit4ClassRunner.class) @SpringApplicationConfiguration( + classes = { FeignClientServerErrorTests.TestConfiguration.class }) @WebIntegrationTest(value = { "spring.application.name=fooservice" }, randomPort = true) public class FeignClientServerErrorTests { - @Autowired TestFeignInterface feignInterface; - @Rule public OutputCapture capture = new OutputCapture(); + @Autowired TestFeignInterface feignInterface; + @Rule public OutputCapture capture = new OutputCapture(); - @Before - public void setup() { - ExceptionUtils.setFail(true); - } + @Before public void setup() { + ExceptionUtils.setFail(true); + } - @Test - public void shouldCloseSpanOnInternalServerError() throws InterruptedException { - try { - this.feignInterface.internalError(); - } catch (HystrixRuntimeException e) { - } + @Test public void shouldCloseSpanOnInternalServerError() throws InterruptedException { + try { + this.feignInterface.internalError(); + } + catch (HystrixRuntimeException e) { + } - // ugly :/ waiting for rx thread to complete - Thread.sleep(100); - then(this.capture.toString()).doesNotContain("Tried to close span but it is not the current span"); - } + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()) + .doesNotContain("Tried to close span but it is not the current span"); + } - @Test - public void shouldCloseSpanOnNotFound() throws InterruptedException { - try { - this.feignInterface.notFound(); - } catch (HystrixRuntimeException e) { - } + @Test public void shouldCloseSpanOnNotFound() throws InterruptedException { + try { + this.feignInterface.notFound(); + } + catch (HystrixRuntimeException e) { + } - // ugly :/ waiting for rx thread to complete - Thread.sleep(100); - then(this.capture.toString()).doesNotContain("Tried to close span but it is not the current span"); - } + // ugly :/ waiting for rx thread to complete + Thread.sleep(100); + then(this.capture.toString()) + .doesNotContain("Tried to close span but it is not the current span"); + } - @Configuration - @EnableAutoConfiguration - @EnableFeignClients - @RibbonClient(value = "fooservice", configuration = SimpleRibbonClientConfiguration.class) - public static class TestConfiguration { + @Configuration @EnableAutoConfiguration @EnableFeignClients + @RibbonClient(value = "fooservice", + configuration = SimpleRibbonClientConfiguration.class) + public static class TestConfiguration { - @Bean - FooController fooController() { - return new FooController(); - } + @Bean FooController fooController() { + return new FooController(); + } - @Bean - Listener listener() { - return new Listener(); - } + @Bean Listener listener() { + return new Listener(); + } - @LoadBalanced - @Bean - public RestTemplate restTemplate() { - return new RestTemplate(); - } + @LoadBalanced @Bean public RestTemplate restTemplate() { + return new RestTemplate(); + } - } + } - @FeignClient(value = "fooservice") - public interface TestFeignInterface { + @FeignClient(value = "fooservice") public interface TestFeignInterface { - @RequestMapping(method = RequestMethod.GET, value = "/internalerror") - ResponseEntity internalError(); + @RequestMapping(method = RequestMethod.GET, value = "/internalerror") + ResponseEntity internalError(); - @RequestMapping(method = RequestMethod.GET, value = "/notfound") - ResponseEntity notFound(); + @RequestMapping(method = RequestMethod.GET, value = "/notfound") + ResponseEntity notFound(); - } + } - @Component - public static class Listener implements SpanReporter { - private List events = new ArrayList<>(); + @Component public static class Listener implements SpanReporter { + private List events = new ArrayList<>(); - public List getEvents() { - return this.events; - } + public List getEvents() { + return this.events; + } - @Override - public void report(Span span) { - this.events.add(span); - } - } + @Override public void report(Span span) { + this.events.add(span); + } + } - @RestController - public static class FooController { + @RestController public static class FooController { - @Autowired Tracer tracer; + @Autowired Tracer tracer; - @RequestMapping("/internalerror") - public ResponseEntity internalError(@RequestHeader(Span.TRACE_ID_NAME) String traceId, - @RequestHeader(Span.SPAN_ID_NAME) String spanId, - @RequestHeader(Span.PARENT_ID_NAME) String parentId) { - return new ResponseEntity<>("internal error", HttpStatus.INTERNAL_SERVER_ERROR); - } + @RequestMapping("/internalerror") public ResponseEntity internalError( + @RequestHeader(Span.TRACE_ID_NAME) String traceId, + @RequestHeader(Span.SPAN_ID_NAME) String spanId, + @RequestHeader(Span.PARENT_ID_NAME) String parentId) { + return new ResponseEntity<>("internal error", + HttpStatus.INTERNAL_SERVER_ERROR); + } - @RequestMapping("/notfound") - public ResponseEntity notFound(@RequestHeader(Span.TRACE_ID_NAME) String traceId, - @RequestHeader(Span.SPAN_ID_NAME) String spanId, - @RequestHeader(Span.PARENT_ID_NAME) String parentId) { - return new ResponseEntity<>("not found", HttpStatus.NOT_FOUND); - } - } + @RequestMapping("/notfound") public ResponseEntity notFound( + @RequestHeader(Span.TRACE_ID_NAME) String traceId, + @RequestHeader(Span.SPAN_ID_NAME) String spanId, + @RequestHeader(Span.PARENT_ID_NAME) String parentId) { + return new ResponseEntity<>("not found", HttpStatus.NOT_FOUND); + } + } - @Configuration - public static class SimpleRibbonClientConfiguration { + @Configuration public static class SimpleRibbonClientConfiguration { - @Value("${local.server.port}") - private int port = 0; + @Value("${local.server.port}") private int port = 0; - @Bean - public ILoadBalancer ribbonLoadBalancer() { - BaseLoadBalancer balancer = new BaseLoadBalancer(); - balancer.setServersList(Collections.singletonList(new Server("localhost", this.port))); - return balancer; - } - } + @Bean public ILoadBalancer ribbonLoadBalancer() { + BaseLoadBalancer balancer = new BaseLoadBalancer(); + balancer.setServersList( + Collections.singletonList(new Server("localhost", this.port))); + return balancer; + } + } } diff --git a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSamplerTests.java b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSamplerTests.java index 685aef142..91aba1ad5 100644 --- a/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSamplerTests.java +++ b/spring-cloud-sleuth-core/src/test/java/org/springframework/cloud/sleuth/sampler/PercentageBasedSamplerTests.java @@ -1,14 +1,14 @@ package org.springframework.cloud.sleuth.sampler; -import static org.assertj.core.api.BDDAssertions.then; -import static org.assertj.core.data.Percentage.withPercentage; - import java.util.Random; import org.junit.Before; import org.junit.Test; +import org.springframework.cloud.sleuth.Sampler; import org.springframework.cloud.sleuth.Span; +import static org.assertj.core.api.BDDAssertions.then; + public class PercentageBasedSamplerTests { SamplerProperties samplerConfiguration = new SamplerProperties(); @@ -39,21 +39,32 @@ public class PercentageBasedSamplerTests { @Test public void should_pass_given_percent_of_samples() throws Exception { - int numberOfIterations = 10000; + int numberOfIterations = 1000; float percentage = 1f; this.samplerConfiguration.setPercentage(percentage); int numberOfSampledElements = countNumberOfSampledElements(numberOfIterations); - then(numberOfSampledElements).isCloseTo((int) (numberOfIterations * percentage), - withPercentage(3)); + then(numberOfSampledElements).isEqualTo((int) (numberOfIterations * percentage)); + } + + @Test + public void should_pass_given_percent_of_samples_with_fractional_element() throws Exception { + int numberOfIterations = 1000; + float percentage = 0.35f; + this.samplerConfiguration.setPercentage(percentage); + + int numberOfSampledElements = countNumberOfSampledElements(numberOfIterations); + + int threshold = (int) (numberOfIterations * percentage); + then(numberOfSampledElements).isEqualTo(threshold); } private int countNumberOfSampledElements(int numberOfIterations) { + Sampler sampler = new PercentageBasedSampler(this.samplerConfiguration); int passedCounter = 0; for (int i = 0; i < numberOfIterations; i++) { - boolean passed = new PercentageBasedSampler(this.samplerConfiguration) - .isSampled(this.span); + boolean passed = sampler.isSampled(newSpan()); passedCounter = passedCounter + (passed ? 1 : 0); } return passedCounter; @@ -61,7 +72,11 @@ public class PercentageBasedSamplerTests { @Before public void setupSpan() { - this.span = Span.builder().traceId(RANDOM.nextLong()).build(); + this.span = newSpan(); + } + + Span newSpan() { + return Span.builder().traceId(RANDOM.nextLong()).build(); } } \ No newline at end of file