Make active span be ignored by default; used as parent span if config…

…ured (#110) * Make active span be ignored by default; used as parent span if configured. The ensures that the damage caused by accidental thread local leakage between requests is minimized Fixes #109 * empty commit to refresh PR * Changes based on review comments from @pavolloffay
opentracing-contrib · Oct 31, 2018 · 1228471 · 1228471
1 parent 189a62e
commit 1228471
Show file tree

Hide file tree

Showing 9 changed files with 132 additions and 16 deletions.
diff --git a/...java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanIgnoredByDefaultTest.java b/...java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanIgnoredByDefaultTest.java
@@ -0,0 +1,18 @@
+package io.opentracing.contrib.jaxrs2.itest.cxf;
+
+import io.opentracing.contrib.jaxrs2.itest.common.AbstractParentSpanResolutionTest;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+public class ApacheCXFParentSpanIgnoredByDefaultTest extends AbstractParentSpanResolutionTest {
+
+  @Override
+  protected boolean shouldUseParentSpan() {
+    return false;
+  }
+
+  @Override
+  protected void initServletContext(ServletContextHandler context) {
+    super.initServletContext(context);
+    ApacheCXFHelper.initServletContext(context);
+  }
+}
diff --git a/.../test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java b/.../test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java
@@ -5,6 +5,11 @@
 
 public class ApacheCXFParentSpanResolutionTest extends AbstractParentSpanResolutionTest {
 
+  @Override
+  protected boolean shouldUseParentSpan() {
+    return true;
+  }
+
   @Override
   protected void initServletContext(ServletContextHandler context) {
     super.initServletContext(context);

diff --git a/...ain/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java b/...ain/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java
@@ -1,26 +1,49 @@
 package io.opentracing.contrib.jaxrs2.itest.common;
 
-import static org.awaitility.Awaitility.await;
-
 import io.opentracing.Scope;
+import io.opentracing.contrib.jaxrs2.client.ClientTracingFeature;
+import io.opentracing.contrib.jaxrs2.server.ServerTracingDynamicFeature;
+import io.opentracing.contrib.jaxrs2.server.SpanFinishingFilter;
 import io.opentracing.mock.MockSpan;
 import io.opentracing.tag.Tags;
 import org.eclipse.jetty.servlet.FilterHolder;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.junit.Assert;
 import org.junit.Test;
 
-import java.io.IOException;
-import java.util.EnumSet;
-import java.util.List;
-
 import javax.servlet.*;
 import javax.ws.rs.client.Client;
 import javax.ws.rs.client.ClientBuilder;
 import javax.ws.rs.core.Response;
+import java.io.IOException;
+import java.util.EnumSet;
+import java.util.List;
+
+import static org.awaitility.Awaitility.await;
 
 public abstract class AbstractParentSpanResolutionTest extends AbstractJettyTest {
 
+    protected abstract boolean shouldUseParentSpan();
+
+    @Override
+    protected void initTracing(ServletContextHandler context) {
+        client.register(new ClientTracingFeature.Builder(mockTracer).build());
+
+        ServerTracingDynamicFeature.Builder builder = new ServerTracingDynamicFeature.Builder(mockTracer);
+        if (shouldUseParentSpan()) {
+            builder = builder.withJoinExistingActiveSpan(true);
+        }
+        ServerTracingDynamicFeature serverTracingFeature = builder.build();
+
+        context.addFilter(new FilterHolder(new SpanFinishingFilter()),
+                "/*", EnumSet.of(DispatcherType.REQUEST));
+
+        context.setAttribute(TRACER_ATTRIBUTE, mockTracer);
+        context.setAttribute(CLIENT_ATTRIBUTE, client);
+        context.setAttribute(SERVER_TRACING_FEATURE, serverTracingFeature);
+    }
+
+
     @Override
     protected void initServletContext(ServletContextHandler context) {
         context.addFilter(new FilterHolder(new FilterThatInitsSpan()), "/*",
@@ -51,7 +74,11 @@ public void testUseActiveSpanIfSet() {
         MockSpan preceding = getSpanWithTag(spans, new ImmutableTag(Tags.COMPONENT.getKey(), "preceding-opentracing-filter"));
         MockSpan original = getSpanWithTag(spans, new ImmutableTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER));
 
-        Assert.assertEquals(preceding.context().spanId(), original.parentId());
+        if (shouldUseParentSpan()) {
+            Assert.assertEquals(preceding.context().spanId(), original.parentId());
+        } else {
+            Assert.assertEquals(0, original.parentId());
+        }
     }
 
     class FilterThatInitsSpan implements Filter {

diff --git a/...java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanIgnoredByDefaultTest.java b/...java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanIgnoredByDefaultTest.java
@@ -0,0 +1,17 @@
+package io.opentracing.contrib.jaxrs2.itest.jersey;
+
+import io.opentracing.contrib.jaxrs2.itest.common.AbstractParentSpanResolutionTest;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+public class JerseyParentSpanIgnoredByDefaultTest extends AbstractParentSpanResolutionTest{
+    @Override
+    protected boolean shouldUseParentSpan() {
+        return true;
+    }
+
+    @Override
+    protected void initServletContext(ServletContextHandler context) {
+        super.initServletContext(context);
+        JerseyHelper.initServletContext(context);
+    }
+}
diff --git a/.../test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanResolutionTest.java b/.../test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanResolutionTest.java
@@ -4,6 +4,11 @@
 import org.eclipse.jetty.servlet.ServletContextHandler;
 
 public class JerseyParentSpanResolutionTest extends AbstractParentSpanResolutionTest{
+    @Override
+    protected boolean shouldUseParentSpan() {
+        return true;
+    }
+
     @Override
     protected void initServletContext(ServletContextHandler context) {
         super.initServletContext(context);

diff --git a/.../io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanIgnoredByDefaultTest.java b/.../io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanIgnoredByDefaultTest.java
@@ -0,0 +1,18 @@
+package io.opentracing.contrib.jaxrs2.itest.resteasy;
+
+import io.opentracing.contrib.jaxrs2.itest.common.AbstractParentSpanResolutionTest;
+import org.eclipse.jetty.servlet.ServletContextHandler;
+
+public class RestEasyParentSpanIgnoredByDefaultTest extends AbstractParentSpanResolutionTest {
+
+  @Override
+  protected boolean shouldUseParentSpan() {
+    return true;
+  }
+
+  protected void initServletContext(ServletContextHandler context) {
+    super.initServletContext(context);
+    RestEasyHelper.initServletContext(context);
+  }
+
+}
diff --git a/...t/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanResolutionTest.java b/...t/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanResolutionTest.java
@@ -5,6 +5,11 @@
 
 public class RestEasyParentSpanResolutionTest extends AbstractParentSpanResolutionTest {
 
+  @Override
+  protected boolean shouldUseParentSpan() {
+    return true;
+  }
+
   protected void initServletContext(ServletContextHandler context) {
     super.initServletContext(context);
     RestEasyHelper.initServletContext(context);

diff --git a/...axrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java b/...axrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java
@@ -4,16 +4,17 @@
 import io.opentracing.contrib.jaxrs2.serialization.InterceptorSpanDecorator;
 import io.opentracing.contrib.jaxrs2.server.OperationNameProvider.WildcardOperationName;
 import io.opentracing.util.GlobalTracer;
+import org.eclipse.microprofile.opentracing.Traced;
+
+import javax.ws.rs.Priorities;
+import javax.ws.rs.container.DynamicFeature;
+import javax.ws.rs.container.ResourceInfo;
+import javax.ws.rs.core.FeatureContext;
 import java.util.Collections;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
-import javax.ws.rs.Priorities;
-import javax.ws.rs.container.DynamicFeature;
-import javax.ws.rs.container.ResourceInfo;
-import javax.ws.rs.core.FeatureContext;
-import org.eclipse.microprofile.opentracing.Traced;
 
 /**
  * This class has to be registered as JAX-RS provider to enable tracing of server requests. It also
@@ -51,7 +52,8 @@ public void configure(ResourceInfo resourceInfo, FeatureContext context) {
                 operationName(resourceInfo),
                 builder.spanDecorators,
                 builder.operationNameBuilder.build(resourceInfo.getResourceClass(), resourceInfo.getResourceMethod()),
-                builder.skipPattern != null ? Pattern.compile(builder.skipPattern) : null),
+                builder.skipPattern != null ? Pattern.compile(builder.skipPattern) : null,
+                builder.joinExistingActiveSpan),
                 builder.priority);
 
             if (builder.traceSerialization) {
@@ -105,6 +107,7 @@ public static class Builder {
         private OperationNameProvider.Builder operationNameBuilder;
         private boolean traceSerialization;
         private String skipPattern;
+        private boolean joinExistingActiveSpan;
 
         public Builder(Tracer tracer) {
             this.tracer = tracer;
@@ -116,6 +119,7 @@ public Builder(Tracer tracer) {
             this.allTraced = true;
             this.operationNameBuilder = WildcardOperationName.newBuilder();
             this.traceSerialization = true;
+            this.joinExistingActiveSpan = false;
         }
 
         /**
@@ -197,6 +201,18 @@ public Builder withSkipPattern(String skipPattern) {
             return this;
         }
 
+        /**
+         * @param joinExistingActiveSpan If true, any active span on the on the current thread.
+         *                               This can be used when combining with servlet instrumentation.
+         *                               If false, parent span will be extracted from HTTP headers.
+         *                               Default is false.
+         * @return builder
+         */
+        public Builder withJoinExistingActiveSpan(boolean joinExistingActiveSpan) {
+            this.joinExistingActiveSpan = joinExistingActiveSpan;
+            return this;
+        }
+
         /**
          * @return server tracing dynamic feature. This feature should be manually registered to {@link javax.ws.rs.core.Application}
          */

diff --git a/...racing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java b/...racing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java
@@ -37,18 +37,21 @@ public class ServerTracingFilter implements ContainerRequestFilter, ContainerRes
     private String operationName;
     private OperationNameProvider operationNameProvider;
     private Pattern skipPattern;
+    private final boolean joinExistingActiveSpan;
 
     protected ServerTracingFilter(
         Tracer tracer,
         String operationName,
         List<ServerSpanDecorator> spanDecorators,
         OperationNameProvider operationNameProvider,
-        Pattern skipPattern) {
+        Pattern skipPattern,
+        boolean joinExistingActiveSpan) {
         this.tracer = tracer;
         this.operationName = operationName;
         this.spanDecorators = new ArrayList<>(spanDecorators);
         this.operationNameProvider = operationNameProvider;
         this.skipPattern = skipPattern;
+        this.joinExistingActiveSpan = joinExistingActiveSpan;
     }
 
     @Context
@@ -64,6 +67,7 @@ public void filter(ContainerRequestContext requestContext) {
         if (tracer != null) {
 
             Tracer.SpanBuilder spanBuilder = tracer.buildSpan(operationNameProvider.operationName(requestContext))
+                    .ignoreActiveSpan()
                     .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER);
 
             SpanContext parentSpanContext = parentSpanContext(requestContext);
@@ -94,13 +98,14 @@ public void filter(ContainerRequestContext requestContext) {
 
     /**
      * Returns a parent for a span created by this filter (jax-rs span).
-     * The context from the active span takes precedence over context in the request.
+     * The context from the active span takes precedence over context in the request,
+     * but only if joinExistingActiveSpan has been set.
      * The current active span should be child-of extracted context and for example
      * created at a lower level e.g. jersey filter.
      */
     private SpanContext parentSpanContext(ContainerRequestContext requestContext) {
         Span activeSpan = tracer.activeSpan();
-        if (activeSpan != null) {
+        if (activeSpan != null && this.joinExistingActiveSpan) {
             return activeSpan.context();
         } else {
             return tracer.extract(