feat(issues): Add GroupActionLog instrumentation for issue mutations#116347
Conversation
github-actions
Bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| from typing import Any | ||
|
|
||
| from sentry.api import client | ||
| from sentry.api.client import ApiClient |
There was a problem hiding this comment.
was getting typing errors on all these tool files, have no idea why when main has them go through fine. Eventually gave up and fixed the typing issue here, seems like import was too ambiguous
There was a problem hiding this comment.
Feel free to spin this off, should get an instant stamp.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
yuvmen
changed the title
kcons
left a comment
kcons
left a comment
There was a problem hiding this comment.
I don't have many useful things to say about the logic for determining Source or the granularity of it, as ultimately I think those things are very tied to what actually works combined with what we need, and I'm not on firm footing in either case.
I'm also not sure how strictly to consider this PR; my sense is that while it is a big footprint and the start of some critical stuff, we're mostly considering it placeholder for now. If the intend is to land it (which i hope it is; starting to actually log stuff and consider the practicalities of collecting the relevant data in context is fairly urgent), I do think we need to go a bit harder on docs and interface, though that may mean being explicitly non-committal more than being very careful.
In principle, I think "a short identifying string with some structure, not large in cardinality, carefully controlled in generation but tolerant in consumption (when we start consuming)" is the right model here for source, and I do think we want source.
So, with that and my interest in getting preliminary stuff with plausible interfaces in play, I conceptually approve.
| def publish_action( | ||
| *, | ||
| action: ActionType, | ||
| source: str, |
There was a problem hiding this comment.
Seems like this should be ActionSource.
I understand why it isn't; we want to be able to qualify things with prefixes to show it is a specific variant of a category of source.. but I don't think we want that to mean we accept a string.
I think it'd be nice to limit the set of legal ActionSources to those that we believe are valid, and know that by virtue of it being able to be constructed an ActionSource is valid; potentially be able to parse str to an ActionSource and know it is then valid. From the API perspective, this makes things simpler and harder to break. From a data management perspective, it means if it is published we think it is valid. The only real burden here is having an ActionSource representation that knows the rules. It could be a wrapped str with a regex, but I'm assuming it'd be some sort of SourceKind(StrEnum) akin to current ActionSource, along with an optional addition str with values restricted by SourceKind. I assume it'd have a __str__ method that sorta does ":".join[self.kind, self.variant] if self.variant else self.kind.
Maybe it's overkill, but interface-wise, easy, and implementation presumably one-shottable by an agent.
There was a problem hiding this comment.
yea I agree this should actually be ActionSource, and with your analysis here of the possible second type that includes variant. I think right now we cna go with this.
I think another possibility to what you suggested is we can keep it simpler and have the source variant be a part of the metadata. Like ActionSource has concrete categories that are well defined and low cardinality, if someone wants to add a more loose optional variant on top it can be in the data field and higher cardinality.
| PAGERDUTY = "pagerduty" | ||
| OPSGENIE = "opsgenie" | ||
| PERFORCE = "perforce" | ||
| UNKNOWN = "unknown" |
There was a problem hiding this comment.
Do you think there's value in distinguishing between 'UNKNOWN' and 'Something bad happened and we were unable to execute the appropriate logic'? Or is 'UNKNOWN' better understood as 'ERROR'?
There was a problem hiding this comment.
Not sure if you think we shouldn't save the action log in when we cant determine source? I mean UNKNOWN is an error state in a way for sure, like we have logic to determine source, if our rules for source fail I dont want to fail the whole flow and prevent the log from getting created, so saving it with UNKNOWN I think is a good representation of this case, would be odd to save with source=ERROR no? though not sure if thats what youre suggesting
There was a problem hiding this comment.
I was trying to clarify for myself what UNKNOWN means. One natural interpretation is "not one of these other ones that we recognize" but sounds like it's an in-band None. That seems reasonable to me; I'm hoping we can move toward an interface where nobody is really hand-picking ActionSource (generated by helpers write-side, opt-in predicates read-side) so it shouldn't matter much. No change needed (as a special value, a one-line doc wouldn't hurt, but I think we'll end up doing a clarification pass on most of these, so it can wait).
|
|
||
|
|
||
| def resolve_action_source(request: Request | None) -> str: | ||
| if request is None: |
There was a problem hiding this comment.
This fallback seems surprising. Are contexts where you might have a request common?
There was a problem hiding this comment.
yea this actually doesnt make much sense when I think about it, had in mind like SYSTEM cases will still call this with None, but as you say we are never unsure if we have request or not, will remove this
|
|
||
|
|
||
| @contextmanager | ||
| def action_context_scope(source: str, actor_id: int | None = None) -> Generator[None]: |
There was a problem hiding this comment.
With actor_id == None meaning SYSTEM, it seems safe to not have a default here. I'd read only setting source to mean only overriding source, not actor, if I saw it in code.
| ) | ||
|
|
||
|
|
||
| def publish_action_from_context( |
There was a problem hiding this comment.
I'm very interested in the context bits here.
It seems like a really helpful way for us to get coverage without having to thread through lots of extra data (with user it was maybe reasonable to expect we'd have it, but source is a different beast). I could imagine that we'd set up a context by default in most endpoints, and probably have a convention for threading it through task arguments so under normal circumstances a context could be assumed.
However, we don't get proof of availability, and refactors will break context and lead to errors we have to hope will be triaged.
It is possible to use static analysis to find cases of context use that don't have a provable source, but I don't know how hard that'd be to set up.
I think in general "_from_context" existing suggests that it's what most people should use outside of some narrow and shallow reporting cases; we should probably specify this if that's the case. Not necessarily in this change, but certainly in a v0 of what we expect anyone to use.
There was a problem hiding this comment.
yea agreed, this is the part that I was the most conflicted about. It does indeed come with this tradeoff you describe, but I think it is probably worth for the amount of clutter it saves. I am open to considering more sophisticated ways to make sure context exists where it needs to, this was my attempt to not get too fancy right away.
I wonder if we can even instruct agents to help with this, like an instruction in some AGENTS.md about "if publish_action_from_context is called, make sure context is populated earlier`, wont be airtight but might reduce times people break it. I say this becase I am not sure how viable static analysis would be in detecting something like, but agentic would probably be able to.
Right now breaking context would cause Sentry issues with the log.error there, and with the addition of throwing in tests you suggested it might help prevent breaking.
regarding from_context I think for sure this is a convention we want to push and yes we should it should be the preferable way outside of narrow cases like you said
There was a problem hiding this comment.
yea agreed, this is the part that I was the most conflicted about. It does indeed come with this tradeoff you describe, but I think it is probably worth for the amount of clutter it saves. I am open to considering more sophisticated ways to make sure context exists where it needs to, this was my attempt to not get too fancy right away.
I wonder if we can even instruct agents to help with this, like an instruction in some AGENTS.md about "if publish_action_from_context is called, make sure context is populated earlier`, wont be airtight but might reduce times people break it. I say this becase I am not sure how viable static analysis would be in detecting something like, but agentic would probably be able to.
Right now breaking context would cause Sentry issues with the log.error there, and with the addition of throwing in tests you suggested it might help prevent breaking.
regarding from_context I think for sure this is a convention we want to push and yes we should it should be the preferable way outside of narrow cases like you said
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ActionContext: |
There was a problem hiding this comment.
This strikes me as pretty central, conceptually and practically.
My pr has GroupActionActor because that's the core attribution information I was dealing with, and once determined it's best treated as one thing, but I think that needs to be extended with source, since in any given context they are really grouped.
There was a problem hiding this comment.
yea agreed as I said above this Context is the main thing here and we need to align on it. Regarding Actor, I see it as - Actor is mostly a "who" and ActionContext is about both who and how, so yea I think it should be extended to this, I dont mind how exactly so whatever works for you
| idempotency_key: str | None = None, | ||
| ) -> None: | ||
| actor_type = ActorType.USER if actor_id is not None else ActorType.SYSTEM | ||
| logger.info( |
There was a problem hiding this comment.
might also be fun to have a counter tagged by action and source, just for now.
|
@kcons thanks for all the comments - I think this work for sure is a bit exploratory but I do intend on actually landing this PR, acknowledging much of this is still subject to change. I fully agree with:
regarding this
I also agree but I am not sure how to address it at this point, do you think like add some more comments / context in code or detail these decisions in our docs better? I made corrections and responded to your comments, I do want to get this PR in shape and land it, there are some instrumentation assumptions it will help me prove and I think setting this in place will move us forward, even if its all subject to change. |
Regardling landability, I think we just want someone seeing a use of this module to be able to answer "what is this doing? is this important? should I be using it?" fairly quickly (ie basic doc coverage on the public interface, perhaps with a standard one-line disclaimer about it being experimental or whatever). If someone is refactoring involved code, they should be able to do it correctly. Also, a module level doc given the basic context and status so someone wondering what action_log is has a reasonable answer ("what is this module? should I be using it?" probably core questions to answer). I hope to get back to my table PR soon (almost done with replies) and once that happens, I'll have some fun merging this in. |
|
@kcons agreed sounds good, added some comments, I think it makes sense when adding something experimental like this, we can always revise later |
Adds `publish_action` calls alongside existing Activity creation sites to log issue mutations with source attribution (web, API, MCP, Seer). Uses a contextvar (`ActionContext`) set at the API boundary in `update_groups()` and read at lower-level mutation sites, so functions like `GroupAssignee.assign()` and `update_priority()` can log actions without signature changes. Instrumented actions: resolve, unresolve, archive, assign, unassign, set_priority, merge, mark_reviewed, view, trigger_autofix, comment (create/edit/delete). Currently a no-op structured logger — will be wired to the real GroupActionLogEntry model once that lands.
The `from sentry.api import client` resolves to the module, not the ApiClient instance, causing mypy attr-defined errors on `.get()` calls.
…rom unresolve - Replace module-level global MCP_APPLICATION_ID with django.core.cache (5min TTL), so a missing app record on first lookup doesn't permanently poison the cache for the worker lifetime - Add ActionType.UNARCHIVE to distinguish un-ignoring (IGNORED->UNRESOLVED) from un-resolving (RESOLVED->UNRESOLVED), matching the existing signal distinction (issue_unignored vs issue_unresolved)
…plication_id - Remove UNARCHIVE — "unresolve" is the destination state regardless of whether the issue was previously resolved or archived - Skip _get_mcp_application_id() cache/DB call when request has no application_id (the common case for non-token-auth requests) - Use ActionSource.MCP constant in f-string for MCP client slug
…ontext When publish_action_from_context is called without an ActionContext, log an error (creates a Sentry issue with stack trace) and record the action with source="unknown" instead of silently dropping it.
…revert UNARCHIVE - Wrap system-initiated mutations (auto_update_priority, ingest assign, ownership auto-assign, release-based assign) with action_context_scope(source=SYSTEM) - Wrap integration sync assign/deassign with action_context_scope(source=integration.provider) - Revert UNARCHIVE action type — UNRESOLVE covers both transitions since "unresolved" is a real state, "unarchived" is not - Keep logger.error for missing context so unexpected callers surface as Sentry issues
- Make actor_id required in action_context_scope (no silent default) - Raise RuntimeError in test environment when context is missing - Remove dead request=None branch from resolve_action_source - Add metrics.incr counter tagged by action and source
The RuntimeError in test environment broke every existing test that calls instrumented functions without action context. Keep logger.error as the safety net instead — it creates Sentry issues for triage without coupling unrelated tests to the action log system.
yuvmen
force-pushed
the
yuvmen/group-action-log-instrumentation
branch
from
0934633 to
ab0f6b4
Compare
|
|
||
|
|
||
| def _get_mcp_application_id() -> int | None: | ||
| from django.core.cache import cache |
|
|
||
| @dataclass(frozen=True) | ||
| class ActionContext: | ||
| source: str |
There was a problem hiding this comment.
Should we enforce that this is an ActionSource?
There was a problem hiding this comment.
ah just saw Kyle's comment below
There was a problem hiding this comment.
yea it would just take solving some of the variant stuff and how we do it, will leave it as a follow up
| data.update({"participants": participants}) | ||
|
|
||
| publish_action( | ||
| action=ActionType.VIEW, |
There was a problem hiding this comment.
seems like we'll end up recording a ton of these. probably not worth adjusting at this point, but i wonder if eventually we may want to sample view events
There was a problem hiding this comment.
yea I wondered about that, I am trying to not log a VIEW on bulk fetches, only specific get for a specific issue, so I am not sure how much is this going to explode or not on a single issue. We will have a lot of these in general that is for sure, I think once we merge and have some numbers we can consider our steps
There was a problem hiding this comment.
When we log these permanently, I think we'll want to throttle them.
I don't think data will necessarily show it to be unimportant, but I think the difference between "they fetched this" and "they fetched this N times over 10 minutes" isn't huge for us, and this carries much less strength of intent than the place where we update GroupSeen (I assume), so if we can do something simple to reduce log load and what we'd count as noise, that's good.
The mechanism is an interesting question. Maybe it doesn't much matter, and a ttl'd cache key for simple throttling is good enough.
It's also not too hard to support time-bounded deduplication with action-specific criteria, but if that's not stepping on idempotency key stuff, that is lossy, so a VIEW-specific thing seems a better starting point.
I'd be interested to add the cache-ttl based checking just as a way to track how often it matters, but not an issue for this PR.
There was a problem hiding this comment.
I see, I guess time-bounded dedup in case its the same actor requesting make sense. I had in mind like - I dont care if they "fetched it N times over 10 minutes" but I do care if they fetched it now or a week ago, like as an indication of when did X person last notice this issue. I think its indeed something to think about in the future, gonna leave it raw for now to let us just log stats as it is
| request = _make_request(meta={"HTTP_X_SEER_REFERRER": "seer-explorer"}) | ||
| assert resolve_action_source(request) == "seer:explorer" | ||
|
|
||
| def test_seer_rpc_authenticator(self) -> None: |
There was a problem hiding this comment.
a test for the priority logic might be good, e.g. when seer referrer header and SeerRpcSignatureAuthentication are both present
kcons
left a comment
kcons
left a comment
There was a problem hiding this comment.
The auto_spec and the api app part are the only "please do" here.
I want to rework the way we type ActionSource, but we don't need to block on that. At this point, we only need to block on wrong things and things that make future iteration harder.
Caveat: I'm reviewing this from the perspective of it enabling GAL iteration; my approval shouldn't be treated as "I think the group lifecycle integration is definitely correct" or "i think we're doing source determination accurately".
| data.update({"participants": participants}) | ||
|
|
||
| publish_action( | ||
| action=ActionType.VIEW, |
There was a problem hiding this comment.
When we log these permanently, I think we'll want to throttle them.
I don't think data will necessarily show it to be unimportant, but I think the difference between "they fetched this" and "they fetched this N times over 10 minutes" isn't huge for us, and this carries much less strength of intent than the place where we update GroupSeen (I assume), so if we can do something simple to reduce log load and what we'd count as noise, that's good.
The mechanism is an interesting question. Maybe it doesn't much matter, and a ttl'd cache key for simple throttling is good enough.
It's also not too hard to support time-bounded deduplication with action-specific criteria, but if that's not stepping on idempotency key stuff, that is lossy, so a VIEW-specific thing seems a better starting point.
I'd be interested to add the cache-ttl based checking just as a way to track how often it matters, but not an issue for this PR.
| assert len(resolve_calls) == 1 | ||
| assert resolve_calls[0].kwargs["group_id"] == self.group.id | ||
|
|
||
| @patch(PUBLISH_UPDATE_CTX) |
There was a problem hiding this comment.
we presumably want to auto_spec=True on these to validate use.
| assert info_record.__dict__["source"] == "unknown" | ||
|
|
||
|
|
||
| PUBLISH_UPDATE = "sentry.api.helpers.group_index.update.publish_action" |
There was a problem hiding this comment.
marginal preference for patch.object(actual_module_maybe_imported_as_alias, 'method_name'). The validation benefit isn't huge, but you get click-through and some tools do better, and it should fail at import time rather than execution.
Not as valuable as auto_spec, so up to you.
| PAGERDUTY = "pagerduty" | ||
| OPSGENIE = "opsgenie" | ||
| PERFORCE = "perforce" | ||
| UNKNOWN = "unknown" |
There was a problem hiding this comment.
I was trying to clarify for myself what UNKNOWN means. One natural interpretation is "not one of these other ones that we recognize" but sounds like it's an in-band None. That seems reasonable to me; I'm hoping we can move toward an interface where nobody is really hand-picking ActionSource (generated by helpers write-side, opt-in predicates read-side) so it shouldn't matter much. No change needed (as a special value, a one-line doc wouldn't hurt, but I think we'll end up doing a clarification pass on most of these, so it can wait).
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Issue Action Log — experimental module for tracking who did what to an issue and how. |
| from sentry.models.apiapplication import ApiApplication | ||
|
|
||
| try: | ||
| app = ApiApplication.objects.filter(name__icontains="sentry-mcp").first() |
There was a problem hiding this comment.
This non-deterministically picks from like 5 api apps, and I think only 1 is ours.
There was a problem hiding this comment.
hmm oh I now realize this doesnt work the way I thought it did, ApiApplication is actually just an integration and anyone can add them and name them, this check is not good enough at all.
We also have more than one as you say that belongs to us, gonna read this off of a secret with specific values
…ec, add UNKNOWN comment
Replace the ApiApplication name lookup (non-deterministic and spoofable) with a check on the User-Agent the MCP sends (sentry-mcp/...), mirroring how sentry-cli is detected. The X-Sentry-MCP-Client-Name header still provides the sub-type (mcp:claude-code, etc.). Source attribution is self-reported and spoofable by design — best-effort, not for audit.
Use the standardized X-Sentry-MCP-Client-Family header (claude-code, cursor, etc.) instead of the free-form client-name, so sub-type values are predictable. Unrecognized families are logged so new ones can be added; the MCP's own "other"/"unknown" catch-alls fall back to plain mcp.
|
MCP started sending client-family in this PR, and it was my actual original preference as it basically uses the MCP to standardize the The question remains from previous discussions here of how/if we limit the values we can get here, same considerations as taking variants from |
resolved_in_commit and resolved_in_pull_request wrapped the self-assign in action_context_scope with actor_id=None while passing acting_user into assign(), so the action log recorded ASSIGN as a system action with no actor. Attribute it to acting_user, matching the activity and group history records.
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 e1aecec. Configure here.
| with action_context_scope(source=integration.provider, actor_id=None): | ||
| GroupAssignee.objects.assign( | ||
| group, | ||
| user, | ||
| assignment_source=AssignmentSource.from_integration(integration), | ||
| ) |
There was a problem hiding this comment.
Bug: The action_context_scope in _handle_assign is incorrectly called with actor_id=None, causing user assignment actions from integrations to be misattributed to the system.
Severity: LOW
Suggested Fix
In _handle_assign, update the action_context_scope to use the available user's ID. Change with action_context_scope(source=integration.provider, actor_id=None): to with action_context_scope(source=integration.provider, actor_id=user.id):.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/integrations/utils/sync.py#L121-L126
Potential issue: In the `_handle_assign` function, the `action_context_scope` is invoked
with `actor_id=None` during integration syncs. A `user` object is available at this
point, but its ID is not used. This causes the action log to incorrectly attribute the
assignment to a system action with no actor, instead of the user being assigned. This
pattern was identified as a bug and fixed in other parts of the codebase
(`resolved_in_commit` and `resolved_in_pull_request`), but this instance was missed.
The POST handler logged TRIGGER_AUTOFIX on any 202, but _post_inner returns 202 for coding_agent_handoff and open_pr too, not only a built-in autofix run — so non-autofix steps were recorded as autofix triggers. Remove the instrumentation (and fold _post_inner back into post) until it can target the actual autofix run. Tracked in ID-1591.
kcons
left a comment
kcons
left a comment
There was a problem hiding this comment.
I favor paring down bits not needed to flex source determination (more prototype and api demo than necessarily useful for now) and sentrybot seems to have a point, but I think we can go forward with this.
Thanks!
Backend Test FailuresFailures on
|
3 participants
Summary
Instruments issue-facing API endpoints to emit structured log entries for the Issue Action Log, with full source attribution (web, API, MCP client, Seer, sentry-cli).
action_log.pymodule withresolve_action_source(),publish_action(), and aContextVar-basedActionContextthat's set at the API boundary and read at lower-level mutation sitespublish_actioncalls next to existingActivity.objects.create()calls, piggybacking on their existing change detection rather than reimplementing itGroupActionLogEntryonce that model landsInstrumented actions
Resolve, unresolve, archive, assign, unassign, set priority, merge, mark reviewed, view, trigger autofix, comment (create/edit/delete)
Edit: Ended up adding instrumentation for some system tasks like
auto_update_priorityand integrations, as I wanted to be able to track missing instrumentation but without those it would get pretty noisy, and it wasn't a big thing to add here.