Binary execution support#1
Binary execution support#1theobisproject merged 33 commits intotheobisproject:binary-execution-supportfrom
Conversation
We should have a way to make plugins define which classes they need to instrument, probably requiring an explicit naming for classes that are loaded from bootstrap classloader in (java.*,sun.*, ...)
theobisproject
left a comment
There was a problem hiding this comment.
Code looks good from my point of view. Just a few minor things
...pm-process-plugin/src/main/java/co/elastic/apm/agent/process/ProcessExitInstrumentation.java
Outdated
Show resolved
Hide resolved
...plugins/apm-process-plugin/src/test/java/co/elastic/apm/agent/process/ProcessHelperTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| int returnValue; | ||
| try { | ||
| Process process = Runtime.getRuntime().exec(cmd); |
There was a problem hiding this comment.
Is it on purpose the integration test only tests the process created with the Runtime class?
Since the ProcessBuilder has a own instrumentation class I would expect it to also have an integration test.
There was a problem hiding this comment.
You are right, Runtime.getRuntime().exec(...) delegates to ProcessBuilder, but that's an implementation detail.
As far as I know creating instances of Process is always done through ProcessBuilder.start, thus in this case I'd say that it's pretty safe.
However, supporting java9 ProcessHandler for example would require to have a different code here to start and wait for process termination.
|
I've added support for commons-exec, thus your use-cases should be fully covered now. I think that capturing parameters and exit value should be easier to do in separate PRs when this plugin is merged to master in order to avoid keeping this PR open for too long. |
|
Everything works now. Thanks again for taking over and make the instrumentation work! |
This PR contains changes required for elastic#903
processfor consistency, part ofincubatinginstrumentations that are disabled by defaultcommons-execinstrumentation as it relies onjava.lang.Processunder the hood