Rework Jetty ThreadPool metrics to work with any ThreadPool

[wilkinsona]

This has been discussed on #593, but I don't want it to get lost in the push to RC1 as it's important for Boot 2.1 as well.

The short of it is that I think the class that binds the metrics for Jetty's thread pool should be reworked so that it doesn't rely on being able to customise the ThreadPool of the Server. Such customisation may overwrite something that a user's already done.

I think the MeterBinder should look something like this:

public class JettyThreadPoolMetrics implements MeterBinder {

	private final ThreadPool threadPool;

	private final Iterable<Tag> tags;

	public JettyThreadPoolMetrics(ThreadPool threadPool, Iterable<Tag> tags) {
		this.threadPool = threadPool;
		this.tags = tags;
	}

	@Override
	public void bindTo(MeterRegistry registry) {
		if (this.threadPool instanceof SizedThreadPool) {
			SizedThreadPool sizedThreadPool = (SizedThreadPool) this.threadPool;
			Gauge.builder("jetty.threads.config.min", sizedThreadPool,
					SizedThreadPool::getMinThreads)
					.description("The minimum number of threads in the pool")
					.tags(this.tags).register(registry);
			Gauge.builder("jetty.threads.config.max", sizedThreadPool,
					SizedThreadPool::getMaxThreads)
					.description("The maximum number of threads in the pool")
					.tags(this.tags).register(registry);
			if (this.threadPool instanceof QueuedThreadPool) {
				QueuedThreadPool queuedThreadPool = (QueuedThreadPool) this.threadPool;
				Gauge.builder("jetty.threads.busy", queuedThreadPool,
						QueuedThreadPool::getBusyThreads)
						.description("The number of busy threads in the pool")
						.tags(this.tags).register(registry);
			}
		}
		Gauge.builder("jetty.threads.current", this.threadPool,
				ThreadPool::getThreads)
				.description("The total number of threads in the pool")
				.tags(this.tags).register(registry);
		Gauge.builder("jetty.threads.idle", this.threadPool,
				ThreadPool::getIdleThreads)
				.description("The number of idle threads in the pool").tags(this.tags)
				.register(registry);
	}

}

Getting hold of the Server needed to create JettyThreadPoolMetrics is a pain and is something we should improve in Boot. It can be done like this with Boot 2:

@ConditionalOnBean(MeterRegistry.class)
@ConditionalOnClass({ Server.class, MeterRegistry.class })
@ConditionalOnWebApplication
public class JettyMetricsAutoConfiguration {

	private volatile Server server;

	@Bean
	@Order(Ordered.LOWEST_PRECEDENCE)
	public WebServerFactoryCustomizer<ConfigurableJettyWebServerFactory> jettyCustomizer() {
		return (jetty) -> {
			jetty.addServerCustomizers(this::setServer);
		};
	}

	@Bean
	@ConditionalOnMissingBean
	public JettyThreadPoolMetrics jettyThreadPoolMetrics() {
		return new JettyThreadPoolMetrics(this.server.getThreadPool(),
				Collections.emptyList());
	}

	private void setServer(Server server) {
		this.server = server;
	}

}

I believe a similar approach should work with Spring Boot 1.

[izeye]

@wilkinsona TomcatMetricsAutoConfiguration.context looks null all the time unless I'm missing something. TomcatMetricsAutoConfiguration.context is allowed to be null but JettyMetricsAutoConfiguration.server isn't allowed to be null. Am I missing something?

[wilkinsona]

It’s set via the customizers. As I said, it’s a pain, but it’s the best way we have at the moment.

[izeye]

@wilkinsona Thanks for the quick feedback! I saw it but expected the TomcatMetrics bean creation is faster than the WebServerFactoryCustomizer execution. Do I have a wrong expectation?

[wilkinsona]

Yes. The customizer beans are created and called very early in the lifecycle. See these tests that verify the behaviour.

[izeye]

@wilkinsona Ah, sorry. I didn't know. I'll look into it to understand how.

[izeye]

I didn't look into it yet but as @wilkinsona explained (Thanks, again), I confirmed that customizer's execution precedes other bean creations in
izeye/sample-micrometer-spring-boot@414e50f for Spring Boot 2.x and izeye/sample-micrometer-spring-boot@fe89129 for Spring 1.x.

I tried to resolve this issue in izeye@925ea22 as @wilkinsona suggested but broke the existing tests having @SpringBootTest. I didn't look into it yet but it could be reproduced easily and I added a simple test reproducing it in izeye/sample-micrometer-spring-boot@5f3ac5a.

@wilkinsona Could you look into it?

[wilkinsona]

I can’t see a test in izeye@925ea22. Have I missed it or did you mean to reference a different commit?

[izeye]

@wilkinsona Sorry, I fixed the wrong link in the comment right away but I guess you've checked it in your email. See izeye/sample-micrometer-spring-boot@5f3ac5a

[wilkinsona]

Yeah, that was it. Thanks.

The problem is that your test is using a mock web environment so the embedded container isn't involved. You need to configure it to use a random or defined port at which point your customiser will be called and server will be set.

I think this may show a problem with the approach. In the Tomcat case, a null context is tolerated. It looks like something similar will be needed here for mocked web tests at least.

[izeye]

@wilkinsona Thanks for the explanation 👍

I just added @ConditionalOnBean(AnnotationConfigEmbeddedWebApplicationContext.class) on JettyMetricsAutoConfiguration to guard against mock web environment and all the failed tests pass now although I'm not sure it's a right approach for this.

I created #914 with it to resolve this and for easier discussion if necessary.