chore(aci): Add event_id column to GroupOpenPeriodActivity model #107305
Conversation
github-actions
bot
added
the
Scope: Backend
mifu67
left a comment
mifu67
left a comment
There was a problem hiding this comment.
I think this is reasonable. Should we remove the column from GroupOpenPeriod?
|
This PR has a migration; here is the generated SQL for for --
-- Add field event_id to groupopenperiodactivity
--
ALTER TABLE "sentry_groupopenperiodactivity" ADD COLUMN "event_id" varchar(32) NULL; |
Yes, since each GroupOpenPeriod has an associated "open" activity we shouldn't need one on the main model anymore |
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
lgtm, iirc this shouldn't be a post deploy (even if we dropping the event_id, since it's only a django change on the first pass) and adding a Null column should be super fast, even on a huge table like this -- might be good to confirm all that with a db-migration-owner tho.
| model_name="groupopenperiodactivity", | ||
| name="event_id", |
There was a problem hiding this comment.
can we also start the removal of the event_id on the GroupOpenPeriod table? (feels like that might get confusing sooner rather than later)
There was a problem hiding this comment.
Yeah, once I get this column added and event_id data coming in I'll remove the GroupOpenPeriod one.
It might be good if we, before dropping that column, transfer the event_id data from GroupOpenPeriod to the first activity entry. That way we don't lose the last month of event_ids
| value = models.IntegerField(null=True) | ||
| # The event ID that triggered this activity | ||
| event_id = models.CharField(max_length=32, null=True) | ||
| notification_uuid = models.UUIDField("notification_uuid", default=generate_random_uuid) |
There was a problem hiding this comment.
unrelated to this pr -- wonder why we have the notification uuid here vs using the WorfklowFireHistory? (cc @mifu67) iirc that would have the event_id + notification uuid so we can have a history of each time a workflow was triggered.
(not sure if this is replicated data or needed to represent a different notification uuid or ?)
There was a problem hiding this comment.
I don't think we use the notification ID on this table. I likely duplicated it from the IncidentActivity model.
…upopenperiodactivity-model
malwilley
deleted the
malwilley/chore/add-event-id-column-to-groupopenperiodactivity-model
branch
Related to #104549
Events can relate to the GroupOpenPeriod, but that was not sufficient. In the case where a detector goes from high priority to medium (or vice versa), we create a GroupOpenPeriodActivity instead. Without tracking the
event_idon the activity entry, we have no way to find the corresponding open period of an arbitrary event.Also see this decision log entry