Jetty InstrumentedHttpChannelListener races to track async request times

[jschlather]

A single version of the InstrumentedHttpChannelListener is used to track all requests. The InstrumentedHttpChannelListener creates a single AsyncListener and stores the request time for async requests in a field on this class. This is overwritten everytime a new async request is started, in high throughput scenarios this results in the latency of async requests being drastically underreported. Since on completion, the time since the last async request started is reported as the latency, rather than the time the request took. This issue persists across all versions of the InstrumentedHttpChannelListener. The fix here is straightforward, a new listener must be constructed for each async request. Or the start time must otherwise be stored uniquely for each async request.

[joschi]

The fix here is straightforward, a new listener must be constructed for each async request. Or the start time must otherwise be stored uniquely for each async request.

Thanks for reporting this! Could you provide a pull request?

[avirlrma]

Hey @joschi @jschlather ,
I understood the issue, Can I work on the PR here?

[jschlather]

I’ve been pretty busy with work recently and haven’t had time to look at patching this. @avirlrma feel free to take this on. Happy to review a patch.

[joschi]

@jschlather @avirlrma Did any of you give this a try and could open a pull request?

[jschlather]

I can probably get this done this week. What branch should I PR against? Should I make the change in each of the jetty 9, 10 and 11 versions of the listener?

[joschi]

@jschlather That would be awesome! ❤️

Please build your PR against the release/4.2.x branch and if you have some time left a second one against release/5.0.x, but we can also try to cherry-pick the one against Metrics 4.2.x.

Please make the change for all Jetty modules (9, 10, 11).

Thanks! 😄