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

OpenTracing no longer automatically added in dd-java-agent. Registers GlobalTracer instance via instrumentation if necessary. #1519

Merged
merged 4 commits into from
Jun 2, 2020
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions .palantir/revapi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,59 @@ acceptedBreaks:
- code: "java.class.removed"
old: "class datadog.trace.core.StringCachingBigInteger"
justification: "internal api"
- code: "java.class.removed"
old: "interface datadog.trace.core.scopemanager.DDScopeManager"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.DDTracer::<init>(===datadog.trace.core.CoreTracer===)"
new: "parameter void datadog.opentracing.DDTracer::<init>(===datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.DefaultLogHandler::log(java.lang.String,\
\ ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.DefaultLogHandler::log(java.lang.String,\
\ ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.DefaultLogHandler::log(java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.DefaultLogHandler::log(java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.DefaultLogHandler::log(long, java.lang.String,\
\ ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.DefaultLogHandler::log(long, java.lang.String,\
\ ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.DefaultLogHandler::log(long, java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.DefaultLogHandler::log(long, java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.LogHandler::log(java.lang.String, ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.LogHandler::log(java.lang.String, ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.LogHandler::log(java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.LogHandler::log(java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.LogHandler::log(long, java.lang.String,\
\ ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.LogHandler::log(long, java.lang.String,\
\ ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.parameterTypeChanged"
old: "parameter void datadog.opentracing.LogHandler::log(long, java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.core.DDSpan===)"
new: "parameter void datadog.opentracing.LogHandler::log(long, java.util.Map<java.lang.String,\
\ ?>, ===datadog.trace.bootstrap.instrumentation.api.AgentSpan===)"
justification: "Use Agent API instead of Core API where possible"
- code: "java.method.removed"
old: "method void datadog.trace.core.serialization.FormatWriter<DEST>::writeBigInteger(byte[],\
\ java.math.BigInteger, DEST) throws java.io.IOException"
Expand Down
1 change: 0 additions & 1 deletion dd-java-agent/agent-tooling/agent-tooling.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ dependencies {
annotationProcessor deps.autoservice
implementation deps.autoservice

compile project(':dd-trace-ot')
compile project(':dd-trace-core')
compile project(':dd-trace-core:jfr-openjdk')

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package datadog.trace.agent.tooling;

import datadog.trace.api.Tracer;
import datadog.trace.bootstrap.PatchLogger;
import datadog.trace.bootstrap.WeakCache;
import io.opentracing.util.GlobalTracer;
import lombok.extern.slf4j.Slf4j;
import net.bytebuddy.matcher.ElementMatcher;

Expand Down Expand Up @@ -86,7 +86,7 @@ private static boolean shouldSkipClass(final ClassLoader loader) {
*/
private static boolean delegatesToBootstrap(final ClassLoader loader) {
boolean delegates = true;
if (!loadsExpectedClass(loader, GlobalTracer.class)) {
if (!loadsExpectedClass(loader, Tracer.class)) {
randomanderson marked this conversation as resolved.
Show resolved Hide resolved
log.debug("loader {} failed to delegate bootstrap opentracing class", loader);
delegates = false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ public final class Constants {
"datadog.trace.bootstrap",
"datadog.trace.context",
"datadog.trace.instrumentation.api",
"io.opentracing"
};

// This is used in IntegrationTestUtils.java
Expand All @@ -37,12 +36,8 @@ public final class Constants {
"com.blogspot.mydailyjava.weaklockfree",
// bytebuddy
"net.bytebuddy",
// OT contribs for dd trace resolver
"io.opentracing.contrib",
// jackson
// msgpack
"org.msgpack",
"com.fasterxml.jackson",
"org.yaml.snakeyaml",
// disruptor
"com.lmax.disruptor",
// okHttp
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package datadog.trace.agent.tooling;

import datadog.opentracing.DDTracer;
import datadog.trace.api.Config;
import datadog.trace.api.GlobalTracer;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.core.CoreTracer;
import lombok.extern.slf4j.Slf4j;
Expand All @@ -11,16 +11,8 @@ public class TracerInstaller {
/** Register a global tracer if no global tracer is already registered. */
public static synchronized void installGlobalTracer() {
if (Config.get().isTraceEnabled()) {
if (!io.opentracing.util.GlobalTracer.isRegistered()) {
final CoreTracer tracer = CoreTracer.builder().build();
final DDTracer tracerOT = new DDTracer(tracer);
try {
io.opentracing.util.GlobalTracer.register(tracerOT);
datadog.trace.api.GlobalTracer.registerIfAbsent(tracer);
AgentTracer.registerIfAbsent(tracer);
} catch (final RuntimeException re) {
log.warn("Failed to register tracer: {}", tracerOT, re);
}
if (!(GlobalTracer.get() instanceof CoreTracer)) {
installGlobalTracer(CoreTracer.builder().build());
} else {
log.debug("GlobalTracer already registered.");
}
Expand All @@ -29,12 +21,18 @@ public static synchronized void installGlobalTracer() {
}
}

public static void installGlobalTracer(final CoreTracer tracer) {
try {
GlobalTracer.registerIfAbsent(tracer);
AgentTracer.registerIfAbsent(tracer);
} catch (final RuntimeException re) {
log.warn("Failed to register tracer: {}", tracer, re);
}
}

public static void logVersionInfo() {
VersionLogger.logAllVersions();
log.debug(
io.opentracing.util.GlobalTracer.class.getName()
+ " loaded on "
+ io.opentracing.util.GlobalTracer.class.getClassLoader());
log.debug(GlobalTracer.class.getName() + " loaded on " + GlobalTracer.class.getClassLoader());
log.debug(
AgentInstaller.class.getName() + " loaded on " + AgentInstaller.class.getClassLoader());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
dependencies {
compile project(':dd-trace-api')
compile project(':dd-java-agent:benchmark-integration')
compile deps.opentracing
compile group: 'io.opentracing', name: 'opentracing-api', version: '0.32.0'
compile group: 'io.opentracing', name: 'opentracing-util', version: '0.32.0'

compile group: 'org.eclipse.jetty', name: 'jetty-server', version: '9.4.1.v20170120'
compile group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '9.4.1.v20170120'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ dependencies {

play project(':dd-trace-api')
play project(':dd-java-agent:benchmark-integration')
play deps.opentracing
play group: 'io.opentracing', name: 'opentracing-api', version: '0.32.0'
play group: 'io.opentracing', name: 'opentracing-util', version: '0.32.0'
}

repositories {
Expand Down
20 changes: 18 additions & 2 deletions dd-java-agent/dd-java-agent.gradle
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar

import java.util.concurrent.atomic.AtomicBoolean

plugins {
id "com.github.johnrengelman.shadow" version "5.2.0"
}
Expand Down Expand Up @@ -45,7 +47,22 @@ ext.generalShadowJarConfig = {
}

def includeShadowJar(shadowJarTask, jarname) {
def opentracingFound = new AtomicBoolean()
project.processResources {
doFirst {
eachFile {
// We seem unlikely to use this name somewhere else.
if (it.path.contains("opentracing") && it.name.contains("Format\$Builtin")) {
opentracingFound.set(true)
}
}
}
doLast {
if (opentracingFound.get()) {
throw new GradleException("OpenTracing direct dependency found!")
}
}

from(zipTree(shadowJarTask.archiveFile)) {
into jarname + '.isolated'
rename '(^.*)\\.class$', '$1.classdata'
Expand Down Expand Up @@ -97,16 +114,15 @@ modifyPom {
dependencies {
testCompile project(':dd-java-agent:agent-bootstrap')
testCompile project(':dd-trace-api')
testCompile project(':dd-trace-ot')
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'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Also, why is it 0.31.0 when we're using 0.32.0 everywhere else

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// Includes for the top level shadow jar
shadowInclude project(path: ':dd-java-agent:agent-bootstrap')
shadowInclude deps.opentracing

// Includes for the shared internal shadow jar
sharedShadowInclude deps.shared
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,6 @@ muzzle {
dependencies {
main_java8CompileOnly group: 'com.typesafe.akka', name: 'akka-http_2.11', version: '10.0.0'

compile project(':dd-trace-api')
compile project(':dd-java-agent:agent-tooling')
compile deps.opentracing
compile deps.autoservice
annotationProcessor deps.autoservice

testCompile group: 'com.typesafe.akka', name: 'akka-http_2.11', version: '10.0.0'
testCompile group: 'com.squareup.okhttp3', name: 'okhttp', version: '3.6.0'
testCompile project(':dd-java-agent:instrumentation:trace-annotation')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
muzzle {
pass {
module = 'opentracing-util'
group = 'io.opentracing'
versions = '[0.31.0,0.31.0]'
assertInverse = true
}
}

apply from: "$rootDir/gradle/java.gradle"

dependencies {
compile project(':dd-java-agent:instrumentation:opentracing')

compileOnly group: 'io.opentracing', name: 'opentracing-util', version: '0.31.0'

testCompile group: 'io.opentracing', name: 'opentracing-util', version: '0.31.0'
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package datadog.trace.instrumentation.opentracing31;

import static datadog.trace.agent.tooling.ClassLoaderMatcher.hasClassesNamed;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isTypeInitializer;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.not;

import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.muzzle.ReferenceCreator;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import io.opentracing.util.GlobalTracer;
import java.util.Map;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

/**
* OT interface is reimplemented here rather than using dd-trace-ot as a dependency to allow muzzle
* to work correctly since it relies on the {@link ReferenceCreator#REFERENCE_CREATION_PACKAGE}
* prefix.
*/
@AutoService(Instrumenter.class)
public class GlobalTracerInstrumentation extends Instrumenter.Default {
public GlobalTracerInstrumentation() {
super("opentracing", "opentracing-globaltracer");
}

@Override
public ElementMatcher<ClassLoader> classLoaderMatcher() {
return not(hasClassesNamed("io.opentracing.tag.Tag"));
}

@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return named("io.opentracing.util.GlobalTracer");
}

@Override
public String[] helperClassNames() {
return new String[] {
packageName + ".OTTracer",
packageName + ".OTTracer$OTSpanBuilder",
packageName + ".OTPropagation$TextMapInjectSetter",
packageName + ".OTPropagation$TextMapExtractGetter",
packageName + ".OTScopeManager",
packageName + ".OTScopeManager$OTScope",
packageName + ".OTScopeManager$OTTraceScope",
packageName + ".TypeConverter",
packageName + ".OTSpan",
packageName + ".OTSpanContext",
"datadog.trace.instrumentation.opentracing.LogHandler",
"datadog.trace.instrumentation.opentracing.DefaultLogHandler",
};
}

@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return singletonMap(
isTypeInitializer(), GlobalTracerInstrumentation.class.getName() + "$GlobalTracerAdvice");
}

public static class GlobalTracerAdvice {
@Advice.OnMethodExit(suppress = Throwable.class)
public static void registerTracer() {
if (AgentTracer.isRegistered()) {
GlobalTracer.register(new OTTracer(AgentTracer.get()));
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package datadog.trace.instrumentation.opentracing31;

import datadog.trace.bootstrap.instrumentation.api.AgentPropagation;
import io.opentracing.propagation.TextMap;
import java.util.HashMap;
import java.util.Map;

class OTPropagation {

static class TextMapInjectSetter implements AgentPropagation.Setter<TextMap> {
static final TextMapInjectSetter INSTANCE = new TextMapInjectSetter();

@Override
public void set(final TextMap carrier, final String key, final String value) {
carrier.put(key, value);
}
}

static class TextMapExtractGetter implements AgentPropagation.Getter<TextMap> {
private final Map<String, String> extracted = new HashMap<>();

TextMapExtractGetter(final TextMap carrier) {
for (final Map.Entry<String, String> entry : carrier) {
extracted.put(entry.getKey(), entry.getValue());
}
}

@Override
public Iterable<String> keys(final TextMap carrier) {
return extracted.keySet();
}

@Override
public String get(final TextMap carrier, final String key) {
// This is the same as the one passed into the constructor
// So using "extracted" is valid
return extracted.get(key);
}
}
}