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

Implement filtering of async requests. #689

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lapo-luchini
Copy link
Contributor

This fixes #676 for me… but there are no tests as I have no idea how to properly test an async servlet responses with Mockito.

Doesn't break any of the existing tests, though:

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running io.prometheus.client.exporter.MetricsServletTest
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.548 sec - in io.prometheus.client.
exporter.MetricsServletTest
Running io.prometheus.client.filter.MetricsFilterTest
Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.232 sec - in io.prometheus.client.
filter.MetricsFilterTest

Results :

Tests run: 11, Failures: 0, Errors: 0, Skipped: 0

Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

This is a nice one, thanks a lot!

Unfortunately, I have moved the Servlet filter implementation in the meantime, all common functionality is now in io.prometheus.client.servlet.common.filter.Filter, and there are two implementations using it: One for javax Servlets, and one for jakarta Servlets.

It will require a bit of boiler-plate code to port your solution to the current implementation, but I think it's worthwhile doing it. Basically you would need a common implementation of the AsyncListener in simpleclient_servlet_common, and an implementation of jakarta.servlet.AsyncListener and javax.servlet.AsyncListener in simpleclient_servlet and simpleclient_servlet_jakarta. This is similar to what I did with MetricsServlet and MetricsFilter.

Could you port your solution to the current master branch? I would really appreciate that.

Comment on lines +191 to +192
if (!done) {
done = true;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't thread safe, because there's a check-then-act race condition. I don't think thread safety is needed here, because my understanding is that meter() will only be called once. If it's only called once, please remove done. If there are cases where it is called multiple times in different threads, fix the race condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense… I can't find anything definitive on the 3.0 specs that only one of the three methods will ever be called, so I'll rather err on the safe side by fixing the race.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for async requests in MetricsFilter
2 participants