feat(issues): Add GroupActionLogEntry#115771
Conversation
|
@shashjar fyi |
|
This PR has a migration; here is the generated SQL for for --
-- Create model IssueActionLogEntry
--
CREATE TABLE "sentry_issueactionlogentry" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "original_group_id" bigint NULL, "type" integer NOT NULL CHECK ("type" >= 0), "actor_type" integer NOT NULL CHECK ("actor_type" >= 0), "actor_id" bigint NOT NULL, "data" jsonb NOT NULL, "date_added" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL, "idempotency_key" varchar(64) NULL);
CREATE UNIQUE INDEX CONCURRENTLY "uniq_issueactionlogentry_group_idempotency_key" ON "sentry_issueactionlogentry" ("group_id", "idempotency_key") WHERE "idempotency_key" IS NOT NULL;
CREATE INDEX CONCURRENTLY "sentry_issu_group_i_ff3639_idx" ON "sentry_issueactionlogentry" ("group_id", "date_added", "id"); |
|
Okay, potentially hot take that you can feel free to ignore: have we considered using "Group" instead of "Issue" throughout? (So it'd be GroupActionLog, etc.) The reason for this is pretty simple: consistency with everything else. All of our tables use Group instead of Issue (e.g. sentry_groupredirect), as do all of our Models (e.g. GroupRedirect)... generally we've used Issues for user-facing stuff but kept Group around internally. Personally I think the consistency here is worth it — but will leave this up to you. |
I find the argument for Group compelling; I used "Issue" since that's what all of the discussion prior to this PR has used. If I can force us to set policy on "Group is what we call it in code, not an old name we haven't managed to change", I will. |
|
This PR has a migration; here is the generated SQL for for --
-- Create model GroupActionLogEntry
--
CREATE TABLE "sentry_groupactionlogentry" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "original_group_id" bigint NULL, "type" integer NOT NULL CHECK ("type" >= 0), "actor_type" integer NOT NULL CHECK ("actor_type" >= 0), "actor_id" bigint NOT NULL, "data" jsonb NOT NULL, "date_added" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL, "date_updated" timestamp with time zone NOT NULL, "idempotency_key" varchar(64) NULL);
CREATE UNIQUE INDEX CONCURRENTLY "uniq_groupactionlogentry_group_idempotency_key" ON "sentry_groupactionlogentry" ("group_id", "idempotency_key") WHERE "idempotency_key" IS NOT NULL;
CREATE INDEX CONCURRENTLY "sentry_grou_group_i_cc465f_idx" ON "sentry_groupactionlogentry" ("group_id", "date_added", "id"); |
wedamija
left a comment
There was a problem hiding this comment.
Isn't this similar to the existing GroupHistory table?
It is fairly similar. Also very similar to Activity. Also a few others. |
|
Secondary database plan is here to move it later. I've started the process of getting it set up, it'll take a bit, and in the meantime I'll configure the getsentry router to yell at us if it is used in any context that is incompatible with separate database. |
| date_added = models.DateTimeField(db_default=Now()) | ||
|
|
||
| # Primarly intended for debugging; not intended to be relied upon | ||
| # for invalidation. | ||
| date_updated = models.DateTimeField(auto_now=True) |
There was a problem hiding this comment.
Nit: Should we add these via using the DefaultFieldsModel base class instead?
There was a problem hiding this comment.
Can't do that. We need to be able to backfill easily, and DefaultFieldsModel specifically doesn't let you pick your own date_added on insert. Might be some way around it, but given that I'm not sold on us keeping date_updated, doesn't seem valuable.
| # DB-defaulted; backfill code may pass an explicit value. | ||
| date_added = models.DateTimeField(db_default=Now()) |
There was a problem hiding this comment.
Will we evict these over time, or just rely on Group to cascade delete? If we want to expire them after a period of time, an index could be useful here
There was a problem hiding this comment.
We intend to keep to keep log entries for a group until the group goes away.
I think we may drop old entries in misbehaving groups in some cases, but the group id-based index should be good enough for those cases if we go that route.
| app_label = "sentry" | ||
| db_table = "sentry_groupactionlogentry" | ||
| indexes = [ | ||
| models.Index(fields=["group_id", "date_added", "id"]), |
There was a problem hiding this comment.
What's the intent of this index? jfyi, usually if you need to filter on two really high cardinality columns an index isn't going to help too much, since there typically won't be more than one row in each leaf.
The main reason this can be helpful is if you're making a query that only returns group_id, date_added, id. If that's not the intent, probably I'd just remove id here and keep the index size a little smaller.
There was a problem hiding this comment.
A good point. id should be a tie-breaker very rarely, so it's probably not worth index size. This is likely to be our hottest index by far, but most of it will also involve full row fetches, so the cost of swapping a few rows should be noise. Maybe if we were doing a lot more "but is it fresh?" checking it'd pay off to include the id.... but probably not.
I'm not trying too hard to optimize anything at the moment, so there's an appeal of "the index is our sort order", but I think this is reasonable to do.
| group_id = BoundedBigIntegerField() | ||
| # The project the group belongs to. | ||
| project_id = BoundedBigIntegerField() |
There was a problem hiding this comment.
You should probably add indexes on all of these fk columns
There was a problem hiding this comment.
Not sure, honestly. For the live data paths I think we're really only expecting to care about the ordering index (on a per-group basis), deleting by group, and merging by group.
Every additional column is potentially interesting for one-off querying, but also has no particular use case yet.
I guess if we have to pick between "index now and drop later if we don't need it" and "index later if we need it", the first is easier, but I suspect the ones we'll find we need will be composites. For now, I'll just do project and we'll see what else comes up.
There was a problem hiding this comment.
Since group_id is part of another compound index that's probably fine to leave out. For project_id and other ids, My concern here is that typically we assume that fks are indexed when we write queries, and so it's an easy footgun that we can run into.
I would say though that if we're not going to be querying by project_id, probably we could just remove it entirely and rely on the relation to group_id instead.
There was a problem hiding this comment.
I'm not sure if I've made this clear enough, but part of the interest in project_id specifically is that this is going to be in another db. So, no FKs, and presumably no expectation of indexing on them, but also we can't rely on other existing data joins to make things easy for us.
I figured (project, group) would let us track project mappings so project-level work is practical at all and also let us join to the other index for not-guaranteed-timeout project-time window analysis. I'm not entirely sure that's reasonable, and assume we'll need a few migrations to get it right, so I'm not too worried about it so long as the theory is plausible.
| # The project the group belongs to. | ||
| project_id = BoundedBigIntegerField() | ||
| # The group_id before any merges, if this entry was migrated. | ||
| original_group_id = BoundedBigIntegerField(null=True) |
There was a problem hiding this comment.
Probably this also needs an index?
There was a problem hiding this comment.
Maybe? I think the aim is to view the history as new once we merge something in, so I don't know what access patterns we'll need that care about original id that can't be replaced with existing id mappings from other tables.
| actor_type = BoundedPositiveIntegerField( | ||
| choices=[(t.value, t.name) for t in GroupActorType], | ||
| ) | ||
| actor_id = BoundedBigIntegerField() |
There was a problem hiding this comment.
as above, unclear what we'll need, I don't want to go too crazy on indices when we can add them later, so I'm inclined to skip this one for now.
wedamija
left a comment
There was a problem hiding this comment.
migration lgtm other than various index comments which are non-blocking
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 02312cd1232ff7af7844f100ae1fc584343c5841. Configure here.
| actor_type=GroupActorType.SYSTEM, | ||
| actor_id=0, | ||
| data={}, | ||
| ) |
There was a problem hiding this comment.
Deletion test missing source
Medium Severity
The new GroupActionLogEntry.objects.create in _create_event_with_many_group_children omits source, but GroupActionLogEntry.source is a non-null CharField with no default. Fixture setup fails before the deletion assertion runs.
Reviewed by Cursor Bugbot for commit 02312cd1232ff7af7844f100ae1fc584343c5841. Configure here.
|
This PR has a migration; here is the generated SQL for for --
-- Create model GroupActionLogEntry
--
CREATE TABLE "sentry_groupactionlogentry" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "original_group_id" bigint NULL, "type" integer NOT NULL CHECK ("type" >= 0), "actor_type" integer NOT NULL CHECK ("actor_type" >= 0), "actor_id" bigint NOT NULL, "source" varchar(64) NOT NULL, "data" jsonb NOT NULL, "date_added" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL, "date_updated" timestamp with time zone NOT NULL, "idempotency_key" varchar(64) NULL);
CREATE UNIQUE INDEX CONCURRENTLY "uniq_groupactionlogentry_group_idempotency_key" ON "sentry_groupactionlogentry" ("group_id", "idempotency_key") WHERE "idempotency_key" IS NOT NULL;
CREATE INDEX CONCURRENTLY "sentry_grou_group_i_cc465f_idx" ON "sentry_groupactionlogentry" ("group_id", "date_added", "id");
CREATE INDEX CONCURRENTLY "sentry_grou_project_6ed08e_idx" ON "sentry_groupactionlogentry" ("project_id", "group_id"); |
yuvmen
left a comment
There was a problem hiding this comment.
turned out nice indeed! just a couple comments, honestly I think with the migration and model being here, you could have an easier life and maybe a bit safer to land those before all the instrumentation tweaks. But since its already done here and I dont think there is much risk involved I guess its fine.
|
|
||
| source = resolve_action_source(request) | ||
| actor_id = acting_user.id if acting_user else None | ||
| actor = GroupActionActor.user(acting_user.id) if acting_user else SYSTEM_ACTOR |
| cause = e.__cause__ | ||
| constraint = getattr(getattr(cause, "diag", None), "constraint_name", None) | ||
| if constraint == "uniq_groupactionlogentry_group_idempotency_key": | ||
| raise DuplicateActionError( |
There was a problem hiding this comment.
theres a question here about how we want to handle instrumentation failures such as these. I dont know if Activity today has the same protections, but I think its not obvious we want to raise and fail the entire flow that called this in this circumstance.
You could argue if the entire flow is allowed it doesnt make sense for the ActionLog to be the thing preventing it
There was a problem hiding this comment.
I think there's a strong case for treating it as successful on conflict in some cases (eg if last entry was a conflict and data matches, easy choice. if very different or years old, treating as benign double write seems wrong..).
I'm defaulting to strict here on principle, but I've been thinking we may want grouptype overrideable method for "is this the same?" to use here. We may want to use idempotency key more actively than Activity does.
I think aggregators and user views should be resilient to redundancy either way, so I'm not sure it's worth dwelling on yet, but I'm interested.
There was a problem hiding this comment.
I guess I conflated two concerns here -
one is what you mention of like "when do we decide a duplicate action is an error" and I agree with your approach on it, but the other is "should we allow errors from ActionLog publish to interrupt the main flow". I guess because the ActionLog is not just an audit log, but will represent the source of truth for state, it cannot be allowed to fail and it should fail the entire action, but then we might need to make many of these actions happen atomically with it right? We don't have to address it now but something to think about, maybe you already considered it
There was a problem hiding this comment.
Yeah, I think in the majority of cases, actions are true things that happened, not triggers for things to happened, and so they should not be expected to fail. The primary use case for the idempotency key imo is to allow us a way of reporting things we don't necessarily have the ability to manage atomically (like external or historical actions) and catch ourselves before claiming they happened twice.
yuvmen
left a comment
There was a problem hiding this comment.
lgtm overall, I think before we turn on writes we should consider the point about atomicity, but we can safely merge this, very nice work 👍
|
This PR has a migration; here is the generated SQL for for --
-- Create model GroupActionLogEntry
--
CREATE TABLE "sentry_groupactionlogentry" ("id" bigint NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "group_id" bigint NOT NULL, "project_id" bigint NOT NULL, "original_group_id" bigint NULL, "type" integer NOT NULL CHECK ("type" >= 0), "actor_type" integer NOT NULL CHECK ("actor_type" >= 0), "actor_id" bigint NOT NULL, "source" varchar(64) NOT NULL, "data" jsonb NOT NULL, "date_added" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL, "date_updated" timestamp with time zone NOT NULL, "idempotency_key" varchar(64) NULL);
CREATE UNIQUE INDEX CONCURRENTLY "uniq_groupactionlogentry_group_idempotency_key" ON "sentry_groupactionlogentry" ("group_id", "idempotency_key") WHERE "idempotency_key" IS NOT NULL;
CREATE INDEX CONCURRENTLY "sentry_grou_group_i_cc465f_idx" ON "sentry_groupactionlogentry" ("group_id", "date_added", "id");
CREATE INDEX CONCURRENTLY "sentry_grou_project_6ed08e_idx" ON "sentry_groupactionlogentry" ("project_id", "group_id"); |
Adapt the external-issue instrumentation (ID-1572) to the GroupActionLog backbone from #115771, scoped to first-party integrations: - Add CREATE/LINK/UNLINK_EXTERNAL_ISSUE to GroupActionType + typed GroupAction subclasses (Create/Link/UnlinkExternalIssueAction, each carrying provider + external_issue_key). - Convert the GroupIntegrationDetailsEndpoint create/link/unlink call sites to the new interface (positional typed action + GroupActionActor/ SYSTEM_ACTOR, no metadata dict). - Defer Sentry App platform external issues (PlatformExternalIssue) to a follow-up: attach is control-silo -> region via RPC and needs a new Sentry-App source, so instrumenting only its unlink here would be incoherent.
…on interface Convert the ID-1573 instrumentation to the GroupActionLog backbone from #115771: action_context_scope now takes a GroupActionActor (GroupActionActor.user(...) / SYSTEM_ACTOR) instead of actor_id, and update_group_status publishes typed Resolve/Archive/Unresolve actions. Refs ID-1573
…116781) Instruments the first-party integration external-issue endpoints (`GroupIntegrationDetailsEndpoint` create/link/unlink) with the action log, on top of the `GroupActionLog` backbone from #115771. Adds `CREATE/LINK/UNLINK_EXTERNAL_ISSUE` to `GroupActionType` plus typed `GroupAction` subclasses (each carrying `provider` + `external_issue_key`), and records them via `publish_action(...)` with source/actor attribution. Logs/metrics only today (DB write gated behind `issues.action-log.write-to-db`). The `UNLINK` action only fires when a `GroupLink` row was actually removed, so unlinking nothing stays a no-op. **Scope: first-party integrations only.** Sentry App platform external issues (`PlatformExternalIssue`) are deferred to ID-1611 — their attach path is control-silo → region via RPC and needs a new Sentry App source, so instrumenting only their unlink here would be incoherent. Refs ID-1572



Adds GroupActionLog entry backend for the action_log code, disabled by default.
This adjusts the interface to be a bit more strict and object-based, but I think the ergonomics are no worse.
The defined Actions are still to be considered provisional, which is part of why we're not logging them yet.
Fixes ISWF-2790.