-
Notifications
You must be signed in to change notification settings - Fork 625
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Better error handling on Zulip #1589
Conversation
6bd2763
to
e190db7
Compare
Gentle poke on this? |
@alexmv Thank you very much! and sorry for the delay. The gozulipbot vendor is uptodate, but it's against the Could you make your PR against https://github.com/matterbridge/gozulipbot/tree/work ? This PR can be closed then, let me know if that's okay for you. |
An unknown error (including an unauthorized error) would fall through with no calls to time.Sleep, resulting in hammering the server as quickly as possible. Add a 10-second sleep in the default error case. The heartbeat is left with no explicit sleep, but all other codepaths now contain one.
This will allow it to be accessed by other sections of the code.
e190db7
to
c0f9acc
Compare
I've pushed the gozulipbot-specific changes to matterbridge/gozulipbot#1; this PR shouldn't be closed, because there are changes that are not contained within the vendored gozulipbot. I've rebased, dropping the gozulipbot-only changes, and re-pushed. This is not expected to pass tests until it is rebased past a commit which pulls in the gozulipbot changes. |
Code Climate has analyzed commit 8e4906a and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Thanks! |
We noticed that matterbridge bots were making thousands of requests to Zulip servers with unauthenticated credentials. This is an attempt to stem that tide.
Specifically, it catches failures when registering, which were previously completely ignored -- most of the unauthenticated requests were to
yourserver.zulipchat.com
which is the example host in the configuration, which isn't currently a valid Zulip realm:matterbridge/matterbridge.toml.sample
Lines 1564 to 1582 in 53cafa9
It also catches the 401's when fetching events, which were not rate-limited previously.
Finally, it provides a more specific user-agent, which will make it easier to track down errors like this in the future. It does so by doing a small re-arrangement of packages to make the version information available via a package which is not
main
.I made commits directly into
vendor/github.com/matterbridge/gozulipbot/
directly, rather than open a PR against https://github.com/matterbridge/gozulipbot. It looks like thegozulipbot
code in this repo has already diverged from the other repository, so it looked easier to fix it here. Regardless, however, the key flaws of failing to check for failure during queue registration and failing to rate-limit 401 responses also apply to thematterbridge/gozulipbot
repository, and should also be applied there.I'm happy to open a second PR there if you'd prefer; let me know how you'd like to proceed, as it looks like there are changes other than these which need backporting out of
vendor/
and intomatterbridge/gozulipbot
.