Skip to content
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

Jetty-12, replacement for HttpChannel.Listener #8885

Closed
gregw opened this issue Nov 10, 2022 · 6 comments · Fixed by #9901
Closed

Jetty-12, replacement for HttpChannel.Listener #8885

gregw opened this issue Nov 10, 2022 · 6 comments · Fixed by #9901
Assignees
Labels
Enhancement Pinned Issues that never go stale

Comments

@gregw
Copy link
Contributor

gregw commented Nov 10, 2022

Jetty version(s)
Jetty-12

Enhancement Description

In Jetty-9/10/11 A HttpChannel.Listener can be added as a bean to an AbstractConnector and then efficiently retrieved in the constructor of every HttpChannel.

Jetty-12 has good interception points (not least with HttpStream wrappers), but has no similar convenience method for installing them. Instead a handler needs to be used to install a stream wrapper on every request.

We could either revive a listener mechanism in 12, or perhaps instead add a method that would allow a bean added to a connector to efficiently wrap every HttpStream? Or perhaps a combination of both, i.e. a stream wrapper that invokes a new listener?

@joakime
Copy link
Contributor

joakime commented Nov 10, 2022

The Listener mechanism doesn't really encourage mucking with the exchange, it's more about announcing certain lifecycle events in the exchange.

IMO, A wrapper approach would be just encouraging all kinds of shenanigans that are not in the right place.
If you want a wrapper, make a proper Handler.

@gregw
Copy link
Contributor Author

gregw commented Nov 10, 2022

The Listener mechanism doesn't really encourage mucking with the exchange, it's more about announcing certain lifecycle events in the exchange.
IMO, A wrapper approach would be just encouraging all kinds of shenanigans that are not in the right place. If you want a wrapper, make a proper Handler.

Good points.

So the combined approach would have a pure listener (incapable of significant or easy shenanigans (yeah I know... where there is a will ....)) and that only if we register such a listener on the connector, would a stream wrapper be installed to collect events for it.

If you want to manipulate a request, then a handler is the way to do that.

@joakime
Copy link
Contributor

joakime commented Apr 24, 2023

Is this still on the agenda?

I was just going through the @Disabled tests in jetty-core and noticed ...

https://github.com/eclipse/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpChannelEventTest.java

@sbordet
Copy link
Contributor

sbordet commented Apr 30, 2023

I'd keep it around as it would be nice to have a single event listener and hiding all the wrappings.

@joakime joakime added the Pinned Issues that never go stale label Apr 30, 2023
joakime added a commit that referenced this issue May 10, 2023
@joakime joakime linked a pull request May 10, 2023 that will close this issue
joakime added a commit that referenced this issue May 10, 2023
joakime added a commit that referenced this issue May 10, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.0.beta2 May 10, 2023
joakime added a commit that referenced this issue May 22, 2023
joakime added a commit that referenced this issue May 22, 2023
joakime added a commit that referenced this issue May 22, 2023
joakime added a commit that referenced this issue May 23, 2023
@gregw gregw moved this from 🏗 In progress to 👀 In review in Jetty 12.0.0.beta2 Jun 6, 2023
@gregw
Copy link
Contributor Author

gregw commented Jun 6, 2023

After some discussion with @lorban and @sbordet, we are now doubtful that a listener interface is really needed and that the benefits can be achieved by:

  • improving the nanotime events captured on the request
  • a utility handler that can be put at various points in the handler tree to capture most "listened for" events
  • review of the HttpStream wrapper mechanism with the thought of potential simplification to just capture completion event

So this #9754 and #9872 should be put on hold whilst we verify this is true.

Meanwhile, some more observations from the discussion about the mechanisms proposed in #9754:

  • It is per Connector, thus it's events cannot be used for any per request handling needed ( eg. sessions commit and complete events).
  • As a per Connector mechanism so it really should see bytes on the wire style events, but it is implemented above any HttpStream wrapping that might be done on a per request basis, thus the bytes and events seen are not what go on the wire, but what are given to the stream. Currently we do not use HttpStream wrappers to mutate content, but nothing stops an application from doing so. If we do need a connector based listener, perhaps it should be a HttpStream.Listener rather than a HttpChannel.Listener
  • The only event that a Handler cannot access that the HttpChannel.Listener can is the onRequestBegin event. This can be assisted by having better nano time getters on the request that record when the request was first seen and when the headers were completely parsed.
  • HttpStream wrapping gives access to the same completion event, but it is perhaps not the most elegant mechanism to intercept just that event.

@joakime
Copy link
Contributor

joakime commented Jun 6, 2023

The stream vs network view is interesting, but ultimately not what most people that use the historical HttpChannel.Listener are interested in.

Metric style events that people want to know about (and we've gotten questions for, and have helped other open source projects with)

  1. Request parsing has begun
  2. Request header parsing is done
  3. Request started (being sent to handler tree) - request has now been fully customized
  4. Request body is being read
  5. Request trailers are being read from network
  6. Request complete - all Request content (headers, body, trailers) have been read from network
  7. Request exit (has exited handler tree)
  8. Response has been committed to network
  9. Response body is being written to network
  10. Response trailers are being written to network
  11. Response complete - all response content (headers, body, trailers) have been written to network
  12. Request / Response exchange is complete - request related resources will now be recycled

The order of usefulness to most people.

  1. things that impact user experience
  2. things that happen from the networks point of view
  3. things that impact server performance / health

@joakime joakime moved this from 👀 In review to 🏗 In progress in Jetty 12.0.0.beta2 Jun 6, 2023
@lorban lorban self-assigned this Jun 7, 2023
lorban added a commit that referenced this issue Jun 12, 2023
Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban linked a pull request Jun 12, 2023 that will close this issue
lorban added a commit that referenced this issue Jun 12, 2023
lorban added a commit that referenced this issue Jun 12, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 12, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 12, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 12, 2023
lorban added a commit that referenced this issue Jun 13, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 13, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 13, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 13, 2023
Signed-off-by: Ludovic Orban <[email protected]>
lorban added a commit that referenced this issue Jun 13, 2023
Signed-off-by: Ludovic Orban <[email protected]>
@gregw gregw moved this from 🏗 In progress to 👀 In review in Jetty 12.0.0.beta2 Jun 14, 2023
lorban added a commit that referenced this issue Jun 15, 2023
* #8885 add EventsHandler API

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban closed this as completed Jun 15, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.0.0.beta2 Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Pinned Issues that never go stale
Projects
None yet
4 participants