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

Add Jetty's ThreadPool metrics #593

Merged
merged 8 commits into from May 24, 2018

Conversation

Projects
None yet
5 participants
@matsumana
Contributor

matsumana commented May 7, 2018

I want to monitor whether Jetty's ThreadPool setting is sufficient so I'd like to add several metrics.

Usage

Example In Spring Boot 2.0.x

@Bean
public JettyServletWebServerFactory webServerFactory(MeterRegistry registry) {
    JettyServletWebServerFactory factory = new JettyServletWebServerFactory();
    tags = Collections.singletonList(Tag.of("id", "0"));
    factory.setThreadPool(new InstrumentedQueuedThreadPool(registry, tags));
    return factory;
}

matsumana added some commits May 7, 2018

.tags(tags)
.description("The number of max threads")
.register(registry);
Gauge.builder("jetty.threads.live", threadPool, InstrumentedQueuedThreadPool::getThreads)

This comment has been minimized.

@checketts

checketts May 7, 2018

Collaborator

How does this compare to the Tomcat equivalent? Wondering if active is a better name and how it differs from busy? Or is it merely idle + busy?

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 9, 2018

Hello.
Would you please review this?

@mweirauch

This comment has been minimized.

Collaborator

mweirauch commented May 9, 2018

Just a thought: Is the jetty.threads.idle superfluous as it could be calculated from jetty.threads.current subtracted by jetty.threads.busy? Wonder who would graph the "idle" metric in a plot with "busy" and "current" and "max".

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 10, 2018

Just a thought: Is the jetty.threads.idle superfluous as it could be calculated from jetty.threads.current subtracted by jetty.threads.busy?

@mweirauch

Good point.
I'll delete jetty.threads.idle and unify metric name to the same it as TomcatMetrics.

  • jetty.threads.config.min
  • jetty.threads.config.max
  • jetty.threads.busy
  • jetty.threads.current

matsumana added some commits May 10, 2018

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 10, 2018

Updated

@mweirauch

This comment has been minimized.

Collaborator

mweirauch commented May 14, 2018

Just tested against Spring Boot 1.5 with

	@Bean
	public JettyEmbeddedServletContainerFactory webServerFactory(MeterRegistry registry) {
		JettyEmbeddedServletContainerFactory factory = new JettyEmbeddedServletContainerFactory();
		factory.setThreadPool(new InstrumentedQueuedThreadPool(registry, Collections.emptyList()));
		return factory;
	}

LGTM.

Perhaps some auto-configuration for micrometer-spring-legacy and Spring Boot 2 should be tracked in follow up tickets?

matsumana added some commits May 19, 2018

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 19, 2018

@mweirauch Thank you for the confirmation.
I implemented JettyMetricsConfiguration.

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 19, 2018

Would you merge this PR?
If this PR is merged, I'll create an issue about JettyMetricsConfiguration on Spring Boot's repo.

@mweirauch

This comment has been minimized.

Collaborator

mweirauch commented May 23, 2018

Just one thing, could you add JettyMetricsConfiguration to the metrics auto configuration so that it is picked up?

I am not too familiar with the EmbeddedServletContainerFactory setup within Spring Boot - one question pops up: Should the JettyEmbeddedServletContainerFactory actually be injected as well in case somebody rolls his own? (Spring Boot creates one if there is none.)

Anything else @checketts @jkschneider ? Rebase against 1.0.x?

@jkschneider jkschneider merged commit f89ff6f into micrometer-metrics:master May 24, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@jkschneider jkschneider added this to the 1.1.0 milestone May 24, 2018

@jkschneider

This comment has been minimized.

Contributor

jkschneider commented May 24, 2018

Merged in 21a375a

@mweirauch

This comment has been minimized.

Collaborator

mweirauch commented May 24, 2018

I fear the JettyMetricsConfiguration::instrumentJettyThreadPool is kicking in too late. EmbeddedWebApplicationContext is calling the factory before afformentioned method is triggered.

Haven't found any other solution except actually defining the factory ourselfs. Dunno if the setup has changed with Spring Boot 2 and it becomes easier.

In case one really needs to roll his own factory and still wants threadpool instrumentation, one needs to exclude the JettyMetricsConfiguration from the auto-configuration. (e.g. with spring.autoconfigure.exclude)

@matsumana

This comment has been minimized.

Contributor

matsumana commented May 25, 2018

@mweirauch Thank you for the opinion.
I'll investigate.

mweirauch added a commit that referenced this pull request May 31, 2018

Fix Jetty metrics instrumentation
The factory is being asked to create a server before our configuration
has a chance to kick in.

Relates to #593.

jkschneider added a commit that referenced this pull request Jun 4, 2018

Fix Jetty metrics instrumentation
The factory is being asked to create a server before our configuration
has a chance to kick in.

Relates to #593.
@wilkinsona

This comment has been minimized.

Contributor

wilkinsona commented Sep 24, 2018

I'm looking at incorporating this change into Spring Boot 2.0. What's the benefit of InstrumentedQueuedThreadPool? I'm concerned about the need to set Jetty's thread pool as it will overwrite a user's customisation of the thread pool. It also limits the usefulness of JettyServerThreadPoolMetrids. Could it not take a QueuedThreadPool or even a ThreadPool? It could then adapt the meters that it binds based on the pool's concrete type.

@mweirauch

This comment has been minimized.

Collaborator

mweirauch commented Oct 2, 2018

So, perhaps we could actually ask for the ThreadPool in the current JettyMetricsPostProcessor.

Would it be allowed to register a new bean within postProcessAfterInitialization() on the held ApplicationContext? In this case a ThreadPool-adaptive variant of the JettyServerThreadPoolMetrics which can later be picked up by the MeterRegistry postprocessing where all MeterBinder are collected and bound.

Edit: Think we could just directly bind to the MeterRegistry.

@wilkinsona

This comment has been minimized.

Contributor

wilkinsona commented Oct 3, 2018

Would it be allowed to register a new bean within postProcessAfterInitialization() on the held ApplicationContext.

Not really, no. A BeanPostProcessor is intended for post-processing individual beans. A BeanFactoryPostProcessor would be a more appropriate place to perform bean registration. However, I'd prefer that this is done without any post-processing. It should be possible to do it in a manner similar to Boot's TomcatMetricsAutoConfiguration.

@matsumana

This comment has been minimized.

Contributor

matsumana commented Oct 8, 2018

How does this look like?
Although I've only tried Spring MVC yet, it seems to work fine

@Configuration
@ConditionalOnClass({ Server.class, Loader.class, WebAppContext.class })
public class JettyMetricsAutoConfiguration {

	@Bean
	public WebServerFactoryCustomizer<JettyServletWebServerFactory> jettyFactoryCustomizer(
			MeterRegistry registry) {
		return (factory) -> factory.setThreadPool(
				new InstrumentedQueuedThreadPool(registry, Collections.emptyList()));
	}
}
@wilkinsona

This comment has been minimized.

Contributor

wilkinsona commented Oct 8, 2018

That's better in terms of the conditions, but it's still setting the factory's thread pool and using InstrumentedQueueThreadPool. I think both of those should be avoided for the reasons I tried to explain above.

@wilkinsona

This comment has been minimized.

Contributor

wilkinsona commented Oct 8, 2018

I've opened #911 to describe and further discuss what I think needs to be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment