[8.0][IMP] Performance issues in mail_tracking addons #97

antespi · 2016-10-11T14:42:42Z

This PR fix some performance issues:

[Fixed] In some cases self.env.cr is not closed properly.
Add index=True to time and recipient_address fields of mail.tracking.email model
Many2many relation between partners (and mail contacts) and mail tracking emails deleted
email_score is now a non-stored computed field

Also includes some new features for processing bounced emails easier:

Adds a menu for listing mass mailing emails and events
Add a boolean field email_bounced to partners and mail contacts. To easy filter them and do manual operations: call the partner, remove the partner, opt-out, ... Also prepare easy inherited methods for programming automatic operations in customer addon.

pedrobaeza · 2016-10-11T17:30:17Z

mail_tracking/models/mail_tracking_email.py

-                    tracking_email.recipient_address)
-                if partners:
-                    partners.email_score_calculate()
+                # partners = self.tracking_ids_recalculate(


Why not make it non stored?

antespi · 2016-10-13T06:48:59Z

We are running this in one production instance, if works well, then I'll add new tests to improve codecov

yajo · 2016-10-13T07:48:28Z

mail_tracking/controllers/main.py

+            raise e
+        finally:
+            if not current:
+                env.cr.close()


Not sure if you can use with env.cr: or with env.cr.savepoint():

This pattern is equivalent to with, but in this case if more explicit and easier to debug if we need it in future. See: http://effbot.org/zone/python-with-statement.htm

I don't see a functional difference though? This pattern fails Lint & I am not sure that debugging is a good enough reason to keep it.

Thanks @lasley, I didn't read transaction related guidelines before. In a multi-db instance we need to get a new env (and cursor) because ORM can initialize it (several databases, none selected). But I can make it better following guidelines. I'm doing it right now

yajo · 2016-10-13T07:54:25Z

Could you please add some docstrings and comments, specially in the performance-critical parts?

lasley · 2016-10-13T09:26:07Z

mail_tracking/controllers/main.py

+            raise e
+        finally:
+            if not current:
+                env.cr.close()


I don't see a functional difference though? This pattern fails Lint & I am not sure that debugging is a good enough reason to keep it.

lasley · 2016-10-13T09:27:11Z

mail_tracking/models/mail_tracking_email.py

@@ -23,14 +24,19 @@ class MailTrackingEmail(models.Model):
    _rec_name = 'display_name'
    _description = 'MailTracking email'

+    # This table is going to growth fast and to infinite, so we index:


s/growth/grow

lasley · 2016-10-13T09:27:43Z

mail_tracking/models/mail_tracking_email.py

-                        tracking_field: [(5, False, False)]
-                    })
-        return objects
+    def _email_score_tracking_filter(self, domain, order='time desc',


@api.model

lasley · 2016-10-13T09:29:48Z

mail_tracking/models/mail_tracking_email.py

        if score > 100.0:
            score = 100.0
+        if score < 0.0:


elif - save some resources - if it already matched the 100, it definitely won't be less than 0

You're right

lasley · 2016-10-13T09:30:59Z

mail_tracking/models/mail_tracking_email.py

        score = 50.0
-        trackings = self._email_score_tracking_filter()
-        for tracking in trackings:
+        for tracking in self:
            if tracking.state in ('error',):


This would be cleaner & more efficient in a dictionary:

states = { 'error': -50.0, 'opened': 5.0, ... } score += states.get(tracking.state, 0.0)

lasley · 2016-10-13T09:32:53Z

mail_tracking/models/mail_tracking_email.py

-                    partners.email_score_calculate()
+                    # Commit to DB to release exclusive lock
+                    if not testing:
+                        self.env.cr.commit()  # pragma: no cover


Why can we not use savepoint here?

when using savepoint, if everything is going well (no exceptions), then transaction is committed?
And, if we are in testing mode, transaction is committed too?

Hmmm ok looking at the intent here, we may not want a Savepoint. It's not immediately committing the transaction, and will roll back entirely if another transaction afterwards fails (assuming that transaction is also not within a savepoint).

Think of it like a save-guard - if the transaction within the Savepoint fails, it will rollback just within that savepoint. If the transaction fails outside of a savepoint, all mutations & savepoints in that transaction will fail.

Note this is a Postgres feature, so it's significantly easier to play with in direct SQL if you're curious. Surprisingly, the Postgres docs on Savepoint are pretty nice & the Odoo implementation matches well.

Where is the exclusive lock that is mentioned in the comments actually created?

I can see this Odoo code: https://github.com/odoo/odoo/blob/d81258ab8e0dcb6c0df3a47ab8084ec99b7d3e24/openerp/sql_db.py#L390
And this PostgreSQL doc: https://www.postgresql.org/docs/9.1/static/sql-release-savepoint.html

According to that, RELEASE SAVEPOINT just remove the savepoint. But in this case, I need to have the event (and mail state) visible to other isolated cursors and release exclusivelock to mail_tracking_email and mail_tracking_event tables. With this commit, then other writes (for instance to res_partner table) can be done after reading mail_tracking_email or mail_tracking_event tables.

After thinking twice, I remove this commit. Now the addon is not reading mail_tracking_email nor email_tracking_event tables after create/write on them, so I don't need this.

lasley · 2016-10-13T09:33:58Z

mail_tracking/models/res_partner.py

    tracking_emails_count = fields.Integer(
-        string="Tracking emails count", store=True, readonly=True,
+        string="Tracking emails count", readonly=True, store=False,


AFAIK computed, non-stored attrs without an inverse are implicitly read only

lasley · 2016-10-13T09:39:44Z

mail_tracking_mass_mailing/hooks.py

@@ -5,7 +5,7 @@
 import logging
 try:
    from openerp.addons.mail_tracking.hooks import column_add_with_value
-except ImportError:
+except ImportError:  # pragma: no cover


Please remove this pragma ~~& vote on OCA/maintainer-quality-tools#372~~

Already merged, thanks 😄

antespi · 2016-10-13T11:06:31Z

@lasley Thanks for your review. Changes done

lasley · 2016-10-13T11:08:22Z

mail_tracking/controllers/main.py

+                with reg.cursor() as new_cr:
+                    new_env = api.Environment(new_cr, SUPERUSER_ID, {})
+                    res = callback(env, tracking_id, event_type, **kw)
+                    new_env.cr.commit()


Seeing as we've confirmed that we're awesome and doing things right, we'll need to disable lint on this line (I'm assuming, wait for Travis to find out for sure):

new_env.cr.commit() #pylint: disable=E8102

This check is for avoiding bad commit() calls without control. If you are sure about the use, then we can put the pylint disable sentence.

lasley

One note on pylint, but we'll wait for Travis for confirmation

antespi · 2016-10-13T11:17:15Z

pylint is cool 😄

antespi · 2016-10-13T16:10:29Z

These improvements have been running well in a production instance today, with one mass mailing sending and Mailgun webhooks been received and 6 workers.

rafaelbn

OK, thanks

pedrobaeza · 2016-10-13T17:57:12Z

mail_tracking/controllers/main.py

@@ -35,49 +48,37 @@ def _request_metadata(self):
            'ua_family': request.user_agent.browser or False,
        }

+    def _tracking_open(self, env, tracking_id, event_type, **kw):
+        tracking_email = env['mail.tracking.email'].search([
+            ('id', '=', tracking_id),


Use search_count for optimizing this (see the tip in https://www.odoo.com/documentation/8.0/reference/orm.html#common-orm-methods)

I use tracking_email recordset after that

pedrobaeza · 2016-10-13T18:00:36Z

mail_tracking/models/res_partner.py

        compute="_compute_tracking_emails_count")
+    email_bounced = fields.Boolean(string="Email bounced", index=True)


Don't put string, as it's automatically inferred from the variable name.

pedrobaeza · 2016-10-13T18:00:45Z

mail_tracking/models/res_partner.py

    email_score = fields.Float(
-        string="Email score", readonly=True, default=50.0)
+        string="Email score", readonly=True,


The same about the string attribute

[IMP] mail_tracking performance and bounce process

antespi added the work in progress label Oct 11, 2016

antespi added this to the 8.0 milestone Oct 11, 2016

antespi changed the title ~~[IMP] Performance issues in mail_tracking addons~~ [8.0][IMP] Performance issues in mail_tracking addons Oct 11, 2016

pedrobaeza reviewed Oct 11, 2016

View reviewed changes

antespi force-pushed the 8.0-mail_tracking_performance branch 6 times, most recently from 3b4f687 to e456ad5 Compare October 12, 2016 23:09

[IMP] mail_tracking performance and bounce process

718f884

antespi force-pushed the 8.0-mail_tracking_performance branch from e456ad5 to 718f884 Compare October 13, 2016 07:00

antespi mentioned this pull request Oct 13, 2016

[9.0][IMP] mail_tracking performance and bounce process #103

Merged

pedrobaeza mentioned this pull request Oct 13, 2016

[FIX][mail_tracking] display_name of tracking_emails with no subject #104

Closed

yajo reviewed Oct 13, 2016

View reviewed changes

Comments and improve test coverage

2afe69c

lasley suggested changes Oct 13, 2016

View reviewed changes

Thanks to lasley comments

b301ecb

Remove useless store=False

147f4e3

antespi added needs review and removed work in progress labels Oct 13, 2016

lasley reviewed Oct 13, 2016

View reviewed changes

lasley approved these changes Oct 13, 2016

View reviewed changes

Fix typo when new environment

e00d331

rafaelbn approved these changes Oct 13, 2016

View reviewed changes

pedrobaeza requested changes Oct 13, 2016

View reviewed changes

Comments by pedrobaeza

70addf6

pedrobaeza approved these changes Oct 13, 2016

View reviewed changes

pedrobaeza merged commit 55250b9 into OCA:8.0 Oct 13, 2016

pedrobaeza deleted the 8.0-mail_tracking_performance branch October 13, 2016 20:51

antespi removed the needs review label Oct 13, 2016

cubells pushed a commit to Tecnativa/social that referenced this pull request Jan 3, 2017

Performance issues in mail_tracking addons (OCA#97)

e5edea0

[IMP] mail_tracking performance and bounce process

cubells pushed a commit to Tecnativa/social that referenced this pull request Jan 3, 2017

Performance issues in mail_tracking addons (OCA#97)

667fd13

[IMP] mail_tracking performance and bounce process

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[8.0][IMP] Performance issues in mail_tracking addons #97

[8.0][IMP] Performance issues in mail_tracking addons #97

antespi commented Oct 11, 2016 •

edited

Loading

pedrobaeza Oct 11, 2016

antespi commented Oct 13, 2016

yajo Oct 13, 2016

antespi Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016

antespi Oct 13, 2016 •

edited

Loading

yajo commented Oct 13, 2016

lasley Oct 13, 2016

lasley Oct 13, 2016

lasley Oct 13, 2016

lasley Oct 13, 2016

antespi Oct 13, 2016

lasley Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016

antespi Oct 13, 2016

lasley Oct 13, 2016 •

edited

Loading

antespi Oct 13, 2016

antespi Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016 •

edited

Loading

antespi Oct 13, 2016

lasley Oct 13, 2016 •

edited

Loading

antespi Oct 13, 2016

antespi commented Oct 13, 2016

lasley Oct 13, 2016

antespi Oct 13, 2016

pedrobaeza Oct 13, 2016

lasley left a comment

antespi commented Oct 13, 2016

antespi commented Oct 13, 2016

rafaelbn left a comment

pedrobaeza Oct 13, 2016

antespi Oct 13, 2016

pedrobaeza Oct 13, 2016

pedrobaeza Oct 13, 2016

		compute="_compute_tracking_emails_count")
		email_bounced = fields.Boolean(string="Email bounced", index=True)

[8.0][IMP] Performance issues in mail_tracking addons #97

[8.0][IMP] Performance issues in mail_tracking addons #97

Conversation

antespi commented Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

antespi commented Oct 13, 2016

Choose a reason for hiding this comment

antespi Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

yajo commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

lasley Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasley left a comment

Choose a reason for hiding this comment

antespi commented Oct 13, 2016

antespi commented Oct 13, 2016

rafaelbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antespi commented Oct 11, 2016 •

edited

Loading

antespi Oct 13, 2016 •

edited

Loading

antespi Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016 •

edited

Loading

antespi Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016 •

edited

Loading

lasley Oct 13, 2016 •

edited

Loading