Adds java-concurrent

[jam01]

Adds byteman rules for intercepting calls to java.util.concurrent.Executor interface and subinterfaces and proxying to appropriate TracedExecutors.

Is dependent on opentracing-contrib/java-concurrent#11

[objectiser]

@jam01 Thanks for the PR - however the rules are trying to do the same work that has been done in the java-concurrent lib. The aim of the rules is just to install the framework instrumentation libs.

For example, the rules should be of the form:

RULE Executors.newCachedThreadPool rule
CLASS java.util.concurrent.Executors
METHOD newCachedThreadPool
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT EXIT
IF TRUE
DO
  $! = new io.opentracing.contrib.concurrent.TracedExecutorService($!, getTracer())
ENDRULE

[jam01]

Hey @objectiser I originally was going to go that route. However the problem with that approach is that that'd only instrument executors that are instantiated through those factory methods. Any concrete implementation instantiated directly would not be instrumented. The framework I'm trying to instrument does not make use of java.util.concurrent.Executors for example. Moreover, the rules are not repeating what the java-concurrent lib is doing. The rules are only intercepting the method invocations, creating a traced instance and proxying the call to it, leaving all the work to the java-concurrent lib. It is exactly what's going on here.

[objectiser]

@jam01 You are right - the executors can be initialised in different ways - however the approach you have taken with the rules requires the executors to be cached, which is a memory leak.

I've experimented with the following rules (replace contents in your otarules.btm file) that does not require the additional helper. They pass all of the existing tests - can you see if they work in your environment? If not, could you add tests that replicate any problem you are seeing with the framework you are trying to instrument.

RULE Executor.execute
INTERFACE ^java.util.concurrent.Executor
METHOD execute
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT ENTRY
IF getTracer().activeSpan() != null
DO
   $1 = new io.opentracing.contrib.concurrent.TracedRunnable($1, getTracer());
ENDRULE

RULE ScheduledExecutorService.schedule(Runnable)
INTERFACE ^java.util.concurrent.ScheduledExecutorService
METHOD schedule(java.lang.Runnable,long,java.util.concurrent.TimeUnit)
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT ENTRY
IF getTracer().activeSpan() != null
DO
   $1 = new io.opentracing.contrib.concurrent.TracedRunnable($1, getTracer());
ENDRULE

RULE ScheduledExecutorService.schedule(Callable)
INTERFACE ^java.util.concurrent.ScheduledExecutorService
METHOD schedule(java.util.concurrent.Callable,long,java.util.concurrent.TimeUnit)
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT ENTRY
IF getTracer().activeSpan() != null
DO
   $1 = new io.opentracing.contrib.concurrent.TracedCallable($1, getTracer());
ENDRULE

RULE ScheduledExecutorService.scheduleAtFixedRate
INTERFACE ^java.util.concurrent.ScheduledExecutorService
METHOD scheduleAtFixedRate
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT ENTRY
IF getTracer().activeSpan() != null
DO
   $1 = new io.opentracing.contrib.concurrent.TracedRunnable($1, getTracer());
ENDRULE

RULE ScheduledExecutorService.scheduleWithFixedDelay
INTERFACE ^java.util.concurrent.ScheduledExecutorService
METHOD scheduleWithFixedDelay
HELPER io.opentracing.contrib.agent.OpenTracingHelper
AT ENTRY
IF getTracer().activeSpan() != null
DO
   $1 = new io.opentracing.contrib.concurrent.TracedRunnable($1, getTracer());
ENDRULE

[jam01]

Hey @objectiser these rules would definitely work out, but at this point I think we're definitely replicating the TracedExecutors' logic. If their logic changed we'd have to change the rules. For example I was working through another PR where the TracedExec would check if the ScopeManager is AutoFinish or not, because if so then we might want to to use a Continuation instead. That might not end up being valid behaviour but I think the argument is valid.

About the caching helper, this was more of an optimization than a necessity, if we can't make it work then we could do w/o it and just create a new TracedExec for every method call. If I understand the spring-cloud-contrib interceptor that's exactly what it's doing, new method invoke = new TracedExec just for that call. But I was actually thinking about the cache before your point on memory leak and maybe something like this might help, plus maybe make all getter methods synchronized?

[jam01]

Oh! The example I provided about AutoFinishScopeManager is a change that would actually be in the Runnable itself. So I'm OK with not proxying the call to a TracedExecutor and just doing its check as a byteman IF. Again the only thing to consider is if anything else is added to the TracedExecutor logic we'd have to add it to the rule. But I'm OK with that, if you're still OK with that then I'll go ahead and patch in your rules and update the PR.

I also take it that since you removed the calls for ExecutorService that it is safe to assume that all implementations just eventually call Executor.execute(Runnble) which I wasn't 100% sure about.

[objectiser]

I think it is less likely that changes would occur in the TracedExecutor logic - more likely in TracedRunnable and TracedCallable - so I think we should be ok with these rules.

I also take it that since you removed the calls for ExecutorService that it is safe to assume that all implementations just eventually call Executor.execute(Runnble) which I wasn't 100% sure about.

Based on the tests you have defined, this seems to be the case.

[objectiser]

Just a few minor things to fix.

One general point - if you could avoid squashing commits until the review process is completed, it makes it easier to review - we then only need to look at what has changed between each review cycle.

[objectiser]

You need to change this to 0.2.0.

[objectiser]

Thanks @jam01