✨ feat(notification): add support for open_period_start for issue alert thread repo#83601
Conversation
github-actions
Bot
added
the
Scope: Backend
| open_period_start=open_period_start, | ||
| ) | ||
|
|
||
| notification_with_period = NotificationMessage.objects.create( |
There was a problem hiding this comment.
is this meant to overwrite the notification_with_period defined above it?
There was a problem hiding this comment.
basically for metric issues and uptime issues, we want 1 thread per "open period" this tests creates 2 notifications, wit the same rule & action, but with different open periods. this means the parent should return the latest notification (with the latest open period).
There was a problem hiding this comment.
nit: maybe name it differently so the intent is a bit more clear?
| result_ids.append(parent_notification.id) | ||
|
|
||
| assert len(result_ids) == 1 | ||
| assert result_ids[0] == n1.id |
There was a problem hiding this comment.
why are you creating 2 of the same rows except for the open period, and why is it expected that the one with an earlier start time is the only parent notification returned? (sorry I lack context on this)
There was a problem hiding this comment.
i made an oopsie on this testcase, i updated it so it ensures that the later notification is the one this returns. what this is testing is that we only return the parent threads for the "latest" open period.
for example, imagine we had a scenario where a metric/uptime issue that opened, then closed --- few minutes later -- it opened again.
when we are updating the thread for it to resolve again, it should only update the latest thread, not the earlier thread.
cathteng
left a comment
cathteng
left a comment
There was a problem hiding this comment.
are we guaranteed to have the open period start time stored in the db = the open period start time from the issue?
| open_period_start=open_period_start, | ||
| ) | ||
|
|
||
| notification_with_period = NotificationMessage.objects.create( |
There was a problem hiding this comment.
nit: maybe name it differently so the intent is a bit more clear?
| query = self._model.objects.filter(group_id_filter & project_id_filter).filter( | ||
| self._parent_notification_message_base_filter() | ||
| open_period_start_filter = ( | ||
| Q(open_period_start=open_period_start) if open_period_start else Q() |
There was a problem hiding this comment.
should this be open_period_start__gte=open_period_start? is it possible that the seconds/milliseconds don't match up exactly?
There was a problem hiding this comment.
this shouldn't be the case, we pull the open_period_start from the Activity model, and we should be storing the exact time
the pr for the open period: #83689
iamrajjoshi
force-pushed
the
raj/slack-threads-ref/update-issue-alert-repo
branch
from
f6a0882 to
5ba2ab0
Compare
…lert thread repo (#83601)
3 participants
this adds support for the
open_period_startattribute that i just added to the notificationmessage model.i will then follow this up by updating the issue alert metric handler to use this additional field. i probably do it via a registry based on group type.
context from a comment:
basically for metric issues and uptime issues, we want 1 thread per "open period" this tests creates 2 notifications, with the same rule & action, but with different open periods. this means the parent should return the latest notification (with the latest open period).
spec: https://www.notion.so/sentry/Slack-Threads-Refactor-1618b10e4b5d807db67ae6d4d85247b9
contributes to: https://getsentry.atlassian.net/browse/ACI-89