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

Task metrics should not expose time-related metrics as these are not supported yet #28535

Closed
filiphr opened this issue Nov 4, 2021 · 0 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@filiphr
Copy link
Contributor

filiphr commented Nov 4, 2021

In #23818 support for Task Execution and Scheduling Metrics was added.

However, the TaskExecutorMetricsAutoConfiguration uses ExecutorServiceMetrics.monitor(registry, threadPoolExecutor, beanName.get()) to perform the registration. Doing it like this will cause the metrics to also have each ExecutorService to be wrapped in TimedExecutorService and this will register the following metrics:

  • executor
  • executor.idle

This metrics are entirely useless because they are never updated since the return type is never used. There is #27041 which is requesting support for those 2 metrics and I think that this is something that can be handled separately. For now I would suggest that the TaskExecutorMetricsAutoConfiguration only calls

new ExecutorServiceMetrics(threadPoolExecutor, beanName.get(), Collections.emptySet()).bindTo(registry)

A test case to see this is

@Test
void taskExecutorUsingAutoConfigurationIsInstrumented() {
    this.contextRunner.withConfiguration(AutoConfigurations.of(TaskExecutionAutoConfiguration.class))
            .run((context) -> {
                MeterRegistry registry = context.getBean(MeterRegistry.class);
                Collection<FunctionCounter> meters = registry.get("executor.completed").functionCounters();
                assertThat(meters).singleElement()
                        .satisfies((meter) -> assertThat(meter.getId().getTag("name")).isEqualTo("application"));

                ThreadPoolTaskExecutor bean = context.getBean(ThreadPoolTaskExecutor.class);
                Timer timerMeter = registry.get("executor").timer();
                assertThat(timerMeter.count()).isZero();
                CountDownLatch latch = new CountDownLatch(1);
                bean.execute(latch::countDown);
                if (latch.await(100, TimeUnit.MILLISECONDS)) {
                    assertThat(timerMeter.count()).isEqualTo(1);
                } else {
                    fail("Task never executed");
                }

            });
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 4, 2021
@snicoll snicoll changed the title TaskExecutorMetricsAutoConfiguration registers obsolete metrics Task metrics should not expose time-related metrics as these are not supported yet Nov 9, 2021
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 9, 2021
@snicoll snicoll added this to the 2.6.x milestone Nov 9, 2021
@scottfrederick scottfrederick self-assigned this Nov 10, 2021
@scottfrederick scottfrederick modified the milestones: 2.6.x, 2.6.0 Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants