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

[Optional] adherence to Telegram's rate limits #7

Merged
merged 4 commits into from
Oct 19, 2016

Conversation

thenaterhood
Copy link
Member

This is most likely less than perfect and I'm not set up to test it (read: make sure to test this before you merge it), but I want to open it up for review and maybe some discussion, as there's a good chance there's a more intelligent way to do this.

As-is, this sends messages immediately until the rate limit gets hit, then starts delaying and bundling messages. For a really active chat period, there might be up to about a 3 second delay for messages arriving in Telegram and the delayed messages will arrive en-masse, using the default settings for Telegram's limits.

Being a little smarter and starting to bundle messages if we notice we're approaching the rate limit might make the delay shorter if chat has short periods of high activity, but with a consistently active chat the 3 second delay would probably end up getting hit anyway. Given that, I'm not sure if the ROI of building logic for that makes it worthwhile.

@jwflory jwflory added improvement Improves on something that already exists needs testing Needs testing or QA before closing labels Sep 10, 2016
@jwflory
Copy link
Member

jwflory commented Sep 10, 2016

If @repkam09 can review the code, I can set up a staging / test version of the bot using this, and we can try putting it to the limit.

@Serubin
Copy link
Member

Serubin commented Sep 10, 2016

We can replace the current RITLUG bot with this one. Or set up a test channel and create another bot to just spam at random intervals.

@ritlug-root
Copy link
Member

I was thinking a test channel would be the best way to go, so we don't
blow up people's phones / IRC channel to test the rate limit.

On 09/10/2016 02:51 PM, Solomon wrote:

We can replace the current RITLUG bot with this one. Or set up a test
channel and create another bot to just spam at random intervals.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/AIA8V-gbsirFS2tfNHN12EeJuSQZ4uVqks5qovw2gaJpZM4J5oEf.

Cheers,
Justin W. Flory, RITlug President
ritlug@gmail.com

@jwflory
Copy link
Member

jwflory commented Sep 21, 2016

@repkam09 @thenaterhood Curious, were there any results that came out of the Software Freedom Day hacking? I've been meaning to test this but haven't had much of a shot to get around to it.

@thenaterhood
Copy link
Member Author

I think @repkam09 took it out for a spin, but I'm not sure what came of it.

@repkam09
Copy link
Member

I was testing how bad the rate-limiting itself was using the existing code, without this patch. Without this patch, you will get rate-limited, but the messages will still be relayed after the rate-limit period is over.

I'm not sure if its the node-telegram-irc library thats handling it, or telegram itself, but it seems to work.

As I had written in the teleirc Telegram group, its still obviously better to not get rate-limited in the first place and this code should help with that. I don't think I actually ended up running this code as a test because I was surprised that the rate-limiting seemed to resolve itself and no messages were actually lost.

If someone reminds me (or I remember...) I'll set up my little testing irc/telegram group tonight and test it for reals.

@thenaterhood
Copy link
Member Author

There are settings that will let you change the rate limiting level. In
this pull it's set to 20 (messages per minute) but if you lower that, you
can make sure you're triggering the code instead of hitting rate limiting
on Telegram's side. If you're still hitting Telegram's limits then the
setting might need a little tweaking too.

On Thu, Sep 22, 2016 at 9:15 AM, Mark Repka notifications@github.com
wrote:

I was testing how bad the rate-limiting itself was using the existing
code, without this patch. Without this patch, you will get rate-limited,
but the messages will still be relayed after the rate-limit period is over.

I'm not sure if its the node-telegram-irc library thats handling it, or
telegram itself, but it seems to work.

As I had written in the teleirc Telegram group, its still obviously better
to not get rate-limited in the first place and this code should help with
that. I don't think I actually ended up running this code as a test because
I was surprised that the rate-limiting seemed to resolve itself and no
messages were actually lost.

If someone reminds me (or I remember...) I'll set up my little testing
irc/telegram group tonight and test it for reals.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACdwxAnNmapuMc_kmh5VQ2JBR6dbnyoJks5qsn9bgaJpZM4J5oEf
.

Nate Levesque
www.natelevesque.com http://www.natelevesque.com

@jwflory
Copy link
Member

jwflory commented Oct 18, 2016

@repkam09 Reminder about giving this a test on your own instance again. Would be cool to see this get merged. 😄

@thenaterhood
Copy link
Member Author

I think at one point we observed this causes some out of order messages and
some missing ones, for whatever reason. Someone who's more familiar with
node may want to take a look at that since I only know enough node to be
dangerous.

On Tue, Oct 18, 2016 at 8:56 AM, Justin W. Flory notifications@github.com
wrote:

@repkam09 https://github.com/repkam09 Reminder about giving this a test
on your own instance again. Would be cool to see this get merged. 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACdwxG16rhRmceAwXdftoNTAefwXUoD_ks5q1MH0gaJpZM4J5oEf
.

Nate Levesque
www.natelevesque.com http://www.natelevesque.com

Copy link
Contributor

@robbyoconnor robbyoconnor left a comment

Choose a reason for hiding this comment

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

LGTM

@jwflory jwflory removed the needs testing Needs testing or QA before closing label Oct 19, 2016
@jwflory jwflory merged commit e8273b9 into RITlug:master Oct 19, 2016
@jwflory
Copy link
Member

jwflory commented Oct 19, 2016

Based on the feedback, I decided to merge this one in now, and if we continue to notice anything strange, we can debug further. Thanks for submitting, @thenaterhood. 🎬

@thenaterhood
Copy link
Member Author

Sounds good! If it becomes a problem it can be disabled by setting the rate
to 0 in the config.

On Wed, Oct 19, 2016 at 6:53 AM, Justin W. Flory notifications@github.com
wrote:

Based on the feedback, I decided to merge this one in now, and if we
continue to notice anything strange, we can debug further. Thanks for
submitting, @thenaterhood https://github.com/thenaterhood. 🎬


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACdwxEqmfieDflX7dfbM87DR5rLw3W0Rks5q1fbBgaJpZM4J5oEf
.

Nate Levesque
www.natelevesque.com http://www.natelevesque.com

@jwflory jwflory added this to the 1.1.0 milestone Oct 23, 2016
@tobsn
Copy link

tobsn commented May 29, 2017

just found this by accident looking for telegram rate limits - does anyone know how to check or even just see what the current rate limit is?

@thenaterhood
Copy link
Member Author

The rate limits are available in Telegram's documentation: https://core.telegram.org/bots/faq#my-bot-is-hitting-limits-how-do-i-avoid-this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves on something that already exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants