Initial Jetty 12 server support

[joakime]

This is a first implementation of Jetty 12 integration with the Jetty core layer (no servlets, as those only exists in the environments).

Fixes: #4261

[pivotal-cla]

@joakime Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@joakime Thank you for signing the Contributor License Agreement!

[shakuzen]

Thank you for the pull request. I was looking at the existing TimedHandler for Jetty 11 and see it has logic to support async requests. I didn't notice anything for async in this pull request. Does it not support instrumenting async requests?

The build failure is due to the build trying to check for API compatibility in the previous version but there is no previous version of micrometer-jetty12. You can add micrometer-jetty12 to here so this check is skipped:

micrometer/build.gradle

Line 340 in c69180d

if (!(project.name in ['micrometer-jakarta9', 'micrometer-java11'])) { // add projects here that do not exist in the previous minor so should be excluded from japicmp

[joakime]

Thank you for the pull request. I was looking at the existing TimedHandler for Jetty 11 and see it has logic to support async requests. I didn't notice anything for async in this pull request. Does it not support instrumenting async requests?

Async requests is a Servlet concept.

Starting in Jetty 12, the core level behaviors no longer use Servlet.
Only the Jetty 12 environments support Servlet now.
Example: If you look closely when comparing TimedHandler from Jetty 11 and Jetty 12, you'll see no servlet imports on Jetty 12. Imports that are mandatory on Jetty 11.

If you want to instrument Async, then a unique micrometer layer for each Environment would need to be created.

Environment Name	Jakarta Version	Servlet Version	Namespace
ee10	EE 10	Servlet 6.0	jakarta.servlet
ee9	EE 9	Servlet 5.0	jakarta.servlet
ee8	EE 8	Servlet 4.0	javax.servlet

This means the following artifacts ..

• /micrometer-jetty12-ee10/
• /micrometer-jetty12-ee9/
• /micrometer-jetty11-ee8/
• and by the end of this year /micrometer-jetty12-ee11/

Or if micrometer already has a jakarta.servlet layer, then that generic layer could be used in the ee10 environment (possibly even the ee9 environment, but not the ee8 environment)

The build failure is due to the build trying to check for API compatibility in the previous version but there is no previous version of micrometer-jetty12. You can add micrometer-jetty12 to here so this check is skipped:

Thanks, that failure was unclear.

[wojtassi]

@shakuzen JettyStatisticsMetrics had "jetty.responses" among other stats. TimedHandler for jetty 11 and below does not have it anymore. Any particular reason why it was removed?

It would be nice to have those statistics back (among other stats that can be translated from jetty's StatisticsHandler)

I'm open to contribute code either under this PR or a new one to bring back stats that were available in JettyStatisticsMetrics.

Thanks.

[joakime]

@shakuzen JettyStatisticsMetrics had "jetty.responses" among other stats. TimedHandler for jetty 11 and below does not have it anymore. Any particular reason why it was removed?

Here's the events that we could build against.

• onBeforeHandling(Request request) - just before calling the server handler tree
• onRequestRead(Request request, Content.Chunk chunk) - every time a request content chunk has been parsed, just before making it available to the application
• onAfterHandling(Request request, boolean handled, Throwable failure) - after application handling
• onResponseBegin(Request request, int status, HttpFields headers) - just before the response is line written to the network
• onResponseWrite(Request request, boolean last, ByteBuffer content) - before each response content chunk has been written
• onResponseWriteComplete(Request request, Throwable failure) - after each response content chunk has been written
• onResponseTrailersComplete(Request request, HttpFields trailers) - after the response trailers have been written and the final onResponseWriteComplete() event was fired
• onComplete(Request request, Throwable failure) - when the request and response processing are complete, just before the request and response will be recycled

From these we can show ..

• how long a Request took to process (onBeforeHandling / onComplete) - whole lifecycle
• how long a Request took to read (onBeforeHandling / onRequestRead where chunk.last=true)
• how long a Response took (onResponseBegin / onResponseWriteComplete)
• how much time was spend in the server handler tree (onBeforeHandling / onAfterHandling) - if this is smaller than the request/response values then there likely a thread doing the read/write.
• if a request lifecycle failed during handler processing and had an uncaught exception (onAfterHandling with a failure)
• if a response failed to write (onResponseWriteComplete with a failure)
• if the response had HTTP trailers.
• how many reads are performed with request
• how many writes are performed with response

[wojtassi]

And for jetty.responses, we can aggregate status codes in onResponseBegin similar to StatisticsHandler:
switch (status / 100) { case 1 -> _responses1xx.increment(); case 2 -> _responses2xx.increment(); case 3 -> _responses3xx.increment(); case 4 -> _responses4xx.increment(); case 5 -> _responses5xx.increment(); }

[joakime]

Looking at https://github.com/micrometer-metrics/micrometer/blob/main/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/jetty/JettyStatisticsMetrics.java

These are Jetty core specific.

"jetty.requests", "Request duration"
"jetty.responses.size", "Total number of bytes across all responses"
"jetty.requests.active", "Number of requests currently active""jetty.dispatched.active", "Number of dispatches currently active"
"jetty.request.time.max", "Maximum time spent handling requests"
"jetty.stats", "Time stats have been collected for"

Note that "Request duration" is imprecise and vague.
Is it the time spent on just the request portion of the exchange? or the entire exchange/lifecycle (full request read, processing, and full response written)?
It is growing quite common for an HTTP Request to be received, the processing starts by looking at the path & headers, then starts writing out a response, THEN it reads the request (in parallel).
Or A big POST request is received, the response is quickly written, and then the request is read. (to a client the exchange is complete, but technically speaking the response is complete, and the request is still in process)

Also "jetty.responses.size" is highly dependent on where the StatisticsHandler is in the handler tree.
This is the number of bytes sent to the write methods.
It will not show the network bytes size (such as what appears with Transfer-Encoding)
Then when you deal with GzipHandler you have differences too.
If the StatisticsHandler is under a GzipHandler, then this will be the number of bytes from the application.
If the GzipHandler is under StatisticsHandler, then this will be the number of compressed bytes.

The following are Servlet specific and would be numbers that would be isolated to each Environment that supports Servlet.
That means if you have a Jetty 12 server with both a EE8 and a EE10 webapp deployed, you would have two separate groups of these numbers.

"jetty.dispatched", "Dispatch duration"
"jetty.async.requests", "Total number of async requests"
"jetty.async.dispatches", "Total number of requests that have been asynchronously dispatched"
"jetty.async.expires", "Total number of async requests that have expired"
"jetty.dispatched.active.max", "Maximum number of active dispatches being handled"
"jetty.dispatched.time.max", "Maximum time spent in dispatch handling"
"jetty.async.requests.waiting", "Currently waiting async requests"
"jetty.async.requests.waiting.max", "Maximum number of waiting async requests"

Also of note, we are working on Cross Context Dispatch support at the moment, each of those will be counted as a nested request/response.
It could be a scenario where a single HTTP request arrives to a Servlet, that cross context dispatches and triggers an internal Request / Response (depending on how the servlet dispatcher is used).
It will be quite possible to have stats that show far more requests/responses than have been received via a User Agent.

[wojtassi]

Regarding EE statistics. Yes they should be servlet specific.
Regarding jetty.requests. It counts number of requests (getRequestTotal) which is precise and request duration (getRequestTimeTotal) which I agree, could be imprecise. But imprecision could/should be noted as limitation in documentation.
Regarding jetty.responses.size, it should also be a limitation described in documentation. i.e. TimedHandler should be the first handler to be precise.

[shakuzen]

JettyStatisticsMetrics had "jetty.responses" among other stats. TimedHandler for jetty 11 and below does not have it anymore. Any particular reason why it was removed?

Because the TimedHandler has the same information and more in jetty.server.requests than was in jetty.responses. This is primarily the difference between a Timer and a FunctionTimer in Micrometer. FunctionTimer doesn't record individual timings, so you lose the ability to have the max and histogram/percentile functionality that is available in Timer. That is why using a Timer when possible is preferred and why JettyStatisticsMetrics was deprecated in favor of instrumenting Jetty with Micrometer rather than reading stats provided by Jetty.

@wojtassi could you be more specific about what is missing? It's wasteful to have multiple metrics for the same thing coming from different sources (TimedHandler and StatisticsHandler). As explained above, jetty.responses is not missing. In fact you have more information available. I'm confused why you would want jetty.responses from JettyStatisticsMetrics with less information when jetty.server.requests is available via TimedHandler.

And for jetty.responses, we can aggregate status codes

That's already done in jetty.server.requests provided by existing implementations of TimedHandler.

[shakuzen]

If you want to instrument Async

@joakime We do have instrumentation for async now, so not including it is offering less instrumentation than we had with Jetty 11, but due to the restructuring, it's unavoidable I suppose. I think my biggest concern, though, is that the instrumentation in this pull request doesn't break (or leak things) when the request Jetty core receives is an async request from a servlet environment. Or will the instrumentation in this pull request not even be called when handling an async request in a servlet on Jetty?

[joakime]

@joakime We do have instrumentation for async now, so not including it is offering less instrumentation than we had with Jetty 11, but due to the restructuring, it's unavoidable I suppose. I think my biggest concern, though, is that the instrumentation in this pull request doesn't break (or leak things) when the request Jetty core receives is an async request from a servlet environment. Or will the instrumentation in this pull request not even be called when handling an async request in a servlet on Jetty?

For async, you would need a new handler that is unique for each environment.
So far, you'll need 3 new handlers that exist at the Servlet level. (ee10, ee9, ee8). Each of those would need to be separate artifacts as they would have different dependencies.

Those can exist entirely independently from the Jetty Core TimedHandler in this PR and collect different metrics into the registry. If you choose to use a servlet level handler, it's metrics would not depend on the TimedHandler metrics you see in this PR.

There are a growing number of projects that use Jetty 12 core exclusively, with no Servlets at all.
Forcing async capture, by forcing a specific environment, is not a good idea for this artifact.

Keep in mind that Async on servlet was entirely optional, even back in Jetty 9 thru Jetty 11 days.
There are many thousands of servlet projects on Jetty that will have zero data on this async portions of the Jetty 11 TimedHandler on this project.

Also the original assumption that things are either blocking or async was bad as well.
By default, Jetty 12 core is neither of those 2 choices.
The default behavior is plain content.source & plain content.sink (which is effectively a per-buffer API), and everything is built on that.
From there each content.source & content.sink can be further adapted to fit the needs of the user.
For example, a user can decide that they want to adapt the request for input stream behaviors.

InputStream input = Content.Source.asInputStream(request);

Or they want to use reactive like APIs for the request.

Flow.Publisher<Chunk> publisher = Content.Source.asPublisher(request);

This is just a small handful of the ways the user can choose to read from the request, or write to the response.

When the request reaches a Servlet environment, the Async processing is unique for that one environment and one handling. Note: things like servlet async timeouts happen entirely within that environment, and core has no knowledge of this layer's chosen threading or behaviors.

One thing to also consider, if the handling (even servlet) results in one (or more) rehandles (eg request dispatching) back into the handler tree, each of those can be on core and/or an environment (even different ones) and using any of the above read/write techniques - plain content.source, plain content.sink, inputstream, outputstream, channels, flow api, servlet api (and blocking and asynccontext within that), etc..

This also means each sub-request is another request in your metrics. The rehandle and subsequent TimedHandler handle would have no way to identify what state the previous request handling was in (eg: down in the environment), only the handlers down at the environment would have that information, and they would be blind to any actions happening in another environment (or core).

There are 2 things this PR does.

• Identify the timing of the entire lifecycle of the request / response (jetty.server.requests.open) - this is the equivalent of the timing on the network layer to read the entire request and respond to the entire response (this is an OK metric for HTTP/1.1 but it means different things for persistent vs non-persistent connections. This is a flawed metric for HTTP/2 or HTTP/3 where the request/response timings should be separate)
• Identify the timing of the handling of the request (jetty.server.handling.open) - this is the equivalent of the time spent in handling the request on a server thread. If that handling results in a off server thread (eg: custom threading, asynccontext threading, etc) then that means the handling is "done" but the request/response might not be.

[shakuzen]

Thanks for the explanation @joakime. Let's separate out the servlet async instrumentation as an enhancement outside the scope of this pull request. We'll wait to hear from users about the need for it before we prioritize that work.

With that, I think this pull request is good to go. I just have a few polishing things I can deal with when merging.

One long-standing gap we've had between our Jetty server instrumentation and other web server instrumentation is lack of ability to tag with low cardinality URIs. It doesn't need to be addressed for this pull request because this is the status quo. But I figured while I have you here, if you have any ideas to tag some info about the URI to better identify request metrics while avoiding cardinality explosion, we'd love to hear. With Jersey or Spring WebMVC/WebFlux we're relying on URI patterns/templates that have path variables to avoid cardinality explosion. As far as I understand, such a concept in Jetty server (beside context paths?) is up to user's custom handler logic. I looked through the docs but only saw URI templates mentioned in the WebSocket section.

[joakime]

One long-standing gap we've had between our Jetty server instrumentation and other web server instrumentation is lack of ability to tag with low cardinality URIs. It doesn't need to be addressed for this pull request because this is the status quo. But I figured while I have you here, if you have any ideas to tag some info about the URI to better identify request metrics while avoiding cardinality explosion, we'd love to hear. With Jersey or Spring WebMVC/WebFlux we're relying on URI patterns/templates that have path variables to avoid cardinality explosion. As far as I understand, such a concept in Jetty server (beside context paths?) is up to user's custom handler logic. I looked through the docs but only saw URI templates mentioned in the WebSocket section.

If you choose to use the PathMappingsHandler, you can map an incoming path to a specific Handler using any PathSpec implementation (we ship with ServletPathSpec, RegexPathSpec, and UriTemplatePathSpec)
In some of the PathSpec implementations you can get more information out of match (context path, path info, the regex groups, the uri-template arguments, etc)
You can even mix-n-match the PathSpec implementations.
Internally, this is all handled by the org.eclipse.jetty.http.pathmap.PathMappings object.

This is available on Jetty 12 core.
Not on Servlet, as Servlet is restricted to only Servlet rules.

[wojtassi]

JettyStatisticsMetrics had "jetty.responses" among other stats. TimedHandler for jetty 11 and below does not have it anymore. Any particular reason why it was removed?

Because the TimedHandler has the same information and more in jetty.server.requests than was in jetty.responses. This is primarily the difference between a Timer and a FunctionTimer in Micrometer. FunctionTimer doesn't record individual timings, so you lose the ability to have the max and histogram/percentile functionality that is available in Timer. That is why using a Timer when possible is preferred and why JettyStatisticsMetrics was deprecated in favor of instrumenting Jetty with Micrometer rather than reading stats provided by Jetty.

Thanks for the explanation.

@wojtassi could you be more specific about what is missing? It's wasteful to have multiple metrics for the same thing coming from different sources (TimedHandler and StatisticsHandler). As explained above, jetty.responses is not missing. In fact you have more information available. I'm confused why you would want jetty.responses from JettyStatisticsMetrics with less information when jetty.server.requests is available via TimedHandler.

And for jetty.responses, we can aggregate status codes

That's already done in jetty.server.requests provided by existing implementations of TimedHandler.

My bad. I didn't realize that responses are now part of jetty.server.request. Once I dived into the implementation, I can see how it is done in TimedHandler. In my case, a reason for using jetty.responses would be backward compatibility to dashboards and alarms that expect jetty.responses but since jetty.responses (as part of JettyStatisticsMetrics) was depreciated, its on me (and not micrometer) to make it work.

[shakuzen]

In my case, a reason for using jetty.responses would be backward compatibility to dashboards and alarms that expect jetty.responses but since jetty.responses (as part of JettyStatisticsMetrics) was depreciated, its on me (and not micrometer) to make it work.

We are mindful of backward compatibility, and that's why JettyStatisticsMetrics is still available (although deprecated) to this day. If there's something we could do (or could have done) to make the migration easier, please let us know and we'll take it as feedback. We don't want to break users' dashboards/alerts or cause work without benefit, and when we do something that requires migration work, we want to make sure it's as smooth as possible for our users. The impact of changes like that can be more easily missed because they aren't API/ABI incompatible changes we programmatically check for, but for end users, they can be even more painful. In this case, as a migration path, you could have both metrics registered so both will be available in your metrics backend for a time, and migrate your dashboards and alerts over without a gap in data. Once everything is migrated over and you have as much historical data as you need, you can stop registering the deprecated JettyStatisticsMetrics. Would that approach work for you?

[wojtassi]

In my case, a reason for using jetty.responses would be backward compatibility to dashboards and alarms that expect jetty.responses but since jetty.responses (as part of JettyStatisticsMetrics) was depreciated, its on me (and not micrometer) to make it work.

We are mindful of backward compatibility, and that's why JettyStatisticsMetrics is still available (although deprecated) to this day. If there's something we could do (or could have done) to make the migration easier, please let us know and we'll take it as feedback. We don't want to break users' dashboards/alerts or cause work without benefit, and when we do something that requires migration work, we want to make sure it's as smooth as possible for our users. The impact of changes like that can be more easily missed because they aren't API/ABI incompatible changes we programmatically check for, but for end users, they can be even more painful. In this case, as a migration path, you could have both metrics registered so both will be available in your metrics backend for a time, and migrate your dashboards and alerts over without a gap in data. Once everything is migrated over and you have as much historical data as you need, you can stop registering the deprecated JettyStatisticsMetrics. Would that approach work for you?

This is pretty much our plan. Use custom JettyStatisticsMetrics that uses Jetty's StatisticsHandler for now (Basically we need to support Jetty 12 and cannot wait until May). Once this PR is merged and released in May, add TimedHandler in parallel to JettyStatisticsMetrics. Switch over our dashboards and alarms and eventually remove JettyStatisticsMetrics.

As far as making migration easier, I don't think you could have done anything different since JettyStatisticsHandler was depreciated 4 years ago so it is on us for not replacing it sooner.