feat(action-log): Instrument the activity read path - #123846
Conversation
github-actions
Bot
added
the
Scope: Backend
|
bugbot review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 428ecb1. Configure here.
kcons
left a comment
kcons
left a comment
There was a problem hiding this comment.
Seems mostly OK, I'm just grasping for ways to make the logging trivial to do correctly without doing much reading and without too much overhead.
In our ideal world, I think "give me activity feed data, ready for serialization" would be a single call that does the relevant logging and backend choice, but I guess for notes stuff it's not quite that simple.
| *, | ||
| endpoint: ActivityReadEndpoint, | ||
| ) -> bool: | ||
| """Whether the action log can back this project's Activity-shaped responses.""" |
There was a problem hiding this comment.
It seems worth documenting that when it returns false, it calls record_activity_read, and when True, the caller is expected to.
|
|
||
|
|
||
| def record_activity_read( | ||
| endpoint: ActivityReadEndpoint, |
There was a problem hiding this comment.
might make sense to use url_name (request.resolver_match.url_name if request.resolver_match else "unknown") here. More standard, and doesn't require a new enum.
| tags = {"endpoint": endpoint.value, "result": result.value} | ||
| if reason is not None: | ||
| tags["reason"] = reason.value | ||
| metrics.incr(ACTIVITY_READ_METRIC, sample_rate=1.0, tags=tags) |
There was a problem hiding this comment.
does this allow us to count those without a reason directly using 'N/A' or something? I can't recall.
There was a problem hiding this comment.
yeah, i think missing tags are countable
| Retrieve all the Activities for a Group | ||
| """ | ||
| if should_serve_action_log_activity(group.project, request.user): | ||
| if should_serve_action_log_activity( |
There was a problem hiding this comment.
might seem a little silly, but I think it's probably worth a "get_serialized_activity_items" or something, so we decouple where we get the data and how we report it from the use.
I think the value is mostly in if you can use that in two or more endpoints. It looks like maybe but if not, feel free to ignore.
This all just feels a little more brittle than we'd like, so if there's a way to abstract it nicely, seems worth it.
Alternately, a vairable for read_result, result_result, and possibly endpoint that get mutated as we go and used at log sites would (somewhat paradoxically) make things simpler, just because of the shared dependency relationship. Or even a mutable ActivityReadRecord with a .report() method and a set_fallback_reason, as then it'd be "we call report on this, and if it's not GALE we update it", though that'd probbaly have get_actions_for_group (or some wrapper) returning a tuple.
There was a problem hiding this comment.
yeah makes sense, i'll try to figure out an abstraction like that
2 participants
Resolves https://linear.app/getsentry/issue/ISWF-3415/instrument-the-gale-activity-read-path.
Adds
issues.action_log.activity_read, tagged withendpoint/result/reason. One metric is recorded per read/attempted read.endpointis the route'surl_name.get_serialized_activity_itemsis implemented and used forgroup_details,group_activities, andgroup_index.update.prepare_response.Behavior changes:
group_details.groupactionlogentry.not_foundand its two counterparts were consolidated into oneissues.action_log.activity_read.not_foundlog withendpointinextragroup_index.update.prepare_responsenow falls back to an empty log instead of omitting theactivitykey, matching the other two endpoints