Fixes bug 814647 - Automatically send emails to all users that crash. #1026

Merged
merged 5 commits into from Jan 16, 2013

Conversation

Projects
None yet
5 participants
Contributor

adngdb commented Dec 26, 2012

No description provided.

Contributor

adngdb commented Jan 2, 2013

This is failing because schema.sql is missing the new emails table. I'm not sure about how to deal with this: add the new table the schema.sql manually, or just ignore that error?

By the way, this made me realize that it's missing a SQL script to create the new table. Will add that soon.

Contributor

selenamarie commented Jan 2, 2013

The way we worked out managing schema.sql was to deploy to stage and then do a pg_dump from stage.

We can deploy to dev and then do a dump from there for pretty much the same effect.

Contributor

selenamarie commented Jan 2, 2013

Based on the SQL_REPORT command, it seems like the 'emails' table will get large. If you can confirm this, I'd like to add partitioning of that table as part of this change.

Contributor

adngdb commented Jan 2, 2013

@selenamarie I just added the new emails table script.

@@ -0,0 +1,362 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
@brandonsavage

brandonsavage Jan 3, 2013

Contributor

Did we actually write this or did we get the code from somewhere else?

@adngdb

adngdb Jan 3, 2013

Contributor

Nope, this is coming from the basket project: https://github.com/mozilla/basket/blob/master/apps/news/backends/exacttarget.py (this is an exact copy of the original file).

+
+ fields = {
+ 'EMAIL_ADDRESS_': report['email'],
+ 'EMAIL_FORMAT_': 'H',
@brandonsavage

brandonsavage Jan 3, 2013

Contributor

I'm assuming that this sends an HTML email. Not sure how I feel about that. Worried it might get us blocked for spam. We might want to get some thoughts on this from others.

@adngdb

adngdb Jan 3, 2013

Contributor

That's why we use ExactTarget, it's supposed to handle all those problems of spam and reputation for us. Anyway, that's how mozilla/basket does it.

Contributor

adngdb commented Jan 11, 2013

Can I get a review here please? I'd like to get it on dev asap so we can start testing it. @brandonsavage ? @peterbe ? @rhelmer maybe?

+ )
+ required_config.add_option(
+ 'restrict_products',
+ default=['Firefox'],
@peterbe

peterbe Jan 11, 2013

Contributor

Does making default a list make configman automatically do a .split(',') work on this config input?
What if you were to type "Waterwolf, Nighttrain" in test_automatic_emails.ini?

@twobraids

twobraids Jan 11, 2013

Contributor

no, configman will just see that the default is of the type 'list' and try to convert from string with list("Waterwolf, Nighttrain"). In other words, the value will end up as ['W', 'a', 't', 'e', ... 'i', 'n']

socorro/cron/jobs/automatic_emails.py
+ default=['Firefox'],
+ doc='List of products for which to send an email. '
+ )
+ required_config.add_option(
@peterbe

peterbe Jan 11, 2013

Contributor

@twobraids It would be cool if you could, here, set something like validator=lambda v: v.strip() to prevent the app from starting with an empty value.

@twobraids

twobraids Jan 11, 2013

Contributor

the default could be set to None and then the from_string_converter could do that function.

socorro/cron/jobs/automatic_emails.py
+ )
+ required_config.add_option(
+ 'email_template',
+ default='socorro_dev_test',
@peterbe

peterbe Jan 11, 2013

Contributor

Is that safe? Is there not a risk then that we accidentally use the "dev" version when this is pushed to prod?

@adngdb

adngdb Jan 11, 2013

Contributor

Good point, we should have an empty value (that would likely raise an error from ET) instead.

socorro/cron/jobs/automatic_emails.py
+ for row in cursor.fetchall():
+ report = dict(zip(SQL_FIELDS, row))
+ self.send_email(report)
+ self.update_user(report, run_datetime, connection)
@peterbe

peterbe Jan 11, 2013

Contributor

This should be self.update_user(report, utc_now(), connection)

If the cron app has been broken for a couple of hours, and the backfill self-healing machinery kicks in then run_datetime != utc_now() so it would be a lie to say the email was sent 1/3/X hours ago when it was actually sent now.

@peterbe

peterbe Jan 11, 2013

Contributor

Perhaps I'm not seeing the forest for all the trees here but what happens if you have 10 people to send to, half way through an exception is raised (e.g. network error with ExactTarget's server) and 5 people still haven't received their email.
When the backfiller sees this it will reattempt with the same run_datetime later.

I don't think that's unrealistic at all. Networks are always flaky.

Do we need to run connection.commit() after each update_user()?

@adngdb

adngdb Jan 11, 2013

Contributor

The point about connection.commit() is interesting indeed, as I'm not doing it now if an errors occurs we might lose all date updates and thus send some emails twice. If we are sure that the database has been updated with the last sending date there is no risk that a user is spamed by us.

I'll trust you on utc_now() versus run_datetime (I actually wanted you to review for this kind of things, I'm not familiar with all the details of crontabber's behavior).

@peterbe

peterbe Jan 11, 2013

Contributor

A) I know it's going to take time but I think this is really important. Feel free to talk to Laura but I think you ought to add a test that checks this stuff. If it's an integration test, what you could do is something like this:

_failures = []
def failer(email, *a, **k):
    if email == 'c@example.org': 
      if email not in _failures:
          _failures.append(email)
          raise FuckupError()
exacttarget_mock.sendfunction.return_value = failer

or something and then feed it a list of a@example.org, b@example.org, c@example.org, d@example.org. But keeping track of the _failures you can simulate that running it again a little later it will work.

I'm more than happy to help you work on this.

@peterbe

peterbe Jan 11, 2013

Contributor

B) regarding the run_datetime stuff. When everything is normal, run_datetime==utc_now(). When it previously failed, run_datetime is basically the date it previously failed.

@@ -0,0 +1,48 @@
+#! /usr/bin/env python
@peterbe

peterbe Jan 11, 2013

Contributor

This is cool! Thanks @rhelmer and @adriangaudebert for pioneering the integrationtest stuff!

Contributor

peterbe commented Jan 11, 2013

Looks good to me. r+ with nit.

The nit is that I'm genuinely worried about errors happening whilst we loop over email addresses. Few things piss people off as much as when they receive the same email twice. Please assure me that it won't do that.

@ghost ghost assigned peterbe Jan 16, 2013

+ now = utc_now()
+ self.assertEqual(cursor.rowcount, 2)
+ for row in cursor.fetchall():
+ assert row[0] in ('a@example.org', 'b@example.org')
@peterbe

peterbe Jan 16, 2013

Contributor

Yay!

Contributor

adngdb commented Jan 16, 2013

Thanks @peterbe, @selenamarie and @brandonsavage for the review here!

adngdb added a commit that referenced this pull request Jan 16, 2013

Merge pull request #1026 from AdrianGaudebert/814647-email-every-user
Fixes bug 814647 - Automatically send emails to all users that crash.

@adngdb adngdb merged commit e5a1ea3 into mozilla-services:master Jan 16, 2013

1 check passed

default Jenkins build 'socorro-github' #384 has succeeded
Details

rhelmer pushed a commit to rhelmer/socorro that referenced this pull request Sep 6, 2013

Merge pull request #1026 from diox/collections-locales-tweaks
Always return all translations in Collections API, and add default_language (bug 907181)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment