Request limiter - prevent hitting Telegram's API limits

[jacklul]

#157

So, I've been using this implementation for a while now and I don't see any issues so far, neither any exceptions about Telegram returning response that I've hit the limit.

By default disabled because it can be heavy on the database.
Will delay requests using sleep(1);.

As mentioned in https://core.telegram.org/bots/faq#my-bot-is-hitting-limits-how-do-i-avoid-this

• No more than 1 message / second (chat)
• No more than 30 messages / second (globally)
• No more than 20 messages / minute (groups and channels)

This also applies to message editing! Does not apply to methods like getChat or sendChatAction.

Currently I also included 'retry' mode, sometimes if the bot is heavily used 60 seconds wait might not be enough... should I remove this and simply give it only 60 seconds, no more? Or leave it as is?

Will appreciate anyone to take a look at this!
Suggestions are welcome!

[noplanman]

Maybe a silly question, but shouldn't we add a request limiter as default behaviour?

Does this slow the bot down a lot?

[noplanman]

Makes sense to put some extra parenthesis here to clearly define what is being tested, making it easier to read the code.

[noplanman]

Wow, this one is quite difficult to understand.

Maybe there's a better way of presenting/evaluating this?

[jacklul]

I could transform it into multi-line statement with comments, should be fine?

[noplanman]

For this it would make sense to add variables for $data['chat_id'] and $data['inline_message_id'] as they're also used further up.

So instead of all the isset() && ... you could just use the variables directly.

Will think about this exact spot and how to make it more readable 👍

[jacklul]

$chat_id = isset($data['chat_id']) ? $data['chat_id'] : null;
$inline_message_id = isset($data['inline_message_id']) ? $data['inline_message_id'] : null;

if (($chat_id || $inline_message_id) && in_array($action, $limited_methods)) {
	$timeout = 60;

	while (true) {
		if ($timeout <= 0) {
			throw new TelegramException('Timed out while waiting for a request spot!');
		}

		$requests = DB::getTelegramRequestCount($chat_id, $inline_message_id);

		if (
			$requests['LIMIT_PER_SEC'] == 0 &&  // No more than one message per second inside a particular chat
			(
				(($chat_id > 0 || $inline_message_id) && $requests['LIMIT_PER_SEC_ALL'] < 30) ||  // No more than 30 messages per second globally
				($chat_id < 0 && $requests['LIMIT_PER_MINUTE'] < 20)  // No more than 20 messages per minute allowed for group chats
			)
		) {
			break;
		}

		$timeout--;
		sleep(1);
	}

	DB::insertTelegramRequest($action, $data);
}

Seems to look a bit better, what you think?

[noplanman]

Yep, looks a lot better, nice 👌

[noplanman]

You could also do $timeout-- here instead of $timeout = $timeout - 1; further down.

[nesttle]

Looks awesome, IMHO this should be implemented by default on next releases.

[nesttle]

I love one-line code instead multiline

[noplanman]

@nesttle Me too! (as long as it's readable 😉)

[noplanman]

Any way I can (realistically) test these limits?

[jacklul]

Doing it alone will be quite hard, you can test the one msg per sec and no more than 20 per minute in groups rules easily, 30 per sec globally will be pretty hard to achieve alone.

Spamming command with callback button editing own message should be good testing ground.

[noplanman]

I noticed that entries once added, never get deleted from the DB once handled.
Shouldn't they be cleared again after being processed?

[jacklul]

But if I remove them next requests will think they are ok to go.
I could add a query to remove entries older than 1 minute but I don't think it will be good practice to run such query every second (or more under heavy load)...?

[noplanman]

Hmm, I'm just thinking long term here. As this is a temporary table, it just gets bigger and bigger, in some cases pretty quick, right?
So some sort of cleanup would be good I think.

Maybe something like:

if (mt_rand(0, 999) === 42) {
    do_cleanup();
}

Or maybe I'm going too far here and this isn't really a problem...

[jacklul]

I guess, in my case I'm cleaning the table every 5 minutes, set as a task in Mysql.

I have no 'good' idea how to implement cleanup inside this code without adding potential delays or performance losses.

[noplanman]

Ok, then let's leave this as is for the moment and passively think of something 😉

[noplanman]

Busy trying to test this and get it merged. Hope to get it in soon 😊

[noplanman]

Didn't find the time to thoroughly test this, but I think it should be ok to merge.
If no one has any additional comments, I'll go ahead and merge soon.