Make the HttpTraceContext be more forgiving in the face of invalid data. (#899)

api/src/main/java/io/opentelemetry/trace/propagation/HttpTraceContext.java

@@ -28,6 +28,7 @@ import io.opentelemetry.trace.TraceState;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.logging.Logger;
 import java.util.regex.Pattern;
 import javax.annotation.concurrent.Immutable;
 
@@ -37,6 +38,8 @@ import javax.annotation.concurrent.Immutable;
  */
 @Immutable
 public class HttpTraceContext implements HttpTextFormat<SpanContext> {
+  private static final Logger logger = Logger.getLogger(HttpTraceContext.class.getName());
+
   private static final TraceState TRACE_STATE_DEFAULT = TraceState.builder().build();
   static final String TRACE_PARENT = "traceparent";
   static final String TRACE_STATE = "tracestate";
@@ -105,39 +108,63 @@ public class HttpTraceContext implements HttpTextFormat<SpanContext> {
   public <C /*>>> extends @NonNull Object*/> SpanContext extract(C carrier, Getter<C> getter) {
     checkNotNull(carrier, "carrier");
     checkNotNull(getter, "getter");
-    TraceId traceId;
-    SpanId spanId;
-    TraceFlags traceFlags;
     String traceparent = getter.get(carrier, TRACE_PARENT);
     if (traceparent == null) {
       return SpanContext.getInvalid();
     }
+
+    SpanContext contextFromParentHeader = extractContextFromTraceParent(traceparent);
+    if (!contextFromParentHeader.isValid()) {
+      return contextFromParentHeader;
+    }
+
+    String traceStateHeader = getter.get(carrier, TRACE_STATE);
+    if (traceStateHeader == null || traceStateHeader.isEmpty()) {
+      return contextFromParentHeader;
+    }
+
     try {
-      // TODO(bdrutu): Do we need to verify that version is hex and that for the version
+      TraceState traceState = extractTraceState(traceStateHeader);
-      // the length is the expected one?
+      return SpanContext.createFromRemoteParent(
-      checkArgument(
+          contextFromParentHeader.getTraceId(),
+          contextFromParentHeader.getSpanId(),
+          contextFromParentHeader.getTraceFlags(),
+          traceState);
+    } catch (IllegalArgumentException e) {
+      logger.info("Unparseable tracestate header. Returning span context without state.");
+      return contextFromParentHeader;
+    }
+  }
+
+  private static SpanContext extractContextFromTraceParent(String traceparent) {
+    // TODO(bdrutu): Do we need to verify that version is hex and that
+    // for the version the length is the expected one?
+    boolean isValid =
         traceparent.charAt(TRACE_OPTION_OFFSET - 1) == TRACEPARENT_DELIMITER
             && (traceparent.length() == TRACEPARENT_HEADER_SIZE
                 || (traceparent.length() > TRACEPARENT_HEADER_SIZE
                     && traceparent.charAt(TRACEPARENT_HEADER_SIZE) == TRACEPARENT_DELIMITER))
             && traceparent.charAt(SPAN_ID_OFFSET - 1) == TRACEPARENT_DELIMITER
-              && traceparent.charAt(TRACE_OPTION_OFFSET - 1) == TRACEPARENT_DELIMITER,
+            && traceparent.charAt(TRACE_OPTION_OFFSET - 1) == TRACEPARENT_DELIMITER;
-          "Missing or malformed TRACEPARENT.");
+    if (!isValid) {
-
+      logger.info("Unparseable traceparent header. Returning INVALID span context.");
-      traceId = TraceId.fromLowerBase16(traceparent, TRACE_ID_OFFSET);
+      return INVALID_SPAN_CONTEXT;
-      spanId = SpanId.fromLowerBase16(traceparent, SPAN_ID_OFFSET);
-      traceFlags = TraceFlags.fromLowerBase16(traceparent, TRACE_OPTION_OFFSET);
-    } catch (IllegalArgumentException e) {
-      throw new IllegalArgumentException("Invalid traceparent: " + traceparent, e);
     }
 
-    String traceState = getter.get(carrier, TRACE_STATE);
     try {
-      if (traceState == null || traceState.isEmpty()) {
+      TraceId traceId = TraceId.fromLowerBase16(traceparent, TRACE_ID_OFFSET);
+      SpanId spanId = SpanId.fromLowerBase16(traceparent, SPAN_ID_OFFSET);
+      TraceFlags traceFlags = TraceFlags.fromLowerBase16(traceparent, TRACE_OPTION_OFFSET);
       return SpanContext.createFromRemoteParent(traceId, spanId, traceFlags, TRACE_STATE_DEFAULT);
+    } catch (IllegalArgumentException e) {
+      logger.info("Unparseable traceparent header. Returning INVALID span context.");
+      return INVALID_SPAN_CONTEXT;
     }
+  }
+
+  private static TraceState extractTraceState(String traceStateHeader) {
     TraceState.Builder traceStateBuilder = TraceState.builder();
-      String[] listMembers = TRACESTATE_ENTRY_DELIMITER_SPLIT_PATTERN.split(traceState);
+    String[] listMembers = TRACESTATE_ENTRY_DELIMITER_SPLIT_PATTERN.split(traceStateHeader);
     checkArgument(
         listMembers.length <= TRACESTATE_MAX_MEMBERS, "TraceState has too many elements.");
     // Iterate in reverse order because when call builder set the elements is added in the
@@ -148,10 +175,6 @@ public class HttpTraceContext implements HttpTextFormat<SpanContext> {
       checkArgument(index != -1, "Invalid TraceState list-member format.");
       traceStateBuilder.set(listMember.substring(0, index), listMember.substring(index + 1));
     }
-      return SpanContext.createFromRemoteParent(
+    return traceStateBuilder.build();
-          traceId, spanId, traceFlags, traceStateBuilder.build());
-    } catch (IllegalArgumentException e) {
-      throw new IllegalArgumentException("Invalid tracestate: " + traceState, e);
-    }
   }
 }

api/src/test/java/io/opentelemetry/trace/propagation/HttpTraceContextTest.java

@@ -203,65 +203,48 @@ public class HttpTraceContextTest {
     Map<String, String> invalidHeaders = new LinkedHashMap<String, String>();
     invalidHeaders.put(
         TRACE_PARENT, "00-" + "abcdefghijklmnopabcdefghijklmnop" + "-" + SPAN_ID_BASE16 + "-01");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: "
-            + "00-"
-            + "abcdefghijklmnopabcdefghijklmnop"
-            + "-"
-            + SPAN_ID_BASE16
-            + "-01");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
   public void extract_InvalidTraceId_Size() {
     Map<String, String> invalidHeaders = new LinkedHashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "00-" + SPAN_ID_BASE16 + "-01");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: " + "00-" + TRACE_ID_BASE16 + "00-" + SPAN_ID_BASE16 + "-01");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
   public void extract_InvalidSpanId() {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + "abcdefghijklmnop" + "-01");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: " + "00-" + TRACE_ID_BASE16 + "-" + "abcdefghijklmnop" + "-01");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
   public void extract_InvalidSpanId_Size() {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "00-01");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: " + "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "00-01");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
   public void extract_InvalidTraceFlags() {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-gh");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: " + "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-gh");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
   public void extract_InvalidTraceFlags_Size() {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-0100");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage(
+        .isSameInstanceAs(HttpTraceContext.INVALID_SPAN_CONTEXT);
-        "Invalid traceparent: " + "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-0100");
-    httpTraceContext.extract(invalidHeaders, getter);
   }
 
   @Test
@@ -269,9 +252,10 @@ public class HttpTraceContextTest {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01");
     invalidHeaders.put(TRACE_STATE, "foo=bar;test=test");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage("Invalid tracestate: " + "foo=bar;test=test");
+        .isEqualTo(
-    httpTraceContext.extract(invalidHeaders, getter);
+            SpanContext.createFromRemoteParent(
+                TRACE_ID, SPAN_ID, SAMPLED_TRACE_OPTIONS, TRACE_STATE_DEFAULT));
   }
 
   @Test
@@ -279,9 +263,10 @@ public class HttpTraceContextTest {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01");
     invalidHeaders.put(TRACE_STATE, "foo=bar,test-test");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage("Invalid tracestate: " + "foo=bar,test-test");
+        .isEqualTo(
-    httpTraceContext.extract(invalidHeaders, getter);
+            SpanContext.createFromRemoteParent(
+                TRACE_ID, SPAN_ID, SAMPLED_TRACE_OPTIONS, TRACE_STATE_DEFAULT));
   }
 
   @Test
@@ -289,9 +274,10 @@ public class HttpTraceContextTest {
     Map<String, String> invalidHeaders = new HashMap<String, String>();
     invalidHeaders.put(TRACE_PARENT, "00-" + TRACE_ID_BASE16 + "-" + SPAN_ID_BASE16 + "-01");
     invalidHeaders.put(TRACE_STATE, "test-test");
-    thrown.expect(IllegalArgumentException.class);
+    assertThat(httpTraceContext.extract(invalidHeaders, getter))
-    thrown.expectMessage("Invalid tracestate: " + "test-test");
+        .isEqualTo(
-    httpTraceContext.extract(invalidHeaders, getter);
+            SpanContext.createFromRemoteParent(
+                TRACE_ID, SPAN_ID, SAMPLED_TRACE_OPTIONS, TRACE_STATE_DEFAULT));
   }
 
   @Test