Metrics not collected after ExecutorService recreation #5367
Conversation
|
Thank you for the pull request. Could you take a look at the test failures? |
Tests were fixed and PR is ready to review. Thank you |
|
@shakuzen kind reminder. Could you take a look this PR? |
There was a problem hiding this comment.
ExecutorServiceMetrics and TimedExecutorService can be used together on the same ExecutorService (e.g. through one of the monitor method on ExecutorServiceMetrics) or separately through the available constructor(s) on each. It looks like right now the metrics will only be deregistered if the ExecutorService is wrapped with monitor. If ExecutorServiceMetrics are bound to an ExecutorService without wrapping and it is shutdown, there is no way to unregister the metrics still. Maybe we can leave this as existing behavior, but it did stand out to me as seeming a bit inconsistent. We could make ExecutorServiceMetrics Closeable and handle unregistering its metrics through that, but it would be up to users to call that close method when shutting down the executor. Thoughts?
|
You're right about slight inconsistency. What threw me off was the comment above the class: "An {@link java.util.concurrent.ExecutorService} that is timed. This class is for internal use." But since it's public, users might have already relied on it. I would prefer not to require users to close ExecutorServiceMetrics, especially since it wouldn't work in my case where I pass the ExecutorService to the Akka framework and it handles the shutdown itself. |
|
Thanks for reminding me that |
| @@ -61,6 +64,8 @@ public class ExecutorServiceMetrics implements MeterBinder { | |||
|
|
|||
| private static final String DESCRIPTION_POOL_SIZE = "The current number of threads in the pool"; | |||
|
|
|||
| private final Set<Meter.Id> registeredMeterIds = ConcurrentHashMap.newKeySet(); | |||
There was a problem hiding this comment.
I don't think we need to use the Concurrent variant here but it also shouldn't matter much if we do.
#5366