Skip to content

Commit 90af2ba

Browse files
authored
Add classloader exclusions for drools generated classes (#11191)
# What Does This Do Excludes in `ClassloaderMatchers` the most commonly used drools classloaders used to load dynamically generated classes from rules. This is usually done by `PackageClassloader`. I added a list of commonly used classloaders (hoping that does not overload too much the matchers) and a test module inside instrumentations to test 6.x, 7.x, 8.x and 10.x # Motivation # Additional Notes # Contributor Checklist - Format the title according to [the contribution guidelines](https://github.com/DataDog/dd-trace-java/blob/master/CONTRIBUTING.md#title-format) - Assign the `type:` and (`comp:` or `inst:`) labels in addition to [any other useful labels](https://github.com/DataDog/dd-trace-java/blob/master/CONTRIBUTING.md#labels) - Avoid using `close`, `fix`, or [any linking keywords](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) when referencing an issue Use `solves` instead, and assign the PR [milestone](https://github.com/DataDog/dd-trace-java/milestones) to the issue - Update the [CODEOWNERS](https://github.com/DataDog/dd-trace-java/blob/master/.github/CODEOWNERS) file on source file addition, migration, or deletion - Update [public documentation](https://docs.datadoghq.com/tracing/trace_collection/library_config/java/) with any new configuration flags or behaviors Jira ticket: [PROJ-IDENT] ***Note:*** **Once your PR is ready to merge, add it to the merge queue by commenting `/merge`.** `/merge -c` cancels the queue request. `/merge -f --reason "reason"` skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see [this doc](https://datadoghq.atlassian.net/wiki/spaces/DEVX/pages/3121612126/MergeQueue). <!-- # Opening vs Drafting a PR: When opening a pull request, please open it as a draft to not auto assign reviewers before you feel the pull request is in a reviewable state. # Linking a JIRA ticket: Please link your JIRA ticket by adding its identifier between brackets (ex [PROJ-IDENT]) in the PR description, not the title. This requirement only applies to Datadog employees. --> Co-authored-by: andrea.marziali <andrea.marziali@datadoghq.com>
1 parent 7c1b47b commit 90af2ba

9 files changed

Lines changed: 271 additions & 0 deletions

File tree

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/ClassLoaderMatchers.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ public static boolean canSkipClassLoaderByName(final ClassLoader loader) {
5454
case "com.alibaba.fastjson.util.ASMClassLoader":
5555
case "datadog.trace.bootstrap.DatadogClassLoader":
5656
case "datadog.trace.bootstrap.InstrumentationClassLoader":
57+
// drools
58+
case "org.drools.core.rule.PackageClassLoader":
59+
case "org.drools.wiring.dynamic.PackageClassLoader":
60+
case "org.drools.core.rule.JavaDialectRuntimeData$PackageClassLoader":
5761
return true;
5862
}
5963
if (CHECK_EXCLUDES) {

dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/test/ClassLoaderMatchersTest.groovy

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import datadog.trace.bootstrap.instrumentation.log.LogContextScopeListener
55
import datadog.trace.bootstrap.DatadogClassLoader
66
import datadog.trace.test.util.DDSpecification
77
import groovy.transform.CompileStatic
8+
import net.bytebuddy.ByteBuddy
9+
import net.bytebuddy.dynamic.loading.ClassLoadingStrategy
10+
import spock.lang.Unroll
811

912
class ClassLoaderMatchersTest extends DDSpecification {
1013

@@ -44,6 +47,29 @@ class ClassLoaderMatchersTest extends DDSpecification {
4447
LogContextScopeListener.name == "datadog.trace.bootstrap.instrumentation.log.LogContextScopeListener"
4548
}
4649

50+
@Unroll
51+
def "skips drools classloader: #loaderName"() {
52+
given:
53+
ClassLoader loader = new ByteBuddy()
54+
.subclass(ClassLoader)
55+
.name(loaderName)
56+
.make()
57+
.load(ClassLoader.getSystemClassLoader(), ClassLoadingStrategy.Default.WRAPPER)
58+
.getLoaded()
59+
.getDeclaredConstructor(ClassLoader)
60+
.newInstance(ClassLoader.getSystemClassLoader())
61+
62+
expect:
63+
ClassLoaderMatchers.canSkipClassLoaderByName(loader)
64+
65+
where:
66+
loaderName << [
67+
"org.drools.core.rule.PackageClassLoader",
68+
"org.drools.wiring.dynamic.PackageClassLoader",
69+
"org.drools.core.rule.JavaDialectRuntimeData\$PackageClassLoader"
70+
]
71+
}
72+
4773
/*
4874
* A URLClassloader which only delegates java.* classes
4975
*/
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
apply from: "$rootDir/gradle/java.gradle"
2+
3+
addTestSuiteForDir("latest7Test", "test")
4+
addTestSuiteForDir("latest8Test", "test")
5+
addTestSuiteForDir("latestDepTest", "test")
6+
7+
testJvmConstraints {
8+
}
9+
repositories {
10+
maven {
11+
url = uri("https://repository.jboss.org/nexus/content/repositories/releases/")
12+
}
13+
}
14+
dependencies {
15+
testImplementation project(':dd-java-agent:testing')
16+
testImplementation group: 'org.drools', name: 'drools-compiler', version: '6.0.0.Final'
17+
testImplementation group: 'org.drools', name: 'drools-core', version: '6.0.0.Final'
18+
testImplementation group: 'org.kie', name: 'kie-api', version: '6.0.0.Final'
19+
20+
latest7TestImplementation group: 'org.drools', name: 'drools-compiler', version: '7.+'
21+
latest7TestImplementation group: 'org.drools', name: 'drools-core', version: '7.+'
22+
latest7TestImplementation group: 'org.drools', name: 'drools-mvel', version: '7.+'
23+
latest7TestImplementation group: 'org.kie', name: 'kie-api', version: '7.+'
24+
25+
latest8TestImplementation group: 'org.drools', name: 'drools-compiler', version: '8.+'
26+
latest8TestImplementation group: 'org.drools', name: 'drools-core', version: '8.+'
27+
latest8TestImplementation group: 'org.drools', name: 'drools-mvel', version: '8.+'
28+
latest8TestImplementation group: 'org.kie', name: 'kie-api', version: '8.+'
29+
30+
// for now this is locked to 10.x not to add maintenance burden
31+
latestDepTestImplementation group: 'org.drools', name: 'drools-compiler', version: '10.+'
32+
latestDepTestImplementation group: 'org.drools', name: 'drools-core', version: '10.+'
33+
latestDepTestImplementation group: 'org.drools', name: 'drools-mvel', version: '10.+'
34+
latestDepTestImplementation group: 'org.kie', name: 'kie-api', version: '10.+'
35+
}
36+
37+
configurations.matching { it.name.startsWith('latestDepTest') }.configureEach {
38+
it.resolutionStrategy {
39+
force group: 'org.slf4j', name: 'slf4j-api', version: libs.versions.slf4j.get()
40+
}
41+
}
42+
43+
tasks.named("test") {
44+
testJvmConstraints {
45+
maxJavaVersion = JavaVersion.VERSION_1_8
46+
}
47+
}
48+
49+
tasks.named("latest7Test") {
50+
testJvmConstraints {
51+
maxJavaVersion = JavaVersion.VERSION_11
52+
}
53+
}
54+
55+
tasks.named("latest8Test") {
56+
testJvmConstraints {
57+
minJavaVersion = JavaVersion.VERSION_11
58+
maxJavaVersion = JavaVersion.VERSION_17
59+
}
60+
}
61+
62+
tasks.named("latestDepTest") {
63+
testJvmConstraints {
64+
minJavaVersion = JavaVersion.VERSION_17
65+
}
66+
}
67+
68+
project.afterEvaluate {
69+
tasks.withType(Test).configureEach {
70+
conditionalJvmArgs(
71+
it,
72+
JavaVersion.VERSION_16,
73+
[
74+
"--add-opens=java.base/java.lang.reflect=ALL-UNNAMED",
75+
"--add-opens=java.base/java.text=ALL-UNNAMED",
76+
"--add-opens=java.desktop/java.awt.font=ALL-UNNAMED",
77+
]
78+
)
79+
}
80+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package datadog.trace.instrumentation.drools
2+
3+
import datadog.trace.agent.test.InstrumentationSpecification
4+
import datadog.trace.config.inversion.ConfigHelper
5+
import example.Person
6+
import org.kie.api.KieServices
7+
import org.kie.api.builder.KieFileSystem
8+
import org.kie.api.runtime.KieContainer
9+
import org.kie.api.runtime.KieSession
10+
11+
class DroolsClassLoaderExclusionTest extends InstrumentationSpecification {
12+
static ConfigHelper.StrictnessPolicy strictness
13+
14+
@Override
15+
protected void configurePreAgent() {
16+
// this is required otherwise checks will fail...
17+
strictness = ConfigHelper.get().configInversionStrictFlag()
18+
ConfigHelper.get().setConfigInversionStrict(ConfigHelper.StrictnessPolicy.TEST)
19+
super.configurePreAgent()
20+
}
21+
22+
def cleanup() {
23+
ConfigHelper.get().setConfigInversionStrict(strictness)
24+
}
25+
26+
def "should not instrument drools generated classes"() {
27+
setup:
28+
KieServices ks = KieServices.Factory.get()
29+
30+
KieFileSystem kfs = ks.newKieFileSystem()
31+
32+
kfs.write(
33+
"src/main/resources/example/rules.drl",
34+
ks.getResources().newClassPathResource("example/rules.drl")
35+
)
36+
37+
ks.newKieBuilder(kfs).buildAll()
38+
KieContainer kc = ks.newKieContainer(ks.getRepository().getDefaultReleaseId())
39+
KieSession ksession = kc.newKieSession()
40+
41+
when:
42+
Person john = new Person("John", 20)
43+
Person bob = new Person("Bob", 15)
44+
45+
ksession.insert(john)
46+
ksession.insert(bob)
47+
int fired = ksession.fireAllRules()
48+
49+
then:
50+
fired == 1
51+
!bob.isAdult()
52+
john.isAdult()
53+
54+
and:
55+
// assert we do not transform the generated rule class (RuleInstrumentation would but the classloader should be ignored)
56+
TRANSFORMED_CLASSES_TYPES.findAll { it.getName().startsWith("example.") }.isEmpty()
57+
58+
cleanup:
59+
ksession?.dispose()
60+
}
61+
}
62+
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package example;
2+
3+
public class Person {
4+
private String name;
5+
private int age;
6+
private boolean adult;
7+
8+
public Person() {}
9+
10+
public Person(String name, int age) {
11+
this.name = name;
12+
this.age = age;
13+
this.adult = false;
14+
}
15+
16+
public String getName() {
17+
return name;
18+
}
19+
20+
public void setName(String name) {
21+
this.name = name;
22+
}
23+
24+
public int getAge() {
25+
return age;
26+
}
27+
28+
public void setAge(int age) {
29+
this.age = age;
30+
}
31+
32+
public boolean isAdult() {
33+
return adult;
34+
}
35+
36+
public void setAdult(boolean adult) {
37+
this.adult = adult;
38+
}
39+
40+
@Override
41+
public String toString() {
42+
return "Person{name='" + name + "', age=" + age + ", adult=" + adult + "}";
43+
}
44+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package test;
2+
3+
import net.bytebuddy.asm.Advice;
4+
5+
public class ConstructorAdvice {
6+
@Advice.OnMethodExit(suppress = Throwable.class)
7+
public static void onExit() {}
8+
}
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package test;
2+
3+
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
4+
5+
import com.google.auto.service.AutoService;
6+
import datadog.trace.agent.tooling.Instrumenter;
7+
import datadog.trace.agent.tooling.InstrumenterModule;
8+
import datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers;
9+
import net.bytebuddy.description.type.TypeDescription;
10+
import net.bytebuddy.matcher.ElementMatcher;
11+
12+
@AutoService(InstrumenterModule.class)
13+
public class RuleInstrumentation extends InstrumenterModule.Tracing
14+
implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice {
15+
16+
public RuleInstrumentation() {
17+
super("drools-test");
18+
}
19+
20+
@Override
21+
public void methodAdvice(MethodTransformer transformer) {
22+
transformer.applyAdvice(isConstructor(), "test.ConstructorAdvice");
23+
}
24+
25+
@Override
26+
public String hierarchyMarkerType() {
27+
return null;
28+
}
29+
30+
@Override
31+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
32+
return NameMatchers.nameStartsWith("example.Rule");
33+
}
34+
}
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
package example
2+
3+
import example.Person
4+
5+
rule "Mark person as adult"
6+
when
7+
$p : Person(age >= 18, adult == false)
8+
then
9+
$p.setAdult(true);
10+
System.out.println("Rule fired for " + $p.getName());
11+
update($p);
12+
end

settings.gradle.kts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@ include(
335335
":dd-java-agent:instrumentation:datastax-cassandra:datastax-cassandra-4.0",
336336
":dd-java-agent:instrumentation:dropwizard:dropwizard-views-0.7",
337337
":dd-java-agent:instrumentation:dropwizard:dropwizard-0.8",
338+
":dd-java-agent:instrumentation:drools:drools-6.0",
338339
":dd-java-agent:instrumentation:elasticsearch:elasticsearch-rest:elasticsearch-rest-5.0",
339340
":dd-java-agent:instrumentation:elasticsearch:elasticsearch-rest:elasticsearch-rest-6.4",
340341
":dd-java-agent:instrumentation:elasticsearch:elasticsearch-rest:elasticsearch-rest-7.0",

0 commit comments

Comments
 (0)