Skip to content

feat: add a control for rate limiting - #13

Open
TheFiordi wants to merge 11 commits into
gtuk:mainfrom
TheFiordi:ratelimiter
Open

feat: add a control for rate limiting#13
TheFiordi wants to merge 11 commits into
gtuk:mainfrom
TheFiordi:ratelimiter

Conversation

@TheFiordi

Copy link
Copy Markdown

Good morning, Discord implements a system for rate limiting post requests to webhooks end point, if the EP is hit too many times too fast it will block the requests and the messages sent are lost, if an ip is repeatedly rate limited discord will ban the IP.

Discord answers with the following headers:

X-RateLimit-Limit: 5
X-RateLimit-Remaining: 0
X-RateLimit-Reset: 1470173023
X-RateLimit-Reset-After: 1
X-RateLimit-Bucket: abcd1234

doc: https://discord.com/developers/docs/topics/rate-limits

I'm proposing to use the X-RateLimit-Reset-After to wait for the rate limit to be reset before sending another request.

This is a naive approach as the rate limits are per webhook and not global (there's also a global rate limit but afaik is only for bots).

I'm very open to any criticism or suggestion as this is what I'm using for my application.
Also, this is my very first pull request!!

@gtuk gtuk left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@TheFiordi Thanks for your PR, really appreciated. I left some comment

Comment thread types.go Outdated
Content *string `json:"content,omitempty"`
Embeds *[]Embed `json:"embeds,omitempty"`
AllowedMentions *AllowedMentions `json:"allowed_mentions,omitempty"`
Timestamp *time.Time `json:"timestamp,omitempty"`

@gtuk gtuk Nov 8, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What is the exact purpose of this new field?

@TheFiordi TheFiordi Nov 8, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oops my bad I left it there, it should be moved in type embed struct {}, see #12

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

As this is not relevant for this MR i would remove it completely for now

@TheFiordi

Copy link
Copy Markdown
Author

Hi, I removed the timestamp from the type.go file, added a comment and handled the resp.Body.Close() better.

@gtuk

gtuk commented Nov 10, 2023

Copy link
Copy Markdown
Owner

As this is MR is about enhancing the the docs / examples, I would suggest to move the Rate limit aware example into a separate example function e.g SendMessageRateLimitAware or something similar and leave the basic example as it is as this is not really necessary for people who want to use the library in a really basic way

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