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

Rate limit by request time #18

Closed
lchayoun opened this issue Sep 14, 2017 · 10 comments
Closed

Rate limit by request time #18

lchayoun opened this issue Sep 14, 2017 · 10 comments
Assignees

Comments

@lchayoun
Copy link
Collaborator

we have seen cases that rate limit by request number is not enough
sometimes a single request can be long and load the gateway

adding an option to measure the request time and add another limit option by time quetta might help in such cases

@marcosbarbero do you think it's a good idea?

@marcosbarbero
Copy link
Owner

Sounds good, can you elaborate a little bit more about what you have in mind?

@lchayoun
Copy link
Collaborator Author

I was thinking of a new parameter to the Policy class called "quota" which will behave as follow:

  1. The user will define the quota (optional), limit will be optional as well, one of them has to be defined
  2. In the RateLimitFilter will save the current time to the request attributes (if quota is defined)
  3. A new "post" filter will be added, the filter will check if the attribute of the start time exists, if so the filter will calculate the elapsed time and reduce it from the quota
  4. If the quota exceeded it will reject the new requests

This will help us differentiate the requests weight on the system.
The only issue is that the the time is calculated at the end of each request, so meantime new requests can enter the system.

I think it will add some power to the rate limit mechanism that can be enhanced further

@marcosbarbero
Copy link
Owner

Thanks for the info @lchayoun.

It's clear for me now, the only concern that I've you already pointed it's about the calculation being done in a later phase meaning new requests can jump in even thou the rate limit capacity has already being reached.

@lchayoun
Copy link
Collaborator Author

I agree, but with a solid timeout I think it will be strong enough, do you want me to create a pull request on it?

@marcosbarbero
Copy link
Owner

@lchayoun if you have time it would be awesome :)

@marcosbarbero
Copy link
Owner

@lchayoun were you able to compile the project locally with your changes?
I'm facing some issues regarding consul implementation.

com.fasterxml.jackson.core.JsonParseException: Unrecognized token 'eyJrZXkiOiJyYXRlLWxpbWl0LWFwcGxpY2F0aW9uOnNlcnZpY2VEOi9zZXJ2aWNlRC9mNzA1NjI2NS1hNThiLTRjNmUtOTliMi04NTI3ZjIxMTM4NDAiLCJyZW1haW5pbmciOjEsInJlbWFpbmluZ1F1b3RhIjpudWxsLCJyZXNldCI6NjAwMDAsImV4cGlyYXRpb24iOiIyMC0wOS0yMDE3IDA4OjAyOjE4In0': was expecting ('true', 'false' or 'null')
 at [Source: eyJrZXkiOiJyYXRlLWxpbWl0LWFwcGxpY2F0aW9uOnNlcnZpY2VEOi9zZXJ2aWNlRC9mNzA1NjI2NS1hNThiLTRjNmUtOTliMi04NTI3ZjIxMTM4NDAiLCJyZW1haW5pbmciOjEsInJlbWFpbmluZ1F1b3RhIjpudWxsLCJyZXNldCI6NjAwMDAsImV4cGlyYXRpb24iOiIyMC0wOS0yMDE3IDA4OjAyOjE4In0=; line: 1, column: 232]
        at com.fasterxml.jackson.core.JsonParser._constructError(JsonParser.java:1702) ~[jackson-core-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:558) ~[jackson-core-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.core.json.ReaderBasedJsonParser._reportInvalidToken(ReaderBasedJsonParser.java:2839) ~[jackson-core-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.core.json.ReaderBasedJsonParser._handleOddValue(ReaderBasedJsonParser.java:1903) ~[jackson-core-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.core.json.ReaderBasedJsonParser.nextToken(ReaderBasedJsonParser.java:749) ~[jackson-core-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.databind.ObjectMapper._initForReading(ObjectMapper.java:3850) ~[jackson-databind-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:3799) ~[jackson-databind-2.8.9.jar:2.8.9]
        at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2858) ~[jackson-databind-2.8.9.jar:2.8.9]
        at com.marcosbarbero.cloud.autoconfigure.zuul.ratelimit.config.repository.ConsulRateLimiter.getRate(ConsulRateLimiter.java:49) ~[spring-cloud-zuul-ratelimit-core-1.3.2.RELEASE.jar:1.3.2.RELEASE]
        at com.marcosbarbero.cloud.autoconfigure.zuul.ratelimit.config.repository.AbstractRateLimiter.create(AbstractRateLimiter.java:31) [spring-cloud-zuul-ratelimit-core-1.3.2.RELEASE.jar:1.3.2.RELEASE]
        at com.marcosbarbero.cloud.autoconfigure.zuul.ratelimit.config.repository.AbstractRateLimiter.consume(AbstractRateLimiter.java:24) [spring-cloud-zuul-ratelimit-core-1.3.2.RELEASE.jar:1.3.2.RELEASE]
        at com.marcosbarbero.cloud.autoconfigure.zuul.ratelimit.filters.RateLimitPreFilter.lambda$run$0(RateLimitPreFilter.java:75) [spring-cloud-zuul-ratelimit-core-1.3.2.RELEASE.jar:1.3.2.RELEASE]
        at java.util.Optional.ifPresent(Optional.java:159) ~[na:1.8.0_131]

I believe it's related to ConsulRateLimiter#getRate(String) at this line:

rate = this.objectMapper.readValue(value.getValue(), Rate.class);

@marcosbarbero marcosbarbero reopened this Sep 20, 2017
@lchayoun
Copy link
Collaborator Author

lchayoun commented Sep 20, 2017

forked it again, seeing the same issue, taking a look

@marcosbarbero
Copy link
Owner

@lchayoun thanks in a bunch

@marcosbarbero
Copy link
Owner

@lchayoun do you have time to work on few tests for post filter? It seems that the whole #run() method has no test.

If you don't have time let me know.

@marcosbarbero
Copy link
Owner

@lchayoun here we go v1.3.3.RELEASE.

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

No branches or pull requests

2 participants