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

Limit the maximum rate of requests per IP #614

Merged
merged 2 commits into from May 16, 2016
Merged

Conversation

milimetric
Copy link
Contributor

Configure the metrics endpoints to filter out requests that happen more
than X times per second from the same IP to the same endpoint. The
exact number is not decided yet but I'm tentatively setting it at 100.

Right now, the filter is just logging if the limit is exceeded so we can
get a feel for how often this happens. We may decide to make it a real
filter as part of this PR or later.

https://phabricator.wikimedia.org/T135240
Bug: T135240

Configure the metrics endpoints to filter out requests that happen more
than X times per second from the same IP to the same endpoint.  The
exact number is not decided yet but I'm tentatively setting it at 100.

Right now, the filter is just logging if the limit is exceeded so we can
get a feel for how often this happens.  We may decide to make it a real
filter as part of this PR or later.

https://phabricator.wikimedia.org/T135240
Bug: T135240
@milimetric
Copy link
Contributor Author

Let us know if this is ok, but we should probably talk tomorrow to see if others agree with doing logOnly at first.

x-request-handler:
- get_from_backend:
request:
uri: '{{options.host}}/pageviews/aggregate/{project}/{access}/{agent}/{granularity}/{start}/{end}'
headers:
cache-control: '{{cache-control}}'
if-unmodified-since: '{{if-unmodified-since}}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, how would AQS use the cache-control and if-unmodified-since headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just thinking they'd be convenient to have in general, but maybe you're right, maybe it doesn't make sense at our layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general we use cache-control to force a rerender, and if-unmodified-since to skip a forced rerender if requests come out of order. Since AQS data is practically immutable, you can't rerender, so unlikely you need them.

@gwicke
Copy link
Member

gwicke commented May 16, 2016

LGTM overall. Some of the forwarded headers (cache-control and if-unmodified-since) probably do nothing for you right now.

@milimetric
Copy link
Contributor Author

I agree, I'll get those out, since there's no immediate use I can think of.

@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage remained the same at 93.543% when pulling 58f7909 on milimetric:master into 1363f1f on wikimedia:master.

@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage remained the same at 93.543% when pulling 58f7909 on milimetric:master into 1363f1f on wikimedia:master.

@gwicke gwicke merged commit 0880155 into wikimedia:master May 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants