Enhance dd-java-agent ClassLoading by realark · Pull Request #218 · DataDog/dd-trace-java

realark · 2018-02-07T03:21:05Z

@tylerbenson If you have time, would you mind taking a look at the gradle project layout and config? Not urgent, and this isn't ready for a reveiw anyways.

Gradle Projects

dd-java-agent:agent-bootstrap: Runs on bootstrap loader. Contains api classes like opentracing, dd-trace-api
dd-java-agent:agent-tooling: renaming of tooling. Contains dd-trace-ot and agent dependencies.
dd-java-agent:instrumentation: A single project which depends on agent-tooling and all the instrumentation. Used to create a single shadowJar which contains both the core agent and the instrumentation.
dd-java-agent: Bare-bones jar with no dependencies. Extracts the bootstrap and instrumentation jar, sets up the classpath and invokes agent and opentracing startup.

tylerbenson · 2018-02-07T03:47:55Z

+
+  // TODO: Write test
+  // for every class in bootstrap jar:
+  //   assert class not present in agent jar


It might be easier to do a whitelist, maybe using something like https://google.github.io/guava/releases/17.0/api/docs/index.html?com/google/common/reflect/ClassPath.html to scan the classpath.

Neat. I'll look into that.

tylerbenson · 2018-02-07T03:49:22Z

+  compile deps.opentracing
+  compile deps.slf4j
+  compile group: 'org.slf4j', name: 'slf4j-simple', version: '1.7.25'
+  // ^ Generally a bad idea for libraries, but we're shadowing.


You will need to apply the shadow rename rules here too.

Never mind... I see it is handled by dd-java-agent instead.

Yeah I wanted to keep the shadow rules in the same block so they're consistently applied.

tylerbenson · 2018-02-07T03:51:16Z

+
+    // 1. Skip classloaders which don't delegate to bootstrap
+
+    // 2. Skip incompatible versions of OpenTracing


These would be nice, but I'm curious how you plan to achieve them.

When we transform classes we're given an instance of the classloader the class is about to load on. We can test that classloader to ensure its loading strategy and classpath are compatible with our instrumentation, and abort the instrumentation otherwise.

tylerbenson · 2018-02-07T03:54:24Z

+@Slf4j
+public class TracerInstaller {
+  /** Register a global tracer if no global tracer is already registered. */
+  public static synchronized void installGlobalTracer() {


One idea that @adriancole suggested to me is instead of trying to register with GlobalTracer, just rewrite the get() method to just return our instance.

What's the benefit of a bytecode rewrite instead of registering the global tracer?

Primarily avoiding the volatile field lookup, but also preventing a possible exception if they are registering their own tracer.

tylerbenson · 2018-02-07T05:00:23Z

+
+      final ClassLoader agentClassLoader = TracingAgent.class.getClassLoader();
+
+      inst.appendToSystemClassLoaderSearch(bootStrapJar);


Shouldn't this be appendToBootstrapClassLoaderSearch?

It will be. At this stage I'm trying to get the gradle project and agent bootstrapping working. Next step is to start using the bootstrap and custom classloader.

tylerbenson · 2018-02-07T05:02:57Z

-    // in static initializers
-    DDJavaAgentInfo.VERSION.toString();
-    DDTraceOTInfo.VERSION.toString();
-    DDTraceApiInfo.VERSION.toString();


It seems these were lost in the shuffle. Can you think of a better way to handle this? Do you think I'm being overly paranoid?

They'll be back. I commented them out because the new dd-java-agent project has no dependencies and can't log. I'm going to move this over to the tooling project.

tylerbenson · 2018-02-07T05:05:01Z

+                  "agent-bootstrap.jar.zip",
+                  "agent-bootstrap.jar"));
+
+      final ClassLoader agentClassLoader = TracingAgent.class.getClassLoader();


I assume the actual classloader will be used later?

tylerbenson · 2018-02-07T05:16:27Z

I realize you asked me to focus on the gradle stuff... I think that is all good. Sorry if I added unnecessary comments to the other stuff.

realark · 2018-02-07T17:56:04Z

No problem, reviewing the other stuff is appreciated!

tylerbenson · 2018-02-08T23:32:20Z

What was wrong with using TraceResolver?

Opentracing is now on the bootstrap loader. When the TracerResolver calls the service loader it checks the classpath of the bootstrap loader. Our DDTracer is in the agent's classloader, so it doesn't see it.

realark · 2018-02-09T22:23:35Z

Closing this out. Going to break this PR into several smaller ones.

realark added the tag: do not merge Do not merge changes label Feb 7, 2018

realark force-pushed the ark/stable_classloading branch from 43ccb2b to acf28e4 Compare February 7, 2018 03:25

realark changed the title ~~Reorganize dd-java-agent subprojects~~ Enhance dd-java-agent ClassLoading Feb 7, 2018

tylerbenson reviewed Feb 7, 2018

View reviewed changes

realark force-pushed the ark/stable_classloading branch 3 times, most recently from a942380 to fd022a1 Compare February 8, 2018 19:46

tylerbenson reviewed Feb 8, 2018

View reviewed changes

realark force-pushed the ark/stable_classloading branch 3 times, most recently from c8ded5f to faa2129 Compare February 9, 2018 02:04

Andrew Kent added 2 commits February 9, 2018 14:05

whitelist ConfigUtils from jacoco

8eb9cdc

Reorganize dd-java-agent subprojects

4471d46

realark force-pushed the ark/stable_classloading branch from faa2129 to f6dddc9 Compare February 9, 2018 22:05

Run api classes on bootstrap and core agent on custom classloader

55e5f57

realark force-pushed the ark/stable_classloading branch from f6dddc9 to 55e5f57 Compare February 9, 2018 22:17

realark closed this Feb 9, 2018

realark deleted the ark/stable_classloading branch February 9, 2018 22:32

tylerbenson added this to the Closed milestone Apr 10, 2020

gh-worker-campaigns-3e9aa4 Bot mentioned this pull request May 7, 2026

chore(ci) update one-pipeline #11309

Merged


		// 1. Skip classloaders which don't delegate to bootstrap

		// 2. Skip incompatible versions of OpenTracing


		final ClassLoader agentClassLoader = TracingAgent.class.getClassLoader();

		inst.appendToSystemClassLoaderSearch(bootStrapJar);

Conversation

realark commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realark Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerbenson commented Feb 7, 2018

Uh oh!

realark commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realark commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

realark commented Feb 7, 2018 •

edited

Loading

realark Feb 7, 2018 •

edited

Loading