From e5027d816b717a74a091bf3807a5391e354def1b Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 10 Sep 2025 12:06:30 +0200 Subject: [PATCH] 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 --- .../debugger/agent/DebuggerTransformer.java | 18 ++++++--- .../com/datadog/debugger/probe/Where.java | 37 +++++++++++++------ .../com/datadog/debugger/probe/WhereTest.java | 29 ++++++++++----- 3 files changed, 58 insertions(+), 26 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java index 4ae05865d22..975b602e9e5 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java @@ -535,10 +535,18 @@ private boolean performInstrumentation( for (MethodNode methodNode : classNode.methods) { List matchingDefs = new ArrayList<>(); for (ProbeDefinition definition : definitions) { - if (definition.getWhere().isMethodMatching(methodNode, classFileLines) - && remainingDefinitions.contains(definition)) { - matchingDefs.add(definition); - remainingDefinitions.remove(definition); + Where.MethodMatching methodMatching = + definition.getWhere().isMethodMatching(methodNode, classFileLines); + if (remainingDefinitions.contains(definition)) { + if (methodMatching == Where.MethodMatching.MATCH) { + // method matches, add into collection of definitions to instrument + matchingDefs.add(definition); + } + if (methodMatching == Where.MethodMatching.MATCH + || methodMatching == Where.MethodMatching.SKIP) + // match or need to skip instrumentation (because bridge) remove from remaining + // definitions to avoid reporting error + remainingDefinitions.remove(definition); } } if (matchingDefs.isEmpty()) { @@ -850,7 +858,7 @@ private List matchMethodDescription( List result = new ArrayList<>(); try { for (MethodNode methodNode : classNode.methods) { - if (where.isMethodMatching(methodNode, classFileLines)) { + if (where.isMethodMatching(methodNode, classFileLines) == Where.MethodMatching.MATCH) { result.add(methodNode); } } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/Where.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/Where.java index e295faa83e3..9383846b7b9 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/Where.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/Where.java @@ -127,27 +127,38 @@ public boolean isMethodNameMatching(String targetMethod) { return methodName == null || methodName.equals("*") || methodName.equals(targetMethod); } - public boolean isMethodMatching(MethodNode methodNode, ClassFileLines classFileLines) { + public enum MethodMatching { + MATCH, + SKIP, + FAIL + } + + public MethodMatching isMethodMatching(MethodNode methodNode, ClassFileLines classFileLines) { String targetName = methodNode.name; String targetMethodDescriptor = methodNode.desc; // try exact matching: name + FQN signature - if (!isMethodNameMatching(targetName) - || ((methodNode.access & Opcodes.ACC_BRIDGE) == Opcodes.ACC_BRIDGE)) { - return false; + if (!isMethodNameMatching(targetName)) { + return MethodMatching.FAIL; + } + if ((methodNode.access & Opcodes.ACC_BRIDGE) == Opcodes.ACC_BRIDGE) { + // name is matching but method is a bridge method + return MethodMatching.SKIP; } if (signature == null) { if (lines == null || lines.length == 0) { - return true; + return MethodMatching.MATCH; } // try matching by line List methodsByLine = classFileLines.getMethodsByLine(lines[0].getFrom()); if (methodsByLine == null || methodsByLine.isEmpty()) { - return false; + return MethodMatching.FAIL; } - return methodsByLine.stream().anyMatch(m -> m == methodNode); + return methodsByLine.stream().anyMatch(m -> m == methodNode) + ? MethodMatching.MATCH + : MethodMatching.FAIL; } if (signature.equals("*") || signature.equals(targetMethodDescriptor)) { - return true; + return MethodMatching.MATCH; } // try full JVM signature: "(Ljava.lang.String;Ljava.util.Map;I)V" if (probeMethodDescriptor == null) { @@ -160,20 +171,22 @@ public boolean isMethodMatching(MethodNode methodNode, ClassFileLines classFileL } } if (probeMethodDescriptor.isEmpty()) { - return true; + return MethodMatching.MATCH; } if (probeMethodDescriptor.equals(targetMethodDescriptor)) { - return true; + return MethodMatching.MATCH; } // fallback to signature without return type: "Ljava.lang.String;Ljava.util.Map;I" String noRetTypeDescriptor = removeReturnType(probeMethodDescriptor); targetMethodDescriptor = removeReturnType(targetMethodDescriptor); if (noRetTypeDescriptor.equals(targetMethodDescriptor)) { - return true; + return MethodMatching.MATCH; } // Fallback to signature without Fully Qualified Name: "LString;LMap;I" String simplifiedSignature = removeFQN(targetMethodDescriptor); - return noRetTypeDescriptor.equals(simplifiedSignature); + return noRetTypeDescriptor.equals(simplifiedSignature) + ? MethodMatching.MATCH + : MethodMatching.FAIL; } private static boolean isMissingReturnType(String probeMethodDescriptor) { diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/probe/WhereTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/probe/WhereTest.java index 7f6d1baf4f0..3723e794ef4 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/probe/WhereTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/probe/WhereTest.java @@ -101,24 +101,33 @@ public void convertLineToMethod() { @Test public void methodMatching() { Where where = new Where("String", "substring", "(int,int)", new String[0], null); - assertTrue( + assertEquals( + Where.MethodMatching.MATCH, where.isMethodMatching(createMethodNode("substring", "(II)Ljava/lang/String;"), null)); where = new Where("String", "replaceAll", "(String,String)", new String[0], null); - assertTrue( + assertEquals( + Where.MethodMatching.MATCH, where.isMethodMatching( createMethodNode( "replaceAll", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;"), null)); where = new Where("HashMap", "", "(Map)", new String[0], null); - assertTrue(where.isMethodMatching(createMethodNode("", "(Ljava/util/Map;)V"), null)); + assertEquals( + Where.MethodMatching.MATCH, + where.isMethodMatching(createMethodNode("", "(Ljava/util/Map;)V"), null)); where = new Where("ArrayList", "removeIf", "(Predicate)", new String[0], null); - assertTrue( + assertEquals( + Where.MethodMatching.MATCH, where.isMethodMatching( createMethodNode("removeIf", "(Ljava/util/function/Predicate;)Z"), null)); where = new Where("String", "concat", "", new String[0], null); - assertTrue(where.isMethodMatching(createMethodNode("concat", "String (String)"), null)); + assertEquals( + Where.MethodMatching.MATCH, + where.isMethodMatching(createMethodNode("concat", "String (String)"), null)); where = new Where("String", "concat", " \t", new String[0], null); - assertTrue(where.isMethodMatching(createMethodNode("concat", "String (String)"), null)); + assertEquals( + Where.MethodMatching.MATCH, + where.isMethodMatching(createMethodNode("concat", "String (String)"), null)); where = new Where( "Inner", @@ -126,18 +135,20 @@ public void methodMatching() { "(com.datadog.debugger.probe.Outer$Inner)", new String[0], null); - assertTrue( + assertEquals( + Where.MethodMatching.MATCH, where.isMethodMatching( createMethodNode("innerMethod", "(Lcom/datadog/debugger/probe/Outer$Inner;)V"), null)); where = new Where("Inner", "innerMethod", "(Outer$Inner)", new String[0], null); - assertTrue( + assertEquals( + Where.MethodMatching.MATCH, where.isMethodMatching( createMethodNode("innerMethod", "(Lcom/datadog/debugger/probe/Outer$Inner;)V"), null)); where = new Where("MyClass", "myMethod", null, new String[] {"42"}, null); ClassFileLines classFileLines = mock(ClassFileLines.class); MethodNode myMethodNode = createMethodNode("myMethod", "()V"); when(classFileLines.getMethodsByLine(42)).thenReturn(Arrays.asList(myMethodNode)); - assertTrue(where.isMethodMatching(myMethodNode, classFileLines)); + assertEquals(Where.MethodMatching.MATCH, where.isMethodMatching(myMethodNode, classFileLines)); } private MethodNode createMethodNode(String name, String desc) {