-
Notifications
You must be signed in to change notification settings - Fork 126
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
Limit the number of ping upload requests per minute #974
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.
I'm fine with this approach, but I'd like @badboy input on it as well.
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.
Indeed that looks like a good start.
I have some comments, but not much against the approach by itself.
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.
This looks good to me overall. Before merging this, can we please add a documentation page describing it? I'm fine with it living within the Glean internal docs. I'd like to see some documentation explaining:
- the overview of how this is supposed to work;
- how are we enforcing the rate limitation;
- what are the default choices.
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.
Looks good, see one comment.
Once we got the docs that Dexter asked for we should be good to go.
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.
Great work! r+wc :)
Related to Bug 1543612
I wanted to get some feedback on my approach to this problem before I continued. My goal here was to try and replicate what Firefox does to rate limit ping uploads, specifically:
This turned out to be tricky. The challenges on the current implementation and how I decided to solve them are:
get_upload_task
can be called per minute.NOTE: I am aware this breaks the ffi code and all the bindings, I'll deal with that after.