From 6559b80c74bc782c9e626e50e3636d0336f6df63 Mon Sep 17 00:00:00 2001 From: Richard North Date: Wed, 17 Oct 2018 13:07:51 +0100 Subject: [PATCH 1/3] 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 --- ...acheCXFParentSpanIgnoredByDefaultTest.java | 18 ++++++++ .../ApacheCXFParentSpanResolutionTest.java | 5 +++ .../AbstractParentSpanResolutionTest.java | 41 +++++++++++++++---- .../JerseyParentSpanIgnoredByDefaultTest.java | 17 ++++++++ .../JerseyParentSpanResolutionTest.java | 5 +++ ...estEasyParentSpanIgnoredByDefaultTest.java | 18 ++++++++ .../RestEasyParentSpanResolutionTest.java | 5 +++ .../server/ServerTracingDynamicFeature.java | 28 ++++++++++--- .../jaxrs2/server/ServerTracingFilter.java | 11 +++-- 9 files changed, 132 insertions(+), 16 deletions(-) create mode 100644 opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanIgnoredByDefaultTest.java create mode 100644 opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanIgnoredByDefaultTest.java create mode 100644 opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanIgnoredByDefaultTest.java diff --git a/opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanIgnoredByDefaultTest.java b/opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanIgnoredByDefaultTest.java new file mode 100644 index 0000000..8407b07 --- /dev/null +++ b/opentracing-jaxrs2-itest/apache-cxf/src/test/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/opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java b/opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java index 55a26e1..6cd5925 100644 --- a/opentracing-jaxrs2-itest/apache-cxf/src/test/java/io/opentracing/contrib/jaxrs2/itest/cxf/ApacheCXFParentSpanResolutionTest.java +++ b/opentracing-jaxrs2-itest/apache-cxf/src/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/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java b/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java index 8ff6cf3..878a144 100644 --- a/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java +++ b/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java @@ -1,8 +1,9 @@ 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; @@ -10,17 +11,39 @@ 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.assertNotEquals(preceding.context().spanId(), original.parentId()); + } } class FilterThatInitsSpan implements Filter { diff --git a/opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanIgnoredByDefaultTest.java b/opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanIgnoredByDefaultTest.java new file mode 100644 index 0000000..8ba102e --- /dev/null +++ b/opentracing-jaxrs2-itest/jersey/src/test/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/opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanResolutionTest.java b/opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanResolutionTest.java index 89cd614..6934e22 100644 --- a/opentracing-jaxrs2-itest/jersey/src/test/java/io/opentracing/contrib/jaxrs2/itest/jersey/JerseyParentSpanResolutionTest.java +++ b/opentracing-jaxrs2-itest/jersey/src/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/opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanIgnoredByDefaultTest.java b/opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanIgnoredByDefaultTest.java new file mode 100644 index 0000000..9b5a735 --- /dev/null +++ b/opentracing-jaxrs2-itest/resteasy/src/test/java/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/opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanResolutionTest.java b/opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanResolutionTest.java index e725d05..de31ff7 100644 --- a/opentracing-jaxrs2-itest/resteasy/src/test/java/io/opentracing/contrib/jaxrs2/itest/resteasy/RestEasyParentSpanResolutionTest.java +++ b/opentracing-jaxrs2-itest/resteasy/src/test/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/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java b/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java index caa9ed5..0e4d15a 100644 --- a/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java +++ b/opentracing-jaxrs2/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 current scope will be used as the parent span. + * If false, and a parent span can be extracted from the HTTP request, that span + * will be used as the parent span instead. + * 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/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java b/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java index 1cc1dea..1390c3b 100644 --- a/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java +++ b/opentracing-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 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( From 049d2e99051f3b1361e591379d3e3760bd5f9473 Mon Sep 17 00:00:00 2001 From: Richard North Date: Mon, 22 Oct 2018 12:23:51 +0100 Subject: [PATCH 2/3] empty commit to refresh PR From f5ef880338e6affc0ae23b0997d57de6ba384b5a Mon Sep 17 00:00:00 2001 From: Richard North Date: Wed, 31 Oct 2018 12:31:52 +0000 Subject: [PATCH 3/3] Changes based on review comments from @pavolloffay --- .../itest/common/AbstractParentSpanResolutionTest.java | 2 +- .../contrib/jaxrs2/server/ServerTracingDynamicFeature.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java b/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java index 878a144..fa8f51b 100644 --- a/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java +++ b/opentracing-jaxrs2-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractParentSpanResolutionTest.java @@ -77,7 +77,7 @@ public void testUseActiveSpanIfSet() { if (shouldUseParentSpan()) { Assert.assertEquals(preceding.context().spanId(), original.parentId()); } else { - Assert.assertNotEquals(preceding.context().spanId(), original.parentId()); + Assert.assertEquals(0, original.parentId()); } } diff --git a/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java b/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java index 0e4d15a..9af8e99 100644 --- a/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java +++ b/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingDynamicFeature.java @@ -202,9 +202,9 @@ public Builder withSkipPattern(String skipPattern) { } /** - * @param joinExistingActiveSpan If true, any active span on the current scope will be used as the parent span. - * If false, and a parent span can be extracted from the HTTP request, that span - * will be used as the parent span instead. + * @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 */