Skip to content

feat: improve rate limiting logic#6780

Draft
MounirDhahri wants to merge 2 commits intomainfrom
chore/adjust-rate-limiting-logic
Draft

feat: improve rate limiting logic#6780
MounirDhahri wants to merge 2 commits intomainfrom
chore/adjust-rate-limiting-logic

Conversation

@MounirDhahri
Copy link
Member

@MounirDhahri MounirDhahri commented May 23, 2025

POC

Description:
After the incident today, we noticed that we have a rate limiting middleware that is not being used in MP. We enabled it shortly in staging, but it wasn't working fine although the limit was 60 requests per minute (we were doing a few hundreds of requests per minute).
The rate limiting kicks in by default based on the user ip. This PR adjusts it to be a combination of IP + queried interfaces. So the identifier will be something like

123.23.413.4:query:artist
123.23.413.4:query:artwork
123.23.413.4:query:artist,query:artwork

This could theoretically help us protect further our interfaces.

What comes next?
We need to try out what is a good rate limit here, we can try it out in staging first. We can set it as below:

hokusai staging env set RATE_LIMIT_MAX=60
hokusai staging env set RATE_LIMIT_WINDOW_MS=60000
hokusai staging refresh

Example of this in practice and how the identifier will look like

Screen.Recording.2025-05-23.at.17.15.16.mov

@artsy-peril
Copy link
Contributor

artsy-peril bot commented May 23, 2025


Warnings
⚠️ The V2 schema in this PR has breaking changes with Force. Remember to update the Force schema if necessary.

Field 'totalListPrice' was removed from object type 'Order'
Input field 'async' was removed from input object type 'CreateArtworkImportInput'
Input field 'parseWithAI' was removed from input object type 'CreateArtworkImportInput'
Field 'queued' was removed from object type 'CreateArtworkImportSuccess'

Generated by 🚫 dangerJS against fd221a1

Copy link
Member Author

Choose a reason for hiding this comment

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

This was generated by o3 for the sake of trying this out over zoom, it might need to be improved if we decide to take this appraoch

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks amazing! Only thing (though can't get around it if we want to use attributes of the query in generating the key) is adding the work of having to parse the query into an AST for every request now - that might be surprisingly slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, that's a valid concern. It would be great to avoid this and generate a key that is easier to generate based on the query. I went ahead with this approach because I believe it's something already happening under the hood when we parse the query.

@MounirDhahri MounirDhahri marked this pull request as draft May 23, 2025 15:24
@MounirDhahri
Copy link
Member Author

We tried this out on local Force and it doesn't play well with the prefetching since we trigger a bunch of queries to the same endpoint. The same thing applies also to Eigen where we also do prefetching (both can be disabled without much effort).

Maybe the limit that I set was too little, maybe the id should be also part of the unique identifier 🤷

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants