502 remediation #5963

smithellis · 2024-04-17T13:48:51Z

Optimize db queries for 502

In send_message in utils.py:

Reduce number of queries
Prefetch group members
One query for users_to_email

In MultiUsernameorGroupnameField in form_fields.py

Get users and groups in one query each
A single pass through to collect found_names

Refactor _add_recipients

Mainly readability

In `send_message` in `utils.py`: * Reduce number of queries * Prefetch group members * One query for users_to_email In `MultiUsernameorGroupnameField` in `form_fields.py` * Get users and groups in one query each * A single pass through to collect `found_names`

kitsune/messages/views.py

kitsune/messages/utils.py

kitsune/messages/jinja2/messages/outbox.html

escattone · 2024-04-18T15:36:40Z

kitsune/messages/utils.py

+    inbox_messages = [
+        InboxMessage(sender=sender, to=user, message=text) for user in all_recipients_of_message
+    ]
+    messages = InboxMessage.objects.bulk_create(inbox_messages)


I like the bulk_create() change! I noticed however that you're not setting the to_group field on each of the InboxMessage's like you used to. Are we getting rid of to_group on the InboxMessage? If not, I wonder if we could do a bulk_update() of the to_group field? That would be nice, but not sure if the bulk_update works with many-to-many fields.

kitsune/messages/utils.py

kitsune/messages/jinja2/messages/outbox.html

escattone · 2024-04-18T21:08:30Z

kitsune/messages/utils.py

-    outbox_message.to.set(users)
+    for obj in to:
+        if isinstance(obj, User):
+            usernames.append(obj)


Suggested change

usernames.append(obj)

usernames.append(obj.username)

Also, since we want the actual User objects later for setting the to field of the OutboxMessage, can't we just collect those here -- in addition to the usernames above -- by creating another list called users and doing a users.append(obj) here?

I think this is bad naming on behalf of a cut and paste - these are objects being collected, not names. Even my comment is broken here - I'll fix this up. But we get the objects separated here.

escattone · 2024-04-18T21:08:44Z

kitsune/messages/utils.py

+        if isinstance(obj, User):
+            usernames.append(obj)
+        else:
+            group_names.append(obj)


Suggested change

group_names.append(obj)

group_names.append(obj.name)

escattone · 2024-04-18T21:13:38Z

kitsune/messages/utils.py

+        if isinstance(obj, User):
+            usernames.append(obj)
+        else:
+            group_names.append(obj)


Since we want the actual Group objects later for setting the to_group of the OutboxMessage, can't we just collect those here -- in addition to the group_names above -- by creating another list called groups and doing a groups.append(obj) here?

Yes - see note above. Bad naming...I'll fix.

escattone · 2024-04-18T21:14:33Z

kitsune/messages/utils.py

+        else:
+            group_names.append(obj)
+
+    groups_qs = Group.objects.filter(name__in=group_names)


I'm thinking we can delete this line, and use the groups list above instead.

escattone · 2024-04-18T21:26:00Z

kitsune/messages/utils.py

+    users_qs = User.objects.filter(username__in=usernames).values_list("id", flat=True)
+
+    # Group users' pks only - could be a lot of them
+    group_users_qs = User.objects.filter(groups__in=groups_qs).values_list("id", flat=True)

-    if groups:
-        # Add the groups from the To field to the message
-        outbox_message.to_group.set(groups)
+    # Resolve all unique user ids, including those in groups
+    # but keep up with users from groups
+    to_users = set(users_qs)
+    group_users = set(group_users_qs)
+    all_recipients_of_message = to_users.union(group_users)


I'm thinking we could replace all of this with a single query:

all_recipients_of_message = set(User.objects.filter(Q(username__in=usernames) | Q(groups__name__in=group_names)).values_list("id", flat=True))

We have to keep up with the users that came from the to field and the users that came from the groups from the to field. So capturing the pks in separate sets will matter later. We don't want to set the to in an outbox message to every user; just to the users that were entered individually.

escattone · 2024-04-18T21:27:29Z

kitsune/messages/utils.py

-    set_of_user_pks_to_email_private_message = set(
+    # Create the outbox message
+    outbox_message = OutboxMessage.objects.create(sender=sender, message=text)
+    to_users = User.objects.filter(id__in=to_users)


I don't think we need to_users since we'd already have the users list from my comments above.

kitsune/messages/utils.py

escattone · 2024-04-18T21:32:21Z

kitsune/messages/utils.py

        Setting.objects.filter(
-            user__in=receivers, name="email_private_messages", value=True
-        ).values_list("user__pk", flat=True)
+            user__in=to_users, name="email_private_messages", value=True


Here we want include all users, so the users from the groups as well, so I think this should be:

Suggested change

user__in=to_users, name="email_private_messages", value=True

user__in=all_recipients_of_message, name="email_private_messages", value=True

We are disabling e-mail (not messaging, just email) for users in Groups for now, pending some follow up.

escattone · 2024-04-18T21:57:29Z

kitsune/messages/utils.py

+    InboxMessage.objects.bulk_create(inbox_messages)
+
+    # Assuming messages are fetched again with IDs if necessary
+    for user_id in users_to_email:
+        inbox_messages = InboxMessage.objects.filter(sender=sender, to__id=user_id, message=text)
+        for message in inbox_messages:
+            email_private_message.delay(inbox_message_id=message.id)


The bulk_create returns the list of created InboxMessage's -- so they each have a valid id -- but we're not using that list.

Instead of querying for each InboxMessage for each of the users_to_email, which could be a large number of queries, over 2k for the Contributors group for example, it might be better to use that list of created InboxMessage's to create a dictionary that maps the to_id to the id of each of the messages, and then we can use that mapping to quickly get the message id for each of the users we need to email.

So I'm thinking of something like:

# Create a mapping from the user id to the message id, so just integer to integer. recipient_id_to_message_id = {msg.to_id : msg.id for msg in InboxMessage.objects.bulk_create(inbox_messages)} # Use the mapping to get the message id for each user. for user_id in users_to_email: if (message_id := recipient_id_to_message_id.get(user_id)): email_private_message.delay(inbox_message_id=message_id)

This was sketchy in the docs vs. info from other sources. Good to know!

Implemented!

* Drop to_group form inboxmessage model * Use `values_list` for large queries * celery for email

escattone

r+wc

escattone · 2024-04-19T19:45:15Z

kitsune/messages/jinja2/messages/outbox.html

+                      {%- else -%}
+                        {{ user.profile.display_name }}
+                      {%- endif -%}
+                    {%- if message.recipients_count>1 -%}, ...{% endif %}


Nit. Add spaces around >.

escattone · 2024-04-19T19:45:45Z

kitsune/messages/jinja2/messages/outbox.html

+                            {% set group_slug = profile.slug %}
+                            <a href="{{ url('groups.profile', group_slug) }}">{{ group }}</a>
+                          {%- endfor -%}
+                          {% if message.to_groups_count>1 %}, ...{% endif %}


Nit. Add spaces around >.

escattone · 2024-04-19T19:47:09Z

kitsune/messages/jinja2/messages/outbox.html

+                            {% set group_slug = profile.slug %}
+                            <a href="{{ url('groups.profile', group_slug) }}">{{ group }}</a>


Nit.

Suggested change

{% set group_slug = profile.slug %}

<a href="{{ url('groups.profile', group_slug) }}">{{ group }}</a>

<a href="{{ url('groups.profile', profile.slug) }}">{{ group }}</a>

escattone · 2024-04-19T19:51:06Z

kitsune/messages/utils.py

        Setting.objects.filter(
-            user__in=receivers, name="email_private_messages", value=True
-        ).values_list("user__pk", flat=True)
+            user__in=users, name="email_private_messages", value=True


Just wanted to record that I'd prefer this to use all_recipients_of_message instead of just the users, but that's for another discussion.

smithellis added 2 commits April 17, 2024 09:42

refactor _add_recipients

b56ce6f

smithellis force-pushed the 502-remediation branch 2 times, most recently from e600cc1 to fc930f3 Compare April 17, 2024 20:55

inbox and outbox optimizations

8d90829

smithellis force-pushed the 502-remediation branch from fc930f3 to 8d90829 Compare April 17, 2024 21:12

akatsoulas requested changes Apr 18, 2024

View reviewed changes

use bulk_create

85bda91

escattone reviewed Apr 18, 2024

View reviewed changes

smithellis force-pushed the 502-remediation branch 3 times, most recently from a2981e5 to 82c249a Compare April 18, 2024 20:21

escattone requested changes Apr 18, 2024

View reviewed changes

Further speed enhancements

de85f24

* Drop to_group form inboxmessage model * Use `values_list` for large queries * celery for email

smithellis force-pushed the 502-remediation branch from cb970fc to de85f24 Compare April 19, 2024 13:37

smithellis added 3 commits April 19, 2024 12:31

fix utils

14d1731

fix outbox

76a8a26

clean up

a41ee80

escattone approved these changes Apr 19, 2024

View reviewed changes

lint issues

e4d9e16

smithellis marked this pull request as ready for review April 19, 2024 20:55

smithellis merged commit dfac0c2 into mozilla:main Apr 19, 2024
2 checks passed

smithellis deleted the 502-remediation branch April 19, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

502 remediation #5963

502 remediation #5963

smithellis commented Apr 17, 2024

escattone Apr 18, 2024

escattone Apr 18, 2024

escattone Apr 18, 2024 •

edited

Loading

smithellis Apr 18, 2024

escattone Apr 18, 2024

escattone Apr 18, 2024

smithellis Apr 18, 2024

escattone Apr 18, 2024

escattone Apr 18, 2024

smithellis Apr 18, 2024

escattone Apr 18, 2024

escattone Apr 18, 2024

smithellis Apr 18, 2024

escattone Apr 18, 2024 •

edited

Loading

smithellis Apr 19, 2024

smithellis Apr 19, 2024

escattone left a comment

escattone Apr 19, 2024

smithellis Apr 19, 2024

escattone Apr 19, 2024

smithellis Apr 19, 2024

escattone Apr 19, 2024

smithellis Apr 19, 2024

escattone Apr 19, 2024

smithellis Apr 19, 2024

	user__in=to_users, name="email_private_messages", value=True
	user__in=all_recipients_of_message, name="email_private_messages", value=True

		{% set group_slug = profile.slug %}
		<a href="{{ url('groups.profile', group_slug) }}">{{ group }}</a>

	{% set group_slug = profile.slug %}
	<a href="{{ url('groups.profile', group_slug) }}">{{ group }}</a>
	<a href="{{ url('groups.profile', profile.slug) }}">{{ group }}</a>

502 remediation #5963

502 remediation #5963

Conversation

smithellis commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escattone Apr 18, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

escattone Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escattone 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

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

escattone Apr 18, 2024 •

edited

Loading

escattone Apr 18, 2024 •

edited

Loading