Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix IntegrityErrors occuring when creating ResolutionNoteSlackMessage objects #2933

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Aug 31, 2023

What this PR does

Which issue(s) this PR fixes

Closes https://github.com/grafana/oncall-private/issues/1822

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

creating ResolutionNoteSlackMessage objects
@joeyorlando joeyorlando added the pr:no public docs Added to a PR that does not require public documentation updates label Aug 31, 2023
@joeyorlando joeyorlando requested a review from a team August 31, 2023 15:01
joeyorlando and others added 3 commits August 31, 2023 17:01
…y-errors' of github.com:grafana/oncall into jorlando/address-resolution-note-slack-message-integrity-errors
Comment on lines +86 to +90
indexes = [
models.Index(fields=["ts", "thread_ts", "alert_group_id"]),
models.Index(fields=["ts", "thread_ts", "slack_channel_id"]),
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added indices to speed up querying these objects. These are the two filter patterns we use when querying these objects.

Comment on lines +85 to +96
slack_thread_message, created = ResolutionNoteSlackMessage.objects.get_or_create(
ts=message_ts,
thread_ts=thread_ts,
alert_group=alert_group,
defaults={
"user": self.user,
"added_by_user": self.user,
"text": text,
"slack_channel_id": channel,
"permalink": permalink,
},
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main issue here seemed to be Slack sending duplicate events to our API endpoint, shortly after one another, which led to this issue.

Example from logs (check trace IDs e3691dda80bd9e67476ceafb68c90a8f/2917201084877566781 and 978e7af66c11d0a680ce8175afdbda6f/15112916125666851927)

According to Django's docs, the solution to this is to use get_or_create:

Returns a tuple of (object, created), where object is the retrieved or created object and created is a boolean specifying whether a new object was created.

This is meant to prevent duplicate objects from being created when requests are made in parallel, and as a shortcut to boilerplatish code. For example:

try:
    obj = Person.objects.get(first_name="John", last_name="Lennon")
except Person.DoesNotExist:
    obj = Person(first_name="John", last_name="Lennon", birthday=date(1940, 10, 9))
    obj.save()

Here, with concurrent requests, multiple attempts to save a Person with the same parameters may be made. To avoid this race condition, the above example can be rewritten using get_or_create() like so:

return
slack_thread_message.text = text
slack_thread_message.save()
slack_thread_message, created = ResolutionNoteSlackMessage.objects.get_or_create(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to check len(text) before creating ResolutionNoteSlackMessage. Too long text may cause issues either with saving it to the db or displaying it in Slack 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

do you happen to know why there is this condition?:

if slack_thread_message.added_to_resolution_note:

if that can be removed, we can still use get_or_create

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternative solution is to keep the separate get/create calls as it was, and add another try/except when creating, and catch any possible IntegrityErrors (but this seems a bit messier imo)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolutionNoteSlackMessage is a message from alert group thread that potentially can be added to resolution notes.

It looks like we post warning message about too long text only either for messages that had already been added to resolution notes or for new messages in thread 🤔
If we post normal message and then edit it to make it long without adding to resolution notes, there won't be any warnings and the "Add Resolution notes" modal window in Slack will still have the old version of the message.
I think we can remove the check if slack_thread_message.added_to_resolution_note and post warning message in any case.

@joeyorlando joeyorlando requested a review from Ferril September 1, 2023 07:01
"chat.postEphemeral",
channel=channel,
user=slack_user_identity.slack_id,
text=":warning: Unable to update the <{}|message> in Resolution Note: the message is too long ({}). "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change the message a bit. Something like "Unable to show the message in Resolution Notes..."

…y-errors' of github.com:grafana/oncall into jorlando/address-resolution-note-slack-message-integrity-errors
@joeyorlando joeyorlando added this pull request to the merge queue Sep 1, 2023
Merged via the queue into dev with commit 1d089aa Sep 1, 2023
@joeyorlando joeyorlando deleted the jorlando/address-resolution-note-slack-message-integrity-errors branch September 1, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants