Last Comment Bug 1062739 - add the ability for administrators to limit the number of emails sent to a user per minute and hour
: add the ability for administrators to limit the number of emails sent to a us...
Status: RESOLVED FIXED
: bmo-goal
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: unspecified
: All All
-- enhancement (vote)
: Bugzilla 6.0
Assigned To: Byron Jones ‹:glob›
: default-qa
:
Mentors:
Depends on:
Blocks: 1275590 1093479 1093481
  Show dependency treegraph
 
Reported: 2014-09-04 00:12 PDT by Byron Jones ‹:glob›
Modified: 2016-05-25 07:54 PDT (History)
5 users (show)
glob: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
1062739_1.patch (15.54 KB, patch)
2014-10-28 07:43 PDT, Byron Jones ‹:glob›
dylan: review-
Details | Diff | Splinter Review
weird-formatting.png (63.14 KB, image/png)
2014-10-29 10:43 PDT, Dylan Hardison [:dylan]
no flags Details
1062739_2.patch (15.98 KB, patch)
2014-10-29 22:15 PDT, Byron Jones ‹:glob›
dylan: review+
Details | Diff | Splinter Review

Description User image Byron Jones ‹:glob› 2014-09-04 00:12:08 PDT
some email providers impose limits on the number of messages a recipient can receive over a period of time.

eg. gmail's limits are:
    180 messages per minute
   3600 messages per hour
  86400 messages per day
(from https://support.google.com/a/answer/1366776)


to avoid emails being lost, we should implement rate limiting within bugzilla:
- applicable only when using queued sending
- add two admin parameters:
  - per-minute limit, default 100
  - per-hour limit, default 2500
  - setting either to 0 disables rate limiting
- add a new table which logs the timestamps of all email sent and the recipient
- update email sending to:
  - delete entries from the new table which are older than 24 hours
  - check the number of messages sent to the specified recipient within the last minute and 24 hours
  - if either exceeds the configure rate, call $job->declined()
- update the minimum TheSchwartz version to 1.10
Comment 1 User image Simon Green 2014-09-04 00:21:43 PDT
After discussion with glob, there are two changes

(In reply to Byron Jones ‹:glob› from comment #0)
>   - delete entries from the new table which are older than 24 hours

This will be older than one hour

>   - check the number of messages sent to the specified recipient within the
> last minute and 24 hours

This also should be last minute and last hour.
Comment 2 User image Simon Green 2014-09-04 00:23:46 PDT
It was also decided to set the limits to 100/minute and 2500/hour for two reasons:

1) To allow other non-Bugzilla e-mails to go though to the user.
2) For those installations that have multiple queues processes running, it is possible to send limit+processes e-mails per minute or hour.
Comment 4 User image Frédéric Buclin 2014-09-04 04:12:23 PDT
Which kind of installations is affected by these limitations, maybe besides *some* bmo or RH users?? Shouldn't this be implemented as an extension instead? This is again another set of new parameters which are added which will be of very limited use (+ the code behind to make this work).
Comment 5 User image Simon Green 2014-09-04 06:29:18 PDT
(In reply to Frédéric Buclin from comment #4)
> Which kind of installations is affected by these limitations, maybe besides
> *some* bmo or RH users?? Shouldn't this be implemented as an extension

Gnome comes to mind, and any other Bugzilla system that mass closes bugs as a version or product becomes EOL and a Bugzilla administrator mass closes thousands of bugs a once.
Comment 6 User image Byron Jones ‹:glob› 2014-09-04 07:02:23 PDT
(In reply to Frédéric Buclin from comment #4)
> Which kind of installations is affected by these limitations, maybe besides
> *some* bmo or RH users??

in bmo's case, "some" is approx 120,000 gmail users.

i any event, we have quite a few features which are only applicable to larger installations (eg. jobqueue, memcahed, ldap integration), and this is one of them.  while it's true that it adds another set of parameters, the defaults should be suitable for practically all installations.
Comment 7 User image Andre Klapper 2014-09-04 09:34:46 PDT
(In reply to sgreen from comment #5)
> Gnome comes to mind, and any other Bugzilla system that mass closes bugs as
> a version or product becomes EOL and a Bugzilla administrator mass closes
> thousands of bugs a once.

Confirming for GNOME; another usecase beside mass-closing EOL tickets would be e.g. merging old "Version" field entries.
Comment 8 User image Frédéric Buclin 2014-09-04 10:38:18 PDT
(In reply to Byron Jones ‹:glob› from comment #6)
> in bmo's case, "some" is approx 120,000 gmail users.

No, my question is: how many users are really affected by these rate limits? I have a gmail.com account, and I certainly don't get that many bugmails per hour or day. So "some" is definitely much less than 120,000.

I think it would make much more sense to put these rate limits into constants in Bugzilla::Constants as it's very unlikely that these values will ever be edited.
Comment 9 User image Byron Jones ‹:glob› 2014-09-07 23:25:16 PDT
(In reply to Frédéric Buclin from comment #8)
> how many users are really affected by these rate limits?

it's rare that we hit those limits currently.  the usage of bmo continues to grow and i think it's important to put preventative measures in place to avoid email being unexpectedly dropped.

> I think it would make much more sense to put these rate limits into
> constants in Bugzilla::Constants as it's very unlikely that these values
> will ever be edited.

that's a reasonable suggestion; will do.
Comment 10 User image Byron Jones ‹:glob› 2014-09-18 22:59:37 PDT
rough plan of attack:
- table email_rates(id, recipient VARCHAR, message_ts)
- in MessageToMTA, use $email's recipient against the email_rates
  - if limits hit, die "some_constant_error_message"
- in Bugzilla::Job::Mailer check for that error and $job->declined()
  - refactor Bugzilla::Job::Mailer and ::BugMail to avoid duplicating that logic
- insert into email_rates at the end of MessageToMTA
- delete old rows from email_rates near the start of MessageToMTA
- upstream bmo's queue reports, as per comment 3
Comment 11 User image Byron Jones ‹:glob› 2014-10-28 07:43:58 PDT
Created attachment 8512705 [details] [diff] [review]
1062739_1.patch
Comment 12 User image Frédéric Buclin 2014-10-29 07:45:28 PDT
Comment on attachment 8512705 [details] [diff] [review]
1062739_1.patch

>+++ b/template/en/default/admin/admin.html.tmpl

>+[% IF user.in_group('admin') %]
>+  <dl>
>+  [% IF Param('use_mailer_queue') %]
>+    <dt><a href="view_job_queue.cgi">View Job Queue</a></dt>
>+    <dd>View the queue of undelivered/deferred jobs/emails.</dd>
>+  [% END %]
>+  </dl>
>+[% END %]

There is no reason to have this item out of the table used for all other items. It should be below the Whining link.



>+++ b/view_job_queue.cgi

>+use Storable ();

You should import the methods you need explicitly instead of using the Storable::foo() notation later.


>+use constant MAX_JOBS => 500;

This constant should be in Bugzilla/Constants.pm. And this way, you can easily use it from templates using [% constants.MAX_JOBS %] without having to store it in $vars.


>+my $user = Bugzilla->login(LOGIN_REQUIRED);
>+Bugzilla->login(LOGIN_REQUIRED)->in_group("admin")

The 2nd line should be $user->in_group("admin").
Comment 13 User image Dylan Hardison [:dylan] 2014-10-29 10:42:41 PDT
Comment on attachment 8512705 [details] [diff] [review]
1062739_1.patch

Review of attachment 8512705 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks good. I set LIMIT_PER_MINUTE to 1 and EMAIL_LIMIT_PER_HOUR to 5 to test.
r- because the "view  the queue of deferred jobs/emails" link looks really pretty bad on bugzilla's current default theme (and probably others.)

::: Bugzilla/Constants.pm
@@ +650,5 @@
> +# Setting a limit to 0 will disable this feature.
> +use constant EMAIL_LIMIT_PER_MINUTE => 1000;
> +use constant EMAIL_LIMIT_PER_HOUR   => 2500;
> +# Don't change this exception message.
> +use constant EMAIL_LIMIT_EXCEPTION  => "email_limit_exceeded\n";

I like this, seems like it could be made into an exception object if we ever need more fine-grained handling.

::: Bugzilla/DB/Schema.pm
@@ +1650,5 @@
> +        INDEXES => [
> +            email_rates_idx => [qw(recipient message_ts)],
> +        ],
> +    },
> +

nit: delete extra line here.

::: template/en/default/admin/admin.html.tmpl
@@ +131,5 @@
> +    <dd>View the queue of undelivered/deferred jobs/emails.</dd>
> +  [% END %]
> +  </dl>
> +[% END %]
> +

Why is this outside of the table? it looks very odd.
Comment 14 User image Dylan Hardison [:dylan] 2014-10-29 10:43:39 PDT
Created attachment 8513645 [details]
weird-formatting.png

screenshot of the view queue link, showing alignment problem.
Comment 15 User image Byron Jones ‹:glob› 2014-10-29 22:15:49 PDT
Created attachment 8514031 [details] [diff] [review]
1062739_2.patch

thanks lpsolit and dylan for the quick turn around.

no idea what i was thinking with regards to the positioning on admin.cgi, this revision should look better.
Comment 16 User image Dylan Hardison [:dylan] 2014-10-30 09:12:27 PDT
Comment on attachment 8514031 [details] [diff] [review]
1062739_2.patch

Review of attachment 8514031 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan
Comment 17 User image Frédéric Buclin 2014-10-30 11:53:36 PDT
Comment on attachment 8514031 [details] [diff] [review]
1062739_2.patch

>+++ b/view_job_queue.cgi

>+Bugzilla->login(LOGIN_REQUIRED)->in_group("admin")

I'm really not a fan of this notation. For new contributors, it won't be clear at all that Bugzilla->login() is a user object. Doing it as we do everywhere else, i.e.

my $user = Bugzilla->login(...);
$user->in_group(...)

is much clearer.
Comment 18 User image Byron Jones ‹:glob› 2014-10-31 00:21:31 PDT
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f1fda7c..908480c  master -> master

(includes lpsolit's change from comment 17)

Note You need to log in before you can comment on or make changes to this bug.