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

WIP: Bot: support bubbling up rate limit info #573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mistydemeo
Copy link
Contributor

Summary

This exposes Slack rate-limiting information to Hubot. This allows custom scripts to react to rate-limiting, for example to slow themselves down or to report to an alerting channel that a rate-limiting event has occurred.

Requirements (place an x in each [ ])

This exposes Slack rate-limiting information to Hubot.
This allows custom scripts to react to rate-limiting, for example
to slow themselves down or to report to an alerting channel that
a rate-limiting event has occurred.
@CLAassistant
Copy link

CLAassistant commented Aug 2, 2019

CLA assistant check
All committers have signed the CLA.

if error.code isnt -1
@robot.emit "error", error
else
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find any official documentation of the rate-limiting code, but this test indicates that code -1 is reliably rate limiting: https://github.com/slackapi/hubot-slack/blob/master/test/bot.coffee#L168-L171

@mistydemeo
Copy link
Contributor Author

/cc @aoberoi

src/bot.coffee Outdated Show resolved Hide resolved
Co-Authored-By: Nina Kaufman <rnkaufman@github.com>
@clavin
Copy link
Contributor

clavin commented Aug 5, 2019

🤔 The Node v4.2 build is mysteriously failing while npm is installing. I'm going to add some debugging (-dd to npm install) to the CI config for this branch (🙇‍♀ thank you for letting maintainers make changes) to see if it'll give any hints.

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #573 into master will increase coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
+ Coverage   85.03%   85.19%   +0.15%     
==========================================
  Files           6        6              
  Lines         381      385       +4     
  Branches       85       85              
==========================================
+ Hits          324      328       +4     
  Misses         33       33              
  Partials       24       24
Impacted Files Coverage Δ
src/bot.coffee 74.52% <100%> (+0.99%) ⬆️

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 ed96aca...ec3b4c8. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 5, 2019

Codecov Report

Merging #573 (ec3b4c8) into main (ed96aca) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
- Coverage   85.03%   85.00%   -0.04%     
==========================================
  Files           6        6              
  Lines         381      500     +119     
  Branches       85      129      +44     
==========================================
+ Hits          324      425     +101     
- Misses         33       45      +12     
- Partials       24       30       +6     
Impacted Files Coverage Δ
src/bot.coffee 77.65% <100.00%> (+4.13%) ⬆️
src/client.coffee 91.54% <0.00%> (-0.20%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mistydemeo
Copy link
Contributor Author

@clavin Looks like it passed this time. ¯_(ツ)_/¯

@mistydemeo
Copy link
Contributor Author

Checking in again about this again; we need this to be able to track metrics on how often we're rate-limited, which helps us measure our Hubot SLOs internally.

cc @aoberoi, @seratch

@seratch
Copy link
Member

seratch commented Apr 10, 2020

@mistydemeo Thanks for following up. I can take a look at this today.

BTW, I know you mentioned that your employer signed the CLA in another PR #567 (comment)
but it'd be greatly appreciated if you could sign the CLA as an individual. That'd be much easier for others to understand the situation.

@mistydemeo
Copy link
Contributor Author

Went ahead and signed the individual CLA.

@@ -39,6 +41,7 @@ class SlackBot extends Adapter
@client.rtm.on "close", @close
@client.rtm.on "disconnect", @disconnect
@client.rtm.on "error", @error
@client.web.on Events.WEB.RATE_LIMITED, @rate_limited
Copy link
Member

@seratch seratch Apr 10, 2020

Choose a reason for hiding this comment

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

@mistydemeo Here is a sample I used for verifying this implementation.

module.exports = (robot) ->
  web = robot.adapter.client.web
  
  robot.hear /call a lot/i, (res) ->
    for x in [1..100]
      web.conversations.list().then (res) ->
        robot.logger.info "#{res.ok} #{res.error}"

  robot.on 'slack-rate-limit', (res) ->
    robot.logger.info "Detected a rate-limited call #{res}"

Does this really work for you? It doesn't work for me. The following is the error I get every time the rate_limited method is called. I haven't investigated details yet but it seems @robot is undefined in any case.

[Fri Apr 10 2020 13:05:29 GMT+0900 (Japan Standard Time)] ERROR TypeError: Cannot read property 'emit' of undefined
  at WebAPIClient.SlackBot.rate_limited (/path-to-project/node_modules/hubot-slack/src/bot.coffee:217:5, <js>:276:25)
  at WebAPIClient.emit (/path-to-project/node_modules/hubot-slack/node_modules/eventemitter3/index.js:116:35)
  at handleRateLimitResponse (/path-to-project/node_modules/hubot-slack/node_modules/@slack/client/lib/clients/transports/call-transport.js:51:10)
  at handleTransportResponse (/path-to-project/node_modules/hubot-slack/node_modules/@slack/client/lib/clients/transports/call-transport.js:148:21)

Also, to utilize this, you need to always use robot.adapter.client.web. If some of your scripts initialize WebClient separately (also, that's the way this adapter currently recommends), rate-limited errors those clients get won't be notified here.

Correct me if my understanding is wrong.

@seratch
Copy link
Member

seratch commented Apr 10, 2020

@mistydemeo
As I mentioned above, I'm still not sure if this change works for your cases yet. Also, if I understand it correctly, there is one limitation where rate-limited errors that happened outside of the bot instance won't be notified.

Although I'm not sure if this is realistic for your use case yet, is it possible to consider using Events API for this purpose? Events API has an event for it: https://api.slack.com/events/app_rate_limited

That'll be a much easier way to detect the errors. However, to use Events API along with RTM bot, you need to have a public endpoint to receive incoming requests from Slack API. Also, if you have been using Hubot integrations, you need to migrate those into "classic" Slack apps. (FYI: the default way is no longer the "classic" model but still you can create from https://api.slack.com/apps?new_classic_app=1 ).

That said, I'm happy to help you out in the short-term by merging this PR once I verify it works.

@seratch seratch changed the title Bot: support bubbling up rate limit info WIP: Bot: support bubbling up rate limit info Apr 24, 2020
@clavin clavin closed this Jul 3, 2020
@clavin
Copy link
Contributor

clavin commented Jul 3, 2020

Eek! Sorry 🙇!! I accidentally deleted the old default branch without changing the target of this PR and it automatically closed this PR! And I couldn't change it unless I cloned the old branch and pushed it again first! 😱 Sorry for the commotion!

@clavin clavin reopened this Jul 3, 2020
@clavin clavin changed the base branch from master to main July 3, 2020 05:51
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.

None yet

5 participants