Skip to content

Hoist local vars for exception probes #11964

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

Open
jpbempel wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Hoist local vars for exception probes #11964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions ...a-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/ConfigurationUpdater.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ DebuggerTransformer supply(
private volatile Configuration currentConfiguration;
private DebuggerTransformer currentTransformer;
private final ProbeMetadata probeMetadata = new ProbeMetadata();
private final Config config;
private final DebuggerSink sink;
private final ClassesToRetransformFinder finder;
private final String serviceName;
Expand All @@ -112,6 +113,7 @@ public ConfigurationUpdater(
this.instrumentation = instrumentation;
this.transformerSupplier = transformerSupplier;
this.serviceName = TagsHelper.sanitize(config.getServiceName());
this.config = config;
this.sink = sink;
this.finder = finder;
}
Expand Down Expand Up @@ -255,11 +257,7 @@ private void installNewDefinitions(Configuration newConfiguration) {
// install new probe definitions
DebuggerTransformer newTransformer =
transformerSupplier.supply(
Config.get(),
newConfiguration,
this::recordInstrumentationProgress,
probeMetadata,
sink);
config, newConfiguration, this::recordInstrumentationProgress, probeMetadata, sink);
instrumentation.addTransformer(newTransformer, true);
currentTransformer = newTransformer;
LOGGER.debug("New transformer installed with probes: {}", newConfiguration.getDefinitions());
Expand Down
4 changes: 2 additions & 2 deletions ...ugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public class CapturedContextInstrumenter extends Instrumenter {
private int exitContextVar = -1;
private int timestampStartVar = -1;
private int throwableListVar = -1;
private Collection<LocalVariableNode> hoistedLocalVars = Collections.emptyList();
protected Collection<LocalVariableNode> hoistedLocalVars = Collections.emptyList();

public CapturedContextInstrumenter(
ProbeDefinition definition,
Expand Down Expand Up @@ -470,7 +470,7 @@ protected void addIsReadyToCaptureCall(InsnList insnList) {

// Initialize and hoist local variables to the top of the method
// if there is name/slot conflict, do nothing for the conflicting local variable
private Collection<LocalVariableNode> initAndHoistLocalVars(MethodNode methodNode) {
protected Collection<LocalVariableNode> initAndHoistLocalVars(MethodNode methodNode) {
int hoistingLevel = Config.get().getDynamicInstrumentationLocalVarHoistingLevel();
if (hoistingLevel == 0 || language != JvmLanguage.JAVA) {
// for now, only hoist local vars for Java
Expand Down
4 changes: 4 additions & 0 deletions ...nt-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ public ExceptionInstrumenter(

@Override
public InstrumentationResult.Status instrument() {
// hoisting is required because exception instrumentation is wrapping the whole method body in
// a try/catch that create a subscobe and even level method local vars are not accessible
// in the catch clause for capture
hoistedLocalVars = initAndHoistLocalVars(methodNode);
Map<AbstractInsnNode, Frame<BasicValue>> frames = computeFrames(classNode.name, methodNode);
processInstructions(frames); // fill returnHandlerLabel
addFinallyHandler(methodEnterLabel, returnHandlerLabel);
Expand Down
5 changes: 5 additions & 0 deletions ...ugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ public void onlyInstrument() throws Exception {
}

@Test
@DisabledIf(
value = "datadog.environment.JavaVirtualMachine#isJ9",
disabledReason = "Bug in J9: no LocalVariableTable for ClassFileTransformer")
public void instrumentAndCaptureSnapshots() throws Exception {
Config config = createConfig();
ExceptionProbeManager exceptionProbeManager = new ExceptionProbeManager(classNameFiltering);
Expand Down Expand Up @@ -162,6 +165,8 @@ public void instrumentAndCaptureSnapshots() throws Exception {
assertEquals(fingerprint, span.getTags().get(DD_DEBUG_ERROR_EXCEPTION_HASH));
assertEquals(Boolean.TRUE, span.getTags().get(Tags.ERROR_DEBUG_INFO_CAPTURED));
assertEquals(snapshot0.getId(), span.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 0)));
assertTrue(snapshot0.getCaptures().getReturn().getArguments().containsKey("arg"));
assertTrue(snapshot0.getCaptures().getReturn().getLocals().containsKey("intLocal"));
assertEquals(1, probeSampler.getCallCount());
assertEquals(1, globalSampler.getCallCount());
}
Expand Down