[OPIK-68] Add rate limit #261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nimrod007
requested changes
Sep 19, 2024
apps/opik-backend/src/test/java/com/comet/opik/infrastructure/ratelimit/RateLimitE2ETest.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/redis/RedisRateLimitService.java
Outdated
Show resolved
Hide resolved
thiagohora
force-pushed
the
thiagohora/OPIK-68_add_rate_limit
branch
3 times, most recently
from
2e312b4
to
9e182bb
Compare
Nimrod007
previously approved these changes
Sep 19, 2024
…m/comet-ml/opik into thiagohora/OPIK-68_add_rate_limit
thiagohora
force-pushed
the
thiagohora/OPIK-68_add_rate_limit
branch
from
9e182bb
to
1f35a9d
Compare
andrescrz
previously approved these changes
Sep 20, 2024
There was a problem hiding this comment.
In general, very well done!!! This is fine to go for the general case. But I left some comments as I believe this should have follow-up PRs in order to cover a few gaps. Also, there's one question that we need to decide and agree for the case of failed requests.
...opik-backend/src/main/java/com/comet/opik/infrastructure/ratelimit/RateLimitInterceptor.java
Outdated
Show resolved
Hide resolved
...opik-backend/src/main/java/com/comet/opik/infrastructure/ratelimit/RateLimitInterceptor.java
Outdated
Show resolved
Hide resolved
...opik-backend/src/main/java/com/comet/opik/infrastructure/ratelimit/RateLimitInterceptor.java
Outdated
Show resolved
Hide resolved
...opik-backend/src/main/java/com/comet/opik/infrastructure/ratelimit/RateLimitInterceptor.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/infrastructure/redis/RedisRateLimitService.java
Outdated
Show resolved
Hide resolved
...opik-backend/src/main/java/com/comet/opik/infrastructure/ratelimit/RateLimitInterceptor.java
Outdated
Show resolved
Hide resolved
…m/comet-ml/opik into thiagohora/OPIK-68_add_rate_limit
Nimrod007
previously approved these changes
Sep 20, 2024
…m/comet-ml/opik into thiagohora/OPIK-68_add_rate_limit
|
Sorry, I found a typo |
Nimrod007
approved these changes
Sep 20, 2024
dsblank
pushed a commit
that referenced
this pull request
Oct 4, 2024
* [OPIK-68] Add rate limit * Add tests * Add batch endpoint rate limit * Add new rate limit headers * Address PR feedback * Address PR feedback
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Add rate limit functionality.
Added
@RateLimitedannotation to apply rate limit to an endpoint.The following properties control the limit buckets (For now, only one
generalEvents)In the case of batch operations, an interface called
RateEventContainerallows the interceptor to know the number of items in the batch.When enabled, the rate limit is communicated via headers and status 429 (TOO_MANY_REQUESTS) when the limit is reached.
Issues
OPIK-68
Testing
Tested locally via Minikube setup and also via Component Tests in the pipeline.