config, bot: Configurable message rate #1518

kwaaak · 2019-03-25T05:05:01Z

Code originally by @larsks in #908

I just made it compatible with the current state.

dgw · 2019-03-25T06:24:06Z

@kwaaak This needs to go into master, not 6.6.x, so it'll need rebasing. (Unfortunately, I discovered an issue with the changelog after tagging the release, so you have an extra commit past what was merged.)

It would also be great to include the original commit message (feel free to correct the spelling—there are a few typos in there), and a Co-authored-by: trailer at the end to give @larsks credit as well. (I won't put his name & email here, to lessen the risk of scraper bots grabbing it from the conversation, but it's easily retrieved. I can send it to you on IRC if you need.)

dgw

Between the two of us, we got the branch rebased and the commit message updated.

Time for a real look at the code.

@kwaaak If you want, you can address these yourself, in a new commit (don't amend/rebase). You can also wait for @Exirel to weigh in, since I'll ask his opinion too. One of us can also make the name changes if you don't want to. 😸

sopel/config/core_section.py

sopel/bot.py

Exirel

Looking good so far, the main logic is here. We can nitpick at naming things, but I feel like it's a valuable improvement.

sopel/bot.py

deathbybandaid · 2019-04-02T13:16:08Z

With the work I'm doing on multi-recipients and reaching the 512byte limit of a message, I'm not sure exactly how this is going to affect that. #1525

dgw · 2019-04-02T21:29:32Z

@deathbybandaid Affect that in what way? Or do you just mean that they both step on the same part of the code? (In which case, by convention, this will get merged first because it's older.)

dgw

I'm happy after the renaming of things. Now to see if @Exirel wants more renames or not. 😛

Exirel · 2019-04-02T21:44:16Z

Code-style and naming-thing wise, I'm happy about your changes! LGTM.

deathbybandaid · 2019-04-02T21:45:27Z

The configfile part of this PR is going to be super helpful for what I'm working on.

The actual implementation of how the flood protection works with multiple recipients and my new bytecounting logic is likely to be a bit different than this.

Not a conflict, just pointing out that I'm not fully sure how I'm working out my stuff yet, and this add a little extra layer to that. :)

dgw · 2019-04-02T21:51:58Z

Plan to build on top of this, @deathbybandaid, and see where it takes you. Remember that git reset can be your friend if you break a pull request and need to go back to a previous good state. 😉

I think it's been said before, but adding "official" support for multiple recipients isn't extremely high on the priority list compared to this since it's already possible to do "manually" (with commas).

deathbybandaid · 2019-04-03T12:43:24Z

I decided to give your branch a test, and have come across an issue:

Signature: AttributeError: 'dict' object has no attribute 'replace' (file "/usr/local/lib/python3.6/dist-packages/sopel/irc.py", line 121, in safe)
from deathbybandaid at 2019-04-03 12:37:43.234871. Message was: .dbbtest
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/sopel/bot.py", line 473, in call
    exit_code = func(sopel, trigger)
  File "/home/sopel/.sopel/modules/dbbtest.py", line 25, in mainfunction
    bot.say(str(lorem_ipsum), trigger.sender)
  File "/usr/local/lib/python3.6/dist-packages/sopel/bot.py", line 410, in say
    self._bot.say(message, destination, max_messages)
  File "/usr/local/lib/python3.6/dist-packages/sopel/bot.py", line 337, in say
    self.write(('PRIVMSG', recipient), text)
  File "/usr/local/lib/python3.6/dist-packages/sopel/bot.py", line 166, in write
    irc.Bot.write(self, args, text=text)
  File "/usr/local/lib/python3.6/dist-packages/sopel/irc.py", line 126, in write
    args = [self.safe(arg) for arg in args]
  File "/usr/local/lib/python3.6/dist-packages/sopel/irc.py", line 126, in <listcomp>
    args = [self.safe(arg) for arg in args]
  File "/usr/local/lib/python3.6/dist-packages/sopel/irc.py", line 121, in safe
    string = string.replace('\n', '')
AttributeError: 'dict' object has no attribute 'replace'

It looks like you may be overwriting the variable recipient?

deathbybandaid

using recipient for the dict overrides the actual recipient that gets sent to self.write causing:

AttributeError: 'dict' object has no attribute 'replace' (file "/usr/local/lib/python3.6/dist-packages/sopel/irc.py", line 121, in safe)

sopel/bot.py

dgw · 2019-05-12T04:02:25Z

@kwaaak Seeing as we keep getting issues asking how to turn off the flood protection delay, I think it's time this was rebased and squashed a bit so we can merge it.

I'll leave sorting out the merge conflict to you. As far as squashing, you can meld the last two or even three commits into the first commit, merging the co-author trailers together. With co-author tags and detailed PR history, we don't have to worry too much about losing attribution in squashes. 😸

dgw · 2019-05-12T04:07:59Z

@kwaaak Alternatively, you can squash just the last commit ("Update bot.py") into the first, and tweak the third commit's message: "Fix variable name" -> "bot: fix variable name in flood prevention logic"

That last commit makes a change to the penalty logic that was already made in #1552 between when this PR was opened and now, so it makes sense to include it in the first commit after rebasing.

@larsks

Original change description by @larsks (from sopel-irc#908, slightly edited): This adds several knobs that control how fast the bot sends multiple messages to the same recipient: - bucket_burst_tokens -- this controls how many messages may be sent in "burst mode", i.e., without any inter-message delay. - bucket_refill_rate -- once exhausted, burst tokens are refilled at a rate of bucket_refill_rate tokens/second. - bucket_empty_wait -- how long to wait between sending messages when not in burst mode. This permits the bot to return a few lines of information quickly and not trigger flood protection. How it works: When sending to a new recipient, we initialize a token counter to bucket_burst_tokens. Every time we send a message, we decrement this by 1. If the token counter reaches 0, we engage the rate limiting behavior (which is identical to the existing code, except that the minimum wait of 0.7 seconds is now configurable). When sending a new message to an existing recipient, we check if the token counter is exhausted. If it is, we refill it based on the elapsed time since the last message and the value of bucket_refill_rate, and then proceed as described above. ---- Adapted to current Sopel code and rebased by these users, respectively: Co-authored-by: kwaaak <kwaaak@users.noreply.github.com> Co-authored-by: dgw <dgw@technobabbl.es>

Co-authored-by: deathbybandaid <sam@deathbybandaid.net>

dgw · 2019-05-13T21:38:29Z

I think this is successfully rescued from the brink of being thrown out and started over. Always make a backup branch when you rebase, people—and don't delete your local repo if it breaks, because you'll want to keep that reflog! (@kwaaak learned a valuable lesson today.)

@Exirel do you stand by your approval of this now that the dust has settled post-squash?

Exirel · 2019-05-14T15:32:26Z

@Exirel do you stand by your approval of this now that the dust has settled post-squash?

@dgw yeah, LGTM. My main concern was naming stuff, and @kwaaak did a great job here. The logic looks good too, but I haven't tested it myself. If you feel confident, it's OK by me!

deathbybandaid · 2019-05-15T16:37:16Z

couldn't figure out how to make github let me review these lines:

            # No messages within the last 3 seconds? Go ahead!
            # Otherwise, wait so it's been at least 0.8 seconds + penalty

May need to update these comments with the new configurable logic. Especially since the default for 'flood_empty_wait' = 0.7

deathbybandaid · 2019-05-15T21:46:42Z

I have taken the time to test different combinations of configured numbers, and I'm very happy with this.

My primary bots sit behind ZNC, and this allows me to essentially disable the flood protection from the bot and rely on ZNCs flood protection, if I want.

Exirel · 2019-05-15T21:52:41Z

I have taken the time to test different combinations of configured numbers, and I'm very happy with this.

Thank you very much for your field testing. This is not a feature that is easy to test, so this type of feedback is very valuable. It's good to know it works as intended!

So I guess 🚢 ? 😄

Loop detection was only running if the bot was out of flood lines. It should ALWAYS try to detect message loops. Also removed an outdated comment and added updated ones for the new flood-protection logic.

dgw · 2019-06-06T15:22:43Z

I was looking over this one last time, partly because of @deathbybandaid's comment, and I was having the damnedest time figuring out how to fix the code comments. Then I realized part of the issue was the loop-detection code being indented too far (so it became part of an if block it shouldn't be).

Re-requesting review so someone else can double-check me 😁

I touched it, so I can't approve my own work.

Exirel

LGTM

dgw · 2019-06-07T05:52:15Z

OK, I'm finally ready to ship this. Anything I missed can get fixed up when @Exirel inevitably moves a big chunk of the new logic over into bot.write(), because I know he wants to! 😁

kwaaak force-pushed the patch-4 branch from 1fd90bd to e44d476 Compare March 25, 2019 05:11

dgw changed the base branch from 6.6.x to master March 25, 2019 06:18

dgw added this to the 7.0.0 milestone Mar 25, 2019

dgw added the Feature label Mar 25, 2019

kwaaak force-pushed the patch-4 branch 4 times, most recently from 8ec333c to 64e71c8 Compare March 25, 2019 07:21

kwaaak closed this Mar 25, 2019

kwaaak force-pushed the patch-4 branch from 64e71c8 to 7131f5e Compare March 25, 2019 07:36

kwaaak reopened this Mar 25, 2019

dgw force-pushed the patch-4 branch from 64e71c8 to 774f6d5 Compare March 25, 2019 07:56

dgw requested changes Mar 25, 2019

View reviewed changes

sopel/config/core_section.py Outdated Show resolved Hide resolved

sopel/bot.py Outdated Show resolved Hide resolved

dgw requested a review from Exirel March 25, 2019 08:14

Exirel requested changes Mar 25, 2019

View reviewed changes

sopel/bot.py Outdated Show resolved Hide resolved

dgw force-pushed the patch-4 branch from 0f642e3 to 9a7bf59 Compare April 2, 2019 21:16

dgw previously approved these changes Apr 2, 2019

View reviewed changes

dgw requested a review from Exirel April 2, 2019 21:31

Exirel previously approved these changes Apr 2, 2019

View reviewed changes

deathbybandaid suggested changes Apr 3, 2019

View reviewed changes

deathbybandaid mentioned this pull request Apr 3, 2019

make message rate configurable, including burst mode #908

Closed

kwaaak force-pushed the patch-4 branch 2 times, most recently from 532a516 to 1eabf4f Compare April 3, 2019 16:56

Exirel added the High Priority label May 12, 2019

kwaaak force-pushed the patch-4 branch 3 times, most recently from b1d8664 to 7dc09fc Compare May 13, 2019 21:01

dgw force-pushed the patch-4 branch from 7dc09fc to fc2f019 Compare May 13, 2019 21:31

bot, config: anti-flood configuration code style

f4636eb

Co-authored-by: deathbybandaid <sam@deathbybandaid.net>

dgw force-pushed the patch-4 branch from fc2f019 to f4636eb Compare May 13, 2019 21:36

dgw mentioned this pull request May 21, 2019

Configuration of Flood Protection #1617

Closed

deathbybandaid mentioned this pull request May 21, 2019

Allow the disabling of repetition detection #1618

Closed

This was referenced May 31, 2019

doc: autoprogram & configuration #1564

Closed

[Proposal] Update flood protection logic #1559

Open

bot: fix loop detection

a46768c

Loop detection was only running if the bot was out of flood lines. It should ALWAYS try to detect message loops. Also removed an outdated comment and added updated ones for the new flood-protection logic.

dgw requested a review from a team June 6, 2019 15:22

Exirel approved these changes Jun 6, 2019

View reviewed changes

dgw merged commit 61a2699 into sopel-irc:master Jun 7, 2019

dgw mentioned this pull request Jun 7, 2019

bot: fix sometimes-undefined variable in flood protection #1638

Merged

Exirel mentioned this pull request Jul 6, 2019

Configuration of Flood Protection #1342

Closed

kwaaak deleted the patch-4 branch December 9, 2019 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config, bot: Configurable message rate #1518

config, bot: Configurable message rate #1518

kwaaak commented Mar 25, 2019

dgw commented Mar 25, 2019 •

edited

Loading

dgw left a comment

Exirel left a comment

deathbybandaid commented Apr 2, 2019 •

edited

Loading

dgw commented Apr 2, 2019

dgw left a comment

Exirel commented Apr 2, 2019

deathbybandaid commented Apr 2, 2019

dgw commented Apr 2, 2019

deathbybandaid commented Apr 3, 2019

deathbybandaid left a comment

dgw commented May 12, 2019

dgw commented May 12, 2019

dgw commented May 13, 2019

Exirel commented May 14, 2019

deathbybandaid commented May 15, 2019

deathbybandaid commented May 15, 2019

Exirel commented May 15, 2019

dgw commented Jun 6, 2019

Exirel left a comment

dgw commented Jun 7, 2019

config, bot: Configurable message rate #1518

config, bot: Configurable message rate #1518

Conversation

kwaaak commented Mar 25, 2019

dgw commented Mar 25, 2019 • edited Loading

dgw left a comment

Choose a reason for hiding this comment

Exirel left a comment

Choose a reason for hiding this comment

deathbybandaid commented Apr 2, 2019 • edited Loading

dgw commented Apr 2, 2019

dgw left a comment

Choose a reason for hiding this comment

Exirel commented Apr 2, 2019

deathbybandaid commented Apr 2, 2019

dgw commented Apr 2, 2019

deathbybandaid commented Apr 3, 2019

deathbybandaid left a comment

Choose a reason for hiding this comment

dgw commented May 12, 2019

dgw commented May 12, 2019

dgw commented May 13, 2019

Exirel commented May 14, 2019

deathbybandaid commented May 15, 2019

deathbybandaid commented May 15, 2019

Exirel commented May 15, 2019

dgw commented Jun 6, 2019

Exirel left a comment

Choose a reason for hiding this comment

dgw commented Jun 7, 2019

dgw commented Mar 25, 2019 •

edited

Loading

deathbybandaid commented Apr 2, 2019 •

edited

Loading