You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zipkin.apache.org by ad...@apache.org on 2019/06/15 13:23:30 UTC
[incubator-zipkin-brave] 01/01: Fixes assertion by making equals
the same between lazy and real span
This is an automated email from the ASF dual-hosted git repository.
adriancole pushed a commit to branch mirror-equals
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin-brave.git
commit d6f6c49ff0a84765d407a0fb28a643d347bb8f1c
Author: Adrian Cole <ac...@pivotal.io>
AuthorDate: Sat Jun 15 21:22:08 2019 +0800
Fixes assertion by making equals the same between lazy and real span
We had an assertion in `ThreadLocalSpan` which broke because our equals
comparison was one way. This fixes it.
---
brave/src/main/java/brave/LazySpan.java | 9 +--
brave/src/main/java/brave/RealSpan.java | 24 +++++--
brave/src/test/java/brave/LazySpanTest.java | 82 ++++++++++++++++++++++
brave/src/test/java/brave/RealSpanTest.java | 73 ++++++++++++++-----
.../brave/propagation/ThreadLocalSpanTest.java | 48 +++++++++++++
5 files changed, 207 insertions(+), 29 deletions(-)
diff --git a/brave/src/main/java/brave/LazySpan.java b/brave/src/main/java/brave/LazySpan.java
index 90aae26..bbf6a45 100644
--- a/brave/src/main/java/brave/LazySpan.java
+++ b/brave/src/main/java/brave/LazySpan.java
@@ -16,6 +16,8 @@ package brave;
import brave.handler.MutableSpan;
import brave.propagation.TraceContext;
+import static brave.RealSpan.isEqualToRealOrLazySpan;
+
/**
* This defers creation of a span until first public method call.
*
@@ -112,12 +114,7 @@ final class LazySpan extends Span {
*/
@Override public boolean equals(Object o) {
if (o == this) return true;
- if (o instanceof LazySpan) {
- return context.equals(((LazySpan) o).context);
- } else if (o instanceof RealSpan) {
- return context.equals(((RealSpan) o).context);
- }
- return false;
+ return isEqualToRealOrLazySpan(context, o);
}
/**
diff --git a/brave/src/main/java/brave/RealSpan.java b/brave/src/main/java/brave/RealSpan.java
index 1764517..b827ac4 100644
--- a/brave/src/main/java/brave/RealSpan.java
+++ b/brave/src/main/java/brave/RealSpan.java
@@ -28,10 +28,10 @@ final class RealSpan extends Span {
final FinishedSpanHandler finishedSpanHandler;
RealSpan(TraceContext context,
- PendingSpans pendingSpans,
- MutableSpan state,
- Clock clock,
- FinishedSpanHandler finishedSpanHandler
+ PendingSpans pendingSpans,
+ MutableSpan state,
+ Clock clock,
+ FinishedSpanHandler finishedSpanHandler
) {
this.context = context;
this.pendingSpans = pendingSpans;
@@ -164,10 +164,22 @@ final class RealSpan extends Span {
return "RealSpan(" + context + ")";
}
+ /**
+ * This also matches equals against a lazy span. The rationale is least surprise to the user, as
+ * code should not act differently given an instance of lazy or {@link RealSpan}.
+ */
@Override public boolean equals(Object o) {
if (o == this) return true;
- if (!(o instanceof RealSpan)) return false;
- return context.equals(((RealSpan) o).context);
+ return isEqualToRealOrLazySpan(context, o);
+ }
+
+ static boolean isEqualToRealOrLazySpan(TraceContext context, Object o) {
+ if (o instanceof LazySpan) {
+ return context.equals(((LazySpan) o).context);
+ } else if (o instanceof RealSpan) {
+ return context.equals(((RealSpan) o).context);
+ }
+ return false;
}
@Override public int hashCode() {
diff --git a/brave/src/test/java/brave/LazySpanTest.java b/brave/src/test/java/brave/LazySpanTest.java
new file mode 100644
index 0000000..ce41044
--- /dev/null
+++ b/brave/src/test/java/brave/LazySpanTest.java
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2013-2019 The OpenZipkin 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
+ *
+ * http://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 brave;
+
+import brave.propagation.CurrentTraceContext.Scope;
+import brave.propagation.ThreadLocalCurrentTraceContext;
+import brave.propagation.TraceContext;
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.After;
+import org.junit.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class LazySpanTest {
+ List<zipkin2.Span> spans = new ArrayList();
+ Tracing tracing = Tracing.newBuilder()
+ .currentTraceContext(ThreadLocalCurrentTraceContext.create())
+ .spanReporter(spans::add)
+ .build();
+
+ TraceContext context = TraceContext.newBuilder().traceId(1L).spanId(1L).sampled(true).build();
+ TraceContext context2 = TraceContext.newBuilder().traceId(1L).spanId(2L).sampled(true).build();
+
+ @After public void close() {
+ tracing.close();
+ }
+
+ @Test public void equals_sameContext() {
+ Span current1, current2;
+ try (Scope ws = tracing.currentTraceContext().newScope(context)) {
+ current1 = tracing.tracer().currentSpan();
+ current2 = tracing.tracer().currentSpan();
+ }
+
+ assertThat(current1)
+ .isInstanceOf(LazySpan.class)
+ .isNotSameAs(current2)
+ .isEqualTo(current2);
+ }
+
+ @Test public void equals_notSameContext() {
+ Span current1, current2;
+ try (Scope ws = tracing.currentTraceContext().newScope(context)) {
+ current1 = tracing.tracer().currentSpan();
+ }
+ try (Scope ws = tracing.currentTraceContext().newScope(context2)) {
+ current2 = tracing.tracer().currentSpan();
+ }
+
+ assertThat(current1).isNotEqualTo(current2);
+ }
+
+ @Test public void equals_realSpan_sameContext() {
+ Span current;
+ try (Scope ws = tracing.currentTraceContext().newScope(context)) {
+ current = tracing.tracer().currentSpan();
+ }
+
+ assertThat(current).isEqualTo(tracing.tracer().toSpan(context));
+ }
+
+ @Test public void equals_realSpan_notSameContext() {
+ Span current;
+ try (Scope ws = tracing.currentTraceContext().newScope(context)) {
+ current = tracing.tracer().currentSpan();
+ }
+
+ assertThat(current).isNotEqualTo(tracing.tracer().toSpan(context2));
+ }
+}
diff --git a/brave/src/test/java/brave/RealSpanTest.java b/brave/src/test/java/brave/RealSpanTest.java
index 5d6d143..eb024e0 100644
--- a/brave/src/test/java/brave/RealSpanTest.java
+++ b/brave/src/test/java/brave/RealSpanTest.java
@@ -13,7 +13,9 @@
*/
package brave;
+import brave.propagation.CurrentTraceContext;
import brave.propagation.ThreadLocalCurrentTraceContext;
+import brave.propagation.TraceContext;
import java.util.ArrayList;
import java.util.List;
import org.junit.After;
@@ -27,9 +29,13 @@ import static org.assertj.core.api.Assertions.entry;
public class RealSpanTest {
List<zipkin2.Span> spans = new ArrayList();
Tracing tracing = Tracing.newBuilder()
- .currentTraceContext(ThreadLocalCurrentTraceContext.create())
- .spanReporter(spans::add)
- .build();
+ .currentTraceContext(ThreadLocalCurrentTraceContext.create())
+ .spanReporter(spans::add)
+ .build();
+
+ TraceContext context = TraceContext.newBuilder().traceId(1L).spanId(1L).sampled(true).build();
+ TraceContext context2 = TraceContext.newBuilder().traceId(1L).spanId(2L).sampled(true).build();
+
Span span = tracing.tracer().newTrace();
@After public void close() {
@@ -53,8 +59,8 @@ public class RealSpanTest {
span.flush();
assertThat(spans).hasSize(1).first()
- .extracting(zipkin2.Span::timestamp)
- .isNotNull();
+ .extracting(zipkin2.Span::timestamp)
+ .isNotNull();
}
@Test public void start_timestamp() {
@@ -62,8 +68,8 @@ public class RealSpanTest {
span.flush();
assertThat(spans).hasSize(1).first()
- .extracting(zipkin2.Span::timestamp)
- .isEqualTo(2L);
+ .extracting(zipkin2.Span::timestamp)
+ .isEqualTo(2L);
}
@Test public void finish() {
@@ -71,8 +77,8 @@ public class RealSpanTest {
span.finish();
assertThat(spans).hasSize(1).first()
- .extracting(zipkin2.Span::duration)
- .isNotNull();
+ .extracting(zipkin2.Span::duration)
+ .isNotNull();
}
@Test public void finish_timestamp() {
@@ -80,8 +86,8 @@ public class RealSpanTest {
span.finish(5);
assertThat(spans).hasSize(1).first()
- .extracting(zipkin2.Span::duration)
- .isEqualTo(3L);
+ .extracting(zipkin2.Span::duration)
+ .isEqualTo(3L);
}
@Test public void abandon() {
@@ -96,8 +102,8 @@ public class RealSpanTest {
span.flush();
assertThat(spans).flatExtracting(zipkin2.Span::annotations)
- .extracting(Annotation::value)
- .containsExactly("foo");
+ .extracting(Annotation::value)
+ .containsExactly("foo");
}
@Test public void remoteEndpoint_nulls() {
@@ -112,7 +118,7 @@ public class RealSpanTest {
span.flush();
assertThat(spans).flatExtracting(zipkin2.Span::annotations)
- .containsExactly(Annotation.create(2L, "foo"));
+ .containsExactly(Annotation.create(2L, "foo"));
}
@Test public void tag() {
@@ -120,7 +126,7 @@ public class RealSpanTest {
span.flush();
assertThat(spans).flatExtracting(s -> s.tags().entrySet())
- .containsExactly(entry("foo", "bar"));
+ .containsExactly(entry("foo", "bar"));
}
@Test public void finished_client_annotation() {
@@ -173,7 +179,7 @@ public class RealSpanTest {
span.flush();
assertThat(spans).flatExtracting(s -> s.tags().entrySet())
- .containsExactly(entry("error", "this cake is a lie"));
+ .containsExactly(entry("error", "this cake is a lie"));
}
@Test public void error_noMessage() {
@@ -181,6 +187,39 @@ public class RealSpanTest {
span.flush();
assertThat(spans).flatExtracting(s -> s.tags().entrySet())
- .containsExactly(entry("error", "RuntimeException"));
+ .containsOnly(entry("error", "RuntimeException"));
+ }
+
+ @Test public void equals_sameContext() {
+ Span one = tracing.tracer().toSpan(context), two = tracing.tracer().toSpan(context);
+
+ assertThat(one)
+ .isInstanceOf(RealSpan.class)
+ .isNotSameAs(two)
+ .isEqualTo(two);
+ }
+
+ @Test public void equals_notSameContext() {
+ Span one = tracing.tracer().toSpan(context), two = tracing.tracer().toSpan(context2);
+
+ assertThat(one).isNotEqualTo(two);
+ }
+
+ @Test public void equals_realSpan_sameContext() {
+ Span current;
+ try (CurrentTraceContext.Scope ws = tracing.currentTraceContext().newScope(context)) {
+ current = tracing.tracer().currentSpan();
+ }
+
+ assertThat(tracing.tracer().toSpan(context)).isEqualTo(current);
+ }
+
+ @Test public void equals_realSpan_notSameContext() {
+ Span current;
+ try (CurrentTraceContext.Scope ws = tracing.currentTraceContext().newScope(context2)) {
+ current = tracing.tracer().currentSpan();
+ }
+
+ assertThat(tracing.tracer().toSpan(context)).isNotEqualTo(current);
}
}
diff --git a/brave/src/test/java/brave/propagation/ThreadLocalSpanTest.java b/brave/src/test/java/brave/propagation/ThreadLocalSpanTest.java
new file mode 100644
index 0000000..ae9b58f
--- /dev/null
+++ b/brave/src/test/java/brave/propagation/ThreadLocalSpanTest.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright 2013-2019 The OpenZipkin 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
+ *
+ * http://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 brave.propagation;
+
+import brave.Tracing;
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.After;
+import org.junit.Test;
+import zipkin2.Span;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ThreadLocalSpanTest {
+
+ List<Span> spans = new ArrayList<>();
+ Tracing tracing = Tracing.newBuilder()
+ .currentTraceContext(ThreadLocalCurrentTraceContext.create())
+ .spanReporter(spans::add)
+ .build();
+
+ ThreadLocalSpan threadLocalSpan = ThreadLocalSpan.create(tracing.tracer());
+
+ @After public void close() {
+ tracing.close();
+ }
+
+ @Test public void next() {
+ assertThat(threadLocalSpan.next())
+ .isEqualTo(threadLocalSpan.remove());
+ }
+
+ @Test public void next_extracted() {
+ assertThat(threadLocalSpan.next(TraceContextOrSamplingFlags.DEBUG))
+ .isEqualTo(threadLocalSpan.remove());
+ }
+}