zipkin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adrianc...@apache.org
Subject [incubator-zipkin-brave] 01/01: Fixes assertion by making equals the same between lazy and real span
Date Sat, 15 Jun 2019 13:23:30 GMT
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 <acole@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());
+  }
+}


Mime
View raw message