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: Emit debugging events #567

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

Conversation

mistydemeo
Copy link
Contributor

Summary

My company's Hubot has been facing some repeated rate-limiting issues, and we've found that hubot-slack has been missing the diagnostics we need to be able to figure out the cause. This adds some new event emitters to let user scripts listen in for these events. So far I've added emitters for every message that's posted, and whenever a rate limit occurs.

An open question for me on the latter is whether there's any other place that rate limiting might come up, and whether there's any detail that I can include in the slack-rate-limit event in order to allow consumers to gather meaningful, actionable data.

cc @aoberoi

Requirements (place an x in each [ ])

@CLAassistant
Copy link

CLAassistant commented Jun 19, 2019

CLA assistant check
All committers have signed the CLA.

@mistydemeo
Copy link
Contributor Author

My employer has signed a CLA.

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #567 (70c7c0b) into main (362885b) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #567      +/-   ##
==========================================
+ Coverage   85.44%   85.60%   +0.15%     
==========================================
  Files           6        6              
  Lines         378      382       +4     
  Branches       85       85              
==========================================
+ Hits          323      327       +4     
  Misses         31       31              
  Partials       24       24              
Impacted Files Coverage Δ
src/bot.coffee 75.00% <100.00%> (+0.25%) ⬆️
src/client.coffee 91.96% <100.00%> (+0.22%) ⬆️

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

@mistydemeo
Copy link
Contributor Author

We are now using this in production and it's been extremely helpful for us. I'd love to get this into master!

@@ -200,6 +200,8 @@ class SlackBot extends Adapter
# NOTE: should rate limit errors also bubble up?
if error.code isnt -1
@robot.emit "error", error
else
@robot.emit "slack-rate-limit"
Copy link
Contributor

Choose a reason for hiding this comment

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

it took quite a bit of detective work, but it turns out that we don't know of any code that triggers the "error" event on the RtmClient, which is the only code path that results in this method ever getting called.

it seems like the comparison of code to -1 is remnants from the v3.x series of hubot-slack, which used the v1.x series of @slack/client, which would have emitted a "error" event.

therefore, i don't think this code is worth merging, because it would just become even more misleading.

@@ -192,10 +192,13 @@ class SlackClient
thread_ts: envelope.message?.thread_ts

if typeof message isnt "string"
@web.chat.postMessage(room, message.text, _.defaults(message, options))
messageOptions = _.defaults(message, options)
@robot.emit "postMessage", room, message.text, messageOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

if the goal is just to understand when chat.postMessage is called, then i don't think emitting on the robot is appropriate.

instead, i think the goal should be to turn on more logging, specifically for the WebClient. currently, there's a pattern established to set more options on the RtmClient using the HUBOT_SLACK_RTM_CLIENT_OPTS environment variable. so an alternative that i would favor is to introduce a HUBOT_SLACK_WEB_CLIENT_OPTS environment variable that can be used to set the logLevel to "debug" (and any of the other options).

what do you think?

@mistydemeo
Copy link
Contributor Author

instead, i think the goal should be to turn on more logging, specifically for the WebClient.

Logging is actually not great for us! We were specifically hooking into this in order to send stats to our stats monitoring service. Parsing logs is inconvenient, but if it's emitted as an event then our user code can listen for it and emit metrics when they're received.

@aoberoi
Copy link
Contributor

aoberoi commented Jul 30, 2019

got it! so the use case is specifically to gather data for a monitoring service.

i think Hubot already provides an abstraction designed for the use case where you want to run some code before (or after) anything is sent by the adapter: response middleware.

would that be sufficient for your use case?

@seratch seratch changed the title Emit debugging events WIP: Emit debugging events Apr 24, 2020
@clavin clavin changed the base branch from master to main July 3, 2020 05:44
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.

3 participants