Hardens integration tests, fixes small bug

zipkin-storage/cassandra/src/main/java/zipkin2/storage/cassandra/InsertSpan.java

@@ -61,9 +61,9 @@ final class InsertSpan extends ResultSetFutureCall<Void> {
 
     @Nullable abstract String annotation_query();
 
-    abstract boolean shared();
-
     abstract boolean debug();
+
+    abstract boolean shared();
   }
 
   static final class Factory {
@@ -74,12 +74,12 @@ static final class Factory {
     Factory(CqlSession session, boolean strictTraceId, boolean searchEnabled) {
       this.session = session;
       String insertQuery = "INSERT INTO " + TABLE_SPAN
-        + " (trace_id,trace_id_high,ts_uuid,parent_id,id,kind,span,ts,duration,l_ep,r_ep,annotations,tags,shared,debug)"
-        + " VALUES (:trace_id,:trace_id_high,:ts_uuid,:parent_id,:id,:kind,:span,:ts,:duration,:l_ep,:r_ep,:annotations,:tags,:shared,:debug)";
+        + " (trace_id,trace_id_high,ts_uuid,parent_id,id,kind,span,ts,duration,l_ep,r_ep,annotations,tags,debug,shared)"
+        + " VALUES (:trace_id,:trace_id_high,:ts_uuid,:parent_id,:id,:kind,:span,:ts,:duration,:l_ep,:r_ep,:annotations,:tags,:debug,:shared)";
 
       if (searchEnabled) {
-        insertQuery = insertQuery.replace(",debug)", ",debug, l_service, annotation_query)");
-        insertQuery = insertQuery.replace(",:debug)", ",:debug, :l_service, :annotation_query)");
+        insertQuery = insertQuery.replace(",shared)", ",shared, l_service, annotation_query)");
+        insertQuery = insertQuery.replace(",:shared)", ",:shared, :l_service, :annotation_query)");
       }
 
       this.preparedStatement = session.prepare(insertQuery);
@@ -165,8 +165,8 @@ Call<Void> create(Input span) {
       bound.setList("annotations", input.annotations(), Annotation.class);
     }
     if (!input.tags().isEmpty()) bound.setMap("tags", input.tags(), String.class, String.class);
-    if (input.shared()) bound.setBoolean("shared", true);
     if (input.debug()) bound.setBoolean("debug", true);
+    if (input.shared()) bound.setBoolean("shared", true);
 
     if (factory.searchEnabled) {
       if (null != input.l_ep()) bound.setString("l_service", input.l_ep().serviceName());

zipkin-storage/cassandra/src/main/java/zipkin2/storage/cassandra/SelectFromSpan.java

@@ -52,7 +52,7 @@ static final class Factory {
     Factory(CqlSession session, boolean strictTraceId, int maxTraceCols) {
       this.session = session;
       this.preparedStatement = session.prepare(
-        "SELECT trace_id_high,trace_id,parent_id,id,kind,span,ts,duration,l_ep,r_ep,annotations,tags,shared,debug"
+        "SELECT trace_id_high,trace_id,parent_id,id,kind,span,ts,duration,l_ep,r_ep,annotations,tags,debug,shared"
           + " FROM " + TABLE_SPAN
           + " WHERE trace_id IN ?"
           + " LIMIT ?");
@@ -195,15 +195,12 @@ static final class ReadSpans extends AccumulateAllResults<List<Span>> {
             // EmptyCatch ignored
           }
         }
-        if (!row.isNull("l_ep")) {
-          builder.localEndpoint(row.get("l_ep", Endpoint.class));
-        }
-        if (!row.isNull("r_ep")) {
-          builder.remoteEndpoint(row.get("r_ep", Endpoint.class));
-        }
 
+        if (!row.isNull("l_ep")) builder.localEndpoint(row.get("l_ep", Endpoint.class));
+        if (!row.isNull("r_ep")) builder.remoteEndpoint(row.get("r_ep", Endpoint.class));
+
+        if (!row.isNull("debug")) builder.debug(row.getBoolean("debug"));
         if (!row.isNull("shared")) builder.shared(row.getBoolean("shared"));
-        if (!row.isNull("debug")) builder.shared(row.getBoolean("debug"));
 
         for (Annotation annotation : row.getList("annotations", Annotation.class)) {
           builder.addAnnotation(annotation.timestamp(), annotation.value());

zipkin-storage/cassandra/src/main/resources/zipkin2-schema.cql

@@ -28,8 +28,8 @@ CREATE TABLE IF NOT EXISTS zipkin2.span (
     r_ep                Endpoint,
     annotations         list<frozen<annotation>>,
     tags                map<text,text>,
-    shared              boolean,
     debug               boolean,
+    shared              boolean,
     PRIMARY KEY (trace_id, ts_uuid, id)
 )
     WITH CLUSTERING ORDER BY (ts_uuid DESC)

zipkin-storage/elasticsearch/src/test/java/zipkin2/elasticsearch/integration/ITElasticsearchStorageV7.java

@@ -13,7 +13,6 @@
  */
 package zipkin2.elasticsearch.integration;
 
-import java.io.IOException;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.TestInstance;

zipkin-storage/mysql-v1/src/test/java/zipkin2/storage/mysql/v1/ITMySQLStorage.java

@@ -46,6 +46,14 @@ class ITTraces extends zipkin2.storage.ITTraces<MySQLStorage> {
       return backend.computeStorageBuilder();
     }
 
+    @Override @Test @Disabled("v1 format is lossy in conversion when rows as upsert")
+    protected void getTrace_differentiatesDebugFromShared(TestInfo testInfo) {
+    }
+
+    @Override @Test @Disabled("v1 format is lossy in conversion when rows as upsert")
+    protected void getTraces_differentiatesDebugFromShared(TestInfo testInfo) {
+    }
+
     @Override protected boolean returnsRawSpans() {
       return false;
     }
@@ -65,6 +73,10 @@ class ITSpanStore extends zipkin2.storage.ITSpanStore<MySQLStorage> {
       return backend.computeStorageBuilder();
     }
 
+    @Override @Test @Disabled("v1 format is lossy in conversion when rows as upsert")
+    protected void getTraces_differentiatesDebugFromShared(TestInfo testInfo) {
+    }
+
     @Override protected boolean returnsRawSpans() {
       return false;
     }

zipkin-tests/src/main/java/zipkin2/storage/ITSpanStore.java

@@ -33,6 +33,7 @@
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static zipkin2.Span.Kind.CLIENT;
+import static zipkin2.Span.Kind.SERVER;
 import static zipkin2.TestObjects.DAY;
 import static zipkin2.TestObjects.TODAY;
 import static zipkin2.TestObjects.appendSuffix;
@@ -546,6 +547,22 @@ void getTraces_spanName(Span clientSpan) throws Exception {
       asList(spanWithoutTimestamp, span));
   }
 
+  /** Prevents subtle bugs which can result in mixed-length traces from linking. */
+  @Test protected void getTraces_differentiatesDebugFromShared(TestInfo testInfo) throws Exception {
+    String testSuffix = testSuffix(testInfo);
+    Span clientSpan = newClientSpan(testSuffix).toBuilder()
+      .debug(true)
+      .build();
+    Span serverSpan = clientSpan.toBuilder().kind(SERVER)
+      .debug(null).shared(true)
+      .build();
+
+    accept(clientSpan, serverSpan);
+
+    // assertGetTracesReturns does recursive comparison
+    assertGetTracesReturns(requestBuilder().build(), asList(clientSpan, serverSpan));
+  }
+
   @Test protected void getTraces_annotation(TestInfo testInfo) throws Exception {
     String testSuffix = testSuffix(testInfo);
     Span clientSpan = newClientSpan(testSuffix).toBuilder()
@@ -645,7 +662,7 @@ protected void getTraces_multipleAnnotationsBecomeAndFilter(TestInfo testInfo) t
       .build();
 
     Span trace1Server = Span.newBuilder().traceId(trace1.traceId()).name("1").id(1)
-      .kind(Span.Kind.SERVER)
+      .kind(SERVER)
       .shared(true)
       .localEndpoint(backend)
       .timestamp((TODAY + 2) * 1000L)
@@ -664,7 +681,7 @@ protected void getTraces_multipleAnnotationsBecomeAndFilter(TestInfo testInfo) t
 
     Span trace2Server = Span.newBuilder().traceId(trace2.traceId()).name("2").id(2)
       .shared(true)
-      .kind(Span.Kind.SERVER)
+      .kind(SERVER)
       .localEndpoint(frontend)
       .timestamp((TODAY + 12) * 1000L)
       .duration(1000L).build();

zipkin-tests/src/main/java/zipkin2/storage/ITStorage.java

@@ -27,6 +27,7 @@
 import zipkin2.Span;
 import zipkin2.internal.Trace;
 
+import static java.lang.Boolean.TRUE;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static zipkin2.TestObjects.DAY;
@@ -137,6 +138,7 @@ protected static QueryRequest.Builder requestBuilder() {
   protected void assertGetTracesReturns(QueryRequest request, List<Span>... traces)
     throws IOException {
     assertThat(sortTraces(store().getTraces(request).execute()))
+      .usingRecursiveFieldByFieldElementComparator()
       .containsAll(sortTraces(asList(traces)));
   }
 
@@ -146,6 +148,7 @@ protected void assertGetTraceReturns(Span onlySpan) throws IOException {
 
   protected void assertGetTraceReturns(String traceId, List<Span> trace) throws IOException {
     assertThat(sortTrace(storage.traces().getTrace(traceId).execute()))
+      .usingRecursiveFieldByFieldElementComparator()
       .containsAll(sortTrace(trace));
   }
 
@@ -160,6 +163,7 @@ protected void assertGetTraceReturnsEmpty(String traceId)
   protected void assertGetTracesReturns(List<String> traceIds, List<Span>... traces)
     throws IOException {
     assertThat(sortTraces(storage.traces().getTraces(traceIds).execute()))
+      .usingRecursiveFieldByFieldElementComparator()
       .containsAll(sortTraces(asList(traces)));
   }
 
@@ -176,6 +180,7 @@ protected void assertGetTracesReturnsCount(QueryRequest request, int traceCount)
     assertThat(countReturned)
       .withFailMessage("Expected <%s> traces for request <%s>, but received <%s>",
         traceCount, request, countReturned)
+      .usingRecursiveComparison()
       .isEqualTo(traceCount);
   }
 
@@ -222,6 +227,8 @@ protected List<Span> sortTrace(List<Span> trace) {
       if (traceId != 0) return traceId;
       int id = l.id().compareTo(r.id());
       if (id != 0) return id;
+      int shared = Boolean.compare(TRUE.equals(l.shared()), TRUE.equals(r.shared()));
+      if (shared != 0) return shared;
 
       if (l.name() != null && r.name() != null) {
         int name = l.name().compareTo(r.name());

zipkin-tests/src/main/java/zipkin2/storage/ITTraces.java

@@ -20,6 +20,7 @@
 import zipkin2.Span;
 
 import static java.util.Arrays.asList;
+import static zipkin2.Span.Kind.SERVER;
 import static zipkin2.TestObjects.newClientSpan;
 import static zipkin2.TestObjects.newTraceId;
 import static zipkin2.TestObjects.spanBuilder;
@@ -48,6 +49,22 @@ public abstract class ITTraces<T extends StorageComponent> extends ITStorage<T>
     assertGetTraceReturnsEmpty(clientSpan.traceId().substring(16));
   }
 
+  /** Prevents subtle bugs which can result in mixed-length traces from linking. */
+  @Test protected void getTrace_differentiatesDebugFromShared(TestInfo testInfo) throws Exception {
+    String testSuffix = testSuffix(testInfo);
+    Span clientSpan = newClientSpan(testSuffix).toBuilder()
+      .debug(true)
+      .build();
+    Span serverSpan = clientSpan.toBuilder().kind(SERVER)
+      .debug(null).shared(true)
+      .build();
+
+    accept(clientSpan, serverSpan);
+
+    // assertGetTraceReturns does recursive comparison
+    assertGetTraceReturns(clientSpan.traceId(), asList(clientSpan, serverSpan));
+  }
+
   @Test protected void getTraces_onlyReturnsTracesThatMatch(TestInfo testInfo) throws Exception {
     String testSuffix = testSuffix(testInfo);
     Span span1 = spanBuilder(testSuffix).build(), span2 = spanBuilder(testSuffix).build();
@@ -64,6 +81,22 @@ public abstract class ITTraces<T extends StorageComponent> extends ITStorage<T>
     assertGetTracesReturnsEmpty(shortTraceIds);
   }
 
+  /** Prevents subtle bugs which can result in mixed-length traces from linking. */
+  @Test protected void getTraces_differentiatesDebugFromShared(TestInfo testInfo) throws Exception {
+    String testSuffix = testSuffix(testInfo);
+    Span clientSpan = newClientSpan(testSuffix).toBuilder()
+      .debug(true)
+      .build();
+    Span serverSpan = clientSpan.toBuilder().kind(SERVER)
+      .debug(null).shared(true)
+      .build();
+
+    accept(clientSpan, serverSpan);
+
+    // assertGetTracesReturns does recursive comparison
+    assertGetTracesReturns(asList(clientSpan.traceId()), asList(clientSpan, serverSpan));
+  }
+
   @Test protected void getTraces_returnsEmptyOnNotFound(TestInfo testInfo) throws Exception {
     String testSuffix = testSuffix(testInfo);
     Span span1 = spanBuilder(testSuffix).build(), span2 = spanBuilder(testSuffix).build();