Support instrumentation for Executors.newVirtualThreadPerTaskExecutor() #6008
Conversation
Closes micrometer-metricsgh-5488 Signed-off-by: Johnny Lim <izeye@naver.com>
There was a problem hiding this comment.
Thanks for the pull request. I wonder how many users will want to run with --add-opens and if we should in some sense be encouraging that. At a minimum, we should document the limitation. I suppose there isn't another option now. I did mean to re-start conversations with the JDK team about providing some way to do this monitoring without illegal reflection access.
Signed-off-by: Johnny Lim <izeye@naver.com>
| } | ||
|
|
||
| @Nullable | ||
| private static Method getMethodForThreadCountFromThreadPerTaskExecutor() { |
There was a problem hiding this comment.
Wouldn't it be better to generate a java.lang.invoke.MethodHandle once (maybe static) and use it (the same one, cached) in every call? Maybe it would be better for performance reasons
There was a problem hiding this comment.
@juliojgd Thanks for the feedback!
I'm not sure how easy implementing this with the MethodHandle would be, but it's not a hot spot, so I think it's okay as is.
There was a problem hiding this comment.
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).
Signed-off-by: Johnny Lim <izeye@naver.com>
| } | ||
| catch (Throwable e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| @Nullable | ||
| private static Method getMethodForThreadCountFromThreadPerTaskExecutor() { | ||
| private static MethodHandle getMethodHandleForThreadCountFromThreadPerTaskExecutor() { | ||
| try { | ||
| Class<?> clazz = Class.forName(CLASS_NAME_THREAD_PER_TASK_EXECUTOR); |
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
@juliojgd MethodHandles.privateLookupIn() requires JDK 9+, but Micrometer needs to work on JDK 8, so we can't call it directly.
There was a problem hiding this comment.
We should add something to the documentation about needing --add-opens for certain cases. But we can do that in a follow-up pull request.
See micrometer-metrics#6008 (review) Signed-off-by: Johnny Lim <izeye@naver.com>
…#6034) See #6008 (review) Signed-off-by: Johnny Lim <izeye@naver.com>
This PR tries to support instrumentation for the
Executors.newVirtualThreadPerTaskExecutor().Closes gh-5488