-
Notifications
You must be signed in to change notification settings - Fork 275
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
Implement the TraceInterceptor API #253
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
apply from: "${rootDir}/gradle/java.gradle" | ||
|
||
dependencies { | ||
compile project(':dd-trace-ot') | ||
compile project(':dd-java-agent:agent-tooling') | ||
|
||
compile deps.bytebuddy | ||
compile deps.opentracing | ||
compile deps.autoservice | ||
|
||
testCompile project(':dd-java-agent:testing') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package datadog.trace.instrumentation.classloaders; | ||
|
||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
public class CallDepthThreadLocalMap { | ||
private static final ThreadLocal<AtomicInteger> tls = new ThreadLocal<>(); | ||
|
||
public static int incrementCallDepth() { | ||
AtomicInteger depth = tls.get(); | ||
if (depth == null) { | ||
depth = new AtomicInteger(0); | ||
tls.set(depth); | ||
return 0; | ||
} else { | ||
return depth.incrementAndGet(); | ||
} | ||
} | ||
|
||
public static void reset() { | ||
tls.remove(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
package datadog.trace.instrumentation.classloaders; | ||
|
||
import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses; | ||
import static net.bytebuddy.matcher.ElementMatchers.failSafe; | ||
import static net.bytebuddy.matcher.ElementMatchers.isConstructor; | ||
import static net.bytebuddy.matcher.ElementMatchers.isSubTypeOf; | ||
|
||
import com.google.auto.service.AutoService; | ||
import datadog.opentracing.DDTracer; | ||
import datadog.trace.agent.tooling.DDAdvice; | ||
import datadog.trace.agent.tooling.HelperInjector; | ||
import datadog.trace.agent.tooling.Instrumenter; | ||
import io.opentracing.util.GlobalTracer; | ||
import java.lang.reflect.Field; | ||
import net.bytebuddy.agent.builder.AgentBuilder; | ||
import net.bytebuddy.asm.Advice; | ||
|
||
@AutoService(Instrumenter.class) | ||
public final class ClassLoaderInstrumentation extends Instrumenter.Configurable { | ||
|
||
public ClassLoaderInstrumentation() { | ||
super("classloader"); | ||
} | ||
|
||
@Override | ||
protected boolean defaultEnabled() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public AgentBuilder apply(final AgentBuilder agentBuilder) { | ||
return agentBuilder | ||
.type( | ||
failSafe(isSubTypeOf(ClassLoader.class)), | ||
classLoaderHasClasses("io.opentracing.util.GlobalTracer")) | ||
.transform( | ||
new HelperInjector( | ||
"datadog.trace.instrumentation.classloaders.CallDepthThreadLocalMap")) | ||
.transform(DDAdvice.create().advice(isConstructor(), ClassloaderAdvice.class.getName())) | ||
.asDecorator(); | ||
} | ||
|
||
public static class ClassloaderAdvice { | ||
|
||
@Advice.OnMethodEnter | ||
public static int constructorEnter() { | ||
// We use this to make sure we only apply the exit instrumentation | ||
// after the constructors are done calling their super constructors. | ||
return CallDepthThreadLocalMap.incrementCallDepth(); | ||
} | ||
|
||
// Not sure why, but adding suppress causes a verify error. | ||
@Advice.OnMethodExit // (suppress = Throwable.class) | ||
public static void constructorExit( | ||
@Advice.This final ClassLoader cl, @Advice.Enter final int depth) { | ||
if (depth == 0) { | ||
CallDepthThreadLocalMap.reset(); | ||
|
||
try { | ||
final Field field = GlobalTracer.class.getDeclaredField("tracer"); | ||
field.setAccessible(true); | ||
|
||
final Object o = field.get(null); | ||
if (o instanceof DDTracer) { | ||
final DDTracer tracer = (DDTracer) o; | ||
tracer.registerClassLoader(cl); | ||
} | ||
} catch (final Throwable e) { | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
import datadog.opentracing.DDTracer | ||
import datadog.trace.agent.test.AgentTestRunner | ||
import datadog.trace.agent.test.TestUtils | ||
|
||
import java.security.SecureClassLoader | ||
|
||
class ClassLoaderInstrumentationTest extends AgentTestRunner { | ||
static { | ||
System.setProperty("dd.integration.classloader.enabled", "true") | ||
} | ||
|
||
DDTracer tracer = Mock() | ||
|
||
def setup() { | ||
TestUtils.registerOrReplaceGlobalTracer(tracer) | ||
} | ||
|
||
def "creating classloader calls register on tracer"() { | ||
when: | ||
new EmptyNonDelegatingLoader() | ||
|
||
then: | ||
1 * tracer.registerClassLoader(_ as EmptyNonDelegatingLoader) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this syntax mean? Not sure what's being asserted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means that |
||
0 * _ | ||
} | ||
|
||
def "creating anonymous classloader calls register on tracer"() { | ||
when: | ||
new EmptyNonDelegatingLoader() {} | ||
|
||
then: | ||
1 * tracer.registerClassLoader(_ as EmptyNonDelegatingLoader) | ||
0 * _ | ||
} | ||
|
||
def "bootstrap classloaders aren't instrumented"() { | ||
// (Because they don't have access to GlobalTracer) | ||
when: | ||
new SecureClassLoader() | ||
new SecureClassLoader(null) {} | ||
new URLClassLoader(new URL[0], ClassLoader.systemClassLoader) | ||
|
||
then: | ||
0 * tracer.registerClassLoader(_) | ||
0 * _ | ||
} | ||
|
||
class EmptyNonDelegatingLoader extends SecureClassLoader { | ||
EmptyNonDelegatingLoader() { | ||
super(null) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import static datadog.trace.agent.tooling.ClassLoaderMatcher.classLoaderHasClasses; | ||
import static datadog.trace.instrumentation.jms.util.JmsUtil.toResourceName; | ||
import static io.opentracing.log.Fields.ERROR_OBJECT; | ||
import static net.bytebuddy.matcher.ElementMatchers.failSafe; | ||
import static net.bytebuddy.matcher.ElementMatchers.hasSuperType; | ||
import static net.bytebuddy.matcher.ElementMatchers.isInterface; | ||
import static net.bytebuddy.matcher.ElementMatchers.isPublic; | ||
|
@@ -40,7 +41,7 @@ public JMS1MessageProducerInstrumentation() { | |
public AgentBuilder apply(final AgentBuilder agentBuilder) { | ||
return agentBuilder | ||
.type( | ||
not(isInterface()).and(hasSuperType(named("javax.jms.MessageProducer"))), | ||
not(isInterface()).and(failSafe(hasSuperType(named("javax.jms.MessageProducer")))), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were causing warnings when dealing with injected classes: raphw/byte-buddy#373 |
||
not(classLoaderHasClasses("javax.jms.JMSContext", "javax.jms.CompletionListener"))) | ||
.transform(JMS1MessageConsumerInstrumentation.JMS1_HELPER_INJECTOR) | ||
.transform( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the count is in a thread-local, why not use an integer instead of an atomic integer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but AtomicInteger has nice increment and decrement operations whereas with Integer, I'd have to do those operations then reset on the threadlocal. Still think I should do Integer instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, AtomicInteger is fine.