Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make active span be ignored by default; used as parent span if configured #110

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@

public class ApacheCXFParentSpanResolutionTest extends AbstractParentSpanResolutionTest {

@Override
protected boolean shouldUseParentSpan() {
return true;
}
Copy link
Contributor Author

@rnorth rnorth Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each of the existing IT classes is basically duplicated for the additional scenario, with the shouldUseParentSpan() method controlling:

  • whether or not the new opt-in parameter is set, and
  • what the assertion looks for.


@Override
protected void initServletContext(ServletContextHandler context) {
super.initServletContext(context);
Expand Down
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is copied from the AbstractServerDefaultConfigurationTest, but with some logic to control whether or not we want to use the new withJoinExistingActiveSpan method in the builder.

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()), "/*",
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -116,6 +119,7 @@ public Builder(Tracer tracer) {
this.allTraced = true;
this.operationNameBuilder = WildcardOperationName.newBuilder();
this.traceSerialization = true;
this.joinExistingActiveSpan = false;
}

/**
Expand Down Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you happy with the naming of this? I couldn't think of much better, but perhaps it could be improved.

this.joinExistingActiveSpan = joinExistingActiveSpan;
return this;
}

/**
* @return server tracing dynamic feature. This feature should be manually registered to {@link javax.ws.rs.core.Application}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -64,6 +67,7 @@ public void filter(ContainerRequestContext requestContext) {
if (tracer != null) {

Tracer.SpanBuilder spanBuilder = tracer.buildSpan(operationNameProvider.operationName(requestContext))
.ignoreActiveSpan()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably always have been in the code as far as I can tell; the thing that actually seems to be more influential is:

if (parentSpanContext != null) {
  spanBuilder.asChildOf(parentSpanContext);
}

below.

.withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER);

SpanContext parentSpanContext = parentSpanContext(requestContext);
Expand Down Expand Up @@ -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(
Expand Down