Binary execution support

[theobisproject]

Support for calls to external binaries. Fixes #767.

Code is currently a little messy but the commons-exec library works.
@felixbarny @eyalkoren I need some help with the Java APIs. Even removing all ignores from the Agent the classes where not instrumented. Maybe I have done something wrong?
Integration Tests for the Java API still has to be created. Any advice on this? Currently I'm using a REST-Resource deployed on Wildfly to test this change.

Checklist

• Implement code
• Add tests
• Update documentation
• Update CHANGELOG.md
• Update supported-technologies.asciidoc

Open for discussion

• Module name
• Module structure (All apis in the same package, own package per API)
• Need some way to add the exit value of the process and the process arguments to the span. What has to be done for this?

[eyalkoren]

Thanks a lot!! This is great stuff!

I didn't really review, as this is still marked WIP. Only looked for general stuff, mainly trying to answer why the core java stuff is not instrumented (not sure if you mean it doesn't work in the unit tests or the WildFly test).
So, first you need to add an exception for the instrumented types to where we say java. package should be ignored. Also, see the review comments- make sure the instrumentation classes are properly listed in the services file and that you are not filtering out the classes you want instrumented, eg the class loading filter.
Use DEBUG log level to see which advices are applied and whether there were type matches and method matches.

More general things- any instrumentation that looks for implementations of interfaces or sub-classes of types should contain a "cheap" implementation of getTypeMatcherPreFilter() (eg based on class name), otherwise startup times will be greatly affected.
Apart from that- the extraction of common code is great, but I would separate classes so that each API is handled through a different instrumentation class.

Lastly- what you did for testing with WildFly is a valid option, but you can also use a basic Java image with a basic Java application that starts transactions through the agent APIs and executes commands as spans.

[SylvainJuge]

Hi @theobisproject this is a great PR, I'd like to give a hand to move this forward.

As there are quite a few things to change, I think the best way to proceed would be to submit a PR against your fork branch. I will try to make the commits as self explanatory as possible. Are you OK with that ?

I have a question regarding commons-exec library (which I'm not very familiar with), as it relies on java.lang.Process that would probably be better to only instrument that part. Is there something that is provided by this library that would not be visible to a java.lang.Process instrumentation ?

One extra benefit of only instrumenting java.lang.Process would be to not having to check for nested method calls, for example when commons-exec calls java.lang.Process span should be created at commons-exec leven, and when java.lang.Process is used without span should be created.

[theobisproject]

Hi @SylvainJuge I'm okay with PR's against my branch.

I'm currently quite stuck with the insturmentation of the Java API's and are happy to get some help. Here ist the latest patch of me trying to get the Java process builder api to work Latest_work_for_supporting_Java_process_builder_api.patch.txt.

Support of commons-exec is mainly motivated because we use it heavily in our application and the Java API intrumentation didn't work. I would be ok to insturment commons-exec via the java.lang.Process instrumentation because we do not use custom executors. But for those who have custom executors which use a different api this would be a benefit.

[SylvainJuge]

@theobisproject I have pushed a PR to your fork (theobisproject#1), I've tried to make the commit history intelligible, but don't hesitate to ask if you have any questions.

Instrumentation is now in the incubating group, thus you have to set disable_instrumentations to an empty string to enable it (incubating features are disabled by default).

It was definitely more tricky than expected, and we found a few things to improve to help further contributions, thus definitely worth to push forward !

[theobisproject]

@SylvainJuge I tried your change and the Java APIs + commons exec synchronous API works. Great job! Will have a look at the code later.
Just noticed the async commons exec API is currently not tracked. See this example

Executor executor = new DefaultExecutor();
DefaultExecuteResultHandler handler = new DefaultExecuteResultHandler();
executor.execute(new CommandLine("java").addArgument("-version"), handler);
handler.waitFor();

What would be the plan going forward from here? I initially planned to also get the arguments of the call as well as the return code of the process. Should we separate this from the changes here?

[SylvainJuge]

Thanks for checking on your side.

I will try to reproduce the issue with asynchronous execution with commons-exec to see where the problem is.

One limitation of the current implementation is that we don't instrument java.lang.Process.toHandle(), but here it's not the case.

What is likely to happen, is that the java.lang.Process or java.lang.ProcessBuilder instance is created in a separate thread than the transaction by commons-exec, thus the span is not being linked to the active transaction. This kind of issue is often referred as "context propagation" and is sometimes tricky to implement.

I will need to investigate a bit to see how we could provide dedicated support for this with commons-exec.

[theobisproject]

@SylvainJuge Have you planned to do the proposed changes in another pull request against my branch. I ask because I won't be that much available from Monday on until next year.

@eyalkoren Would it be possible to merge the current state (including the incubating status) and improve the functionality by additional pull requests?

[SylvainJuge]

Yes, I'm currently doing those changes, and I can push and merge the PR without any extra action required on your side. But I can't do it today. Thanks for keeping us updated on your availability, that's really kind of you! Enjoy your end of the year vacations.

…

On Fri, Dec 13, 2019, 10:57 theobisproject ***@***.***> wrote: @SylvainJuge <https://github.com/SylvainJuge> Have you planned to do the proposed changes in another pull request against my branch. I ask because I won't be that much available from Monday on until next year. @eyalkoren <https://github.com/eyalkoren> Would it be possible to merge the current state (including the incubating status) and improve the functionality by additional pull requests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#903?email_source=notifications&email_token=AAF2JSSPU6H6SY3NF5GXU73QYNMBVA5CNFSM4JFCMVX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGZPY4Y#issuecomment-565378163>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAF2JSQFR5WQADTSOHZNP73QYNMBVANCNFSM4JFCMVXQ> .

[SylvainJuge]

jenkins, please run the tests