Add tracing support for ThreadPoolExecutor invokeAll and invokeAny

[xuzhiguang]

Support tracing for ThreadPoolExecutor.invokeAll and invokeAny

• Tests(including UT, IT, E2E) are added to verify the new feature.
• Closes [Feature] Add tracing support for ThreadPoolExecutor.invokeAll and invokeAny in the JDK thread pool plugin skywalking#13945.
• Update the CHANGES log.

[wu-sheng]

Thanks for the PR. The premise is correct — invokeAll/invokeAny convert tasks to FutureTask/QueueingFuture before calling execute, and ThreadPoolExecuteMethodInterceptor.wrap() explicitly returns null for RunnableFuture, so today these four overloads propagate nothing. Wrapping the Callables before the JDK converts them is the right hook, iteration order is preserved, the caller's collection is not mutated, and cancelled invokeAny losers simply never run.

There is one blocking problem, though.

Duplicate nested spans on every ThreadPoolExecutor subclass

ThreadPoolExecutorInstrumentation#enhanceClass() matches HierarchyMatch.byHierarchyMatch(...) or MultiClassNameMatch(...), and the new intercept point is a plain InstanceMethodsInterceptPoint. ClassEnhancePluginDefine only adds isDeclaredBy(typeDescription) for DeclaredInstanceMethodsInterceptPoint:

ElementMatcher.Junction<MethodDescription> junction = not(isStatic()).and(instanceMethodsInterceptPoint.getMethodsMatcher());
if (instanceMethodsInterceptPoint instanceof DeclaredInstanceMethodsInterceptPoint) {
    junction = junction.and(ElementMatchers.<MethodDescription>isDeclaredBy(typeDescription));
}

So named("invokeAll").or(named("invokeAny")) matches the inherited AbstractExecutorService methods on java.util.concurrent.ThreadPoolExecutor and on every subclass, and ByteBuddy generates an override in each one. The subclass's generated override then super-calls the base class's enhanced method, so the interceptor runs twice.

Reproduction on current main

This is not specific to your patch — submit is inherited in exactly the same way, so it can be demonstrated on main today. Given a subclass that declares nothing:

public static class MyPool extends ThreadPoolExecutor {
    public MyPool() { super(2, 2, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<Runnable>()); }
}

run with -Dskywalking.plugin.mount=plugins,activations,bootstrap-plugins and SW_AGENT_OPEN_DEBUG=true, then look at the dumped bytecode.

1. The base class gets a generated override. ThreadPoolExecutor never declared submit, but after enhancement:

== java.util.concurrent.ThreadPoolExecutor  ifaces=[EnhancedInstance]
   declared: public Future ThreadPoolExecutor.submit(Callable)        <-- added by ByteBuddy
   declared: public Future ThreadPoolExecutor.submit(Runnable)        <-- added
   declared: public Future ThreadPoolExecutor.submit(Runnable,Object) <-- added

ThreadPoolExecutor.submit(Callable):
  20: invokestatic  ThreadPoolSubmitMethodInterceptor_internal.intercept(...)

ThreadPoolExecutor.submit$accessor$$sw$g28bpd0(Callable):
   2: invokespecial java/util/concurrent/AbstractExecutorService.submit    <-- real JDK impl, OK

2. The subclass gets one too, even though it declares nothing:

== Probe$MyPool  ifaces=[EnhancedInstance]
   declared: public Future MyPool.submit(Callable)

MyPool.submit(Callable):
  20: invokestatic  ThreadPoolSubmitMethodInterceptor_internal.intercept(...)

3. And its super-call lands on the enhanced base method, not on AbstractExecutorService:

MyPool.submit$accessor$$sw$ssa2tc1(Callable):
   2: invokespecial java/util/concurrent/ThreadPoolExecutor.submit    <-- the enhanced one

So a single myPool.submit(task) runs:

MyPool.submit                 -> interceptor run #1 -> invokespecial ThreadPoolExecutor.submit
ThreadPoolExecutor.submit     -> interceptor run #2 -> invokespecial AbstractExecutorService.submit
AbstractExecutorService.submit -> actual JDK work

Why this is harmless today but not with this PR

ThreadPoolSubmitMethodInterceptor and ThreadPoolExecuteMethodInterceptor create no span, and wrap() is idempotent (instanceof SwCallableWrapper -> null), so run #2 is a silent no-op.

ThreadPoolInvokeMethodInterceptor#beforeMethod opens with an unconditional ContextManager.createLocalSpan(...). On any subclass that produces:

GET:/greet/{username}                    spanId 0
└── ThreadPoolExecutor/invokeAll         spanId 1   <- subclass level: wraps tasks, captures snapshot
    └── ThreadPoolExecutor/invokeAll     spanId 2   <- base level: duplicate, childless leaf
    SwCallableWrapper/...  (worker segments) --ref--> spanId 1

The inner span is a pure artifact — the outer call already wrapped everything, so the worker segments reference span 1 and span 2 has no children. A three-level hierarchy produces three.

Affected in practice: java.util.concurrent.ScheduledThreadPoolExecutor (it declares its own submit, which is why submit is safe there, but it does not declare invokeAll/invokeAny), Tomcat's org.apache.tomcat.util.threads.ThreadPoolExecutor, and any framework or user pool extending ThreadPoolExecutor. The scenario uses a bare new ThreadPoolExecutor(...), which is the single case that does not double, so CI cannot catch this.

Suggested fix

Only open the span when this invocation actually wrapped something. That is false on the nested super-call, because everything is already a SwCallableWrapper:

Collection<?> callables = (Collection<?>) allArguments[0];
List<Object> wrapped = new ArrayList<>(callables.size());
boolean wrappedAny = false;
for (Object c : callables) {
    Object w = wrap(c, snapshot);
    wrappedAny |= (w != c);
    wrapped.add(w);
}
if (!wrappedAny) {
    return;   // nested super-call: no span, no re-wrap
}

Note this also has to drive afterMethod/handleMethodException, which currently recompute shouldEnhance(allArguments) independently — see the next point.

Please also add a class NestedPool extends ThreadPoolExecutor {} to jdk-threadpool-scenario and assert the segment still contains exactly one ThreadPoolExecutor/invokeAll span, so this stays covered.

Other comments

afterMethod recomputes the guard instead of remembering the decision. shouldEnhance(allArguments) is evaluated independently in beforeMethod, afterMethod and handleMethodException. It happens to stay consistent today, but it is a fragile contract, and it breaks outright with any fix for the above that is not purely argument-derived.

The unit tests do not cover the feature. The three cases exercise null/empty/non-Collection arguments plus shouldTraceAlternativeInvokeAllSignature, which reflects on a synthetic private invokeAll(Collection, String) that exists on no executor. Nothing asserts that tasks become SwCallableWrapper, that iteration order is preserved, that the caller's collection is untouched, that already-wrapped tasks are skipped, or anything at all about invokeAny. Please use the real invokeAll(Collection, long, TimeUnit) overload instead of the synthetic signature, and add assertions on the wrapping itself.

expectedData.yaml span order (nit). The new spans are listed 4, 3, 2, 1, 0, but spans are archived in finish order (TraceSegment#archive), i.e. 1, 2, 3, 4, 0. It passes only because SegmentAssert#spansEquals falls back to matching by spanId. Listing them in real order makes future failures much easier to read.

Design point for the maintainers. apache/skywalking#13945 asks for context propagation; this PR additionally emits a caller-side ThreadPoolExecutor/invokeAll local span, which execute/submit deliberately do not. It is defensible — invokeAll blocks, so the span carries real timing — but it is an unconditional per-call span-count increase, and the operation name reports ThreadPoolExecutor even for subclasses. Worth a conscious decision before merging.

[xuzhiguang]

@wu-sheng Thanks for the detailed review. My plan is to handle only the outermost enhanced method by checking:

objInst.getClass() == method.getDeclaringClass()

For a ThreadPoolExecutor subclass, this is true in the subclass-generated override, but false when its super call enters the generated ThreadPoolExecutor override.

I will also use a ThreadLocal to record whether the outermost invocation actually created a span. afterMethod and handleMethodException will use this recorded state rather than recomputing from the mutated arguments, and only the invocation that created the span will stop or log it.

Regarding the caller-side ThreadPoolExecutor/invokeAll and invokeAny local span, I believe it is useful and intentional. Unlike execute and submit, these methods block the caller: invokeAll waits for all tasks, while invokeAny waits until one task completes successfully (or times out/fails). The local span therefore represents the batch invocation's wall-clock waiting time.

It also provides a clear parent for the context snapshots captured for the submitted callables, so the worker-side segments are associated with the specific invokeAll / invokeAny operation rather than directly with the broader caller span.

I understand that this adds one caller-side span per invocation. If maintainers prefer this plugin to provide context propagation only, without an invocation span, I can adjust the design accordingly; otherwise I will keep the span with the duplicate-span protection described above.

[wu-sheng]

We usually don't recommend threadlocal, unless you have to. It's easy to be polluted and stale.
We are better to instrument stricted cases to work perfectly.