Skip to content
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

feat: add ratelimit support #68

Merged
merged 8 commits into from
Feb 21, 2020

Conversation

awj
Copy link
Contributor

@awj awj commented Nov 4, 2017

Fixes

Partial support for Rate limit handling as described in sendgrid/sendgrid-ruby#119. Adds functionality to assist users in handling rate limiting. Without more feedback on goals for any kind of in-client automatic rate limiting I'm not able to address that any further.

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the master branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Add a SendGrid::Response::Ratelimit class to gather up ratelimit-related functionality/behavior
  • Expose a Ratelimit instance as a property of SendGrid::Response

SendGrid::Response::Ratelimit instance for that specific response.

Add appropriate docs.
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 4, 2017
@SendGridDX
Copy link

SendGridDX commented Nov 4, 2017

CLA assistant check
All committers have signed the CLA.

@thinkingserious thinkingserious added difficulty: medium fix is medium in difficulty type: twilio enhancement feature request on Twilio's roadmap labels Mar 6, 2018
@awj
Copy link
Contributor Author

awj commented Oct 20, 2018

@thinkingserious Getting back into Hacktoberfest and I can see that this slipped through the cracks from last time. I've merged latest master to it and touched up some style violations, looks like it's good to go!

@javmorin
Copy link

javmorin commented Apr 3, 2019

Encountering this issue as well, would really appreciate official support for the rate limiting behavior so as to mitigate the thundering herd problem which can currently happen.

@cseeman
Copy link

cseeman commented Nov 11, 2019

I'm starting to do some implementation with the Ruby gem and calling out to the SendGrid APIs (specifically subusers and white label) and would really appreciate this feature. Thank you so much for explaining what the X-RateLimit-Reset number is in your comment @awj

The time (in seconds since Unix Epoch) when the rate limit will reset

I was confused on what that number was supposed to represent. I did have one question @awj, why were you setting the '@reset' instance variable and then overwriting it in the next line:
https://github.com/sendgrid/ruby-http-client/pull/68/files#diff-85d68e873667434927113a0b379b8e86R20

@awj
Copy link
Contributor Author

awj commented Nov 11, 2019

🤦‍♂ that doesn't need to be happening, and looks like some kind of editing oversight on my part. I'll get it touched up.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@52bf4a4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #68   +/-   ##
=========================================
  Coverage          ?   95.49%           
=========================================
  Files             ?        2           
  Lines             ?      333           
  Branches          ?        0           
=========================================
  Hits              ?      318           
  Misses            ?       15           
  Partials          ?        0
Impacted Files Coverage Δ
test/test_ruby_http_client.rb 97.71% <100%> (ø)
lib/ruby_http_client.rb 91.22% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52bf4a4...bcb7462. Read the comment docs.

@childish-sambino childish-sambino changed the title Feature/ratelimit support feat: add ratelimit support Feb 21, 2020
@childish-sambino childish-sambino merged commit 248b8cd into sendgrid:master Feb 21, 2020
@thinkingserious
Copy link
Contributor

Hello @awj,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

@awj awj deleted the feature/ratelimit-support branch April 29, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: medium fix is medium in difficulty status: code review request requesting a community code review or review from Twilio type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants