polishing up

openzipkin · May 5, 2020 · dd20f10 · dd20f10
1 parent c85f812
commit dd20f10
Show file tree

Hide file tree

Showing 11 changed files with 290 additions and 261 deletions.
diff --git a/propagation/pom.xml b/propagation/pom.xml
@@ -20,7 +20,7 @@
   <parent>
     <groupId>io.zipkin.brave</groupId>
     <artifactId>brave-parent</artifactId>
-    <version>5.10.3-SNAPSHOT</version>
+    <version>5.11.3-SNAPSHOT</version>
   </parent>
 
   <artifactId>brave-propagation-parent</artifactId>

diff --git a/propagation/w3c/pom.xml b/propagation/w3c/pom.xml
@@ -18,7 +18,7 @@
   <parent>
     <groupId>io.zipkin.brave</groupId>
     <artifactId>brave-propagation-parent</artifactId>
-    <version>5.10.3-SNAPSHOT</version>
+    <version>5.11.3-SNAPSHOT</version>
   </parent>
   <modelVersion>4.0.0</modelVersion>
 

diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextExtractor.java
@@ -14,38 +14,37 @@
 package brave.propagation.w3c;
 
 import brave.propagation.Propagation.Getter;
-import brave.propagation.SamplingFlags;
 import brave.propagation.TraceContext;
 import brave.propagation.TraceContext.Extractor;
 import brave.propagation.TraceContextOrSamplingFlags;
-import brave.propagation.w3c.TraceContextPropagation.Extra;
-import java.util.Collections;
-import java.util.List;
 
 import static brave.propagation.B3SingleFormat.parseB3SingleFormat;
 
 // TODO: this class has no useful tests wrt traceparent yet
-final class TraceContextExtractor<C, K> implements Extractor<C> {
-  final Getter<C, K> getter;
+final class TraceContextExtractor<R, K> implements Extractor<R> {
+  static final TraceContextOrSamplingFlags EXTRACTED_EMPTY =
+      TraceContextOrSamplingFlags.EMPTY.toBuilder().addExtra(Tracestate.EMPTY).build();
+
+  final Getter<R, K> getter;
   final K traceparentKey, tracestateKey;
   final TracestateFormat tracestateFormat;
   final B3SingleFormatHandler handler = new B3SingleFormatHandler();
 
-  TraceContextExtractor(TraceContextPropagation<K> propagation, Getter<C, K> getter) {
+  TraceContextExtractor(TraceContextPropagation<K> propagation, Getter<R, K> getter) {
     this.getter = getter;
     this.traceparentKey = propagation.traceparent;
     this.tracestateKey = propagation.tracestate;
     this.tracestateFormat = new TracestateFormat(propagation.tracestateKey);
   }
 
-  @Override public TraceContextOrSamplingFlags extract(C carrier) {
-    if (carrier == null) throw new NullPointerException("carrier == null");
-    String traceparent = getter.get(carrier, traceparentKey);
-    if (traceparent == null) return EMPTY;
+  @Override public TraceContextOrSamplingFlags extract(R request) {
+    if (request == null) throw new NullPointerException("request == null");
+    String traceparentString = getter.get(request, traceparentKey);
+    if (traceparentString == null) return EXTRACTED_EMPTY;
 
     // TODO: add link that says tracestate itself is optional
-    String tracestate = getter.get(carrier, tracestateKey);
-    if (tracestate == null) {
+    String tracestateString = getter.get(request, tracestateKey);
+    if (tracestateString == null) {
       // NOTE: we may not want to pay attention to the sampled flag. Since it conflates
       // not-yet-sampled with sampled=false, implementations that always set flags to -00 would
       // never be traced!
@@ -54,33 +53,20 @@ final class TraceContextExtractor<C, K> implements Extractor<C> {
       // span ID. Ex we don't know if upstream are sending to the same system or not, when we can't
       // read the tracestate header. Trusting the span ID (traceparent calls the span ID parent-id)
       // could result in a headless trace.
-      TraceContext maybeUpstream = TraceparentFormat.parseTraceparentFormat(traceparent);
-      return TraceContextOrSamplingFlags.newBuilder()
-        .context(maybeUpstream)
-        .extra(DEFAULT_EXTRA) // marker for outbound propagation
-        .build();
+      TraceContext maybeUpstream = TraceparentFormat.parseTraceparentFormat(traceparentString);
+      return TraceContextOrSamplingFlags.newBuilder(maybeUpstream)
+          .addExtra(Tracestate.EMPTY) // marker for outbound propagation
+          .build();
     }
 
-    CharSequence otherEntries = tracestateFormat.parseAndReturnOtherEntries(tracestate, handler);
-
-    List<Object> extra;
-    if (otherEntries == null) {
-      extra = DEFAULT_EXTRA;
-    } else {
-      Extra e = new Extra();
-      e.otherEntries = otherEntries;
-      extra = Collections.singletonList(e);
-    }
+    Tracestate tracestate = tracestateFormat.parseAndReturnOtherEntries(tracestateString, handler);
 
     TraceContext context = handler.context;
     if (context == null) {
-      if (extra == DEFAULT_EXTRA) return EMPTY;
-      return TraceContextOrSamplingFlags.newBuilder()
-        .extra(extra)
-        .samplingFlags(SamplingFlags.EMPTY)
-        .build();
+      if (tracestate == Tracestate.EMPTY) return EXTRACTED_EMPTY;
+      return EXTRACTED_EMPTY.toBuilder().addExtra(tracestate).build();
     }
-    return TraceContextOrSamplingFlags.newBuilder().context(context).extra(extra).build();
+    return TraceContextOrSamplingFlags.newBuilder(context).addExtra(tracestate).build();
   }
 
   static final class B3SingleFormatHandler implements TracestateFormat.Handler {
@@ -93,11 +79,4 @@ public boolean onThisEntry(CharSequence tracestate, int beginIndex, int endIndex
       return context != null;
     }
   }
-
-  /** When present, this context was created with TracestatePropagation */
-  static final Extra MARKER = new Extra();
-
-  static final List<Object> DEFAULT_EXTRA = Collections.singletonList(MARKER);
-  static final TraceContextOrSamplingFlags EMPTY =
-    TraceContextOrSamplingFlags.EMPTY.toBuilder().extra(DEFAULT_EXTRA).build();
 }
diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextInjector.java
@@ -16,37 +16,36 @@
 import brave.propagation.Propagation.Setter;
 import brave.propagation.TraceContext;
 import brave.propagation.TraceContext.Injector;
-import brave.propagation.w3c.TraceContextPropagation.Extra;
 
 import static brave.propagation.B3SingleFormat.writeB3SingleFormat;
 import static brave.propagation.w3c.TraceparentFormat.writeTraceparentFormat;
 
-final class TraceContextInjector<C, K> implements Injector<C> {
+final class TraceContextInjector<R, K> implements Injector<R> {
   final TracestateFormat tracestateFormat;
-  final Setter<C, K> setter;
+  final Setter<R, K> setter;
   final K traceparentKey, tracestateKey;
 
-  TraceContextInjector(TraceContextPropagation<K> propagation, Setter<C, K> setter) {
+  TraceContextInjector(TraceContextPropagation<K> propagation, Setter<R, K> setter) {
     this.tracestateFormat = new TracestateFormat(propagation.tracestateKey);
     this.traceparentKey = propagation.traceparent;
     this.tracestateKey = propagation.tracestate;
     this.setter = setter;
   }
 
-  @Override public void inject(TraceContext traceContext, C carrier) {
+  @Override public void inject(TraceContext traceContext, R request) {
 
-    setter.put(carrier, traceparentKey, writeTraceparentFormat(traceContext));
+    setter.put(request, traceparentKey, writeTraceparentFormat(traceContext));
 
     CharSequence otherState = null;
     for (int i = 0, length = traceContext.extra().size(); i < length; i++) {
       Object next = traceContext.extra().get(i);
-      if (next instanceof Extra) {
-        otherState = ((Extra) next).otherEntries;
+      if (next instanceof Tracestate) {
+        otherState = ((Tracestate) next).otherEntries;
         break;
       }
     }
 
     String tracestate = tracestateFormat.write(writeB3SingleFormat(traceContext), otherState);
-    setter.put(carrier, tracestateKey, tracestate);
+    setter.put(request, tracestateKey, tracestate);
   }
 }
diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceContextPropagation.java
@@ -21,7 +21,6 @@
 import java.util.List;
 
 public final class TraceContextPropagation<K> implements Propagation<K> {
-
   public static Factory newFactory() {
     return new FactoryBuilder().build();
   }
@@ -87,27 +86,13 @@ static final class Factory extends Propagation.Factory {
     return keys;
   }
 
-  @Override public <C> Injector<C> injector(Setter<C, K> setter) {
+  @Override public <R> Injector<R> injector(Setter<R, K> setter) {
     if (setter == null) throw new NullPointerException("setter == null");
     return new TraceContextInjector<>(this, setter);
   }
 
-  @Override public <C> Extractor<C> extractor(Getter<C, K> getter) {
+  @Override public <R> Extractor<R> extractor(Getter<R, K> getter) {
     if (getter == null) throw new NullPointerException("getter == null");
     return new TraceContextExtractor<>(this, getter);
   }
-
-  /**
-   * This only contains other entries. The entry for the current trace is only written during
-   * injection.
-   */
-  static final class Extra { // hidden intentionally
-    CharSequence otherEntries;
-
-    @Override public String toString() {
-      return "TracestatePropagation{"
-        + (otherEntries != null ? ("entries=" + otherEntries.toString()) : "")
-        + "}";
-    }
-  }
 }
diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TraceparentFormat.java
@@ -16,7 +16,6 @@
 import brave.internal.Nullable;
 import brave.internal.Platform;
 import brave.propagation.TraceContext;
-import java.lang.ref.WeakReference;
 import java.nio.ByteBuffer;
 
 import static brave.internal.HexCodec.writeHexLong;
@@ -29,10 +28,10 @@ final class TraceparentFormat {
   static final int FORMAT_LENGTH = 3 + 32 + 1 + 16 + 3; // 00-traceid128-spanid-01
 
   static final int // instead of enum for smaller bytecode
-    FIELD_VERSION = 1,
-    FIELD_TRACE_ID = 2,
-    FIELD_PARENT_ID = 3,
-    FIELD_TRACE_FLAGS = 4;
+      FIELD_VERSION = 1,
+      FIELD_TRACE_ID = 2,
+      FIELD_PARENT_ID = 3,
+      FIELD_TRACE_FLAGS = 4;
 
   /** Writes all "traceparent" defined fields in the trace context to a hyphen delimited string. */
   public static String writeTraceparentFormat(TraceContext context) {
@@ -42,7 +41,7 @@ public static String writeTraceparentFormat(TraceContext context) {
   }
 
   /**
-   * Like {@link #writeTraceparentFormat(TraceContext)}, but for carriers with byte array or byte
+   * Like {@link #writeTraceparentFormat(TraceContext)}, but for requests with byte array or byte
    * buffer values. For example, {@link ByteBuffer#wrap(byte[])} can wrap the result.
    */
   public static byte[] writeTraceparentFormatAsBytes(TraceContext context) {
@@ -89,7 +88,7 @@ public static TraceContext parseTraceparentFormat(CharSequence parent) {
    */
   @Nullable
   public static TraceContext parseTraceparentFormat(CharSequence value, int beginIndex,
-    int endIndex) {
+      int endIndex) {
     int length = endIndex - beginIndex;
 
     if (length == 0) {
@@ -214,9 +213,9 @@ public static TraceContext parseTraceparentFormat(CharSequence value, int beginI
   }
 
   static boolean validateFieldLength(int field, int length) {
-    int expectedLength =
-      (field == FIELD_TRACE_FLAGS || field == FIELD_VERSION) ? 2
-        : field == FIELD_TRACE_ID ? 32 : 16;
+    int expectedLength = (field == FIELD_VERSION || field == FIELD_TRACE_FLAGS)
+        ? 2  // There are two fields that are 2 characters long: version and flags
+        : field == FIELD_TRACE_ID ? 32 : 16; // trace ID or span ID
     if (length == 0) {
       log(field, "Invalid input: empty {0}");
       return false;

diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java b/propagation/w3c/src/main/java/brave/propagation/w3c/Tracestate.java
@@ -0,0 +1,50 @@
+/*
+ * Copyright 2013-2020 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.w3c;
+
+import brave.internal.Nullable;
+
+/**
+ * This only contains other entries. The entry for the current trace is only written during
+ * injection.
+ */
+final class Tracestate { // hidden intentionally
+  static final Tracestate EMPTY = new Tracestate("");
+
+  // TODO: this will change
+  final String otherEntries;
+
+  static Tracestate create(@Nullable CharSequence otherEntries) {
+    if (otherEntries == null || otherEntries.length() == 0) return EMPTY;
+    return new Tracestate(otherEntries.toString());
+  }
+
+  private Tracestate(String otherEntries) {
+    this.otherEntries = otherEntries;
+  }
+
+  @Override public String toString() {
+    return "tracestate: " + otherEntries;
+  }
+
+  @Override public final boolean equals(Object o) {
+    if (o == this) return true;
+    if (!(o instanceof Tracestate)) return false;
+    return otherEntries.equals(((Tracestate) o).otherEntries);
+  }
+
+  @Override public final int hashCode() {
+    return otherEntries.hashCode();
+  }
+}
diff --git a/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java b/propagation/w3c/src/main/java/brave/propagation/w3c/TracestateFormat.java
@@ -13,7 +13,6 @@
  */
 package brave.propagation.w3c;
 
-import brave.internal.Nullable;
 import brave.internal.Platform;
 
 import static brave.propagation.w3c.TraceparentFormat.FORMAT_LENGTH;
@@ -79,7 +78,7 @@ String write(String thisValue, CharSequence otherEntries) {
   // TODO: characters were added to the valid list, so it is possible this impl no longer works
   // TODO: 32 max entries https://w3c.github.io/trace-context/#tracestate-header-field-values
   // TODO: empty and whitespace-only allowed Ex. 'foo=' or 'foo=  '
-  @Nullable CharSequence parseAndReturnOtherEntries(String tracestate, Handler handler) {
+  Tracestate parseAndReturnOtherEntries(String tracestate, Handler handler) {
     StringBuilder currentString = new StringBuilder(), otherEntries = null;
     Op op;
     OUTER:
@@ -120,7 +119,10 @@ String write(String thisValue, CharSequence otherEntries) {
           break;
       }
     }
-    return otherEntries;
+    if (otherEntries != null && otherEntries.charAt(0) == ',') {
+      otherEntries.deleteCharAt(0); // TODO: fix the parser so this is eaten before now
+    }
+    return Tracestate.create(otherEntries);
   }
 
   // Simplify other rules by allowing value-based lookup on an ASCII value.
@@ -142,7 +144,7 @@ String write(String thisValue, CharSequence otherEntries) {
 
   static boolean isValidTracestateKeyChar(char c) {
     return (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9')
-      || c == '@' || c == '_' || c == '-' || c == '*' || c == '/';
+        || c == '@' || c == '_' || c == '-' || c == '*' || c == '/';
   }
 
   /**