The default bug view has changed. See this FAQ.

add the ability for administrators to limit the number of emails sent to a user per minute and hour

RESOLVED FIXED in Bugzilla 6.0

Status

()

Bugzilla
Email Notifications
--
enhancement
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: glob, Assigned: glob)

Tracking

(Blocks: 1 bug, {bmo-goal})

unspecified
Bugzilla 6.0
bmo-goal
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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

3 years ago
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

3 years ago
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.
(Assignee)

Comment 3

3 years ago
to assist administrators, i'll also upstream bmo's jobqueue viewer as part of this work:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BMO/lib/Reports/EmailQueue.pm
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/BMO/template/en/default/pages/email_queue.html.tmpl

Comment 4

3 years ago
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).
Severity: normal → enhancement

Comment 5

3 years ago
(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.
(Assignee)

Comment 6

3 years ago
(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

3 years ago
(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

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Updated

3 years ago
Assignee: email-notifications → glob
(Assignee)

Comment 10

3 years ago
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

Updated

3 years ago
Keywords: bmo-goal
(Assignee)

Comment 11

2 years ago
Created attachment 8512705 [details] [diff] [review]
1062739_1.patch
Attachment #8512705 - Flags: review?(dylan)

Comment 12

2 years ago
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 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.
Attachment #8512705 - Flags: review?(dylan) → review-
Created attachment 8513645 [details]
weird-formatting.png

screenshot of the view queue link, showing alignment problem.
(Assignee)

Comment 15

2 years ago
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.
Attachment #8512705 - Attachment is obsolete: true
Attachment #8514031 - Flags: review?(dylan)
Comment on attachment 8514031 [details] [diff] [review]
1062739_2.patch

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

r=dylan
Attachment #8514031 - Flags: review?(dylan) → review+

Comment 17

2 years ago
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.
(Assignee)

Comment 18

2 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f1fda7c..908480c  master -> master

(includes lpsolit's change from comment 17)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: approval+
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 6.0
(Assignee)

Updated

2 years ago
Blocks: 1093479
(Assignee)

Updated

2 years ago
Blocks: 1093481

Updated

10 months ago
Blocks: 1275590
You need to log in before you can comment on or make changes to this bug.