Implement the TraceInterceptor API #253
Conversation
Still doesn’t work with Spring Boot because the way they structure their Jars.
tylerbenson
force-pushed
the
tyler/traceinterceptor
branch
from
b815c2a
to
60a1746
Compare
| @@ -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.
These were causing warnings when dealing with injected classes: raphw/byte-buddy#373
Added the failSafe to suppress the noise.
tylerbenson
added
comp: core
To prevent errors on injected classes.
tylerbenson
force-pushed
the
tyler/traceinterceptor
branch
from
60a1746
to
fb74c7b
Compare
| new EmptyNonDelegatingLoader() | ||
|
|
||
| then: | ||
| 1 * tracer.registerClassLoader(_ as EmptyNonDelegatingLoader) |
There was a problem hiding this comment.
What does this syntax mean? Not sure what's being asserted.
There was a problem hiding this comment.
This means that tracer.registerClassLoader was called one time with an argument of type EmptyNonDelegatingLoader.
Basically a way of asserting the mock interactions.
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| public class CallDepthThreadLocalMap { | ||
| private static final ThreadLocal<AtomicInteger> tls = new ThreadLocal<>(); |
There was a problem hiding this comment.
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.
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.
No, AtomicInteger is fine.
There was a problem hiding this comment.
Few random questions, but the changes look good.
However, there's a problem for javaagent users.
We relocate the api namespace when we run as an agent: https://github.com/DataDog/dd-trace-java/blob/master/dd-java-agent/dd-java-agent.gradle#L53-L56
So the ServiceLocator will end up searching the datadog.trace.agent.api.
I'd be in favor of removing those relocate rules. See any downside to that?
|
Good point about the relocation. I forgot about that, but I agree the API classes should not be relocated. I'll just do that. |
We expect customers to be using these also, so we can’t change them.
Add a TraceInterceptor API with the design intent to only require a dependency on
dd-trace-api. This is done by using java'sServiceLoader. Unfortunately in my testing, this doesn't seem to work smoothly with Spring Boot's executable jar. If this issue comes up, I can revisit.(Merging into
configurable-scope-managerbranch)