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

Issue #5032 - Servlet / WebApp Metrics Listener Support #5033

Closed

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jul 8, 2020

Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime added Enhancement Sponsored This issue affects a user with a commercial support agreement labels Jul 8, 2020
@joakime joakime requested a review from gregw July 8, 2020 20:16
@joakime joakime self-assigned this Jul 8, 2020
@joakime joakime added this to In progress in Jetty 9.4.31 via automation Jul 8, 2020
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this approach because it is just too invasive and relies on the JIT to elide out lots of extra conditions so that non metric code will not be affected.

Also for filters, this approach is not very suitable as you only get enter/exit events when you really need enter/chain/chained/exit events. So I think the listener API needs work, but you also need to wrap the chain instance passed to each filter.

Also the exit events need to be in finally blocks, so any listeners that want to balance resources or calculate aggregate metrics don't get confused by enters that never exit!

So I still think the approach needs to use wrapping. The SingleThreadedWrapper mechanism in ServletHolder could be generalised and moved to AbstractHandler so that we are able to easily wrap Listeners, Filters and Servlet - both for ones we create ourselves and those that are passed in.

I think it would be perfectly fine for holders to have the concept of the instance (as created or passed in) and a optional wrapper (or onion shells of wrappers) used to execute. This can then be used for a Jetty provided metric mechanism... or users could even just make their own wrappers to do any kind of metric they want... in which case we'd just provide some simple metric examples.

Jetty 9.4.31 automation moved this from In progress to Review in progress Jul 14, 2020
@joakime joakime moved this from Review in progress to In progress in Jetty 9.4.31 Jul 22, 2020
@joakime joakime marked this pull request as draft July 22, 2020 19:40
@joakime joakime added this to In progress in Jetty 9.4.32 Jul 22, 2020
@joakime joakime removed this from In progress in Jetty 9.4.31 Jul 22, 2020
@joakime
Copy link
Contributor Author

joakime commented Sep 16, 2020

Closing.

PR #5273 is the minimal experimental changes to Jetty itself to support this.
And a new project at https://github.com/jetty-project/jetty-metrics has been created to test out some approaches to using these changes.

@joakime joakime closed this Sep 16, 2020
Jetty 9.4.32 automation moved this from In progress to Done Sep 16, 2020
@joakime joakime deleted the jetty-9.4.x-5032-servlet-webapp-metrics-listener branch September 16, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Jetty 9.4.32
  
Done
Development

Successfully merging this pull request may close these issues.

Introduce Listeners to aid in tracking timings within ServletContext and WebApp
2 participants