-
Notifications
You must be signed in to change notification settings - Fork 0
Add rate limiting support #2
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I'm wondering how I would use a middleware to update existing rate limit config based on header values for both rate limit remaining and reate limit reset time
I don't really see it as something we should be updating dynamically. You can do that by just going to the provider.rateLimiter.config but this is just really meant more to keep us from DoS ourselves due to bad reactive code. |
Our api returns the rare limit in real time, would be nice to use that. Also some routes are not rate limited. |
Yeah, that would be kind of nice to respond to that. Although, wasn't it just for posting events? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for rate limiting by introducing a new RateLimiter
class and middleware, integrating it across the provider and helper APIs, and updating documentation and tests accordingly.
- Added
RateLimiter
class andcreateRateLimitMiddleware
insrc/RateLimiter.ts
- Integrated rate limiting into
FetchClientProvider
andDefaultHelpers
- Expanded tests, exports in
mod.ts
, documentation inreadme.md
, and updated build/config files
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/RateLimiter.ts | New rate limiter implementation and middleware |
src/RateLimiter.test.ts | Unit tests for limiting logic and middleware behavior |
src/FetchClientProvider.ts | Added enableRateLimit method and provider integration |
src/FetchClient.test.ts | Updated Zod import and schema type for zod/mini |
src/DefaultHelpers.ts | Exposed global enableRateLimit helper |
scripts/build.ts | Removed importMap from build configuration |
readme.md | Documented rate limiting in README |
mod.ts | Exported RateLimitConfig and RateLimiter |
deno.json | Bumped dependency versions |
.github/copilot-instructions.md | Added Copilot instructions |
Comments suppressed due to low confidence (4)
src/RateLimiter.ts:159
- The
getTimeUntilNextRequest
method isn't covered by existing tests; add unit tests to verify that it returns correct millisecond durations and integrates with theRetry-After
header.
public getTimeUntilNextRequest(): number {
src/FetchClientProvider.ts:201
- [nitpick] Consider returning
this
fromenableRateLimit
to allow method chaining (e.g.,provider.enableRateLimit(...).useMiddleware(...)
).
public enableRateLimit(config: RateLimitConfig) {
readme.md:10
- [nitpick] The table entry uses lowercase 'Rate limiting' while the section header is 'Rate Limiting'; consider matching the capitalization for consistency.
* [Rate limiting](#rate-limiting)
blob: () => Promise.resolve(new Blob()), | ||
formData: () => Promise.resolve(new FormData()), | ||
clone: () => { | ||
throw new Error("Not implemented"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clone
method currently throws a generic error; consider implementing a proper clone or removing this unimplemented stub to avoid runtime failures if clone()
is used.
throw new Error("Not implemented"); | |
return { | |
...context.response, | |
headers: new Headers(context.response.headers), | |
problem: context.response.problem | |
? { ...context.response.problem } | |
: null, | |
json: () => Promise.resolve({ ...context.response.problem }), | |
text: () => Promise.resolve(JSON.stringify(context.response.problem)), | |
arrayBuffer: () => Promise.resolve(new ArrayBuffer(0)), | |
bytes: () => Promise.resolve(new Uint8Array()), | |
blob: () => Promise.resolve(new Blob()), | |
formData: () => Promise.resolve(new FormData()), | |
clone: context.response.clone, // Reference the same clone method | |
}; |
Copilot uses AI. Check for mistakes.
We rate limit every endpoint but a few: https://github.com/exceptionless/Exceptionless/blob/main/src/Exceptionless.Web/Utility/Handlers/ThrottlingMiddleware.cs#L107 |
Closing this one in favor of #3 |
should we delete this branch or are you looking at it? |
No description provided.