-
Notifications
You must be signed in to change notification settings - Fork 148
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 API Calls #502 #509
Conversation
0048fc3
to
8724da3
Compare
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.
Hey @amvanbaren! Thanks for taking time to do this.
I like the approach, it seems to work reliably and the default of 15 / sec sounds reasonable. I also like that we have the x-rate-limit-remaining
and x-rate-limit-retry-after-seconds
headers.
A couple of things I found:
- when a request to the API is rate-limited, its CORS headers are not present. I think we should keep
access-control-allow-origin *
for every request, so that clients can know what the response body is as well. - the web client seems to not recognize this response, so it's just displayed as a Network error. I think we should add this response code to the frontend:
- https://github.com/eclipse/openvsx/blob/master/webui/src/pages/admin-dashboard/extension-admin.tsx
- https://github.com/eclipse/openvsx/blob/master/webui/src/pages/admin-dashboard/extension-admin.tsx
- https://github.com/eclipse/openvsx/blob/master/webui/srct/pages/admin-dashboard/extension-admin.tsx
- https://github.com/eclipse/openvsx/blob/master/webui/src/pages/extension-detail/extension-detail.tsx
- I looked through VS Code's source code and could not find any checks for
HTTP 429
. Maybe this is worth exploring further and seeing what VS Code does in this scenario (I don't think the Microsoft Marketplace has rate-limiting in place).
I've tested this on VSCodium 1.70.1. There's no error popups, but it does break images and content isn't loaded. There's no retry logic. This is with heavy rate limiting (1 request every 15 seconds). With a rate limit of 15 requests every second, I wasn't able to hit the rate limit. A You can test this yourself, by:
|
Thanks for testing, @amvanbaren 🙏! Would it make sense to disable/increase the rate limit for just icons? So that we could really make it practically impossible to hit when using it with VS Code? |
Yes, that would make sense. Then |
if we want to adjust rate limiting in production how the process would look like? |
Then you change the bucket4j configuration in https://github.com/EclipseFdn/open-vsx.org/blob/main/configuration/application.yml and redeploy. Currently |
It sounds great, if it is nuder our control then we can start with such generous limiting and then modify it as we go 🙏 |
15 requests a second for the same client should be enough for VS Code. |
I've increased the limit for icons to 75 per second, just in case. And yes, it that is too generous you can modify it as you go. |
@amvanbaren This is excellent! We also need to think about documentation updates before we deploy this, API and deployment info. |
- capacity: 15 | ||
time: 1 | ||
unit: seconds | ||
|
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.
suggestion (non-blocking): can we add heavier rate-limiting for /sitemap.xml
? Maybe there are even more endpoints which take a look of compute to respond, but this is one that's really easy to hit and it takes sometimes even 5 seconds to generate (on https://open-vsx.org).
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.
Yes, the sitemap endpoint is heavy. It gets all active extensions from the database and turns it into a XML document. It does return caching headers to the client, but it does re-generate the document for every request. So, even if clients respect the caching response headers, the server still re-generates the sitemap for every other client that requests the sitemap.
I think it's a good idea to cache the result on the server (possibly in combination with ETag headers), so that it only re-generates the sitemap when an extension is added, removed or updated. Or maybe even just generate it once a day.
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.
Hey @amvanbaren, gave this another round of testing.
I think we can postpone my suggestion from #509 (review) to a later PR, since it's nothing blocking.
I tried the VS Code icon endpoint and it seems to work just fine as well.
Added CORS response headers to bucket4j configuration Retry request in webui if request got rate limited Abort in-flight requests when component unmounts Load extension icon through `extension-registry-service.ts`, so that it reloads if the request got rate limited. Increase rate-limit for VS Code icons # Conflicts: # server/build.gradle # server/src/main/resources/ehcache.xml # webui/src/pages/user/user-settings-namespaces.tsx
62b875a
to
72722f5
Compare
Fixes #502
Testing steps
server/src/dev/resources/application.yml
file, changecapacity
to 1 andunit
to hours./vscode
or/api
), it shouldn't get rate limited.