Auto-configure JettyMetrics

[snicoll]

We have metrics support for Tomcat and micrometer has support for Jetty so we should configure that as well for consistency.

[wilkinsona]

I looked at this briefly when I was adding support for Tomcat's metrics. It's not completely straightforward as it relies on Jetty being configured with a StatisticsHandler which it isn't by default. I wondered if the Jetty metrics auto-configuration should customise Jetty with a StatisticsHandler but didn't manage to convince myself it was the right thing to do.

[wilkinsona]

Given the potential complications with StatisticsHandler, we decided that this can wait until 2.1.

[evenh]

In my mind this sounds pretty useful. If I were to have a look at this, where would I start (never contributed to the boot codebase before)?

Regarding adding a StatisticsHandler customization to Jetty, one option would be to add a property for explicitly disabling it?

[snicoll]

@evenh we've decided to postpone this to 2.1 (it's not a matter of a lack of time working on it). Thanks anyway. You can look at the contributing page to get started. We're also on Gitter if you want to chat.

[davidkarlsen]

Should this be picked up again? https://github.com/micrometer-metrics/micrometer/blob/master/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/TimedHandler.java

[wilkinsona]

Thanks, @davidkarlsen. Unfortunately, TimedHandler makes things more complicated rather than less. We now use Jetty's StatisticsHandler when graceful shutdown is enabled and we would not want to use both StatisticsHandler and TimedHandler when Micrometer's on the classpath as well.

@jkschneider @shakuzen What was the motivation for creating TimedHandler rather than using Jetty's StatisticsHandler?

[jkschneider]

@wilkinsona Spring's http.server.requests > TimedHandler > StatisticsHandler. Because you have good framework-level instrumentation of API endpoints, adding TimedHandler or StatisticsHandler would not be useful.

TimedHandler is like StatisticsHandler, but much higher resolution. Rather than relying on StatisticsHandler's pre-aggregation of request throughput, etc. which are lossy it uses Micrometer timers.

TimedHandler was designed for projects that are using the Jetty API more directly, like Javalin to get a request-level metrics experience similar to Spring Boot's WebMvc and WebFlux metrics.

For Spring Boot, I think just stick with what you've already got for http.server.requests and maybe autoconfigure the JettyServerThreadPoolMetrics, JettyConnectionMetrics, and JettySslHandshakeMetrics binders? As LongTaskTimer continues to evolve into a more general purpose measure of saturation, the pool and connection metrics will just keep getting more and more useful.

[nyilmaz]

@wilkinsona JettyServerThreadPoolMetrics is now supported by JettyMetricsAutoConfiguration. Would *Metrics classes which @jkschneider mentioned, be considered for further auto configuration?

[wilkinsona]

@nyilmaz Yes, we can consider them. I think, although can't remember for certain, that's why I left this issue open when #14591 was implemented.

[nyilmaz]

oh nice, any plans or target release candidates for this issue?

[wilkinsona]

The issue's assigned to the general backlog milestone which means we don't have any concrete plans for it at the moment.

[wilkinsona]

We now bind connection and SSL handshake metrics in addition to the existing thread pool metrics binding so I think we're done here thanks to @bono007.

[nyilmaz]

@wilkinsona @snicoll is there any chance to include TimedHandler to the autoconfiguration process?

[wilkinsona]

As per Jon's comment above, that's not something we're considering at the moment.