Supports backfilling timestamp for one-way spans

Before, we only supported backfilling timestamp and duration for spans
that have at least two annotations. This special cases an incomplete
one-way span (ex only cs), by adding its timestamp. This prevents the
UI from crashing.

zipkin/src/main/java/zipkin/internal/ApplyTimestampAndDuration.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015-2016 The OpenZipkin Authors
+ * Copyright 2015-2017 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
@@ -20,30 +20,37 @@
 /**
  * <h3>Derived timestamp and duration</h3>
  *
- * <p>Instrumentation should log timestamp and duration, but since these fields are recent
- * (Nov-2015), a lot of tracers will not. Accordingly, this will backfill timestamp and duration to
- * if possible, based on interpretation of annotations.
+ * <p>Instrumentation should log timestamp and duration in most cases, but since these fields are
+ * recent (Nov-2015), a lot of tracers will not. They also will not log timestamp or duration in
+ * one-way spans ("cs", "sr"). This includes a utility to backfill timestamp and duration at query
+ * time. It also includes a utility to guess a timestamp, which is useful when indexing incomplete
+ * spans.
  */
 public class ApplyTimestampAndDuration {
 
-  // For spans that core client annotations, the distance between "cs" and "cr" should be the
-  // authoritative duration. We are special-casing this to avoid setting incorrect duration
-  // when there's skew between the client and the server.
+  /**
+   * For RPC two-way spans, the duration between "cs" and "cr" is authoritative. RPC one-way spans
+   * lack a response, so the duration is between "cs" and "sr". We special-case this to avoid
+   * setting incorrect duration when there's skew between the client and the server.
+   *
+   * <p>Note: this should only be used for query, not storage commands!
+   */
   public static Span apply(Span span) {
     // Don't overwrite authoritatively set timestamp and duration!
     if (span.timestamp != null && span.duration != null) {
       return span;
     }
 
-    // Only calculate span.timestamp and duration on complete spans. This avoids
-    // persisting an inaccurate timestamp due to a late arriving annotation.
+    // We cannot backfill duration on a span with less than two annotations. However, we can
+    // backfill timestamp.
     if (span.annotations.size() < 2) {
-      return span;
+      if (span.timestamp != null) return span;
+      Long guess = guessTimestamp(span);
+      if (guess == null) return span;
+      return span.toBuilder().timestamp(guess).build();
     }
 
-    // For spans that core client annotations, the distance between "cs" and "cr" should be the
-    // authoritative duration. We are special-casing this to avoid setting incorrect duration
-    // when there's skew between the client and the server.
+    // Prefer RPC one-way (cs -> sr) vs arbitrary annotations.
     Long first = span.annotations.get(0).timestamp;
     Long last = span.annotations.get(span.annotations.size() - 1).timestamp;
     for (int i = 0, length = span.annotations.size(); i < length; i++) {

zipkin/src/test/java/zipkin/internal/ApplyTimestampAndDurationTest.java

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015-2016 The OpenZipkin Authors
+ * Copyright 2015-2017 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
@@ -19,22 +19,55 @@
 import zipkin.Span;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static zipkin.Constants.CLIENT_RECV;
 import static zipkin.Constants.CLIENT_SEND;
 import static zipkin.Constants.SERVER_RECV;
+import static zipkin.Constants.SERVER_SEND;
+import static zipkin.internal.ApplyTimestampAndDuration.apply;
 import static zipkin.internal.ApplyTimestampAndDuration.guessTimestamp;
 
 public class ApplyTimestampAndDurationTest {
 
   Endpoint frontend =
       Endpoint.builder().serviceName("frontend").ipv4(192 << 24 | 12 << 16 | 1).port(8080).build();
   Annotation cs = Annotation.create((50) * 1000, CLIENT_SEND, frontend);
+  Annotation cr = Annotation.create((100) * 1000, CLIENT_RECV, frontend);
 
   Endpoint backend =
         Endpoint.builder().serviceName("backend").ipv4(192 << 24 | 12 << 16 | 2).port(8080).build();
-  Annotation sr = Annotation.create((95) * 1000, SERVER_RECV, backend);
+  Annotation sr = Annotation.create((70) * 1000, SERVER_RECV, backend);
+  Annotation ss = Annotation.create((80) * 1000, SERVER_SEND, backend);
 
   Span.Builder span = Span.builder().traceId(1).name("method1").id(666);
 
+  @Test
+  public void apply_onlyCs() {
+    assertThat(apply(span.addAnnotation(cs).build()).timestamp)
+        .isEqualTo(cs.timestamp);
+  }
+
+  @Test
+  public void apply_rpcSpan() {
+    assertThat(apply(span
+        .addAnnotation(cs)
+        .addAnnotation(sr)
+        .addAnnotation(ss)
+        .addAnnotation(cr).build()).duration)
+        .isEqualTo(cr.timestamp - cs.timestamp);
+  }
+
+  @Test
+  public void apply_serverOnly() {
+    assertThat(apply(span.addAnnotation(sr).addAnnotation(ss).build()).duration)
+        .isEqualTo(ss.timestamp - sr.timestamp);
+  }
+
+  @Test
+  public void apply_oneWay() {
+    assertThat(apply(span.addAnnotation(cs).addAnnotation(sr).build()).duration)
+        .isEqualTo(sr.timestamp - cs.timestamp);
+  }
+
   @Test
   public void bestTimestamp_isSpanTimestamp() {
     assertThat(guessTimestamp(span.timestamp(1L).build()))