OpenTracing no longer automatically added in dd-java-agent. Registers GlobalTracer instance via instrumentation if necessary.
#1519
Conversation
tylerbenson
force-pushed
the
tyler/ot-split
branch
from
bb066a1
to
77fe8a3
Compare
tylerbenson
force-pushed
the
tyler/ot-split
branch
from
77fe8a3
to
1ea1927
Compare
tylerbenson
changed the title
dd-java-agentdd-java-agent with specific instrumentation.
There was a problem hiding this comment.
Straightforward and not straightforward at the same time lol. I tried to review everything. I'm relying on the tests for some of the changes to the delegates. Only a couple inline comments and these general thoughts:
Duplicate classes: No need to hold this PR up. We could probably do some shadowJar/gradle magic to get it to work but it's complicated by the fact that we need OT31 and OT32 wrappers. I'm not sure it would be simpler. I looked into Lombok's @Delegated annotation but that doesn't quite work with how we return this.
Interfaces: I still hate how everything has to implement everything. There's no fix right now until we cleanup and hopefully simplify the APIs. The Agent.Context changes do help things a little.
Overall 👍
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java
Show resolved
Hide resolved
| testCompile project(':dd-trace-core') | ||
| testCompile project(':utils:test-utils') | ||
|
|
||
| testCompile deps.testLogging | ||
| testCompile deps.guava | ||
| testCompile group: 'io.opentracing', name: 'opentracing-util', version: '0.31.0' |
There was a problem hiding this comment.
Is this necessary? Also, why is it 0.31.0 when we're using 0.32.0 everywhere else
There was a problem hiding this comment.
This is to enable OpenTracingTest. I don't remember why I went with 31 instead of 32, but I don't think it matters. I can change if you like.
There was a problem hiding this comment.
I would argue that this test belongs with the OT instrumentation. It's directly testing that the OT instrumentation is enabled and works. None of the other tests at the dd-java-agent level are that specific.
I don't feel strongly about it though.
There was a problem hiding this comment.
I already have similar tests at the OT instrumentation level. I felt this was a bit more core behavior and warranted a test at the agent level.
tylerbenson
changed the title
dd-java-agent with specific instrumentation.dd-java-agent. Registers GlobalTracer instance via instrumentation if necessary.
With this PR, if
GlobalTraceris used, instrumentation will activate to automatically register aTracerinstance for it, but this behavior can be disabled.To disable OpenTracing:
-Ddd.integration.opentracing.enabled=falseDD_INTEGRATION_OPENTRACING_ENABLED=falseThere are several classes that are now duplicated around the codebase. I'm not sure of a good strategy to reduce that duplication.