Make requests emit metrics when rate limited #423

andrew4699 · 2024-11-05T01:17:36Z

Description

Makes requests emit metrics when they get rate limited.

There were 2 issues preventing this:

RateLimiterFilter ran outside the Jersey Servlet, which is before application events get sent
Changing it to run inside the servlet isn't enough. Aborted requests don't emit the RESOURCE_METHOD_START event.

This PR makes the rate limiting filter a Jersey Servlet filter and makes the per-request metric emitter emit count metrics for requests that have a REQUEST_MATCHED But not RESOURCE_METHOD_START event.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

See the test that verifies emitted metrics

Checklist:

Please delete options that are not relevant.

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue

andrew4699 · 2024-11-05T01:20:42Z

polaris-service/src/main/java/org/apache/polaris/service/ratelimiter/RateLimiterFilter.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 /** Request filter that returns a 429 Too Many Requests if the rate limiter says so */
-@Priority(Priorities.AUTHORIZATION + 1)


Fun fact: this actually did nothing previously. It's a lucky coincidence that this ran after the ContextResolverFilter due to addFilter ordering.

Removing this moves it back to USER right?

Yes. Previously I don't think it did anything because it was outside Jersey. Also we don't seem to have any Jersey filters so the priority doesn't really matter currently.

eric-maynard · 2024-11-05T08:25:50Z

polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java

-      environment
-          .servlets()
-          .addFilter("ratelimiter", new RateLimiterFilter(configuration.getRateLimiter()))
-          .addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true, "/*");
+      environment.jersey().register(new RateLimiterFilter(configuration.getRateLimiter()));


This seems like a semantic change unrelated to the metrics. If I'm reading this correctly, this changes the rate limiter to only apply to Jersey resources. Do we want that?

Yes because TimedApplicationEventListener, which is what emits metrics on requests, depends on Jersey events.

I get that, but my concern is whether this actually changes the behavior of the system by rate-limiting less things now

(Please fact check me as I only started working with Java web services recently)

I think all of our endpoints are in Jersey, so it shouldn't change the scope, but at a minimum it does push rate limiting further into the request handling chain.

The fundamental problem seems to be that only Jersey knows how to map a request path to its handling method (and thus its metric name). If that mapping was available everywhere, most of TimedApplicationEventListener could be re-written as a top level filter.

Semantically, there is a difference, as ServletFilters are applied before Jersey Filters. Practically, however, it shouldn't have an impact. I think the only real dependency the RateLimiter might have is that the RealmContext is set in case there are different rate limits per realm. Since the ContextResolverFilter still applies first, we shouldn't see a practical difference.

jbonofre · 2024-11-05T14:11:55Z

polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java

-          .servlets()
-          .addFilter("ratelimiter", new RateLimiterFilter(configuration.getRateLimiter()))
-          .addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true, "/*");
+      environment.jersey().register(new RateLimiterFilter(configuration.getRateLimiter()));


This change is unrelated to metrics, right ?

See #423 (comment)

jbonofre · 2024-11-05T14:12:45Z

polaris-service/src/main/java/org/apache/polaris/service/TimedApplicationEventListener.java

@@ -66,16 +66,16 @@ private class TimedRequestEventListener implements RequestEventListener {
    @Override
    public void onEvent(RequestEvent event) {
      String realmId = CallContext.getCurrentContext().getRealmContext().getRealmIdentifier();
-      if (event.getType() == RequestEvent.Type.RESOURCE_METHOD_START) {
+      if (event.getType() == RequestEvent.Type.REQUEST_MATCHED) {


FYI, this listener to @TimedApi (populated in mustache template) is not required with Quarkus.

andrew4699 · 2024-11-06T22:16:29Z

polaris-service/src/test/java/org/apache/polaris/service/ratelimiter/RateLimiterFilterTest.java

+            > 0);
+  }
+
+  private double getCounter(String metricName) {


Will replace this with https://github.com/apache/polaris/blob/cfd908ad3bc7527d5fbd2127f8eb7bce54a0acbd/polaris-service/src/test/java/org/apache/polaris/service/test/TestMetricsUtil.java once my other PR is merged

collado-mike · 2024-11-06T23:27:24Z

polaris-service/src/main/java/org/apache/polaris/service/PolarisApplication.java

-      environment
-          .servlets()
-          .addFilter("ratelimiter", new RateLimiterFilter(configuration.getRateLimiter()))
-          .addMappingForUrlPatterns(EnumSet.of(DispatcherType.REQUEST), true, "/*");
+      environment.jersey().register(new RateLimiterFilter(configuration.getRateLimiter()));


Semantically, there is a difference, as ServletFilters are applied before Jersey Filters. Practically, however, it shouldn't have an impact. I think the only real dependency the RateLimiter might have is that the RealmContext is set in case there are different rate limits per realm. Since the ContextResolverFilter still applies first, we shouldn't see a practical difference.

Make requests emit metrics when rate limited

9a2c537

andrew4699 requested review from jbonofre, ashvina, RussellSpitzer, snazy, vvcephei, takidau, jackye1995, flyrain, eric-maynard, collado-mike and ebyhr as code owners November 5, 2024 01:17

andrew4699 commented Nov 5, 2024

View reviewed changes

eric-maynard reviewed Nov 5, 2024

View reviewed changes

jbonofre reviewed Nov 5, 2024

View reviewed changes

andrew4699 requested review from jbonofre and eric-maynard November 5, 2024 18:07

andrew4699 commented Nov 6, 2024

View reviewed changes

collado-mike approved these changes Nov 6, 2024

View reviewed changes

Merge branch 'main' into aguterman-rate-limiter-metrics

8b78b32

eric-maynard requested a review from adutra as a code owner November 8, 2024 17:19

Merge branch 'main' into aguterman-rate-limiter-metrics

4a1b16f

eric-maynard enabled auto-merge (squash) November 12, 2024 18:01

eric-maynard merged commit 455423d into apache:main Nov 12, 2024
5 checks passed

andrew4699 deleted the aguterman-rate-limiter-metrics branch November 12, 2024 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make requests emit metrics when rate limited #423

Make requests emit metrics when rate limited #423

andrew4699 commented Nov 5, 2024 •

edited

Loading

andrew4699 Nov 5, 2024

eric-maynard Nov 5, 2024

andrew4699 Nov 5, 2024 •

edited

Loading

eric-maynard Nov 5, 2024 •

edited

Loading

andrew4699 Nov 5, 2024

eric-maynard Nov 5, 2024

andrew4699 Nov 5, 2024 •

edited

Loading

collado-mike Nov 6, 2024

jbonofre Nov 5, 2024

andrew4699 Nov 5, 2024

jbonofre Nov 5, 2024

andrew4699 Nov 6, 2024

collado-mike Nov 6, 2024

Make requests emit metrics when rate limited #423

Make requests emit metrics when rate limited #423

Conversation

andrew4699 commented Nov 5, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew4699 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

eric-maynard Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew4699 Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew4699 commented Nov 5, 2024 •

edited

Loading

andrew4699 Nov 5, 2024 •

edited

Loading

eric-maynard Nov 5, 2024 •

edited

Loading

andrew4699 Nov 5, 2024 •

edited

Loading