Log invalid arguments rather than throwing exceptions. (#5012)

Author: carlosalberto

opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/ScopeManagerShim.java

@@ -50,9 +50,10 @@ public Span activeSpan() {
   @Override
   @SuppressWarnings("MustBeClosedChecker")
   public Scope activate(@Nullable Span span) {
-    if (span == null) {
+    SpanShim spanShim = ShimUtil.getSpanShim(span);
+    if (spanShim == null) {
       return new ScopeShim(Context.current().with(NOOP_SPANSHIM).makeCurrent());
     }
-    return new ScopeShim(Context.current().with(ShimUtil.getSpanShim(span)).makeCurrent());
+    return new ScopeShim(Context.current().with(spanShim).makeCurrent());
   }
 }

opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/ShimUtil.java

@@ -7,23 +7,40 @@
 
 import io.opentracing.Span;
 import io.opentracing.SpanContext;
+import io.opentracing.noop.NoopSpan;
 import java.util.function.Supplier;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 class ShimUtil {
+  private static final Logger logger = Logger.getLogger(ShimUtil.class.getName());
 
   private ShimUtil() {}
 
-  static SpanContextShim getContextShim(SpanContext context) {
+  @Nullable
+  static SpanContextShim getContextShim(@Nullable SpanContext context) {
+    if (context == null) {
+      return null;
+    }
+
     if (!(context instanceof SpanContextShim)) {
-      throw new IllegalArgumentException(
-          "context is not a valid SpanContextShim object: " + className(context));
+      logger.log(
+          Level.INFO,
+          "Expected to have an OpenTelemetry SpanContext but got {0}",
+          className(context));
+      return null;
     }
 
     return (SpanContextShim) context;
   }
 
-  static SpanShim getSpanShim(Span span) {
+  @Nullable
+  static SpanShim getSpanShim(@Nullable Span span) {
+    if (span == null || span instanceof NoopSpan) {
+      return null;
+    }
+
     if (!(span instanceof SpanShim)) {
       if (span instanceof Supplier<?>) {
         // allow libraries to implement a delegate span,
@@ -32,11 +49,13 @@ static SpanShim getSpanShim(Span span) {
         if (wrapped instanceof Span) {
           return getSpanShim((Span) wrapped);
         } else {
-          throw new IllegalArgumentException(
-              "span wrapper didn't return a span: " + className(wrapped));
+          logger.log(Level.INFO, "Span wrapper didn't return a span: {0}", className(wrapped));
+          return null;
         }
       }
-      throw new IllegalArgumentException("span is not a valid SpanShim object: " + className(span));
+
+      logger.log(Level.INFO, "Expected to have an OpenTelemetry Span but got {0}", className(span));
+      return null;
     }
 
     return (SpanShim) span;

opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java

@@ -23,7 +23,6 @@
 import io.opentracing.Span;
 import io.opentracing.SpanContext;
 import io.opentracing.Tracer.SpanBuilder;
-import io.opentracing.noop.NoopSpan;
 import io.opentracing.tag.Tag;
 import io.opentracing.tag.Tags;
 import java.util.ArrayList;
@@ -64,11 +63,12 @@ public SpanBuilderShim(TelemetryInfo telemetryInfo, String spanName) {
 
   @Override
   public SpanBuilder asChildOf(Span parent) {
-    if (parent == null || parent instanceof NoopSpan) {
-      return this;
-    }
     SpanShim spanShim = ShimUtil.getSpanShim(parent);
-    return addReference(References.CHILD_OF, spanShim.context());
+    if (spanShim != null) {
+      addReference(References.CHILD_OF, spanShim.context());
+    }
+
+    return this;
   }
 
   @Override
@@ -78,7 +78,8 @@ public SpanBuilder asChildOf(SpanContext parent) {
 
   @Override
   public SpanBuilder addReference(@Nullable String referenceType, SpanContext referencedContext) {
-    if (referencedContext == null) {
+    SpanContextShim contextShim = ShimUtil.getContextShim(referencedContext);
+    if (contextShim == null) {
       return this;
     }
 
@@ -92,8 +93,6 @@ public SpanBuilder addReference(@Nullable String referenceType, SpanContext refe
       return this;
     }
 
-    SpanContextShim contextShim = ShimUtil.getContextShim(referencedContext);
-
     // Optimization for 99% situations, when there is only one parent.
     if (allParents.size() == 0) {
       allParents =

opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java

@@ -61,7 +61,10 @@ public <C> void inject(SpanContext context, Format<C> format, C carrier) {
       return;
     }
 
-    SpanContextShim contextShim = getContextShim(context);
+    SpanContextShim contextShim = ShimUtil.getContextShim(context);
+    if (contextShim == null) {
+      return;
+    }
 
     if (format == Format.Builtin.TEXT_MAP
         || format == Format.Builtin.TEXT_MAP_INJECT
@@ -94,12 +97,4 @@ public <C> SpanContext extract(Format<C> format, C carrier) {
   public void close() {
     isClosed = true;
   }
-
-  static SpanContextShim getContextShim(SpanContext context) {
-    if (!(context instanceof SpanContextShim)) {
-      throw new IllegalArgumentException("context is not a valid SpanContextShim object");
-    }
-
-    return (SpanContextShim) context;
-  }
 }

opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/ShimUtilTest.java

@@ -6,7 +6,6 @@
 package io.opentelemetry.opentracingshim;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.api.trace.Tracer;
@@ -40,20 +39,14 @@ void spanWrapper() {
     SpanShim shim = new SpanShim(telemetryInfo, span);
     assertThat(ShimUtil.getSpanShim(shim)).isEqualTo(shim);
     assertThat(ShimUtil.getSpanShim(new SpanWrapper(shim))).isEqualTo(shim);
-    assertThatThrownBy(() -> ShimUtil.getSpanShim(new SpanWrapper("not a span")))
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessage("span wrapper didn't return a span: java.lang.String");
-    assertThatThrownBy(() -> ShimUtil.getSpanShim(null))
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessage("span is not a valid SpanShim object: null");
+    assertThat(ShimUtil.getSpanShim(new SpanWrapper("not a span"))).isNull();
+    assertThat(ShimUtil.getSpanShim(null)).isNull();
   }
 
   @Test
   void getContextShim() {
     SpanContextShim contextShim = new SpanContextShim(new SpanShim(telemetryInfo, span));
     assertThat(ShimUtil.getContextShim(contextShim)).isEqualTo(contextShim);
-    assertThatThrownBy(() -> ShimUtil.getContextShim(null))
-        .isInstanceOf(IllegalArgumentException.class)
-        .hasMessage("context is not a valid SpanContextShim object: null");
+    assertThat(ShimUtil.getContextShim(null)).isNull();
   }
 }

opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/TracerShimTest.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.opentracingshim;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.mock;
 
 import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator;
 import io.opentelemetry.api.trace.Tracer;
@@ -262,6 +263,13 @@ void inject_nullContext() {
     assertThat(map).isEmpty();
   }
 
+  @Test
+  void inject_invalid() {
+    Map<String, String> map = new HashMap<>();
+    tracerShim.inject(mock(SpanContext.class), Format.Builtin.TEXT_MAP, new TextMapAdapter(map));
+    assertThat(map).isEmpty();
+  }
+
   @Test
   void inject_textMap() {
     Map<String, String> map = new HashMap<>();