Introduces TraceSampler which replaces TraceFilter

TraceFilter worked off a span id, which isn't always the trace id. This is confusing and could lead to sparse traces. This eliminates use of TraceFilter, replaced with TraceSampler, which works off the trace id and is implemented by a benchmarked implementation in zipkin-java. The use of zipkin-java's TraceIdSampler is shaded, so as to neither introduce a dependency, nor the perils of copy/paste. Impact on users is that they should stop using TraceFilter immediately as Brave will ignore them. Most often, they will want to only provide a threshold, ex `TraceSampler.create(0.3f)` means retain 30%. We also no longer publish `brave-tracefilters` and instead an equivalent `brave-tracesamplers`, which has the same implementation, except based on trace id as opposed to span id.
openzipkin · Jan 7, 2016 · eb888a5 · eb888a5
1 parent 88e2f5d
commit eb888a5
Show file tree

Hide file tree

Showing 26 changed files with 384 additions and 405 deletions.
diff --git a/README.md b/README.md
@@ -62,7 +62,7 @@ of the code) to the [Metrics](http://metrics.codahale.com) library.  Metrics bui
 to a back-end system for storage and visualisation. Metrics supports multiple back-ends but the sink implementation today supports
 [graphite](http://graphite.wikidot.com).
 
-If you use the `ZooKeeperSamplingTraceFilter` from `brave-tracefilters` module you can enable/disable tracing or adjust
+If you use the `ZooKeeperTraceSampler` from `brave-tracesamplers` module you can enable/disable tracing or adjust
 sample rate using [ZooKeeper](http://zookeeper.apache.org) as also indicated on the drawing.
 
 

diff --git a/brave-core-spring/README.md b/brave-core-spring/README.md
@@ -13,6 +13,6 @@ dependency to you own application. This gives you the freedom to choose the Spri
 your choice (the config classes are tested with Spring 4.1.6.RELEASE).
 
 There is no configuration provided for the `Brave` instance (see `brave-core`). You have to provide a Spring
-config class for this yourself as you probably want to decide anyway which `SpanCollector` and which `TraceFilters`
+config class for this yourself as you probably want to decide anyway which `SpanCollector` and which `TraceSampler`
 to use.
 
diff --git a/brave-core-spring/src/main/java/com/github/kristofa/brave/BraveApiConfig.java b/brave-core-spring/src/main/java/com/github/kristofa/brave/BraveApiConfig.java
@@ -9,7 +9,7 @@
  * Spring configuration for Brave api components.
  * <p>
  * You will need to provide your own configuration for the Brave object which is
- * configured through the Brave.Builder and which configures SpanCollector, TraceFilters,...
+ * configured through the Brave.Builder and which configures SpanCollector, TraceSampler,...
  * </p>
  */
 @Configuration

diff --git a/brave-core-spring/src/test/java/com/github/kristofa/brave/BraveConfig.java b/brave-core-spring/src/test/java/com/github/kristofa/brave/BraveConfig.java
@@ -4,8 +4,6 @@
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Scope;
 
-import java.util.Arrays;
-
 import static org.mockito.Mockito.mock;
 
 @Configuration
@@ -17,7 +15,7 @@ public Brave brave() {
             final Brave.Builder builder = new Brave.Builder();
             return builder
                     .spanCollector(mock(SpanCollector.class))
-                    .traceFilters(Arrays.asList(mock(TraceFilter.class)))
+                    .traceSampler(mock(TraceSampler.class))
                     .build();
         }
 }
diff --git a/brave-core/README.md b/brave-core/README.md
@@ -27,7 +27,7 @@ You should check this out of you want to do http integrations.
 final Brave.Builder builder = new Brave.Builder();
 final Brave brave = builder
   .spanCollector(aSpanCollector)
-  .traceFilters(Arrays.<TraceFilter>asList(aTraceFilter))
+  .traceSampler(TraceSampler.create(0.1f)) // retain 10% of traces
   .build();
 
 // Creates different interceptors  
@@ -41,7 +41,7 @@ As you can see in above example, Since brave 3.0 the way to instantiate the api
 We use a Builder now that lets you configure custom:
 
    * SpanCollector. Default value = `LoggingSpanCollector`
-   * list of TraceFilters. Default value is `FixedSampleRateTraceFilter` with value = 1.
+   * TraceSampler. Default value is to send every trace.
    * ServerAndClientSpanState. Default value is `ThreadLocalServerAndClientSpanState`.
 
 Once the `Brave` object is created you can get the different interceptors. 
@@ -55,33 +55,31 @@ implementations but have some implementations that you can use:
    * `EmptySpanCollector` : Part of brave-core. Does nothing.
    * `ZipkinSpanCollector` : Part of `brave-zipkin-spancollector` module. Span collector that supports sending spans directly to `zipkin-collector` service or Scribe.
 
-### TraceFilter ###
+### TraceSampler ###
 
 You might not want to trace all requests that are being submitted:
 
-   * to avoid performance overhead
+   * to avoid performance overhea
    * to avoid running out of storage
 
 and you don't need to trace all requests to come to usable data.
 
-A TraceFilter's purpose (`com.github.kristofa.brave.TraceFilter`) is to decide if a given 
-request should get traced or not. 
+A TraceSampler's purpose (`com.github.kristofa.brave.TraceSampler`) is to decide if a given
+request should get traced or not.
 
 The decision, should trace (true) or not (false) is taken by the first request (client side or server side) and should
 be passed through to all subsequent requests. This has as a consequence that we either
 trace a full request tree or none of the requests at all which is good. We don't want incomplete traces.
 
-There is a TraceFilter implementation that comes with brave-core which is 
-`com.github.kristofa.brave.FixedSampleRateTraceFilter`. This 
-TraceFilter is created with a fixed sample rate provided through its constructor. The
+TraceSampler has a factory method that accepts a a fixed sample rate as a percentage. The
 sample rate can't be adapted at run time.  Behaviour:
 
-*   sample rate <= 0 : Non of the requests will be traced. Means tracing is disabled
-*   sample rate = 1 : All requests will be traced.
-*   sample rate > 1 : For example 3, every third request will be traced.
+*   sample rate 0.0f : Non of the requests will be traced. Means tracing is disabled
+*   sample rate 1.0f : All requests will be traced.
+*   sample rate (0.0, 1.0) : For example 0.3f, 30% of requests will be traced.
 
-If you want to use a TraceFilter implementation which allows adapting sample rate at run
-time see `brave-tracefilters` project which contains a TraceFilter with ZooKeeper support.
+If you want to use a TraceSampler implementation which allows adapting sample rate at run
+time see `brave-tracesamplers` project which contains a TraceSampler with ZooKeeper support.
 
 
 

diff --git a/brave-core/pom.xml b/brave-core/pom.xml
@@ -26,6 +26,11 @@
   </properties>
 
   <dependencies>
+    <dependency>
+        <groupId>io.zipkin</groupId>
+        <artifactId>zipkin-java-core</artifactId>
+        <version>0.2.0</version>
+    </dependency>
     <dependency>
         <groupId>com.google.auto.value</groupId>
         <artifactId>auto-value</artifactId>
@@ -55,4 +60,32 @@
         <scope>test</scope>
     </dependency>
   </dependencies>
+
+    <build>
+        <plugins>
+            <!-- Use of zipkin-java is internal only; don't add a dependency -->
+            <plugin>
+                <artifactId>maven-shade-plugin</artifactId>
+                <executions>
+                    <execution>
+                        <phase>package</phase>
+                        <goals>
+                            <goal>shade</goal>
+                        </goals>
+                        <configuration>
+                            <shadeTestJar>false</shadeTestJar>
+                            <minimizeJar>true</minimizeJar>
+                            <dependencyReducedPomLocation>${project.build.directory}/dependency-reduced-pom.xml</dependencyReducedPomLocation>
+                            <relocations>
+                                <relocation>
+                                    <pattern>io.zipkin</pattern>
+                                    <shadedPattern>com.github.kristofa.brave.internal.io.zipkin</shadedPattern>
+                                </relocation>
+                            </relocations>
+                        </configuration>
+                    </execution>
+                </executions>
+            </plugin>
+        </plugins>
+    </build>
 </project>
diff --git a/brave-core/src/main/java/com/github/kristofa/brave/Brave.java b/brave-core/src/main/java/com/github/kristofa/brave/Brave.java
@@ -1,9 +1,7 @@
 package com.github.kristofa.brave;
 
 import com.github.kristofa.brave.internal.Util;
-
 import java.net.UnknownHostException;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Random;
 
@@ -28,16 +26,17 @@ public class Brave {
      * <p>
      * <ul>
      * <li>ThreadLocalServerClientAndLocalSpanState which binds trace/span state to current thread.</li>
-     * <li>FixedSampleRateTraceFilter which traces every request.</li>
      * <li>LoggingSpanCollector</li>
+     * <li>TraceSampler that samples all traces</li>
      * </ul>
      */
     public static class Builder {
 
         private final ServerClientAndLocalSpanState state;
-        private final List<TraceFilter> traceFilters = new ArrayList<>();
         private SpanCollector spanCollector = new LoggingSpanCollector();
         private Random random = new Random();
+        // default added so callers don't need to check null.
+        private TraceSampler traceSampler = TraceSampler.create(1.0f);
 
         /**
          * Builder which initializes with serviceName = "unknown".
@@ -68,7 +67,6 @@ public Builder(String serviceName) {
             } catch (UnknownHostException e) {
                 throw new IllegalStateException("Unable to get Inet address", e);
             }
-            traceFilters.add(new FixedSampleRateTraceFilter(1));
         }
 
         /**
@@ -80,25 +78,25 @@ public Builder(String serviceName) {
          */
         public Builder(int ip, int port, String serviceName) {
             state = new ThreadLocalServerClientAndLocalSpanState(ip, port, serviceName);
-            traceFilters.add(new FixedSampleRateTraceFilter(1));
         }
 
         /**
          * Use for control of how tracing state propagates across threads.
          */
         public Builder(ServerClientAndLocalSpanState state) {
             this.state = Util.checkNotNull(state, "state must be specified.");
-            traceFilters.add(new FixedSampleRateTraceFilter(1));
         }
 
         /**
-         * Initialize trace filters. If not specified a default filter will be configured which traces every request.
-         *
-         * @param filters trace filters.
+         * @deprecated use {@link #traceSampler(TraceSampler)} as filters here will be ignored.
          */
-        public Builder traceFilters(List<TraceFilter> filters) {
-            traceFilters.clear();
-            traceFilters.addAll(filters);
+        @Deprecated
+        public Builder traceFilters(List<TraceFilter> ignored) {
+            return this; // noop
+        }
+
+        public Builder traceSampler(TraceSampler traceSampler) {
+            this.traceSampler = traceSampler;
             return this;
         }
 
@@ -201,19 +199,19 @@ private Brave(Builder builder) {
                 .randomGenerator(builder.random)
                 .spanCollector(builder.spanCollector)
                 .state(builder.state)
-                .traceFilters(builder.traceFilters).build();
+                .traceSampler(builder.traceSampler).build();
 
         clientTracer = ClientTracer.builder()
                 .randomGenerator(builder.random)
                 .spanCollector(builder.spanCollector)
                 .state(builder.state)
-                .traceFilters(builder.traceFilters).build();
+                .traceSampler(builder.traceSampler).build();
 
         localTracer = LocalTracer.builder()
                 .randomGenerator(builder.random)
                 .spanCollector(builder.spanCollector)
                 .spanAndEndpoint(SpanAndEndpoint.LocalSpanAndEndpoint.create(builder.state))
-                .traceFilters(builder.traceFilters).build();
+                .traceSampler(builder.traceSampler).build();
 
         serverRequestInterceptor = new ServerRequestInterceptor(serverTracer);
         serverResponseInterceptor = new ServerResponseInterceptor(serverTracer);

diff --git a/brave-core/src/main/java/com/github/kristofa/brave/ClientTracer.java b/brave-core/src/main/java/com/github/kristofa/brave/ClientTracer.java
@@ -8,7 +8,6 @@
 import com.twitter.zipkin.gen.Span;
 import com.twitter.zipkin.gen.zipkinCoreConstants;
 
-import java.util.List;
 import java.util.Random;
 
 /**
@@ -37,7 +36,7 @@ public static Builder builder() {
     abstract ClientSpanAndEndpoint spanAndEndpoint();
     abstract Random randomGenerator();
     abstract SpanCollector spanCollector();
-    abstract List<TraceFilter> traceFilters();
+    abstract TraceSampler traceSampler();
 
     @AutoValue.Builder
     public abstract static class Builder {
@@ -55,11 +54,7 @@ public Builder state(ServerClientAndLocalSpanState state) {
 
         public abstract Builder spanCollector(SpanCollector spanCollector);
 
-        /**
-         * Will be executed in order. If one returns <code>false</code> there will be no tracing and
-         * next ones will not be executed anymore. So order is important.
-         */
-        public abstract Builder traceFilters(List<TraceFilter> traceFilters);
+        public abstract Builder traceSampler(TraceSampler traceSampler);
 
         abstract ClientTracer build();
     }
@@ -114,12 +109,10 @@ public SpanId startNewSpan(String requestName) {
         SpanId newSpanId = getNewSpanId();
         if (sample == null) {
             // No sample indication is present.
-            for (TraceFilter traceFilter : traceFilters()) {
-                if (!traceFilter.trace(newSpanId.getSpanId(), requestName)) {
-                    spanAndEndpoint().state().setCurrentClientSpan(null);
-                    spanAndEndpoint().state().setCurrentClientServiceName(null);
-                    return null;
-                }
+            if (!traceSampler().test(newSpanId.getTraceId())) {
+                spanAndEndpoint().state().setCurrentClientSpan(null);
+                spanAndEndpoint().state().setCurrentClientServiceName(null);
+                return null;
             }
         }
 

diff --git a/brave-core/src/main/java/com/github/kristofa/brave/FixedSampleRateTraceFilter.java b/brave-core/src/main/java/com/github/kristofa/brave/FixedSampleRateTraceFilter.java
@@ -6,7 +6,9 @@
  * {@link TraceFilter} that is initialized with a fixed sample rate.
  * 
  * @author kristof
+ * @deprecated Use {@link TraceSampler} instead.
  */
+@Deprecated
 public class FixedSampleRateTraceFilter implements TraceFilter {
 
     private final int sampleRate;

diff --git a/brave-core/src/main/java/com/github/kristofa/brave/LocalTracer.java b/brave-core/src/main/java/com/github/kristofa/brave/LocalTracer.java
@@ -9,7 +9,6 @@
 import com.twitter.zipkin.gen.zipkinCoreConstants;
 
 import java.nio.ByteBuffer;
-import java.util.List;
 import java.util.Random;
 
 import static com.twitter.zipkin.gen.zipkinCoreConstants.LOCAL_COMPONENT;
@@ -55,7 +54,7 @@ static Builder builder(LocalTracer source) {
 
     abstract SpanCollector spanCollector();
 
-    abstract List<TraceFilter> traceFilters();
+    abstract TraceSampler traceSampler();
 
     @AutoValue.Builder
     abstract static class Builder {
@@ -66,7 +65,7 @@ abstract static class Builder {
 
         abstract Builder spanCollector(SpanCollector spanCollector);
 
-        abstract Builder traceFilters(List<TraceFilter> traceFilters);
+        abstract Builder traceSampler(TraceSampler traceSampler);
 
         abstract LocalTracer build();
     }
@@ -116,11 +115,9 @@ public SpanId startNewSpan(String component, String operation, long timestamp) {
         SpanId newSpanId = getNewSpanId();
         if (sample == null) {
             // No sample indication is present.
-            for (TraceFilter traceFilter : traceFilters()) {
-                if (!traceFilter.trace(newSpanId.getSpanId(), operation)) {
-                    spanAndEndpoint().state().setCurrentLocalSpan(null);
-                    return null;
-                }
+            if (!traceSampler().test(newSpanId.getTraceId())) {
+                spanAndEndpoint().state().setCurrentLocalSpan(null);
+                return null;
             }
         }
 

diff --git a/brave-core/src/main/java/com/github/kristofa/brave/ServerTracer.java b/brave-core/src/main/java/com/github/kristofa/brave/ServerTracer.java
@@ -7,7 +7,6 @@
 import com.twitter.zipkin.gen.Endpoint;
 import com.twitter.zipkin.gen.zipkinCoreConstants;
 
-import java.util.List;
 import java.util.Random;
 
 import static com.github.kristofa.brave.internal.Util.checkNotBlank;
@@ -38,7 +37,7 @@ public static Builder builder() {
     abstract ServerSpanAndEndpoint spanAndEndpoint();
     abstract Random randomGenerator();
     abstract SpanCollector spanCollector();
-    abstract List<TraceFilter> traceFilters();
+    abstract TraceSampler traceSampler();
 
     @AutoValue.Builder
     public abstract static class Builder {
@@ -56,11 +55,7 @@ public Builder state(ServerSpanState state) {
 
         public abstract Builder spanCollector(SpanCollector spanCollector);
 
-        /**
-         * Will be executed in order. If one returns <code>false</code> there will be no tracing and
-         * next ones will not be executed anymore. So order is important.
-         */
-        public abstract Builder traceFilters(List<TraceFilter> traceFilters);
+        public abstract Builder traceSampler(TraceSampler traceSampler);
 
         public abstract ServerTracer build();
     }
@@ -108,15 +103,13 @@ public void setStateNoTracing() {
      */
     public void setStateUnknown(String spanName) {
         checkNotBlank(spanName, "Null or blank span name");
-        long newSpanId = randomGenerator().nextLong();
-        for (TraceFilter traceFilter : traceFilters()) {
-            if (traceFilter.trace(newSpanId, spanName) == false) {
-                spanAndEndpoint().state().setCurrentServerSpan(ServerSpan.NOT_SAMPLED);
-                return;
-            }
+        long newTraceId = randomGenerator().nextLong();
+        if (!traceSampler().test(newTraceId)) {
+            spanAndEndpoint().state().setCurrentServerSpan(ServerSpan.NOT_SAMPLED);
+            return;
         }
         spanAndEndpoint().state().setCurrentServerSpan(
-            ServerSpan.create(newSpanId, newSpanId, null, spanName));
+            ServerSpan.create(newTraceId, newTraceId, null, spanName));
     }
 
     /**