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

Add API Error handling and local rate-limiting #53

Merged
merged 10 commits into from
Feb 18, 2018
Merged

Add API Error handling and local rate-limiting #53

merged 10 commits into from
Feb 18, 2018

Conversation

hmmwhatsthisdo
Copy link
Contributor

@hmmwhatsthisdo hmmwhatsthisdo commented Feb 16, 2018

This PR covers two separate, yet two related issues:

  • PSSlack does not gracefully handle errors from the Slack API (Some of these come with 4xx/5xx HTTP status codes, meaning Invoke-RestMethod throws an exception)
  • PSSlack does not respect Slack's API rate-limiting (see Rate-Limiting is not handled gracefully #52)

By adding in a basic error-handler on the API - one that handles both 4xx/5xx errors from the API as well as results where ok is $false - we can handle rate-limiting a little better.

Local rate-limiting has been implemented behind a switch (-RateLimit) on Send-SlackApi. This uses the Leaky Bucket algorithm to allow for 25 burst requests, with 1 second between requests otherwise. This seems to be very similar to Slack's rate-limit parameters, and will automatically recover from rate-limit errors returned by Slack's API.

This PR should resolve #52.

@RamblingCookieMonster
Copy link
Owner

Awesome! At a quick glance, don't see any blockers - will poke around tomorrow or this weekend and merge it in.

At some point, it might be handy to:

  • Add the param to other functions that might trigger rate limiting, pass on to send-slackapi
  • Add a RateLimit option to Set/Get PSSlackConfig

Thanks again, this will be quite handy!

@hmmwhatsthisdo
Copy link
Contributor Author

hmmwhatsthisdo commented Feb 16, 2018

Add the param to other functions that might trigger rate limiting, pass on to send-slackapi

Right - now that Remove-SlackMessage has been merged in, I suppose it'd be a good idea to set it on that function.

Add a RateLimit option to Set/Get PSSlackConfig

I briefly considered this - however, I'm unsure what would be best here from a usability perspective. Perhaps a config flag to globally disable rate-limiting?

@RamblingCookieMonster
Copy link
Owner

Alrighty, ran through again, this looks good to me, and no one else has chimed in, merging! Thanks again for taking the time to add this!

@RamblingCookieMonster
Copy link
Owner

I briefly considered this - however, I'm unsure what would be best here from a usability perspective. Perhaps a config flag to globally disable rate-limiting?

Not sure on specifics. Maybe:

  • A -RateLimit bool or switch - reflects same parameter name
  • Default to not present, allowing us to use it as default value for functions that are affected by rate limiting
  • Avoid as default on Send-SlackApi, unless we know it applies to everything Send-SlackApi can hit

Open to other options though - cheers!

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.

Rate-Limiting is not handled gracefully
2 participants