wu-sheng commented on PR #816: URL: https://github.com/apache/skywalking-java/pull/816#issuecomment-5143013401
Overall this is in good shape — the design is correct and I could verify the key decisions: - `NameMatch` (instead of the sibling's `HierarchyMatch + MultiClassNameMatch`) is the right call here: these interceptors **create spans**, so enhancing subclasses too would produce nested duplicate `ThreadPoolExecutor/invokeAll` spans through the `super.invokeAll()` chain. Subclasses are still traced via the inherited instrumented override — the scenario's `NestedThreadPoolExecutor` proves exactly that path. - Span lifecycle is safe: `afterMethod` always runs in `finally`, and the span is stored in `MethodInvocationContext` before the wrap loop, so no leak even if wrapping fails. - The internal `execute(FutureTask)` calls don't double-wrap because the existing execute interceptor skips `RunnableFuture` — the two instrumentations compose cleanly. - Unit tests pass locally and the `jdk-threadpool-scenario` is green; the `ge 14 -> ge 20` segment math checks out. A few suggested improvements before merging: 1. **`expectedData.yaml`** — the six new child-segment refs assert `parentSpanId: not null`, which also passes with `0`. The headline property of this PR (child segments parented to the invoke spans, not the entry span) is therefore never asserted: if `ContextManager.capture()` were later moved above `createLocalSpan()`, CI would stay green. Please use `parentSpanId: gt 0` — it keeps the six expected segments interchangeable (no flakiness) but catches that regression. 2. **Unit test for the inactive-context guard** — every test creates an entry span first, so the `ContextManager.isActive()` branch of `shouldEnhance` is never exercised. Add one test without an entry span asserting `arguments[0]` is unchanged, `context.getContext()` is null, and no segments are recorded. 3. **`@Mock EnhancedInstance` is never initialized** — `TracingSegmentRunner` doesn't process Mockito annotations and there is no `MockitoRule`, so the field is silently `null` (harmless today since the interceptor never touches `objInst`). Either add `@Rule public MockitoRule rule = MockitoJUnit.rule();` like other plugin tests, or drop `@Mock` and pass `null` explicitly. 4. **`CHANGES.md`** — please include the plugin id so users can map it to `exclude_plugins`, e.g. `...in the JDK thread pool plugin (`jdk-threadpool-plugin`)`. 5. **`docs/en/setup/service-agent/java-agent/Supported-list.md`** — the *Thread Schedule Framework* section lists `JRE Callable and Runnable` and `JRE ForkJoinPool` but never listed the thread pool plugin. Since this PR grows its coverage, add a `JRE ThreadPoolExecutor (Optional²)` entry there. 6. Nit: a one-line comment on `NestedThreadPoolExecutor` noting it deliberately exercises the inherited-dispatch path (the added override lives on `ThreadPoolExecutor` itself) would keep someone from "simplifying" it to a plain pool later. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
