Skip to content

Commit

Permalink
Updates to zipkin 1.31 and removes more internal usages (#477)
Browse files Browse the repository at this point in the history
  • Loading branch information
adriancole committed Sep 1, 2017
1 parent 417708e commit 78dff73
Show file tree
Hide file tree
Showing 15 changed files with 55 additions and 53 deletions.
Expand Up @@ -2,8 +2,6 @@

import java.util.Random;

import static zipkin.internal.Util.checkArgument;

/**
* This sampler is appropriate for high-traffic instrumentation (ex edge web servers that each
* receive >100K requests) who provision random trace ids, and make the sampling decision only once.
Expand All @@ -28,7 +26,9 @@ public final class BoundarySampler extends Sampler {
public static Sampler create(float rate) {
if (rate == 0) return Sampler.NEVER_SAMPLE;
if (rate == 1.0) return ALWAYS_SAMPLE;
checkArgument(rate >= 0.0001f && rate < 1, "rate should be between 0.0001 and 1: was %s", rate);
if (rate < 0.0001f || rate > 1) {
throw new IllegalArgumentException("rate should be between 0.0001 and 1: was " + rate);
}
final long boundary = (long) (rate * 10000); // safe cast as less <= 1
return new BoundarySampler(boundary);
}
Expand Down
Expand Up @@ -19,7 +19,6 @@

import static com.github.kristofa.brave.InetAddressUtilities.getLocalHostLANAddress;
import static com.github.kristofa.brave.InetAddressUtilities.toInt;
import static zipkin.internal.Util.checkNotNull;

/** @deprecated Replaced by {@link brave.Tracing} */
@Deprecated
Expand Down Expand Up @@ -150,7 +149,8 @@ public Builder traceSampler(Sampler sampler) {
* <p>See https://github.com/openzipkin/zipkin-reporter-java
*/
public Builder reporter(Reporter<zipkin.Span> reporter) {
this.reporter = checkNotNull(reporter, "reporter");
if (reporter == null) throw new NullPointerException("reporter == null");
this.reporter = reporter;
return this;
}

Expand All @@ -175,7 +175,8 @@ public Builder spanCollector(SpanCollector spanCollector) {
}

public Builder clock(Clock clock) {
this.clock = checkNotNull(clock, "clock");
if (clock == null) throw new NullPointerException("clock == null");
this.clock = clock;
return this;
}

Expand Down
Expand Up @@ -3,8 +3,6 @@
import java.util.BitSet;
import java.util.Random;

import static zipkin.internal.Util.checkArgument;

/**
* 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
Expand All @@ -25,7 +23,9 @@ public final class CountingSampler extends Sampler {
public static Sampler create(final float rate) {
if (rate == 0) return NEVER_SAMPLE;
if (rate == 1.0) return ALWAYS_SAMPLE;
checkArgument(rate >= 0.01f && rate < 1, "rate should be between 0.01 and 1: was %s", rate);
if (rate < 0.01f || rate > 1) {
throw new IllegalArgumentException("rate should be between 0.01 and 1: was " + rate);
}
return new CountingSampler(rate);
}

Expand Down
Expand Up @@ -4,8 +4,6 @@
import javax.annotation.Generated;
import javax.annotation.Nullable;

import static zipkin.internal.Util.equal;

/**
* An annotation is similar to a log statement. It includes a host field which
* allows these events to be attributed properly, and also aggregatable.
Expand Down Expand Up @@ -52,6 +50,10 @@ public boolean equals(Object o) {
return false;
}

static boolean equal(Object a, Object b) {
return a == b || a != null && a.equals(b);
}

@Override
public int hashCode() {
int h = 1;
Expand Down
Expand Up @@ -7,7 +7,6 @@

import static com.github.kristofa.brave.internal.Util.checkNotNull;
import static com.github.kristofa.brave.internal.Util.equal;
import static zipkin.internal.Util.checkArgument;

/**
* Indicates the network context of a service recording an annotation with two
Expand Down Expand Up @@ -121,7 +120,7 @@ public Endpoint.Builder ipv4(int ipv4) {
/** @see Endpoint#ipv6 */
public Endpoint.Builder ipv6(byte[] ipv6) {
if (ipv6 != null) {
checkArgument(ipv6.length == 16, "ipv6 addresses are 16 bytes: " + ipv6.length);
if (ipv6.length != 16) throw new IllegalArgumentException("ipv6.length != 16");
this.ipv6 = ipv6;
}
return this;
Expand All @@ -138,7 +137,7 @@ public Endpoint.Builder ipv6(byte[] ipv6) {
* @see Endpoint#port
*/
public Endpoint.Builder port(int port) {
checkArgument(port >= 0 && port <= 0xffff, "invalid port %s", port);
if (port < 0 || port > 0xffff) throw new IllegalArgumentException("invalid port " + port);
this.port = port == 0 ? null : (short) (port & 0xffff);
return this;
}
Expand Down
Expand Up @@ -6,6 +6,7 @@
import com.github.kristofa.brave.SpanId;
import com.github.kristofa.brave.TracerAdapter;
import com.twitter.zipkin.gen.Span;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.atomic.AtomicLong;
Expand All @@ -18,9 +19,9 @@
import static com.github.kristofa.brave.TracerAdapter.toSpan;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.tuple;
import static zipkin.internal.Util.UTF_8;

public class TracerAdapterTest {
static final Charset UTF_8 = Charset.forName("UTF-8");

List<zipkin.Span> spans = new ArrayList<>();
AtomicLong epochMicros = new AtomicLong();
Expand Down
Expand Up @@ -19,7 +19,6 @@
import zipkin.Endpoint;
import zipkin.Span;
import zipkin.TraceKeys;
import zipkin.internal.Util;
import zipkin.storage.InMemoryStorage;

import static java.util.Arrays.asList;
Expand Down Expand Up @@ -275,7 +274,7 @@ public void httpUrlTagIncludesQueryParams() throws Exception {
assertThat(collectedSpans())
.flatExtracting(s -> s.binaryAnnotations)
.filteredOn(b -> b.key.equals(TraceKeys.HTTP_URL))
.extracting(b -> new String(b.value, Util.UTF_8))
.extracting(b -> new String(b.value, "UTF-8"))
.containsExactly(url(path).toString());
}

Expand Down
6 changes: 4 additions & 2 deletions brave/pom.xml
Expand Up @@ -142,10 +142,12 @@
</artifactSet>
<filters>
<filter>
<!-- Shade references to span2 until zipkin2 is out -->
<!-- Shade references to v2 until zipkin2 is out -->
<artifact>io.zipkin.java:zipkin</artifact>
<includes>
<include>zipkin/internal/*Span2*.class</include>
<include>zipkin/internal/v2/*Span*.class</include>
<include>zipkin/internal/V2SpanConverter*.class</include>
<!-- TODO: v2 currently indirectly references Util -->
<include>zipkin/internal/Util.class</include>
</includes>
<excludes>
Expand Down
9 changes: 4 additions & 5 deletions brave/src/main/java/brave/internal/recorder/MutableSpan.java
Expand Up @@ -4,17 +4,16 @@
import brave.propagation.TraceContext;
import javax.annotation.Nullable;
import zipkin.Endpoint;
import zipkin.internal.Span2;

final class MutableSpan {
final Span2.Builder span;
final zipkin.internal.v2.Span.Builder span;
boolean finished;
long timestamp;

// Since this is not exposed, this class could be refactored later as needed to act in a pool
// to reduce GC churn. This would involve calling span.clear and resetting the fields below.
MutableSpan(TraceContext context, Endpoint localEndpoint) {
this.span = Span2.builder()
this.span = zipkin.internal.v2.Span.builder()
.traceIdHigh(context.traceIdHigh())
.traceId(context.traceId())
.parentId(context.parentId())
Expand All @@ -37,7 +36,7 @@ synchronized MutableSpan name(String name) {

synchronized MutableSpan kind(Span.Kind kind) {
try {
span.kind(Span2.Kind.valueOf(kind.name()));
span.kind(zipkin.internal.v2.Span.Kind.valueOf(kind.name()));
} catch (IllegalArgumentException e) {
// TODO: log
}
Expand Down Expand Up @@ -70,7 +69,7 @@ synchronized MutableSpan finish(@Nullable Long finishTimestamp) {
return this;
}

synchronized Span2 toSpan() {
synchronized zipkin.internal.v2.Span toSpan() {
return span.build();
}
}
Expand Up @@ -12,7 +12,7 @@
import java.util.logging.Logger;
import javax.annotation.Nullable;
import zipkin.Endpoint;
import zipkin.internal.Span2Converter;
import zipkin.internal.V2SpanConverter;
import zipkin.reporter.Reporter;

/**
Expand Down Expand Up @@ -80,7 +80,7 @@ void reportOrphanedSpans() {
if (value == null || noop.get()) continue;
try {
value.annotate(clock.currentTimeMicroseconds(), "brave.flush");
reporter.report(Span2Converter.toSpan(value.toSpan()));
reporter.report(V2SpanConverter.toSpan(value.toSpan()));
} catch (RuntimeException e) {
// don't crash the caller if there was a problem reporting an unrelated span.
if (context != null && logger.isLoggable(Level.FINE)) {
Expand Down
6 changes: 3 additions & 3 deletions brave/src/main/java/brave/internal/recorder/Recorder.java
Expand Up @@ -6,7 +6,7 @@
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.Nullable;
import zipkin.Endpoint;
import zipkin.internal.Span2Converter;
import zipkin.internal.V2SpanConverter;
import zipkin.reporter.Reporter;

/** Dispatches mutations on a span to a shared object per trace/span id. */
Expand Down Expand Up @@ -81,7 +81,7 @@ public void finish(TraceContext context, long finishTimestamp) {
if (span == null || noop.get()) return;
synchronized (span) {
span.finish(finishTimestamp);
reporter.report(Span2Converter.toSpan(span.toSpan()));
reporter.report(V2SpanConverter.toSpan(span.toSpan()));
}
}

Expand All @@ -96,7 +96,7 @@ public void flush(TraceContext context) {
if (span == null || noop.get()) return;
synchronized (span) {
span.finish(null);
reporter.report(Span2Converter.toSpan(span.toSpan()));
reporter.report(V2SpanConverter.toSpan(span.toSpan()));
}
}
}
6 changes: 3 additions & 3 deletions brave/src/main/java/brave/sampler/BoundarySampler.java
Expand Up @@ -2,8 +2,6 @@

import java.util.Random;

import static zipkin.internal.Util.checkArgument;

/**
* This sampler is appropriate for high-traffic instrumentation (ex edge web servers that each
* receive >100K requests) who provision random trace ids, and make the sampling decision only once.
Expand All @@ -28,7 +26,9 @@ public final class BoundarySampler extends Sampler {
public static Sampler create(float rate) {
if (rate == 0) return Sampler.NEVER_SAMPLE;
if (rate == 1.0) return ALWAYS_SAMPLE;
checkArgument(rate >= 0.0001f && rate < 1, "rate should be between 0.0001 and 1: was %s", rate);
if (rate < 0.0001f || rate > 1) {
throw new IllegalArgumentException("rate should be between 0.0001 and 1: was " + rate);
}
final long boundary = (long) (rate * 10000); // safe cast as less <= 1
return new BoundarySampler(boundary);
}
Expand Down
6 changes: 3 additions & 3 deletions brave/src/main/java/brave/sampler/CountingSampler.java
Expand Up @@ -3,8 +3,6 @@
import java.util.BitSet;
import java.util.Random;

import static zipkin.internal.Util.checkArgument;

/**
* 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
Expand All @@ -25,7 +23,9 @@ public final class CountingSampler extends Sampler {
public static Sampler create(final float rate) {
if (rate == 0) return NEVER_SAMPLE;
if (rate == 1.0) return ALWAYS_SAMPLE;
checkArgument(rate >= 0.01f && rate < 1, "rate should be between 0.01 and 1: was %s", rate);
if (rate < 0.01f || rate > 1) {
throw new IllegalArgumentException("rate should be between 0.01 and 1: was " + rate);
}
return new CountingSampler(rate);
}

Expand Down
31 changes: 15 additions & 16 deletions brave/src/test/java/brave/internal/recorder/MutableSpanTest.java
Expand Up @@ -8,8 +8,7 @@
import org.junit.Test;
import zipkin.Annotation;
import zipkin.Endpoint;
import zipkin.internal.Span2;
import zipkin.internal.Span2Converter;
import zipkin.internal.V2SpanConverter;

import static brave.Span.Kind.CLIENT;
import static brave.Span.Kind.SERVER;
Expand Down Expand Up @@ -54,57 +53,57 @@ public class MutableSpanTest {
}

@Test public void finished_client() {
finish(Span.Kind.CLIENT, Span2.Kind.CLIENT);
finish(Span.Kind.CLIENT, zipkin.internal.v2.Span.Kind.CLIENT);
}

@Test public void finished_server() {
finish(Span.Kind.SERVER, Span2.Kind.SERVER);
finish(Span.Kind.SERVER, zipkin.internal.v2.Span.Kind.SERVER);
}

@Test public void finished_producer() {
finish(Span.Kind.PRODUCER, Span2.Kind.PRODUCER);
finish(Span.Kind.PRODUCER, zipkin.internal.v2.Span.Kind.PRODUCER);
}

@Test public void finished_consumer() {
finish(Span.Kind.CONSUMER, Span2.Kind.CONSUMER);
finish(Span.Kind.CONSUMER, zipkin.internal.v2.Span.Kind.CONSUMER);
}

private void finish(Span.Kind braveKind, Span2.Kind span2Kind) {
private void finish(Span.Kind braveKind, zipkin.internal.v2.Span.Kind span2Kind) {
MutableSpan span = newSpan();
span.kind(braveKind);
span.start(1L);
span.finish(2L);

Span2 span2 = span.toSpan();
zipkin.internal.v2.Span span2 = span.toSpan();
assertThat(span2.annotations()).isEmpty();
assertThat(span2.timestamp()).isEqualTo(1L);
assertThat(span2.duration()).isEqualTo(1L);
assertThat(span2.kind()).isEqualTo(span2Kind);
}

@Test public void flushed_client() {
flush(Span.Kind.CLIENT, Span2.Kind.CLIENT);
flush(Span.Kind.CLIENT, zipkin.internal.v2.Span.Kind.CLIENT);
}

@Test public void flushed_server() {
flush(Span.Kind.SERVER, Span2.Kind.SERVER);
flush(Span.Kind.SERVER, zipkin.internal.v2.Span.Kind.SERVER);
}

@Test public void flushed_producer() {
flush(Span.Kind.PRODUCER, Span2.Kind.PRODUCER);
flush(Span.Kind.PRODUCER, zipkin.internal.v2.Span.Kind.PRODUCER);
}

@Test public void flushed_consumer() {
flush(Span.Kind.CONSUMER, Span2.Kind.CONSUMER);
flush(Span.Kind.CONSUMER, zipkin.internal.v2.Span.Kind.CONSUMER);
}

private void flush(Span.Kind braveKind, Span2.Kind span2Kind) {
private void flush(Span.Kind braveKind, zipkin.internal.v2.Span.Kind span2Kind) {
MutableSpan span = newSpan();
span.kind(braveKind);
span.start(1L);
span.finish(null);

Span2 span2 = span.toSpan();
zipkin.internal.v2.Span span2 = span.toSpan();
assertThat(span2.annotations()).isEmpty();
assertThat(span2.timestamp()).isEqualTo(1L);
assertThat(span2.duration()).isNull();
Expand All @@ -131,12 +130,12 @@ private void flush(Span.Kind braveKind, Span2.Kind span2Kind) {
span.kind(SERVER);
span.finish(2L);

assertThat(Span2Converter.toSpan(span.toSpan())).extracting(s -> s.timestamp, s -> s.duration)
assertThat(V2SpanConverter.toSpan(span.toSpan())).extracting(s -> s.timestamp, s -> s.duration)
.allSatisfy(u -> assertThat(u).isNull());
}

@Test public void flushUnstartedNeitherSetsTimestampNorDuration() {
Span2 flushed = newSpan().finish(null).toSpan();
zipkin.internal.v2.Span flushed = newSpan().finish(null).toSpan();
assertThat(flushed).extracting(s -> s.timestamp(), s -> s.duration())
.allSatisfy(u -> assertThat(u).isNull());
}
Expand Down
4 changes: 2 additions & 2 deletions pom.xml
Expand Up @@ -66,8 +66,8 @@
<jetty-servlet25.version>7.6.21.v20160908</jetty-servlet25.version>
<!-- Note: 3.1.x requires Java 8; 3.0.20.Final is broken -->
<resteasy.version>3.1.3.Final</resteasy.version>
<zipkin.version>1.30.3</zipkin.version>
<zipkin-reporter.version>1.0.1</zipkin-reporter.version>
<zipkin.version>1.31.0</zipkin.version>
<zipkin-reporter.version>1.0.2</zipkin-reporter.version>
<finagle.version>6.45.0</finagle.version>
<log4j.version>2.8.2</log4j.version>
<okhttp.version>3.8.1</okhttp.version>
Expand Down

0 comments on commit 78dff73

Please sign in to comment.