Skip to content

Extend DD_TRACE_METHODS to support wildcarded method argument #2171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Dec 14, 2020
Merged

Extend DD_TRACE_METHODS to support wildcarded method argument #2171

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2d9572c
add support for wildcards in place of method names
Dec 7, 2020
b52bf35
update warning message
Dec 7, 2020
9f4aa96
update transformer to support wildcard method config
Dec 7, 2020
f923217
spotless
Dec 7, 2020
2953126
add private method support
Dec 7, 2020
e1b7c07
fix instrumentation and add initial test
Dec 8, 2020
6c36c63
add configtest
Dec 9, 2020
55b930c
experiment with any()
Dec 9, 2020
b0400ec
modify element matchers and add initial tests
Dec 9, 2020
e03d8d5
add testing for interfaces and abstract classes
Dec 9, 2020
4fa8742
accommodate codenarc tests
Dec 9, 2020
d05ba72
codenarc
Dec 10, 2020
ec3295e
explicit imports, remove whitespace, add finalizer support
Dec 10, 2020
b7ed860
ignore getters and setters and update test for abstract classes
Dec 10, 2020
8154b28
accommodate interface tests
Dec 10, 2020
dd8de9e
add more tests
Dec 11, 2020
a34065a
fix codenarc test failure
Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions .../main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import static datadog.trace.agent.tooling.ClassLoaderMatcher.hasClassesNamed;
import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.safeHasSuperType;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought spotless check should fail on star imports ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a limitation of spotless, but you can add a custom rule for this: diffplug/spotless#240 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ziquanmiao could you change this wildcard import to explicit imports of the used types?

Copy link
Contributor Author

@ziquanmiao ziquanmiao Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ill fix and explicitly import the ones I use


import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
Expand Down Expand Up @@ -32,8 +32,9 @@
@AutoService(Instrumenter.class)
public class TraceConfigInstrumentation implements Instrumenter {

static final String PACKAGE_CLASS_NAME_REGEX = "[\\w.\\$]+";
private static final String METHOD_LIST_REGEX = "\\s*(?:\\w+\\s*,)*\\s*(?:\\w+\\s*,?)\\s*";
static final String PACKAGE_CLASS_NAME_REGEX = "[\\w .\\$]+";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now accepts spaces in package/class names which doesn't sound right

private static final String METHOD_LIST_REGEX =
"\\s*(?:\\*|(?:\\w+\\s*,)*\\s*(?:\\w+\\s*,?))\\s*";
private static final String CONFIG_FORMAT =
"(?:\\s*"
+ PACKAGE_CLASS_NAME_REGEX
Expand Down Expand Up @@ -63,7 +64,7 @@ public TraceConfigInstrumentation() {

} else if (!validateConfigString(configString)) {
log.warn(
"Invalid trace method config '{}'. Must match 'package.Class$Name[method1,method2];*'.",
"Invalid trace method config '{}'. Must match 'package.Class$Name[method1,method2];*' or 'package.Class$Name[*];*'.",
configString);
classMethodsToTrace = Collections.emptyMap();

Expand Down Expand Up @@ -147,7 +148,17 @@ public Map<ElementMatcher<? super MethodDescription>, String> transformers() {
ElementMatcher.Junction<MethodDescription> methodMatchers = null;
for (final String methodName : methodNames) {
if (methodMatchers == null) {
methodMatchers = named(methodName);
if (methodName.equals("*")) {
methodMatchers =
not(
isHashCode()
.or(isEquals())
.or(isToString())
.or(isConstructor())
.or(isSynthetic()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add isFinalizer because I've seen issues intercepting that in the past on other AOP systems

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and test that calls each of these method types and no span is created

} else {
methodMatchers = named(methodName);
}
} else {
methodMatchers = methodMatchers.or(named(methodName));
}
Expand Down
129 changes: 125 additions & 4 deletions dd-java-agent/instrumentation/trace-annotation/src/test/groovy/TraceConfigTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class TraceConfigTest extends AgentTestRunner {
@Override
void configurePreAgent() {
super.configurePreAgent()
injectSysConfig("dd.trace.methods", "package.ClassName[method1,method2];${ConfigTracedCallable.name}[call]")
injectSysConfig("dd.trace.methods", "package.ClassName[method1,method2];${ConfigTracedCallable.name}[call];${ConfigTracedCallable2.name}[*];${Human.name}[*];${Pig.name}[*]")
}

class ConfigTracedCallable implements Callable<String> {
Expand All @@ -19,18 +19,135 @@ class TraceConfigTest extends AgentTestRunner {
}
}

class ConfigTracedCallable2 implements Callable<String> {
@Override
String call() throws Exception {
return call_helper()
}

String call_helper() throws Exception {
return "Hello2!"
}
}

interface Mammal {
void setName(String newName)
void setHeight(int newHeight)
}

class Human implements Mammal {
String name
String height

void setName(String newName){
name = newName
}
void setHeight(int newHeight){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do getters suppose to be instrumented under '*' pattern or no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i chatted with tyler about this --
if i didnt ignore getters/setters, then in the implementation would instrument custom groovy classes as well, so it was a question whether to write accommodations for groovy or ignore getters/setters and defer customers to our profiling product if they were interested in those methods

height = newHeight
}
}


abstract class Animal {
abstract void animalSound()
void sleep() {
System.out.println("Zzz")
}
}

class Pig extends Animal {
void animalSound() {
System.out.println("The pig says: wee wee")
}
}

def "test configuration based trace"() {
expect:
when:
new ConfigTracedCallable().call() == "Hello!"
then:
assertTraces(1) {
trace(1) {
span {
resourceName "ConfigTracedCallable.call"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
defaultTags()
}
}
}
}
}

def "test wildcard configuration"() {

when:
new ConfigTracedCallable2().call() == "Hello2!"

then:
assertTraces(1) {
trace(2) {
span {
resourceName "ConfigTracedCallable2.call"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
defaultTags()
}
}
span {
resourceName "ConfigTracedCallable2.call_helper"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
defaultTags()
}
}
}
}
}

def "test wildcard configuration with class implementing interface"() {

when:
TEST_WRITER.waitForTraces(1)
Human charlie = new Human()
charlie.setName("Charlie")
charlie.setHeight(4)

then:
assertTraces(2) {
trace(1) {
span {
resourceName "Human.setName"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
defaultTags()
}
}
}
trace(1) {
span {
resourceName "Human.setHeight"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
defaultTags()
}
}
}
}
}
def "test wildcard configuration based on class extending abstract class"() {

when:
new Pig().animalSound()

then:
assertTraces(1) {
trace(1) {
span {
resourceName "ConfigTracedCallable.call"
resourceName "Pig.animalSound"
operationName "trace.annotation"
tags {
"$Tags.COMPONENT" "trace"
Expand Down Expand Up @@ -64,5 +181,9 @@ class TraceConfigTest extends AgentTestRunner {
"ClassName[method1 , method2]" | ["ClassName": ["method1", "method2"].toSet()]
"Class\$1[method1 ] ; Class\$2[ method2];" | ["Class\$1": ["method1"].toSet(), "Class\$2": ["method2"].toSet()]
"Duplicate[method1] ; Duplicate[method2] ;Duplicate[method3];" | ["Duplicate": ["method3"].toSet()]
"ClassName[*]" | ["ClassName": ["*"].toSet()]
"ClassName[*,asdfg]" | [:]
"ClassName[asdfg,*]" | [:]
"Class[*] ; Class\$2[ method2];" | ["Class": ["*"].toSet(), "Class\$2": ["method2"].toSet()]
}
}