Initial fix #420
Initial fix #420
Conversation
Codecov Report
@@ Coverage Diff @@
## master #420 +/- ##
========================================
- Coverage 79.29% 78.5% -0.8%
========================================
Files 5 5
Lines 198 200 +2
Branches 46 47 +1
========================================
Hits 157 157
- Misses 30 32 +2
Partials 11 11
Continue to review full report at Codecov.
|
|
@aoberoi if you can give me some help on the tests it would be good to get this in. It's a bad performance hit and really unnecessary hammering on the slack api. |
|
@chapmanc thanks for investigating and the PR. i'll be able to review this by mid-week. if everything looks 🆗 , i hope to ship it before the end of the week. |
|
now that i've gotten some time to look at this (sorry!)... i think we can go a bit further here. rather than letting the adapter refresh the whole user list every hour, we only really want to do it once. the individual user updates will happen in @chapmanc if you'd like to adjust this PR to work in that way, i'd be happy to help review and merge it. if you don't have the time, let me know and i'll make sure its a P1 and we can get the fix out soon. |
|
Is there any update on this? It is impacting our team heavily as we our leveraging the brain quite a bit. |
|
I can update it but won't be for a while. Just on holidays now but will look into it when I'm back. |
|
@chapmanc if you have a moment, can you make me a collaborator on this PR and i'll finish up the changes? |
|
Hey @aoberoi I allowed edits is checked already 👍 |
|
Hey @aoberoi I have added to code to stop it loading after the initial load but since the stubs of the tests fake the brain to be an event emitter there isn't much testing to do here so the code coverage is below. I could stub out the event emitter but at that point I don't know what we are really testing. Let me know if there is something else you would rather do. |
|
Just wanted to swing by and say thanks for fixing this in v4.4.0. For the sake of future Googlers, the error message that appears in the log is |
|
This is still happening for us, so I've had to set DISABLE_USER_SYNC. Perhaps it's because we have a lot of users + it seems like, at least on heroku, hubot has to reconnect to slack every minute on the minute. |
|
@perplexes thanks for reporting. i'd like to ask for some clarifications. are you also getting the its possible that issue #479 is related. its also possible that #524 introduced a regression. |
|
@perplexes one more question: can you take a look at your Heroku app metrics to see if your memory usage is spiking and Heroku is potentially terminating your app because of that? that's one theory for why you're experiencing a reconnect at a regular interval. |
|
No memory usage spike. It still reconnects every minute, even though I've disabled user sync. And I've enabled debugging, but it doesn't help much: |
|
@aoberoi any ideas? |
|
@perplexes sorry, this fell off my radar. let's move this to a new issue so that it isn't hidden inside something that appears closed. would you mind creating an issue and mentioning this issue (by number) in it? I'm looking at the logs now and i'll try to leave any thoughts I have about next steps in the new issue. |
PR Summary
Currently the hubot slack adapter makes a call to users.list everytime we call robot.brain.set since that fires the 'loaded' event. If a call updates several properties separately then you get multiple calls to the slack api to load users.
Related Issues
For #419
Test strategy
Currently this is manually tested and after a couple of hours of trying to figure out the test framework I had to cut my losses. If someone else can write a test for this or can help me write a test that would be appreciated. Used to using sinon where I can clearly see the calls made to functions.