Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Java concurrency instrumentations #7101

Merged
merged 5 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy;
import static net.bytebuddy.matcher.ElementMatchers.isTypeInitializer;

Expand All @@ -27,7 +28,7 @@ public final class AsyncPropagatingDisableInstrumentation extends InstrumenterMo
implements Instrumenter.CanShortcutTypeMatching {

public AsyncPropagatingDisableInstrumentation() {
super("java_concurrent");
super(EXEC_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of adding a dependency to AbstractExecutorInstrumentation just to share a string constant.

If we have to share these string constants then I would rather put them in a separate class, such as ExecNames or similar. Although TBH when it comes to instrumentations I actually prefer to see the real string, rather than have it hidden behind a constant which I then have to look up.

Note that the compiler will inline string constants, so the class holding them isn't actually used at runtime.

}

private static final ElementMatcher.Junction<TypeDescription> RX_WORKERS =
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package datadog.trace.instrumentation.java.concurrent;

import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;

import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
Expand All @@ -11,7 +13,7 @@
public class TaskUnwrappingInstrumentation extends InstrumenterModule.Profiling
implements Instrumenter.ForKnownTypes, Instrumenter.HasTypeAdvice {
public TaskUnwrappingInstrumentation() {
super("java_concurrent", "task-unwrapping");
super(EXEC_NAME, "task-unwrapping");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.not;
Expand All @@ -25,7 +26,7 @@
public final class WrapRunnableAsNewTaskInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForKnownTypes {
public WrapRunnableAsNewTaskInstrumentation() {
super("java_concurrent", "new-task-for");
super(EXEC_NAME, "new-task-for");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.executor;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.executor;

import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.executor;

import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static java.util.Collections.singletonMap;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.executor;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.nameEndsWith;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.cancelTask;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

Expand All @@ -29,7 +30,7 @@ public class RejectedExecutionHandlerInstrumentation extends InstrumenterModule.
implements Instrumenter.ForBootstrap, Instrumenter.CanShortcutTypeMatching {

public RejectedExecutionHandlerInstrumentation() {
super("java_concurrent", "rejected-execution-handler");
super(EXEC_NAME, "rejected-execution-handler");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.executor;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE_FUTURE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
import static datadog.trace.instrumentation.java.concurrent.AbstractExecutorInstrumentation.EXEC_NAME;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.forkjoin;

import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.cancelTask;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.capture;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.FORK_JOIN_TASK;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;

Expand All @@ -25,8 +26,10 @@
public class JavaForkJoinPoolInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType {

public static final String FJP_NAME = "fjp";

public JavaForkJoinPoolInstrumentation() {
super("java_concurrent", "fjp");
super(EXEC_NAME, FJP_NAME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.forkjoin;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.declaresMethod;
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
Expand All @@ -11,6 +11,8 @@
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.FORK_JOIN_TASK;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE_FUTURE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static datadog.trace.instrumentation.java.concurrent.forkjoin.JavaForkJoinPoolInstrumentation.FJP_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;

Expand Down Expand Up @@ -41,7 +43,7 @@ public final class JavaForkJoinTaskInstrumentation extends InstrumenterModule.Tr
implements Instrumenter.ForBootstrap, Instrumenter.ForTypeHierarchy, ExcludeFilterProvider {

public JavaForkJoinTaskInstrumentation() {
super(AbstractExecutorInstrumentation.EXEC_NAME);
super(EXEC_NAME, FJP_NAME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.runnable;

import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.endTaskScope;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.startTaskScope;
import static datadog.trace.instrumentation.java.concurrent.AbstractExecutorInstrumentation.EXEC_NAME;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;

Expand All @@ -18,6 +18,11 @@
import java.util.concurrent.ForkJoinTask;
import net.bytebuddy.asm.Advice;

/**
* Instrument the Runnable implementation of {@code ConsumerTask} (JDK 9+) where the ForkJoinTask
* parent class is already instrumented by {@link
* datadog.trace.instrumentation.java.concurrent.forkjoin.JavaForkJoinTaskInstrumentation}
*/
@AutoService(InstrumenterModule.class)
public class ConsumerTaskInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.runnable;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.nameEndsWith;
Expand All @@ -11,6 +11,7 @@
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.startTaskScope;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE_FUTURE;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isConstructor;
import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy;
Expand Down Expand Up @@ -38,7 +39,7 @@
public final class RunnableFutureInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForTypeHierarchy, ExcludeFilterProvider {
public RunnableFutureInstrumentation() {
super("java_concurrent", "runnable-future");
super(EXEC_NAME, "runnable-future");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.runnable;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.notExcludedByName;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.not;
Expand All @@ -28,8 +29,10 @@
public final class RunnableInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForTypeHierarchy {

public static final String RUNNABLE_NAME = "runnable";

public RunnableInstrumentation() {
super(AbstractExecutorInstrumentation.EXEC_NAME, "runnable");
super(EXEC_NAME, RUNNABLE_NAME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.timer;

import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.cancelTask;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils.capture;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.ExcludeType.RUNNABLE;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.ExcludeFilter.exclude;
import static datadog.trace.bootstrap.instrumentation.java.concurrent.QueueTimerHelper.startQueuingTimer;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static datadog.trace.instrumentation.java.concurrent.runnable.RunnableInstrumentation.RUNNABLE_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPrivate;
Expand All @@ -27,7 +29,7 @@
public class JavaTimerInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForSingleType {
public JavaTimerInstrumentation() {
super("java_timer", "java_concurrent", "runnable");
super("java_timer", EXEC_NAME, RUNNABLE_NAME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package datadog.trace.instrumentation.java.concurrent;
package datadog.trace.instrumentation.java.concurrent.timer;

import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static datadog.trace.instrumentation.java.concurrent.executor.AbstractExecutorInstrumentation.EXEC_NAME;
import static datadog.trace.instrumentation.java.concurrent.runnable.RunnableInstrumentation.RUNNABLE_NAME;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
Expand All @@ -12,6 +14,7 @@
import datadog.trace.bootstrap.InstrumentationContext;
import datadog.trace.bootstrap.instrumentation.java.concurrent.AdviceUtils;
import datadog.trace.bootstrap.instrumentation.java.concurrent.State;
import datadog.trace.instrumentation.java.concurrent.runnable.RunnableInstrumentation;
import java.util.Map;
import java.util.TimerTask;
import net.bytebuddy.asm.Advice;
Expand All @@ -29,7 +32,7 @@ public final class TimerTaskInstrumentation extends InstrumenterModule.Tracing
implements Instrumenter.ForBootstrap, Instrumenter.ForTypeHierarchy {

public TimerTaskInstrumentation() {
super("java_timer", AbstractExecutorInstrumentation.EXEC_NAME, "runnable");
super("java_timer", EXEC_NAME, RUNNABLE_NAME);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import io.netty.channel.nio.NioEventLoopGroup
import io.netty.channel.oio.OioEventLoopGroup
import io.netty.util.concurrent.DefaultEventExecutor
import org.apache.tomcat.util.threads.TaskQueue
import runnable.Descendant
import spock.lang.Shared

import java.util.concurrent.ExecutorService
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
package executor

import com.google.common.util.concurrent.MoreExecutors
import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.api.Trace
import datadog.trace.bootstrap.instrumentation.api.Tags
import datadog.trace.bootstrap.instrumentation.java.concurrent.RunnableWrapper
import datadog.trace.core.DDSpan
import forkjoin.PeriodicTask
import org.apache.tomcat.util.threads.TaskQueue
import runnable.ComparableAsyncChild
import runnable.JavaAsyncChild
import spock.lang.Shared

import java.lang.reflect.InvocationTargetException
Expand Down Expand Up @@ -63,8 +68,8 @@ abstract class ExecutorInstrumentationTest extends AgentTestRunner {
void configurePreAgent() {
super.configurePreAgent()

injectSysConfig("dd.trace.executors", "CustomThreadPoolExecutor")
injectSysConfig("trace.thread-pool-executors.exclude", "ExecutorInstrumentationTest\$ToBeIgnoredExecutor")
injectSysConfig("dd.trace.executors", CustomThreadPoolExecutor.name)
injectSysConfig("trace.thread-pool-executors.exclude", ToBeIgnoredExecutor.name)
}

def "#poolName '#name' propagates"() {
Expand Down Expand Up @@ -482,12 +487,6 @@ abstract class ExecutorInstrumentationTest extends AgentTestRunner {

poolName = poolImpl.class.simpleName
}

static class ToBeIgnoredExecutor extends ThreadPoolExecutor {
ToBeIgnoredExecutor() {
super(1, 1, 0, TimeUnit.MILLISECONDS, new ArrayBlockingQueue<Runnable>(10))
}
}
}

class ExecutorInstrumentationForkedTest extends ExecutorInstrumentationTest {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
package executor

import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.api.Trace
import datadog.trace.core.DDSpan
Expand All @@ -6,6 +8,7 @@ import io.netty.channel.epoll.EpollEventLoopGroup
import io.netty.channel.local.LocalEventLoopGroup
import io.netty.channel.nio.NioEventLoopGroup
import io.netty.util.concurrent.DefaultEventExecutorGroup
import runnable.JavaAsyncChild
import spock.lang.Shared

import java.lang.reflect.InvocationTargetException
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package executor

import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.core.DDSpan
import executor.recursive.RecursiveThreadPoolExecution
import executor.recursive.RecursiveThreadPoolMixedSubmissionAndExecution
import executor.recursive.RecursiveThreadPoolSubmission
import io.netty.channel.DefaultEventLoopGroup
import spock.lang.Shared

Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
package executor

import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.api.DDSpanId
import datadog.trace.core.DDSpan
import executor.rejectedexecutionhandler.ExecutingRejectedExecutionHandler
import executor.rejectedexecutionhandler.SwallowingRejectedExecutionHandler
import io.netty.util.concurrent.DefaultEventExecutor
import io.netty.util.concurrent.DefaultThreadFactory
import runnable.JavaAsyncChild

import java.util.concurrent.ArrayBlockingQueue
import java.util.concurrent.CountDownLatch
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package executor;

import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;

public class ToBeIgnoredExecutor extends ThreadPoolExecutor {
public ToBeIgnoredExecutor() {
super(1, 1, 0, TimeUnit.MILLISECONDS, new ArrayBlockingQueue<>(10));
}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
package forkjoin

import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.core.DDSpan

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package runnable

import datadog.trace.agent.test.AgentTestRunner

import java.util.concurrent.ExecutionException
import java.util.concurrent.FutureTask

import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
Expand Down
Loading
Loading