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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d921105
Bot: support bubbling up rate limit info
mistydemeo Aug 2, 2019
b374e16
Fix spacing
mistydemeo Aug 2, 2019
ec3b4c8
Move CI install to `install` & add verbosity
clavin Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ after_success:
# by the `env` specified in the matrix. That lets us test against
# multiple Hubot versions. This is a bit messy, but works around
# package.json only being able to specify one version.
script: npm install && npm install hubot@"$HUBOT_VERSION" && npm test
install: npm install && npm install -dd hubot@"$HUBOT_VERSION"
16 changes: 15 additions & 1 deletion src/bot.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
SlackClient = require "./client"
pkg = require "../package"

Events = require("@slack/client").CLIENT_EVENTS

class SlackBot extends Adapter

###*
Expand Down Expand Up @@ -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 •
edited
Loading

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.

@client.rtm.on "authenticated", @authenticated
@client.onEvent @eventHandler

Expand Down Expand Up @@ -206,9 +209,20 @@ class SlackBot extends Adapter
error: (error) =>
@robot.logger.error "Slack RTM error: #{JSON.stringify error}"
# Assume that scripts can handle slowing themselves down, all other errors are bubbled up through Hubot
# NOTE: should rate limit errors also bubble up?
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

# The actual duration of the rate limit isn't exposed. :(
@rate_limited(-1)

###*
# Bubbles up rate-limit info, allowing the bot to react
#
# @private
# @param {number seconds}
###
rate_limited: (seconds) ->
@robot.emit "slack-rate-limit", seconds

###*
# Incoming Slack event handler
Expand Down