fix agent for zulu8 by mar-kolya · Pull Request #1084 · DataDog/dd-trace-java

mar-kolya · 2019-11-06T15:13:22Z

This patch should fix the agent on zulu8. Zulu8 recently made some changes that LoggerManager when JFR is loaded, and JFR gets loaded with some certificate handling code provided by JDK. The whole thing leads to LoggerManager being configured prematurely.

It should use real class to try to load

The problem was that on zulu8 loading OkHttp touches JFR which in turn touches log manager - which would break things like JBOSS. The fix is to delay installing agent (and writer) until log manager things have settled down - in way similar to jmxfetch. Unfortunately for 'main' agent this turns out to be more involved because of classloader shenanigans.

This reverts commit 0b4b07b.

Using `datadog.trace.agent` to hold agent class causes problems due to shadowing into this package of other classes.

jbachorik

Looks good. Tricky classloading :)
Just a few minor comments.

dougqh · 2019-11-07T14:35:42Z

+
+    @Override
+    public void run() {
+      /*


Makes sense, I'm little surprised we weren't doing this already.

'Before' we only used this logic for jmxfetch which happens to fork out its own threads before doing anything useful so that wasn't really a problem.

dougqh

Looks good to me. I appreciate the comments. It helps a lot in explaining what we're working around.

tylerbenson

Nice work @mar-kolya. I have some concerns about delaying agent install unnecessarily (like Oracle Java 8 with JBOSS), since I'm not sure what impact that would have.

See questions below.

tylerbenson · 2019-11-07T16:48:20Z

+  private static synchronized void installDatadogTracer(
+      final Instrumentation inst, final URL bootstrapURL) throws Exception {
+    if (AGENT_CLASSLOADER == null) {
+      throw new IllegalStateException("Datadog agent should have been started already");


If someone declares the javaagent on the command line multiple times, does this also prevent it from being started multiple times?

This actually should be safe and there is a comment about that few lines down.

tylerbenson · 2019-11-07T16:54:25Z

+        final Method agentInstallerMethod =
+            agentInstallerClass.getMethod("installBytebuddyAgent", Instrumentation.class);
+        agentInstallerMethod.invoke(null, inst);
+        AGENT_CLASSLOADER = agentClassLoader;


Should we rename the AgentInstaller and AGENT_CLASSLOADER to be something else?

Suggestions?

tylerbenson · 2019-11-07T17:03:11Z

+ *   <li>Do not store any static data in this class
+ *   <li>Do dot touch any logging facilities here so we can configure them later
+ * </ul>
+ */


Nice comment! I wonder if we should add protection somehow to assert this class is never loaded on the bootstrap classpath.

I'm not sure how to do that and it is really solves much...

tylerbenson · 2019-11-07T17:06:19Z

    throw new TimeoutException();
  }
+
+  private static class StreamGobbler extends Thread {


If you didn't live in 🇨🇦 I'd say you had 🦃 on your mind. Nice name though.

This originates from SO somewhere.

tylerbenson · 2019-11-07T17:22:15Z

+     * events which in turn loads LogManager. This is not a problem on newer JDKs because there JFR uses different
+     * logging facility.
+     */
+    if (isJavaBefore9() && appUsingCustomLogManager) {


I wonder if we should isolate this behavior to only when JFR is available on Java 8. I'm concerned about unintended side effects of delaying tracer installation.

should be fixed now

tylerbenson · 2019-11-07T17:23:35Z

-        if (entry.getKey().equals(typeName)) {
-          entry.getValue().run();
+      synchronized (CLASS_LOAD_CALLBACKS) {
+        final List<Runnable> callbacks = CLASS_LOAD_CALLBACKS.get(typeName);


Should they get removed after successful execution?

I'm not sure - this code originally didn't do removal

tylerbenson · 2019-11-07T17:30:10Z

 // Note: this test is fails on IBM JVM, we would need to investigate this at some point
 @Requires({ !System.getProperty("java.vm.name").contains("IBM J9 VM") })
-@Retry
-@Timeout(30)


This test is notorious for locking up the test suite. I'd much prefer to keep the timeout.

This test was locking up the testsuite because we didn't do proper capturing of stdout/stderr of forked process - which locked up the whole thing. I've fixed that. I didn't see it block anymore after the fix. I'd suggest we keep this removed and read that if it fails again.

tylerbenson

Please update description.

mar-kolya added 4 commits November 6, 2019 09:38

Allow big outputs in forked test processes

58d6103

Fix ClassLoadingTest

3715a3d

It should use real class to try to load

Revert "Disable zulu8 test for time being due to test incompatibility"

06a8956

This reverts commit 0b4b07b.

mar-kolya changed the title ~~Mar kolya/fix agent for zulu8~~ fix agent for zulu8 Nov 6, 2019

mar-kolya force-pushed the mar-kolya/fix-agent-for-zulu8 branch from 1d8af37 to 63b5504 Compare November 6, 2019 18:49

mar-kolya added 2 commits November 6, 2019 13:58

Fix scala warning

17155f7

Fix namespace clashes

42cece6

Using `datadog.trace.agent` to hold agent class causes problems due to shadowing into this package of other classes.

mar-kolya force-pushed the mar-kolya/fix-agent-for-zulu8 branch from 63b5504 to 42cece6 Compare November 6, 2019 18:58

mar-kolya added 2 commits November 6, 2019 15:12

More renames due to shadow clashes

fdfe4b1

Fix test for case when tracer install is not delayed

df6cb66

mar-kolya marked this pull request as ready for review November 6, 2019 20:53

mar-kolya requested a review from a team as a code owner November 6, 2019 20:53

mar-kolya requested review from jbachorik and tylerbenson November 6, 2019 20:53

jbachorik approved these changes Nov 7, 2019

View reviewed changes

Fix typo

f371e39

dougqh reviewed Nov 7, 2019

View reviewed changes

dougqh approved these changes Nov 7, 2019

View reviewed changes

tylerbenson suggested changes Nov 7, 2019

View reviewed changes

Do not delay tracer on OpenJDK8 with JBOSS

8f80a42

tylerbenson approved these changes Nov 7, 2019

View reviewed changes

mar-kolya merged commit 2789324 into master Nov 7, 2019

mar-kolya deleted the mar-kolya/fix-agent-for-zulu8 branch November 7, 2019 19:39

tylerbenson added this to the 0.38.0 milestone Nov 22, 2019

randomanderson mentioned this pull request Feb 11, 2020

Shared classloader for agent and jmx-fetch #1199

Merged

Conversation

mar-kolya commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbachorik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougqh left a comment

Choose a reason for hiding this comment

Uh oh!

tylerbenson left a comment • 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mar-kolya commented Nov 6, 2019 •

edited

Loading

tylerbenson left a comment •

edited

Loading