Extend DD_TRACE_METHODS to support wildcarded method argument #2171
Conversation
ziquanmiao
changed the title
...src/main/java/datadog/trace/instrumentation/trace_annotation/TraceConfigInstrumentation.java
Outdated
Show resolved
Hide resolved
| @@ -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.*; | |||
There was a problem hiding this comment.
I thought spotless check should fail on star imports ...
There was a problem hiding this comment.
it's a limitation of spotless, but you can add a custom rule for this: diffplug/spotless#240 (comment)
There was a problem hiding this comment.
@ziquanmiao could you change this wildcard import to explicit imports of the used types?
There was a problem hiding this comment.
ill fix and explicitly import the ones I use
| @@ -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 .\\$]+"; | |||
There was a problem hiding this comment.
This now accepts spaces in package/class names which doesn't sound right
| .or(isEquals()) | ||
| .or(isToString()) | ||
| .or(isConstructor()) | ||
| .or(isSynthetic())); |
There was a problem hiding this comment.
I'd also add isFinalizer because I've seen issues intercepting that in the past on other AOP systems
There was a problem hiding this comment.
and test that calls each of these method types and no span is created
| void setName(String newName){ | ||
| name = newName | ||
| } | ||
| void setHeight(int newHeight){ |
There was a problem hiding this comment.
Do getters suppose to be instrumented under '*' pattern or no ?
There was a problem hiding this comment.
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
| .or(isEquals()) | ||
| .or(isToString()) | ||
| .or(isConstructor()) | ||
| .or(isSynthetic())); |
There was a problem hiding this comment.
and test that calls each of these method types and no span is created
There was a problem hiding this comment.
👍
Hey, @ziquanmiao since your PR is made up of many small commits, several which don't pass all the checks by themselves, would you mind either squashing them all before you merge the PR, or use the "Squash and merge" button? We are trying to ensure that every commit is a working commit, so things like git bisect can be used to find regressions.
PR to extend support of DD_TRACE_METHODS configuration to support wildcards in method calls
EX:
ClassName[*]