Skip to content

Commit b7276e9

Browse files
dividedminddividedmind
committed
refactor(agent): Refactor Hook.apply and ClassFileTransformer
Refactor `Hook.apply` to return a `Set<ApplicationAction>` indicating whether the method was marked for ByteBuddy instrumentation or instrumented by Javassist. This allows `ClassFileTransformer` to conditionally apply the `AppMapInstrumented` annotation only when ByteBuddy instrumentation is actually needed. Additionally, add improved logging for Javassist instrumentation failures and guard against excessive logging of these exceptions.
1 parent 886332e commit b7276e9

File tree

2 files changed

+103
-87
lines changed

2 files changed

+103
-87
lines changed

‎agent/src/main/java/com/appland/appmap/transform/ClassFileTransformer.java‎

Lines changed: 20 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -189,47 +189,24 @@ private void processClass(CtClass ctClass) {
189189
}
190190
}
191191

192-
private boolean applyHooks(CtBehavior behavior) {
193-
boolean traceClass = tracePrefix == null || behavior.getDeclaringClass().getName().startsWith(tracePrefix);
194-
192+
private Set<Hook.ApplicationAction> applyHooks(CtBehavior behavior, boolean traceClass) {
195193
try {
196194
List<HookSite> hookSites = getHookSites(behavior);
197195

198196
if (hookSites == null) {
199197
if (traceClass) {
200198
logger.trace("no hook sites");
201199
}
202-
return false;
200+
return java.util.Collections.emptySet();
203201
}
204202

205-
Hook.apply(behavior, hookSites);
206-
207-
if (logger.isDebugEnabled()) {
208-
for (HookSite hookSite : hookSites) {
209-
final Hook hook = hookSite.getHook();
210-
String className = behavior.getDeclaringClass().getName();
211-
if (tracePrefix != null && !className.startsWith(tracePrefix)) {
212-
continue;
213-
}
214-
215-
if (traceClass) {
216-
logger.trace("hooked {}.{}{} on ({},{}) with {}",
217-
className,
218-
behavior.getName(),
219-
behavior.getMethodInfo().getDescriptor(),
220-
hook.getMethodEvent().getEventString(),
221-
hook.getPosition(),
222-
hook);
223-
}
224-
}
225-
}
226-
return true;
203+
return Hook.apply(behavior, hookSites, traceClass);
227204

228205
} catch (NoSourceAvailableException e) {
229206
Logger.println(e);
230207
}
231208

232-
return false;
209+
return java.util.Collections.emptySet();
233210
}
234211

235212
public List<HookSite> getHookSites(CtBehavior behavior) {
@@ -292,7 +269,7 @@ public byte[] transform(ClassLoader loader,
292269
try {
293270
ClassPool classPool = AppMapClassPool.get();
294271
if (traceClass) {
295-
logger.debug("className: {}", className);
272+
logger.trace("className: {}", className);
296273
}
297274

298275
ctClass = classPool.makeClass(new ByteArrayInputStream(bytes));
@@ -317,7 +294,8 @@ public byte[] transform(ClassLoader loader,
317294
return null;
318295
}
319296

320-
boolean hookApplied = false;
297+
boolean bytecodeModified = false;
298+
boolean needsByteBuddy = false;
321299
for (CtBehavior behavior : ctClass.getDeclaredBehaviors()) {
322300
if (traceClass) {
323301
logger.trace("behavior: {}", behavior.getLongName());
@@ -331,24 +309,27 @@ public byte[] transform(ClassLoader loader,
331309
}
332310

333311
methodsExamined++;
334-
if (this.applyHooks(behavior)) {
335-
hookApplied = true;
312+
Set<Hook.ApplicationAction> actions = this.applyHooks(behavior, traceClass);
313+
if (!actions.isEmpty()) {
314+
bytecodeModified = true;
336315
methodsHooked++;
316+
if (actions.contains(Hook.ApplicationAction.MARKED)) {
317+
needsByteBuddy = true;
318+
}
337319
}
338320
}
339321

340-
if (hookApplied) {
322+
if (bytecodeModified) {
341323
// One or more of the methods in the the class were marked as needing to
342324
// be instrumented. Mark the class so the bytebuddy transformer will
343325
// know it needs to be instrumented.
344-
ClassFile classFile = ctClass.getClassFile();
345-
ConstPool constPool = classFile.getConstPool();
346-
Annotation annot = new Annotation(AppMapInstrumented.class.getName(), constPool);
347-
AnnotationUtil.setAnnotation(new AnnotatedClass(ctClass), annot);
348-
349-
if (traceClass) {
350-
logger.trace("hooks applied to {}", className);
326+
if (needsByteBuddy) {
327+
ClassFile classFile = ctClass.getClassFile();
328+
ConstPool constPool = classFile.getConstPool();
329+
Annotation annot = new Annotation(AppMapInstrumented.class.getName(), constPool);
330+
AnnotationUtil.setAnnotation(new AnnotatedClass(ctClass), annot);
351331
}
332+
352333
if (logger.isDebugEnabled()) {
353334
packagesHooked.compute(ctClass.getPackageName(), (k, v) -> v == null ? 1 : v + 1);
354335
}

‎agent/src/main/java/com/appland/appmap/transform/annotations/Hook.java‎

Lines changed: 83 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java.util.Arrays;
44
import java.util.Comparator;
5+
import java.util.EnumSet;
56
import java.util.HashSet;
67
import java.util.List;
78
import java.util.Map;
@@ -107,25 +108,67 @@ public HookSite prepare(CtBehavior targetBehavior, Map<String, Object> hookConte
107108
return new HookSite(this, behaviorOrdinal, binding);
108109
}
109110

110-
public static void apply(CtBehavior targetBehavior, List<HookSite> hookSites) {
111+
public enum ApplicationAction {
112+
MARKED,
113+
INSTRUMENTED
114+
}
115+
116+
private static boolean exceptionLogged = false;
117+
118+
public static Set<ApplicationAction> apply(CtBehavior targetBehavior, List<HookSite> hookSites, boolean traceClass) {
119+
Set<ApplicationAction> actions = EnumSet.noneOf(ApplicationAction.class);
111120
MethodInfo methodInfo = targetBehavior.getMethodInfo();
112121
AnnotationsAttribute attr =
113122
(AnnotationsAttribute)methodInfo.getAttribute(AnnotationsAttribute.visibleTag);
114123

115-
// If the behavior is marked as an app method, update the annotation with
116-
// the behavior ordinals so the bytebuddy transformer can instrument it.
117-
if (attr.getAnnotation(AppMapAppMethod.class.getName()) != null) {
118-
setBehaviorOrdinals(targetBehavior, hookSites);
119-
}
124+
if (attr != null) {
125+
// If the behavior is marked as an app method, update the annotation with
126+
// the behavior ordinals so the bytebuddy transformer can instrument it.
127+
if (attr.getAnnotation(AppMapAppMethod.class.getName()) != null) {
128+
setBehaviorOrdinals(targetBehavior, hookSites);
129+
actions.add(ApplicationAction.MARKED);
130+
if (traceClass) {
131+
logger.debug("tracing {}.{}{}",
132+
targetBehavior.getDeclaringClass().getName(),
133+
targetBehavior.getName(),
134+
targetBehavior.getMethodInfo().getDescriptor());
135+
}
136+
}
120137

121-
// If it's (also) marked as an agent method, it needs to be instrumented
122-
// by javassist.
123-
if (attr.getAnnotation(AppMapAgentMethod.class.getName()) != null) {
124-
instrument(targetBehavior, hookSites);
138+
// If it's (also) marked as an agent method, it needs to be instrumented
139+
// by javassist.
140+
if (attr.getAnnotation(AppMapAgentMethod.class.getName()) != null) {
141+
try {
142+
instrument(targetBehavior, hookSites);
143+
actions.add(ApplicationAction.INSTRUMENTED);
144+
if (traceClass) {
145+
String hooks = hookSites.stream()
146+
.map(h -> h.getHook().toString())
147+
.collect(Collectors.joining(", "));
148+
logger.debug("{}.{}{} instrumented with hooks: {}",
149+
targetBehavior.getDeclaringClass().getName(),
150+
targetBehavior.getName(),
151+
targetBehavior.getMethodInfo().getDescriptor(),
152+
hooks);
153+
}
154+
} catch (CannotCompileException | NotFoundException e) {
155+
String msg = String.format("failed to instrument %s.%s: %s",
156+
targetBehavior.getDeclaringClass().getName(), targetBehavior.getName(), e.getMessage());
157+
if (!exceptionLogged) {
158+
logger.debug(e, msg);
159+
exceptionLogged = true;
160+
} else {
161+
logger.debug(msg);
162+
}
163+
}
164+
}
125165
}
166+
167+
return actions;
126168
}
127169

128-
public static void instrument(CtBehavior targetBehavior, List<HookSite> hookSites) {
170+
public static void instrument(CtBehavior targetBehavior, List<HookSite> hookSites)
171+
throws CannotCompileException, NotFoundException {
129172
final CtClass returnType = getReturnType(targetBehavior);
130173
final Boolean returnsVoid = (returnType == CtClass.voidType);
131174

@@ -150,44 +193,36 @@ public static void instrument(CtBehavior targetBehavior, List<HookSite> hookSite
150193

151194
}
152195

153-
try {
154-
String beforeSrcBlock = beforeSrcBlock(uniqueLocks.toString(),
155-
invocations[MethodEvent.METHOD_INVOCATION.getIndex()]);
156-
logger.trace("{}: beforeSrcBlock:\n{}", targetBehavior::getName, beforeSrcBlock::toString);
157-
targetBehavior.insertBefore(
158-
beforeSrcBlock);
159-
160-
String afterSrcBlock = afterSrcBlock(invocations[MethodEvent.METHOD_RETURN.getIndex()]);
161-
logger.trace("{}: afterSrcBlock:\n{}", targetBehavior::getName, afterSrcBlock::toString);
162-
163-
targetBehavior.insertAfter(
164-
afterSrcBlock);
165-
166-
ClassPool cp = AppMapClassPool.get();
167-
String exitEarlyCatchSrc = "{com.appland.appmap.process.ThreadLock.current().exit();return;}";
168-
if (returnsVoid) {
169-
targetBehavior.addCatch(exitEarlyCatchSrc,
170-
cp.get("com.appland.appmap.process.ExitEarly"));
171-
} else if (!returnType.isPrimitive()) {
172-
exitEarlyCatchSrc = "{com.appland.appmap.process.ThreadLock.current().exit();return("
173-
+ returnType.getName() + ")$e.getReturnValue();}";
174-
targetBehavior
175-
.addCatch(exitEarlyCatchSrc, cp.get("com.appland.appmap.process.ExitEarly"));
176-
}
177-
logger.trace("{}: catch1Src:\n{}", targetBehavior::getName, exitEarlyCatchSrc::toString);
178-
179-
String catchSrcBlock = catchSrcBlock(invocations[MethodEvent.METHOD_EXCEPTION.getIndex()]);
180-
targetBehavior.addCatch(
181-
catchSrcBlock,
182-
cp.get("java.lang.Throwable"));
183-
logger.trace("{}: catchSrcBlock:\n{}", targetBehavior::getName, catchSrcBlock::toString);
184-
185-
} catch (CannotCompileException e) {
186-
logger.debug(e, "failed to compile {}.{}", targetBehavior.getDeclaringClass().getName(),
187-
targetBehavior.getName());
188-
} catch (NotFoundException e) {
189-
logger.debug(e);
196+
String beforeSrcBlock = beforeSrcBlock(uniqueLocks.toString(),
197+
invocations[MethodEvent.METHOD_INVOCATION.getIndex()]);
198+
logger.trace("{}: beforeSrcBlock:\n{}", targetBehavior::getName, beforeSrcBlock::toString);
199+
targetBehavior.insertBefore(
200+
beforeSrcBlock);
201+
202+
String afterSrcBlock = afterSrcBlock(invocations[MethodEvent.METHOD_RETURN.getIndex()]);
203+
logger.trace("{}: afterSrcBlock:\n{}", targetBehavior::getName, afterSrcBlock::toString);
204+
205+
targetBehavior.insertAfter(
206+
afterSrcBlock);
207+
208+
ClassPool cp = AppMapClassPool.get();
209+
String exitEarlyCatchSrc = "{com.appland.appmap.process.ThreadLock.current().exit();return;}";
210+
if (returnsVoid) {
211+
targetBehavior.addCatch(exitEarlyCatchSrc,
212+
cp.get("com.appland.appmap.process.ExitEarly"));
213+
} else if (!returnType.isPrimitive()) {
214+
exitEarlyCatchSrc = "{com.appland.appmap.process.ThreadLock.current().exit();return("
215+
+ returnType.getName() + ")$e.getReturnValue();}";
216+
targetBehavior
217+
.addCatch(exitEarlyCatchSrc, cp.get("com.appland.appmap.process.ExitEarly"));
190218
}
219+
logger.trace("{}: catch1Src:\n{}", targetBehavior::getName, exitEarlyCatchSrc::toString);
220+
221+
String catchSrcBlock = catchSrcBlock(invocations[MethodEvent.METHOD_EXCEPTION.getIndex()]);
222+
targetBehavior.addCatch(
223+
catchSrcBlock,
224+
cp.get("java.lang.Throwable"));
225+
logger.trace("{}: catchSrcBlock:\n{}", targetBehavior::getName, catchSrcBlock::toString);
191226
}
192227

193228
private static void setBehaviorOrdinals(CtBehavior behavior,

0 commit comments

Comments
 (0)