TaskExecutorMetricsAutoConfiguration cannot support recording task execution time

[csdbianhua]

Description: I need to expose task execution time and idle time metrics of ‘ThreadPoolTaskExecutor’. After few tasks being executed, all metrics except those two show up. I found that TaskExecutorMetricsAutoConfiguration's behavior is iterating over ThreadPoolTaskExecutors and invoking ExecutorServiceMetrics#monitor. It do can register "executor.active", "executor.completed" and other metrics, but "executor"(task execution time) and "executor.idle"(task idle time) which processing in TimedExecutorService are left out.

Expectation: As I mentioned in #23818, I need to use the TimedExecutorService returned by ExecutorServiceMetrics#monitor to wrap the tasks I submitted, then I can get task execution time and idle time. So maybe we can wrap every ThreadPoolTaskExecutor, then task execution time and idle time should be recorded properly.

Version: 2.6.0-SNAPSHOT

Sample: pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>2.6.0-SNAPSHOT</version>
        <relativePath/> <!-- lookup parent from repository -->
    </parent>
    <groupId>com.example</groupId>
    <artifactId>thread-pool-metrics-demo</artifactId>
    <version>0.0.1-SNAPSHOT</version>
    <name>thread-pool-metrics-demo</name>
    <description>Demo project for Spring Boot</description>
    <properties>
        <java.version>11</java.version>
    </properties>
    <dependencies>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-actuator</artifactId>
        </dependency>
        <dependency>
            <groupId>io.micrometer</groupId>
            <artifactId>micrometer-registry-prometheus</artifactId>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-maven-plugin</artifactId>
            </plugin>
        </plugins>
    </build>
    <repositories>
        <repository>
            <id>spring-milestones</id>
            <name>Spring Milestones</name>
            <url>https://repo.spring.io/milestone</url>
            <snapshots>
                <enabled>false</enabled>
            </snapshots>
        </repository>
        <repository>
            <id>spring-snapshots</id>
            <name>Spring Snapshots</name>
            <url>https://repo.spring.io/snapshot</url>
            <releases>
                <enabled>false</enabled>
            </releases>
        </repository>
    </repositories>
    <pluginRepositories>
        <pluginRepository>
            <id>spring-milestones</id>
            <name>Spring Milestones</name>
            <url>https://repo.spring.io/milestone</url>
            <snapshots>
                <enabled>false</enabled>
            </snapshots>
        </pluginRepository>
        <pluginRepository>
            <id>spring-snapshots</id>
            <name>Spring Snapshots</name>
            <url>https://repo.spring.io/snapshot</url>
            <releases>
                <enabled>false</enabled>
            </releases>
        </pluginRepository>
    </pluginRepositories>

</project>

ThreadPoolMetricsDemoApplication.java

@SpringBootApplication
public class ThreadPoolMetricsDemoApplication {

    @Bean
    ThreadPoolTaskExecutor myExecutor() {
        return new ThreadPoolTaskExecutor();
    }

    public static void main(String[] args) throws ExecutionException, InterruptedException {
        try (var ctx = SpringApplication.run(ThreadPoolMetricsDemoApplication.class, args)) {
            var executor = ctx.getBean("myExecutor", ThreadPoolTaskExecutor.class);
            executor.submit(()->{}).get();
            var meterRegistry = ctx.getBean(MeterRegistry.class);
            var timer = meterRegistry.get("executor").timer();
            System.out.printf("Timer count should be 1 but got %s \n", timer.count());
            var timedExecutor = ExecutorServiceMetrics.monitor(meterRegistry, executor.getThreadPoolExecutor(), "myExecutor");
            timedExecutor.submit(()->{}).get();
            System.out.printf("Timer count should be 1 and got %s \n", timer.count());
        }
    }
}

Timer count should be 1 but got 0 
Timer count should be 1 and got 1 

[wilkinsona]

So maybe we can wrap every ThreadPoolTaskExecutor

I don't think this is possible. Once wrapped, a ThreadPoolTaskExecutor would become an Executor which would require changing the bean's type. This would break anyone who was trying to inject it as a more specific type.

To do something here, I think we might need a way to customize things when ThreadPoolTaskExecutor is initializing its executor in initializeExecutor. Perhaps @snicoll has a better idea.

[snicoll]

Certainly we need to be able to customize the way the executor is created, and that would require a framework change. The current API has a TaskDecorator that we might be able to use alongside TimedRunnable that's package private at the moment.

Another solution would be to provide this feature in the framework API itself and relying on it to decorate the executor accordingly.

[hugogu]

This would break anyone who was trying to inject it as a more specific type.

I agree this is indeed a problem, generally. But when it comes to a specific case like 'ThreadPoolTaskExecutor', I'd think injecting it as an Executor is better in terms of design. (And I have to admit this is NOT a reason to enforce all users to accept it.)

Comparing with writing a lot of code to setup a monitor against ThreadPoolTaskExecutor by myself, only to maintain the ability to inject it as a specific type, which by itself a design smell, I'd rather having an automatic way to setup it all, and also stop me from 'programming to concrete class.'

It makes it a lot easier to go along with the right way, and makes the smelly way harder.

Of course, another way of doing it, is to change how the ExectuterServiceMetrcis#monitor was implemented.

[hugogu]

ExectuterServiceMetrcis#monitor can work with any Executor or ExecutorService,

Can we make the TaskExecutorMetricsAutoConfiguration to bind against all ExecutorService?

By changing

         public void bindTaskExecutorsToRegistry(Map<String, Executor> executors, MeterRegistry registry) {
        executors.forEach((beanName, executor) -> {
            if (executor instanceof ThreadPoolTaskExecutor) {
                monitor(registry, safeGetThreadPoolExecutor((ThreadPoolTaskExecutor) executor),
                        () -> getTaskExecutorName(beanName));
            }
            else if (executor instanceof ThreadPoolTaskScheduler) {
                monitor(registry, safeGetThreadPoolExecutor((ThreadPoolTaskScheduler) executor),
                        () -> getTaskSchedulerName(beanName));
            }
        });
    }

to something like:

         public void bindTaskExecutorsToRegistry(Map<String, Executor> executors, MeterRegistry registry) {
        executors.forEach((beanName, executor) -> {
            if (executor instanceof ThreadPoolTaskExecutor) {
                monitor(registry, safeGetThreadPoolExecutor((ThreadPoolTaskExecutor) executor),
                        () -> getTaskExecutorName(beanName));
            }
            else if (executor instanceof ThreadPoolTaskScheduler) {
                monitor(registry, safeGetThreadPoolExecutor((ThreadPoolTaskScheduler) executor),
                        () -> getTaskSchedulerName(beanName));
            }  else if (executor instanceof ExecutorService) {
                new ExecutorServiceMetrics((ExecutorService) executor, beanName, Collections.emptyList()).bindTo(registry);
            }
        });
    }

[wilkinsona]

Thanks for the suggestion, @hugogu. I think it would be confusing for TaskExecutorMetricsAutoConfiguration to bind metrics for all ExecutorService beans and not just those used for task execution.

[hugogu]

Thanks for the suggestion, @hugogu. I think it would be confusing for TaskExecutorMetricsAutoConfiguration to bind metrics for all ExecutorService beans and not just those used for task execution.

Yes, and I assume the confusion can hopefully be reduced a bit when the class was named ExecutorMetricsAutoConfiguration and moved out of the task package.

Hmm, was it really about the scope of spring-boot project? Since it is called spring-boot, not Java-boot, then it makes a lot sense to only support TaskExecutor instead of all Executors.