open-telemetry
/
opentelemetry-java-instrumentation
mirror of https://github.com/open-telemetry/opentelemetry-java-instrumentation.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Improve OSGi class loader instrumentation 

It turns out that Eclipse's OSGi implementation has two problems:
* It doesn't respect system properties by default
* It has tricky classloader implementation that loads bootstrap
  classes from the classloader-has-delegation-to-bootstrap check but
  doesn't load bootstrap classes from 'normal' code.

This should address second problem and make Eclipse's OSGi
implementation 'safe' by default.
This commit is contained in:
Nikolay Martynov 2019-01-24 22:48:14 -05:00
parent 63779c7816
commit 04fbb5085f
9 changed files with 161 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/ClassLoaderMatcher.java
View File

@ -91,6 +91,12 @@ public class ClassLoaderMatcher {
}
}
/**
* TODO: this turns out to be useless with OSGi: {@code
* }org.eclipse.osgi.internal.loader.BundleLoader#isRequestFromVM} returns {@code true} when
* class loading is issued from this check and {@code false} for 'real' class loads. We should
* come up with some sort of hack to avoid this problem.
*/
private boolean delegatesToBootstrap(final ClassLoader loader) {
boolean delegates = true;
if (!loadsExpectedClass(loader, GlobalTracer.class)) {

45
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Constants.java Normal file
View File

@ -0,0 +1,45 @@
package datadog.trace.agent.tooling;
/**
* Some useful constants.
*
* <p>Idea here is to keep this class safe to inject into client's class loader.
*/
public final class Constants {
/**
* packages which will be loaded on the bootstrap classloader
*
* <p>Updates should be mirrored in TestUtils#BOOTSTRAP_PACKAGE_PREFIXES_COPY
*/
public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = {
"io.opentracing",
"datadog.slf4j",
"datadog.trace.bootstrap",
"datadog.trace.api",
"datadog.trace.context"
};
// This is used in IntegrationTestUtils.java
public static final String[] AGENT_PACKAGE_PREFIXES = {
"datadog.trace.common",
"datadog.trace.agent",
"datadog.trace.instrumentation",
// guava
"com.google.auto",
"com.google.common",
"com.google.thirdparty.publicsuffix",
// WeakConcurrentMap
"com.blogspot.mydailyjava.weaklockfree",
// bytebuddy
"net.bytebuddy",
// OT contribs for dd trace resolver
"io.opentracing.contrib",
// jackson
"org.msgpack",
"com.fasterxml.jackson",
"org.yaml.snakeyaml",
};
private Constants() {}
}

33
dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Utils.java
View File

@ -10,39 +10,8 @@ import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDefinition;
public class Utils {
/**
* packages which will be loaded on the bootstrap classloader
*
* <p>Updates should be mirrored in TestUtils#BOOTSTRAP_PACKAGE_PREFIXES_COPY
*/
public static final String[] BOOTSTRAP_PACKAGE_PREFIXES = {
"io.opentracing",
"datadog.slf4j",
"datadog.trace.bootstrap",
"datadog.trace.api",
"datadog.trace.context"
};
public static final String[] AGENT_PACKAGE_PREFIXES = {
"datadog.trace.common",
"datadog.trace.agent",
"datadog.trace.instrumentation",
// guava
"com.google.auto",
"com.google.common",
"com.google.thirdparty.publicsuffix",
// WeakConcurrentMap
"com.blogspot.mydailyjava.weaklockfree",
// bytebuddy
"net.bytebuddy",
// OT contribs for dd trace resolver
"io.opentracing.contrib",
// jackson
"org.msgpack",
"com.fasterxml.jackson",
"org.yaml.snakeyaml",
};
// This is used in HelperInjectionTest.groovy
private static Method findLoadedClassMethod = null;
private static final BootstrapClassLoaderProxy unitTestBootstrapProxy =

16
dd-java-agent/instrumentation/jboss-classloading/src/main/java/datadog/trace/instrumentation/jboss/JBossClassloadingInstrumentation.java
View File

@ -3,8 +3,8 @@ package datadog.trace.instrumentation.jboss;
import static net.bytebuddy.matcher.ElementMatchers.named;
import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Constants;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.Utils;
import java.security.ProtectionDomain;
import java.util.Collections;
import java.util.Map;
@ -32,18 +32,18 @@ public final class JBossClassloadingInstrumentation extends Instrumenter.Default
final Class<?> classBeingRedefined,
final ProtectionDomain protectionDomain) {
// Set the system prop to tell jboss to delegate classloads for datadog bootstrap classes
final StringBuilder ddPrefixes = new StringBuilder("");
for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
final StringBuilder prefixes = new StringBuilder("");
for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
if (i > 0) {
ddPrefixes.append(",");
prefixes.append(",");
}
ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]);
prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]);
}
final String existing = System.getProperty("jboss.modules.system.pkgs");
if (null == existing) {
System.setProperty("jboss.modules.system.pkgs", ddPrefixes.toString());
} else if (!existing.contains(ddPrefixes)) {
System.setProperty("jboss.modules.system.pkgs", existing + "," + ddPrefixes.toString());
System.setProperty("jboss.modules.system.pkgs", prefixes.toString());
} else if (!existing.contains(prefixes)) {
System.setProperty("jboss.modules.system.pkgs", existing + "," + prefixes.toString());
}
}

7
dd-java-agent/instrumentation/osgi-classloading/osgi-classloading.gradle
View File

@ -6,6 +6,11 @@ dependencies {
annotationProcessor deps.autoservice
implementation deps.autoservice
testCompile group: 'org.osgi', name: 'org.osgi.core', version: '4.0.0'
// TODO: we should separate core and Eclipse tests at some point,
// but right now core-specific tests are quite dump and are run with
// core version provided by Eclipse implementation.
//testCompile group: 'org.osgi', name: 'org.osgi.core', version: '4.0.0'
testCompile group: 'org.eclipse.platform', name: 'org.eclipse.osgi', version: '3.13.200'
testCompile project(':dd-java-agent:testing')
}

87
dd-java-agent/instrumentation/osgi-classloading/src/main/java/datadog/trace/instrumentation/osgi/OSGIClassloadingInstrumentation.java
View File

@ -1,13 +1,17 @@
package datadog.trace.instrumentation.osgi;
import static java.util.Collections.emptyMap;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Constants;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.Utils;
import java.security.ProtectionDomain;
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;
@ -21,8 +25,15 @@ public final class OSGIClassloadingInstrumentation extends Instrumenter.Default
@Override
public ElementMatcher<TypeDescription> typeMatcher() {
// OSGI Bundle class loads the sys prop which defines bootstrap classes
return named("org.osgi.framework.Bundle");
// OSGi Bundle class loads the system property which defines bootstrap classes
return named("org.osgi.framework.Bundle").or(named("org.eclipse.osgi.launch.EquinoxFactory"));
}
@Override
public String[] helperClassNames() {
return new String[] {
Constants.class.getName(), OSGIClassloadingInstrumentation.class.getName() + "$Helper"
};
}
@Override
@ -32,27 +43,59 @@ public final class OSGIClassloadingInstrumentation extends Instrumenter.Default
final JavaModule module,
final Class<?> classBeingRedefined,
final ProtectionDomain protectionDomain) {
// Set the system prop to tell osgi to delegate classloads for datadog bootstrap classes
final StringBuilder ddPrefixes = new StringBuilder("");
for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
if (i > 0) {
// must append twice. Once for exact package and wildcard for child packages
ddPrefixes.append(",");
}
ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]).append(".*,");
ddPrefixes.append(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i]);
}
final String existing = System.getProperty("org.osgi.framework.bootdelegation");
if (null == existing) {
System.setProperty("org.osgi.framework.bootdelegation", ddPrefixes.toString());
} else if (!existing.contains(ddPrefixes)) {
System.setProperty(
"org.osgi.framework.bootdelegation", existing + "," + ddPrefixes.toString());
}
// Set the system prop to tell OSGi to delegate classloads for datadog bootstrap classes
System.setProperty(
Helper.PROPERTY_KEY, Helper.getNewValue(System.getProperty(Helper.PROPERTY_KEY)));
}
@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return emptyMap();
return singletonMap(
isMethod().and(isPublic()).and(named("newFramework")).and(takesArgument(0, Map.class)),
EquinoxFactoryAdvice.class.getName());
}
/**
* Sometimes OSGi doesn't read configuration from system properties. Handle this case for {@code
* EquinoxFactory}.
*/
public static class EquinoxFactoryAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void methodEnter(@Advice.Argument(0) final Map<String, String> configuration) {
if (configuration != null) {
configuration.put(
Helper.PROPERTY_KEY, Helper.getNewValue(configuration.get(Helper.PROPERTY_KEY)));
}
}
}
public static class Helper {
public static final String PROPERTY_KEY = "org.osgi.framework.bootdelegation";
public static final String PROPERTY_VALUE;
static {
// Set the config option to tell osgi to delegate classloads for datadog bootstrap classes
final StringBuilder prefixes = new StringBuilder("");
for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
if (i > 0) {
// must append twice. Once for exact package and wildcard for child packages
prefixes.append(",");
}
prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]).append(".*,");
prefixes.append(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i]);
}
PROPERTY_VALUE = prefixes.toString();
}
public static String getNewValue(final String existingValue) {
if (null != existingValue
&& !"".equals(existingValue)
&& !existingValue.contains(PROPERTY_VALUE)) {
return existingValue + "," + PROPERTY_VALUE;
} else {
return PROPERTY_VALUE;
}
}
}
}

25
dd-java-agent/instrumentation/osgi-classloading/src/test/groovy/OSGIClassloadingTest.groovy
View File

@ -1,11 +1,32 @@
import datadog.trace.agent.test.AgentTestRunner
import org.eclipse.osgi.launch.EquinoxFactory
import org.junit.Rule
import org.junit.contrib.java.lang.system.RestoreSystemProperties
import org.osgi.framework.launch.Framework
import org.osgi.framework.launch.FrameworkFactory
class OSGIClassloadingTest extends AgentTestRunner {
@Rule
public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties()
def "delegation property set on module load"() {
setup:
when:
org.osgi.framework.Bundle.getName()
expect:
then:
System.getProperty("org.osgi.framework.bootdelegation") == "io.opentracing.*,io.opentracing,datadog.slf4j.*,datadog.slf4j,datadog.trace.bootstrap.*,datadog.trace.bootstrap,datadog.trace.api.*,datadog.trace.api,datadog.trace.context.*,datadog.trace.context"
}
def "test Eclipse OSGi framework factory"() {
setup:
def config = ["osgi.support.class.certificate": "false"]
FrameworkFactory factory = new EquinoxFactory()
when:
Framework framework = factory.newFramework(config)
then:
framework != null
}
}

6
dd-java-agent/src/test/java/datadog/trace/agent/test/IntegrationTestUtils.java
View File

@ -99,7 +99,7 @@ public class IntegrationTestUtils {
final Manifest manifest = new Manifest();
if (mainClassname != null) {
Attributes mainAttributes = manifest.getMainAttributes();
final Attributes mainAttributes = manifest.getMainAttributes();
mainAttributes.put(Attributes.Name.MANIFEST_VERSION, "1.0");
mainAttributes.put(Attributes.Name.MAIN_CLASS, mainClassname);
mainAttributes.put(new Attributes.Name("Premain-Class"), mainClassname);
@ -174,7 +174,7 @@ public class IntegrationTestUtils {
public static String[] getBootstrapPackagePrefixes() throws Exception {
final Field f =
getAgentClassLoader()
.loadClass("datadog.trace.agent.tooling.Utils")
.loadClass("datadog.trace.agent.tooling.Constants")
.getField("BOOTSTRAP_PACKAGE_PREFIXES");
return (String[]) f.get(null);
}
@ -182,7 +182,7 @@ public class IntegrationTestUtils {
public static String[] getAgentPackagePrefixes() throws Exception {
final Field f =
getAgentClassLoader()
.loadClass("datadog.trace.agent.tooling.Utils")
.loadClass("datadog.trace.agent.tooling.Constants")
.getField("AGENT_PACKAGE_PREFIXES");
return (String[]) f.get(null);
}

8
dd-java-agent/testing/src/test/groovy/AgentTestRunnerTest.groovy
View File

@ -2,7 +2,7 @@ import com.google.common.reflect.ClassPath
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.agent.test.SpockRunner
import datadog.trace.agent.test.TestUtils
import datadog.trace.agent.tooling.Utils
import datadog.trace.agent.tooling.Constants
import io.opentracing.Span
import io.opentracing.Tracer
import spock.lang.Shared
@ -29,15 +29,15 @@ class AgentTestRunnerTest extends AgentTestRunner {
def "spock runner bootstrap prefixes correct for test setup"() {
expect:
SpockRunner.BOOTSTRAP_PACKAGE_PREFIXES_COPY == Utils.BOOTSTRAP_PACKAGE_PREFIXES
SpockRunner.BOOTSTRAP_PACKAGE_PREFIXES_COPY == Constants.BOOTSTRAP_PACKAGE_PREFIXES
}
def "classpath setup"() {
setup:
final List<String> bootstrapClassesIncorrectlyLoaded = []
for (ClassPath.ClassInfo info : TestUtils.getTestClasspath().getAllClasses()) {
for (int i = 0; i < Utils.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
if (info.getName().startsWith(Utils.BOOTSTRAP_PACKAGE_PREFIXES[i])) {
for (int i = 0; i < Constants.BOOTSTRAP_PACKAGE_PREFIXES.length; ++i) {
if (info.getName().startsWith(Constants.BOOTSTRAP_PACKAGE_PREFIXES[i])) {
Class<?> bootstrapClass = Class.forName(info.getName())
if (bootstrapClass.getClassLoader() != BOOTSTRAP_CLASSLOADER) {
bootstrapClassesIncorrectlyLoaded.add(bootstrapClass)