Skip to content

Commit e5027d8

Browse files
jpbempeljpbempel
committed
Fix error logged for Bridge methods
We avoid to instrument Bridge method but we don't want to log error for method not found. So introduce a skip status for method matching to differentiate if we skip instrumentation but still we have found the method
1 parent 4644198 commit e5027d8

3 files changed

Lines changed: 58 additions & 26 deletions

File tree

‎dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -535,10 +535,18 @@ private boolean performInstrumentation(
535535
for (MethodNode methodNode : classNode.methods) {
536536
List<ProbeDefinition> matchingDefs = new ArrayList<>();
537537
for (ProbeDefinition definition : definitions) {
538-
if (definition.getWhere().isMethodMatching(methodNode, classFileLines)
539-
&& remainingDefinitions.contains(definition)) {
540-
matchingDefs.add(definition);
541-
remainingDefinitions.remove(definition);
538+
Where.MethodMatching methodMatching =
539+
definition.getWhere().isMethodMatching(methodNode, classFileLines);
540+
if (remainingDefinitions.contains(definition)) {
541+
if (methodMatching == Where.MethodMatching.MATCH) {
542+
// method matches, add into collection of definitions to instrument
543+
matchingDefs.add(definition);
544+
}
545+
if (methodMatching == Where.MethodMatching.MATCH
546+
|| methodMatching == Where.MethodMatching.SKIP)
547+
// match or need to skip instrumentation (because bridge) remove from remaining
548+
// definitions to avoid reporting error
549+
remainingDefinitions.remove(definition);
542550
}
543551
}
544552
if (matchingDefs.isEmpty()) {
@@ -850,7 +858,7 @@ private List<MethodNode> matchMethodDescription(
850858
List<MethodNode> result = new ArrayList<>();
851859
try {
852860
for (MethodNode methodNode : classNode.methods) {
853-
if (where.isMethodMatching(methodNode, classFileLines)) {
861+
if (where.isMethodMatching(methodNode, classFileLines) == Where.MethodMatching.MATCH) {
854862
result.add(methodNode);
855863
}
856864
}

‎dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/Where.java‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -127,27 +127,38 @@ public boolean isMethodNameMatching(String targetMethod) {
127127
return methodName == null || methodName.equals("*") || methodName.equals(targetMethod);
128128
}
129129

130-
public boolean isMethodMatching(MethodNode methodNode, ClassFileLines classFileLines) {
130+
public enum MethodMatching {
131+
MATCH,
132+
SKIP,
133+
FAIL
134+
}
135+
136+
public MethodMatching isMethodMatching(MethodNode methodNode, ClassFileLines classFileLines) {
131137
String targetName = methodNode.name;
132138
String targetMethodDescriptor = methodNode.desc;
133139
// try exact matching: name + FQN signature
134-
if (!isMethodNameMatching(targetName)
135-
|| ((methodNode.access & Opcodes.ACC_BRIDGE) == Opcodes.ACC_BRIDGE)) {
136-
return false;
140+
if (!isMethodNameMatching(targetName)) {
141+
return MethodMatching.FAIL;
142+
}
143+
if ((methodNode.access & Opcodes.ACC_BRIDGE) == Opcodes.ACC_BRIDGE) {
144+
// name is matching but method is a bridge method
145+
return MethodMatching.SKIP;
137146
}
138147
if (signature == null) {
139148
if (lines == null || lines.length == 0) {
140-
return true;
149+
return MethodMatching.MATCH;
141150
}
142151
// try matching by line
143152
List<MethodNode> methodsByLine = classFileLines.getMethodsByLine(lines[0].getFrom());
144153
if (methodsByLine == null || methodsByLine.isEmpty()) {
145-
return false;
154+
return MethodMatching.FAIL;
146155
}
147-
return methodsByLine.stream().anyMatch(m -> m == methodNode);
156+
return methodsByLine.stream().anyMatch(m -> m == methodNode)
157+
? MethodMatching.MATCH
158+
: MethodMatching.FAIL;
148159
}
149160
if (signature.equals("*") || signature.equals(targetMethodDescriptor)) {
150-
return true;
161+
return MethodMatching.MATCH;
151162
}
152163
// try full JVM signature: "(Ljava.lang.String;Ljava.util.Map;I)V"
153164
if (probeMethodDescriptor == null) {
@@ -160,20 +171,22 @@ public boolean isMethodMatching(MethodNode methodNode, ClassFileLines classFileL
160171
}
161172
}
162173
if (probeMethodDescriptor.isEmpty()) {
163-
return true;
174+
return MethodMatching.MATCH;
164175
}
165176
if (probeMethodDescriptor.equals(targetMethodDescriptor)) {
166-
return true;
177+
return MethodMatching.MATCH;
167178
}
168179
// fallback to signature without return type: "Ljava.lang.String;Ljava.util.Map;I"
169180
String noRetTypeDescriptor = removeReturnType(probeMethodDescriptor);
170181
targetMethodDescriptor = removeReturnType(targetMethodDescriptor);
171182
if (noRetTypeDescriptor.equals(targetMethodDescriptor)) {
172-
return true;
183+
return MethodMatching.MATCH;
173184
}
174185
// Fallback to signature without Fully Qualified Name: "LString;LMap;I"
175186
String simplifiedSignature = removeFQN(targetMethodDescriptor);
176-
return noRetTypeDescriptor.equals(simplifiedSignature);
187+
return noRetTypeDescriptor.equals(simplifiedSignature)
188+
? MethodMatching.MATCH
189+
: MethodMatching.FAIL;
177190
}
178191

179192
private static boolean isMissingReturnType(String probeMethodDescriptor) {

‎dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/probe/WhereTest.java‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -101,43 +101,54 @@ public void convertLineToMethod() {
101101
@Test
102102
public void methodMatching() {
103103
Where where = new Where("String", "substring", "(int,int)", new String[0], null);
104-
assertTrue(
104+
assertEquals(
105+
Where.MethodMatching.MATCH,
105106
where.isMethodMatching(createMethodNode("substring", "(II)Ljava/lang/String;"), null));
106107
where = new Where("String", "replaceAll", "(String,String)", new String[0], null);
107-
assertTrue(
108+
assertEquals(
109+
Where.MethodMatching.MATCH,
108110
where.isMethodMatching(
109111
createMethodNode(
110112
"replaceAll", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"),
111113
null));
112114
where = new Where("HashMap", "<init>", "(Map)", new String[0], null);
113-
assertTrue(where.isMethodMatching(createMethodNode("<init>", "(Ljava/util/Map;)V"), null));
115+
assertEquals(
116+
Where.MethodMatching.MATCH,
117+
where.isMethodMatching(createMethodNode("<init>", "(Ljava/util/Map;)V"), null));
114118
where = new Where("ArrayList", "removeIf", "(Predicate)", new String[0], null);
115-
assertTrue(
119+
assertEquals(
120+
Where.MethodMatching.MATCH,
116121
where.isMethodMatching(
117122
createMethodNode("removeIf", "(Ljava/util/function/Predicate;)Z"), null));
118123
where = new Where("String", "concat", "", new String[0], null);
119-
assertTrue(where.isMethodMatching(createMethodNode("concat", "String (String)"), null));
124+
assertEquals(
125+
Where.MethodMatching.MATCH,
126+
where.isMethodMatching(createMethodNode("concat", "String (String)"), null));
120127
where = new Where("String", "concat", " \t", new String[0], null);
121-
assertTrue(where.isMethodMatching(createMethodNode("concat", "String (String)"), null));
128+
assertEquals(
129+
Where.MethodMatching.MATCH,
130+
where.isMethodMatching(createMethodNode("concat", "String (String)"), null));
122131
where =
123132
new Where(
124133
"Inner",
125134
"innerMethod",
126135
"(com.datadog.debugger.probe.Outer$Inner)",
127136
new String[0],
128137
null);
129-
assertTrue(
138+
assertEquals(
139+
Where.MethodMatching.MATCH,
130140
where.isMethodMatching(
131141
createMethodNode("innerMethod", "(Lcom/datadog/debugger/probe/Outer$Inner;)V"), null));
132142
where = new Where("Inner", "innerMethod", "(Outer$Inner)", new String[0], null);
133-
assertTrue(
143+
assertEquals(
144+
Where.MethodMatching.MATCH,
134145
where.isMethodMatching(
135146
createMethodNode("innerMethod", "(Lcom/datadog/debugger/probe/Outer$Inner;)V"), null));
136147
where = new Where("MyClass", "myMethod", null, new String[] {"42"}, null);
137148
ClassFileLines classFileLines = mock(ClassFileLines.class);
138149
MethodNode myMethodNode = createMethodNode("myMethod", "()V");
139150
when(classFileLines.getMethodsByLine(42)).thenReturn(Arrays.asList(myMethodNode));
140-
assertTrue(where.isMethodMatching(myMethodNode, classFileLines));
151+
assertEquals(Where.MethodMatching.MATCH, where.isMethodMatching(myMethodNode, classFileLines));
141152
}
142153

143154
private MethodNode createMethodNode(String name, String desc) {

0 commit comments

Comments
 (0)