Skip to content

Commit

Permalink
Remove exemption where we didn't defer if the custom logging manager
Browse files Browse the repository at this point in the history
or JMX builder was on the system classpath (because the main thread
would find it there if OkHttp triggered initialization of JUL.).

We now make OkHttp calls from our own background threads, which are
isolated from the system classloader, not the main thread - so this
exemption no longer makes sense.
  • Loading branch information
mcculls committed Dec 28, 2024
1 parent 9a09d98 commit 9ee929b
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 102 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import static datadog.trace.util.AgentThreadFactory.AgentThread.PROFILER_STARTUP;
import static datadog.trace.util.AgentThreadFactory.AgentThread.TRACE_STARTUP;
import static datadog.trace.util.AgentThreadFactory.newAgentThread;
import static datadog.trace.util.Strings.getResourceName;
import static datadog.trace.util.Strings.propertyNameToSystemPropertyName;
import static datadog.trace.util.Strings.toEnvVar;

Expand Down Expand Up @@ -1286,14 +1285,8 @@ private static boolean isAppUsingCustomLogManager(final EnumSet<Library> librari

final String logManagerProp = System.getProperty("java.util.logging.manager");
if (logManagerProp != null) {
final boolean onSysClasspath =
ClassLoader.getSystemResource(getResourceName(logManagerProp)) != null;
log.debug("Prop - logging.manager: {}", logManagerProp);
log.debug("logging.manager on system classpath: {}", onSysClasspath);
// Some applications set java.util.logging.manager but never actually initialize the logger.
// Check to see if the configured manager is on the system classpath.
// If so, it should be safe to initialize jmxfetch which will setup the log manager.
return !onSysClasspath;
return true;
}

return false;
Expand Down Expand Up @@ -1324,14 +1317,8 @@ private static boolean isAppUsingCustomJMXBuilder(final EnumSet<Library> librari

final String jmxBuilderProp = System.getProperty("javax.management.builder.initial");
if (jmxBuilderProp != null) {
final boolean onSysClasspath =
ClassLoader.getSystemResource(getResourceName(jmxBuilderProp)) != null;
log.debug("Prop - javax.management.builder.initial: {}", jmxBuilderProp);
log.debug("javax.management.builder.initial on system classpath: {}", onSysClasspath);
// Some applications set javax.management.builder.initial but never actually initialize JMX.
// Check to see if the configured JMX builder is on the system classpath.
// If so, it should be safe to initialize jmxfetch which will setup JMX.
return !onSysClasspath;
return true;
}

return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class CustomLogManagerTest extends Specification {
, true) == 0
}

def "agent services starts up in premain if configured log manager on system classpath"() {
def "agent services startup is delayed even if configured log manager on system classpath"() {
expect:
IntegrationTestUtils.runOnSeparateJvm(LogManagerSetter.getName()
, [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class CustomMBeanServerBuilderTest extends Specification {
, true) == 0
}

def "JMXFetch starts up in premain if configured MBeanServerBuilder on system classpath"() {
def "JMXFetch startup is delayed even if configured MBeanServerBuilder on system classpath"() {
expect:
IntegrationTestUtils.runOnSeparateJvm(MBeanServerBuilderSetter.getName()
, [
Expand Down
88 changes: 28 additions & 60 deletions dd-java-agent/src/test/java/jvmbootstraptest/LogManagerSetter.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,55 +39,38 @@ public static void main(final String... args) throws Exception {
} else if (System.getProperty("java.util.logging.manager") != null) {
System.out.println("java.util.logging.manager != null");

if (ClassLoader.getSystemResource(
System.getProperty("java.util.logging.manager").replaceAll("\\.", "/") + ".class")
== null) {
assertTraceInstallationDelayed(
"tracer install must be delayed when log manager system property is present.");
customAssert(
isJmxfetchStarted(false),
false,
"jmxfetch startup must be delayed when log manager system property is present.");
if (isJFRSupported()) {
assertProfilingStartupDelayed(
"profiling startup must be delayed when log manager system property is present.");
}
// Change back to a valid LogManager.
System.setProperty("java.util.logging.manager", CUSTOM_LOG_MANAGER_CLASS_NAME);
customAssert(
LogManager.getLogManager().getClass(),
LogManagerSetter.class
.getClassLoader()
.loadClass(System.getProperty("java.util.logging.manager")),
"Javaagent should not prevent setting a custom log manager");
customAssert(
isTracerInstalled(true), true, "tracer should be installed after loading LogManager.");
customAssert(
isJmxfetchStarted(true), true, "jmxfetch should start after loading LogManager.");
if (isJFRSupported()) {
customAssert(
isProfilingStarted(true), true, "profiling should start after loading LogManager.");
}
} else {
customAssert(
isTracerInstalled(false),
true,
"tracer should be installed in premain when custom log manager found on classpath.");
customAssert(
isTracerInstalled(false),
true,
"tracer install is not delayed when log manager system property is present.");
customAssert(
isJmxfetchStarted(false),
false,
"jmxfetch startup must be delayed when log manager system property is present.");
if (isJFRSupported()) {
assertProfilingStartupDelayed(
"profiling startup must be delayed when log manager system property is present.");
}
// Change back to a valid LogManager.
System.setProperty("java.util.logging.manager", CUSTOM_LOG_MANAGER_CLASS_NAME);
customAssert(
LogManager.getLogManager().getClass(),
LogManagerSetter.class
.getClassLoader()
.loadClass(System.getProperty("java.util.logging.manager")),
"Javaagent should not prevent setting a custom log manager");
customAssert(
isJmxfetchStarted(true), true, "jmxfetch should start after loading LogManager.");
if (isJFRSupported()) {
customAssert(
isJmxfetchStarted(false),
true,
"jmxfetch should start in premain when custom log manager found on classpath.");
if (isJFRSupported()) {
customAssert(
isProfilingStarted(false),
true,
"profiling should start in premain when custom log manager found on classpath.");
}
isProfilingStarted(true), true, "profiling should start after loading LogManager.");
}
} else if (System.getenv("JBOSS_HOME") != null) {
System.out.println("JBOSS_HOME != null");
assertTraceInstallationDelayed(
"tracer install must be delayed when JBOSS_HOME property is present.");
customAssert(
isTracerInstalled(false),
true,
"tracer install is not delayed when JBOSS_HOME property is present.");
customAssert(
isJmxfetchStarted(false),
false,
Expand All @@ -104,10 +87,6 @@ public static void main(final String... args) throws Exception {
.getClassLoader()
.loadClass(System.getProperty("java.util.logging.manager")),
"Javaagent should not prevent setting a custom log manager");
customAssert(
isTracerInstalled(true),
true,
"tracer should be installed after loading with JBOSS_HOME set.");
customAssert(
isJmxfetchStarted(true),
true,
Expand Down Expand Up @@ -147,17 +126,6 @@ private static void customAssert(
}
}

private static void assertTraceInstallationDelayed(final String message) {
if (okHttpMayIndirectlyLoadJUL()) {
customAssert(isTracerInstalled(false), false, message);
} else {
customAssert(
isTracerInstalled(false),
true,
"We can safely install tracer on java9+ since it doesn't indirectly trigger logger manager init");
}
}

private static void assertProfilingStartupDelayed(final String message) {
if (okHttpMayIndirectlyLoadJUL()) {
customAssert(isProfilingStarted(false), false, message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,31 +26,19 @@ public static void main(final String... args) throws Exception {
} else if (System.getProperty("javax.management.builder.initial") != null) {
System.out.println("javax.management.builder.initial != null");

if (ClassLoader.getSystemResource(
System.getProperty("javax.management.builder.initial").replaceAll("\\.", "/")
+ ".class")
== null) {
customAssert(
isJmxfetchStarted(false),
false,
"jmxfetch startup must be delayed when management builder system property is present.");
// Change back to a valid MBeanServerBuilder.
System.setProperty(
"javax.management.builder.initial", "jvmbootstraptest.CustomMBeanServerBuilder");
customAssert(
isCustomMBeanRegistered(),
true,
"Javaagent should not prevent setting a custom MBeanServerBuilder");
customAssert(
isJmxfetchStarted(true),
true,
"jmxfetch should start after loading MBeanServerBuilder.");
} else {
customAssert(
isJmxfetchStarted(false),
true,
"jmxfetch should start in premain when custom MBeanServerBuilder found on classpath.");
}
customAssert(
isJmxfetchStarted(false),
false,
"jmxfetch startup must be delayed when management builder system property is present.");
// Change back to a valid MBeanServerBuilder.
System.setProperty(
"javax.management.builder.initial", "jvmbootstraptest.CustomMBeanServerBuilder");
customAssert(
isCustomMBeanRegistered(),
true,
"Javaagent should not prevent setting a custom MBeanServerBuilder");
customAssert(
isJmxfetchStarted(true), true, "jmxfetch should start after loading MBeanServerBuilder.");
} else {
System.out.println("No custom MBeanServerBuilder");

Expand Down

0 comments on commit 9ee929b

Please sign in to comment.