-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Support instrumentation for Executors.newVirtualThreadPerTaskExecutor() #6008
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| import io.micrometer.core.instrument.internal.TimedScheduledExecutorService; | ||
|
|
||
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Method; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
| import java.util.concurrent.*; | ||
|
|
@@ -56,6 +57,11 @@ | |
| @NonNullFields | ||
| public class ExecutorServiceMetrics implements MeterBinder { | ||
|
|
||
| private static final String CLASS_NAME_THREAD_PER_TASK_EXECUTOR = "java.util.concurrent.ThreadPerTaskExecutor"; | ||
|
|
||
| @Nullable | ||
| private static final Method METHOD_THREAD_COUNT_FROM_THREAD_PER_TASK_EXECUTOR = getMethodForThreadCountFromThreadPerTaskExecutor(); | ||
|
|
||
| private static boolean allowIllegalReflectiveAccess = true; | ||
|
|
||
| private static final InternalLogger log = InternalLoggerFactory.getInstance(ExecutorServiceMetrics.class); | ||
|
|
@@ -315,6 +321,9 @@ else if (className.equals("java.util.concurrent.Executors$FinalizableDelegatedEx | |
| monitor(registry, | ||
| unwrapThreadPoolExecutor(executorService, executorService.getClass().getSuperclass())); | ||
| } | ||
| else if (className.equals(CLASS_NAME_THREAD_PER_TASK_EXECUTOR)) { | ||
| monitorThreadPerTaskExecutor(registry, executorService); | ||
| } | ||
| else { | ||
| log.warn("Failed to bind as {} is unsupported.", className); | ||
| } | ||
|
|
@@ -439,6 +448,39 @@ private void monitor(MeterRegistry registry, ForkJoinPool fj) { | |
| registeredMeterIds.addAll(meters.stream().map(Meter::getId).collect(toSet())); | ||
| } | ||
|
|
||
| private void monitorThreadPerTaskExecutor(MeterRegistry registry, ExecutorService executorService) { | ||
| List<Meter> meters = asList(Gauge | ||
| .builder(metricPrefix + "executor.active", executorService, | ||
| ExecutorServiceMetrics::getThreadCountFromThreadPerTaskExecutor) | ||
| .tags(tags) | ||
| .description("The approximate number of threads that are actively executing tasks") | ||
| .baseUnit(BaseUnits.THREADS) | ||
| .register(registry)); | ||
| registeredMeterIds.addAll(meters.stream().map(Meter::getId).collect(toSet())); | ||
| } | ||
|
|
||
| private static long getThreadCountFromThreadPerTaskExecutor(ExecutorService executorService) { | ||
| try { | ||
| return (long) METHOD_THREAD_COUNT_FROM_THREAD_PER_TASK_EXECUTOR.invoke(executorService); | ||
| } | ||
| catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| @Nullable | ||
| private static Method getMethodForThreadCountFromThreadPerTaskExecutor() { | ||
| try { | ||
| Class<?> clazz = Class.forName(CLASS_NAME_THREAD_PER_TASK_EXECUTOR); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could work Class<?> clazz = Class.forName(CLASS_NAME_THREAD_PER_TASK_EXECUTOR);
// Get private access to the class
MethodHandles.Lookup privateLookup = MethodHandles.privateLookupIn(clazz, MethodHandles.lookup());
// Find the private method
return privateLookup.findVirtual(clazz, "threadCount", MethodType.methodType(long.class));There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with methodhandles in theory the accesscheck occurs only once (during the lookup) but with plain reflection the access check occurs in every invocation. At least this is my understanding
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @juliojgd |
||
| Method method = clazz.getMethod("threadCount"); | ||
| method.setAccessible(true); | ||
| return method; | ||
| } | ||
| catch (Throwable e) { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Disable illegal reflective accesses. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to generate a
java.lang.invoke.MethodHandleonce (maybe static) and use it (the same one, cached) in every call? Maybe it would be better for performance reasonsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliojgd Thanks for the feedback!
I'm not sure how easy implementing this with the
MethodHandlewould be, but it's not a hot spot, so I think it's okay as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, the gauge will call the method every N seconds, right? And the method in turn will make the reflective call each time, so maybe it could be a hotspot (due to the number of calls made during the JVM instance life).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@juliojgd I'm not familiar with the
MethodHandle, so I'm not sure how effective it is here, but I attempted to switch to it in e5d159b.