diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index 7c9126de41af..b725464c375e 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -22,7 +22,7 @@ from sentry.analytics.events.manual_issue_assignment import ManualIssueAssignment from sentry.api.serializers import serialize from sentry.api.serializers.models.actor import ActorSerializer, ActorSerializerResponse -from sentry.api.serializers.models.groupactionlogentry import serialize_first_seen_entry +from sentry.api.serializers.models.groupactionlogentry import get_serialized_activity_items from sentry.hybridcloud.rpc import coerce_id_from from sentry.integrations.tasks.kick_off_status_syncs import kick_off_status_syncs from sentry.issues.action_log import ( @@ -32,12 +32,11 @@ resolve_action_actor, resolve_action_source, ) +from sentry.issues.action_log.read_metrics import activity_read_endpoint from sentry.issues.action_log.types import MergeIntoOtherAction -from sentry.issues.derived.gate import should_serve_action_log_activity from sentry.issues.grouptype import GroupCategory from sentry.issues.ignored import handle_archived_until_escalating, handle_ignored from sentry.issues.merge import MergedGroup, handle_merge -from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.issues.priority import update_priority from sentry.issues.status_change import handle_status_update, infer_substatus from sentry.issues.update_inbox import update_inbox @@ -784,21 +783,14 @@ def prepare_response( if len(group_list) == 1: if res_type in (GroupResolution.Type.in_next_release, GroupResolution.Type.in_release): group = group_list[0] - if should_serve_action_log_activity(group.project, acting_user): - action_log = GroupActionLogEntry.objects.get_actions_for_group( - group, ACTIVITIES_COUNT - 1 - ) - if action_log: - result["activity"] = [ - *serialize(action_log, acting_user), - serialize_first_seen_entry(group), - ] - else: - logger.info( - "group_index.groupactionlogentry.not_found", - extra={"group_id": group.id}, - ) - + activity_items = get_serialized_activity_items( + group, + acting_user, + endpoint=activity_read_endpoint(request), + limit=ACTIVITIES_COUNT - 1, + ) + if activity_items is not None: + result["activity"] = activity_items else: result["activity"] = serialize( Activity.objects.get_activities_for_group( diff --git a/src/sentry/api/serializers/models/groupactionlogentry.py b/src/sentry/api/serializers/models/groupactionlogentry.py index e1f7808068ce..5d21df088c3e 100644 --- a/src/sentry/api/serializers/models/groupactionlogentry.py +++ b/src/sentry/api/serializers/models/groupactionlogentry.py @@ -1,3 +1,4 @@ +import logging from collections.abc import Mapping, Sequence from datetime import datetime from typing import TYPE_CHECKING, Any, TypedDict @@ -7,6 +8,11 @@ from sentry.api.serializers import Serializer, register, serialize from sentry.api.serializers.models.activity import _ActivitySentryAppEmbed from sentry.api.serializers.models.commit import CommitWithReleaseSerializer +from sentry.issues.action_log.read_metrics import ( + ActivityReadReason, + ActivityReadResult, + record_activity_read, +) from sentry.issues.action_log.types import ( ACTION_TYPES_WITH_COMMIT_DATA, COMMIT_ACTION_TYPES, @@ -15,6 +21,7 @@ GroupActionType, GroupActorType, ) +from sentry.issues.derived.gate import should_serve_action_log_activity from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.models.commit import Commit from sentry.models.pullrequest import PullRequest @@ -35,6 +42,8 @@ if TYPE_CHECKING: from sentry.models.group import Group +logger = logging.getLogger(__name__) + class GroupActionLogEntrySerializerResponse(TypedDict): id: str @@ -83,6 +92,36 @@ def _serialized_id(obj: GroupActionLogEntry) -> str: return str(obj.id) +def get_serialized_activity_items( + group: "Group", + user: User | RpcUser | AnonymousUser | None, + *, + endpoint: str, + limit: int = 99, +) -> list[Any] | None: + """ + Activity-shaped items for a group, read from the action log. + + Returns None when the log can't back the response — either the gate is closed or it's + open and the log is empty — and the caller should fall back to Activity. Reports the + read outcome in both cases, so callers don't have to. + """ + if not should_serve_action_log_activity(group.project, user, endpoint=endpoint): + return None + + action_log = GroupActionLogEntry.objects.get_actions_for_group(group, limit) + if not action_log: + record_activity_read(endpoint, ActivityReadResult.FELL_BACK, ActivityReadReason.EMPTY_LOG) + logger.info( + "issues.action_log.activity_read.not_found", + extra={"endpoint": endpoint, "group_id": group.id}, + ) + return None + + record_activity_read(endpoint, ActivityReadResult.GALE) + return [*serialize(action_log, user), serialize_first_seen_entry(group)] + + @register(GroupActionLogEntry) class GroupActionLogEntrySerializer(Serializer): def get_attrs( diff --git a/src/sentry/issues/action_log/read_metrics.py b/src/sentry/issues/action_log/read_metrics.py new file mode 100644 index 000000000000..af8c0febc521 --- /dev/null +++ b/src/sentry/issues/action_log/read_metrics.py @@ -0,0 +1,47 @@ +from enum import StrEnum + +from rest_framework.request import Request + +from sentry.utils import metrics + +ACTIVITY_READ_METRIC = "issues.action_log.activity_read" + + +class ActivityReadResult(StrEnum): + #: The log backed the response. + GALE = "gale" + #: The gate was open, or the flag was on, but we served Activity anyway. + FELL_BACK = "fell_back" + #: The read flag is off for this project, so the log was never consulted. + FLAG_OFF = "flag_off" + + +class ActivityReadReason(StrEnum): + #: The project is enrolled but its backfill hasn't finished. + NOT_BACKFILLED = "not_backfilled" + #: The gate was open and the log came back empty. + EMPTY_LOG = "empty_log" + + +def activity_read_endpoint(request: Request) -> str: + """The route being served, for the `endpoint` tag.""" + if request.resolver_match is None or request.resolver_match.url_name is None: + return "unknown" + return request.resolver_match.url_name + + +def record_activity_read( + endpoint: str, + result: ActivityReadResult, + reason: ActivityReadReason | None = None, +) -> None: + """ + Record the outcome of one attempt to serve activity from the action log. + + Called once per read: by ``should_serve_action_log_activity`` when the gate closes, + otherwise by the caller once it knows whether the read produced anything. + """ + tags = {"endpoint": endpoint, "result": result.value} + if reason is not None: + tags["reason"] = reason.value + metrics.incr(ACTIVITY_READ_METRIC, sample_rate=1.0, tags=tags) diff --git a/src/sentry/issues/derived/gate.py b/src/sentry/issues/derived/gate.py index a3361ddd87d8..4870b0e9cc55 100644 --- a/src/sentry/issues/derived/gate.py +++ b/src/sentry/issues/derived/gate.py @@ -1,6 +1,11 @@ from django.contrib.auth.models import AnonymousUser from sentry import features +from sentry.issues.action_log.read_metrics import ( + ActivityReadReason, + ActivityReadResult, + record_activity_read, +) from sentry.models.options.project_option import ProjectOption from sentry.models.project import Project from sentry.users.models.user import User @@ -29,8 +34,24 @@ def derived_should_be_correct(project: Project) -> bool: def should_serve_action_log_activity( project: Project, actor: User | RpcUser | AnonymousUser | None = None, + *, + endpoint: str, ) -> bool: - """Whether the action log can back this project's Activity-shaped responses.""" - return features.has( - "projects:issue-action-log-activity", project, actor=actor - ) and derived_should_be_correct(project) + """ + Whether the action log can back this project's Activity-shaped responses. + + Records the read outcome itself when it returns False, because only it knows which + condition closed the gate. Returning True records nothing: the caller goes on to read + the log, so the caller reports whether that produced anything. + """ + if not features.has("projects:issue-action-log-activity", project, actor=actor): + record_activity_read(endpoint, ActivityReadResult.FLAG_OFF) + return False + + if not derived_should_be_correct(project): + record_activity_read( + endpoint, ActivityReadResult.FELL_BACK, ActivityReadReason.NOT_BACKFILLED + ) + return False + + return True diff --git a/src/sentry/issues/endpoints/group_activities.py b/src/sentry/issues/endpoints/group_activities.py index a166790ae865..1dc4e3d649eb 100644 --- a/src/sentry/issues/endpoints/group_activities.py +++ b/src/sentry/issues/endpoints/group_activities.py @@ -7,11 +7,10 @@ from sentry.api.base import cell_silo_endpoint from sentry.api.helpers.deprecation import deprecated from sentry.api.serializers import serialize -from sentry.api.serializers.models.groupactionlogentry import serialize_first_seen_entry +from sentry.api.serializers.models.groupactionlogentry import get_serialized_activity_items from sentry.constants import CELL_API_DEPRECATION_DATE -from sentry.issues.derived.gate import should_serve_action_log_activity +from sentry.issues.action_log.read_metrics import activity_read_endpoint from sentry.issues.endpoints.bases.group import GroupEndpoint -from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.models.activity import Activity from sentry.models.group import Group @@ -33,19 +32,11 @@ def get(self, request: Request, group: Group) -> Response: """ Retrieve all the Activities for a Group """ - if should_serve_action_log_activity(group.project, request.user): - action_log = GroupActionLogEntry.objects.get_actions_for_group(group, 99) - if action_log: - serialized = serialize(action_log, request.user) - serialized.append(serialize_first_seen_entry(group)) - return Response( - { - "activity": serialized, - } - ) - logger.info( - "group_activities.groupactionlogentry.not_found", extra={"group_id": group.id} - ) + activity_items = get_serialized_activity_items( + group, request.user, endpoint=activity_read_endpoint(request) + ) + if activity_items is not None: + return Response({"activity": activity_items}) activity = Activity.objects.get_activities_for_group(group, num=100) return Response( diff --git a/src/sentry/issues/endpoints/group_details.py b/src/sentry/issues/endpoints/group_details.py index 7b5878064166..22145766eb86 100644 --- a/src/sentry/issues/endpoints/group_details.py +++ b/src/sentry/issues/endpoints/group_details.py @@ -27,7 +27,7 @@ from sentry.api.helpers.group_index.validators import GroupValidator from sentry.api.serializers import GroupSerializer, GroupSerializerSnuba, serialize from sentry.api.serializers.models.group import BaseGroupSerializerResponse, GroupDetailsResponse -from sentry.api.serializers.models.groupactionlogentry import serialize_first_seen_entry +from sentry.api.serializers.models.groupactionlogentry import get_serialized_activity_items from sentry.apidocs.constants import ( RESPONSE_ACCEPTED, RESPONSE_BAD_REQUEST, @@ -47,6 +47,7 @@ resolve_action_actor, resolve_action_source, ) +from sentry.issues.action_log.read_metrics import activity_read_endpoint from sentry.issues.action_log.types import ViewAction from sentry.issues.constants import ( ISSUE_VIEW_CACHE_KEY_TTL, @@ -54,10 +55,9 @@ get_issue_tsdb_group_model, ) from sentry.issues.derived.check import record_status_consistency -from sentry.issues.derived.gate import derived_should_be_correct, should_serve_action_log_activity +from sentry.issues.derived.gate import derived_should_be_correct from sentry.issues.endpoints.bases.group import GroupEndpoint from sentry.issues.escalating.escalating_group_forecast import EscalatingGroupForecast -from sentry.issues.models.groupactionlogentry import GroupActionLogEntry from sentry.issues.models.groupderiveddata import GroupDerivedData from sentry.models.activity import Activity from sentry.models.eventattachment import EventAttachment @@ -342,18 +342,12 @@ def get(self, request: Request, group: Group) -> Response[GroupDetailsResponse]: } ) - if should_serve_action_log_activity(group.project, request.user): - action_log = GroupActionLogEntry.objects.get_actions_for_group(group, 99) - if action_log: - # swap action log data in under the activity name - first_seen_entry = cast(dict[str, Any], serialize_first_seen_entry(group)) - data.update( - {"activity": [*serialize(action_log, request.user), first_seen_entry]} - ) - else: - logger.info( - "group_details.groupactionlogentry.not_found", extra={"group_id": group.id} - ) + # swap action log data in under the activity name + activity_items = get_serialized_activity_items( + group, request.user, endpoint=activity_read_endpoint(request) + ) + if activity_items is not None: + data.update({"activity": activity_items}) if "stats" not in collapse: hourly_stats, daily_stats = self.__group_hourly_daily_stats(group, environment_ids) diff --git a/src/sentry/issues/endpoints/group_notes.py b/src/sentry/issues/endpoints/group_notes.py index b71087573e94..4efd806d38b5 100644 --- a/src/sentry/issues/endpoints/group_notes.py +++ b/src/sentry/issues/endpoints/group_notes.py @@ -18,6 +18,12 @@ from sentry.apidocs.utils import inline_sentry_response_serializer from sentry.constants import CELL_API_DEPRECATION_DATE from sentry.issues.action_log import action_context_scope, resolve_action_source +from sentry.issues.action_log.read_metrics import ( + ActivityReadReason, + ActivityReadResult, + activity_read_endpoint, + record_activity_read, +) from sentry.issues.action_log.types import ( CommentDeleteAction, CommentEditAction, @@ -58,7 +64,11 @@ class GroupNotesEndpoint(GroupEndpoint): url_names=["sentry-api-0-group-notes"], ) def get(self, request: Request, group: Group) -> Response: - if should_serve_action_log_activity(group.project, request.user): + endpoint = activity_read_endpoint(request) + if should_serve_action_log_activity(group.project, request.user, endpoint=endpoint): + # No empty-log fallback on this path: once the gate is open the log is + # authoritative for comments, including when the group has none. + record_activity_read(endpoint, ActivityReadResult.GALE) edit_entries = GroupActionLogEntry.objects.filter( group_id=group.id, type=GroupActionType.COMMENT_EDIT.value ).order_by("-date_added", "-id") @@ -187,13 +197,20 @@ def post(self, request: Request, group: Group) -> Response: sender="post", ) - if should_serve_action_log_activity(group.project, request.user): + endpoint = activity_read_endpoint(request) + if should_serve_action_log_activity(group.project, request.user, endpoint=endpoint): entry = GroupActionLogEntry.objects.filter( group_id=group.id, idempotency_key=activity_action_idempotency_key(activity), ).first() if entry: + record_activity_read(endpoint, ActivityReadResult.GALE) return Response(serialize(entry, request.user), status=201) + record_activity_read( + endpoint, + ActivityReadResult.FELL_BACK, + ActivityReadReason.EMPTY_LOG, + ) logger.info("group_notes.groupactionlogentry.not_found", extra={"group_id": group.id}) return Response(serialize(activity, request.user), status=201) diff --git a/src/sentry/issues/endpoints/group_notes_details.py b/src/sentry/issues/endpoints/group_notes_details.py index b505d4dff682..059e1fdc5e92 100644 --- a/src/sentry/issues/endpoints/group_notes_details.py +++ b/src/sentry/issues/endpoints/group_notes_details.py @@ -22,6 +22,11 @@ publish_action, resolve_action_source, ) +from sentry.issues.action_log.read_metrics import ( + ActivityReadResult, + activity_read_endpoint, + record_activity_read, +) from sentry.issues.action_log.types import CommentDeleteAction, CommentEditAction from sentry.issues.derived.gate import should_serve_action_log_activity from sentry.issues.endpoints.bases.group import GroupEndpoint @@ -80,9 +85,15 @@ def delete(self, request: Request, group: Group, note_id: str) -> Response: group_id=group.id, idempotency_key=activity_action_idempotency_key(note), ).first() - if original_comment_log_action is None and should_serve_action_log_activity( - group.project, request.user - ): + endpoint = activity_read_endpoint(request) + serve_from_log = should_serve_action_log_activity( + group.project, request.user, endpoint=endpoint + ) + if serve_from_log: + # The log is authoritative for existence whether or not the entry is + # there: a missing one means the comment is already gone. + record_activity_read(endpoint, ActivityReadResult.GALE) + if original_comment_log_action is None and serve_from_log: raise ResourceDoesNotExist webhook_data = { @@ -164,9 +175,13 @@ def put(self, request: Request, group: Group, note_id: str) -> Response: group_id=group.id, idempotency_key=activity_action_idempotency_key(note), ).first() - if original_comment_log_action is None and should_serve_action_log_activity( - group.project, request.user - ): + endpoint = activity_read_endpoint(request) + serve_from_log = should_serve_action_log_activity( + group.project, request.user, endpoint=endpoint + ) + if serve_from_log: + record_activity_read(endpoint, ActivityReadResult.GALE) + if original_comment_log_action is None and serve_from_log: raise ResourceDoesNotExist # Would be nice to have a last_modified timestamp we could bump here @@ -208,7 +223,7 @@ def put(self, request: Request, group: Group, note_id: str) -> Response: sender="put", ) - if should_serve_action_log_activity(group.project, request.user): + if serve_from_log: if original_comment_log_action is not None: # editing a note doesn't update its COMMENT entry (instead it # appends a separate COMMENT_EDIT entry), so patch in the fresh diff --git a/tests/sentry/api/helpers/test_group_index.py b/tests/sentry/api/helpers/test_group_index.py index d9a640b77846..750341a27a95 100644 --- a/tests/sentry/api/helpers/test_group_index.py +++ b/tests/sentry/api/helpers/test_group_index.py @@ -761,9 +761,10 @@ def test_resolve_in_next_release_activity_from_action_log(self) -> None: assert "set_resolved" in [entry["type"] for entry in activity] assert activity[-1]["id"] == "0" - def test_resolve_in_next_release_no_activity_when_action_log_is_empty(self) -> None: + def test_resolve_in_next_release_falls_back_when_action_log_is_empty(self) -> None: # A gated project can still read an empty log: the GALE write for this - # resolve goes through an outbox that may not have drained yet. + # resolve goes through an outbox that may not have drained yet. Fall back to + # Activity rather than omitting the key, matching the other feed endpoints. self.create_release(project=self.project, version="test@1.0.0.0") group = self.create_group(status=GroupStatus.UNRESOLVED) @@ -775,15 +776,18 @@ def test_resolve_in_next_release_no_activity_when_action_log_is_empty(self) -> N with ( action_log_activity_enabled(), patch.object(GroupActionLogEntry.objects, "get_actions_for_group", return_value=[]), - self.assertLogs("sentry.api.helpers.group_index.update", level="INFO") as logs, + self.assertLogs( + "sentry.api.serializers.models.groupactionlogentry", level="INFO" + ) as logs, ): response = update_groups(request, group_list) assert any( - record.message == "group_index.groupactionlogentry.not_found" for record in logs.records + record.message == "issues.action_log.activity_read.not_found" for record in logs.records ) assert response is not None - assert "activity" not in response.data + # the log read is patched to return nothing, so anything here came from Activity + assert "activity" in response.data def test_resolve_in_next_release_ignores_action_log_when_disabled(self) -> None: # With the gate closed the log may cover only part of this project's history, diff --git a/tests/sentry/testutils/helpers/test_action_log.py b/tests/sentry/testutils/helpers/test_action_log.py index 15bbc5394086..69b1f3d5e13b 100644 --- a/tests/sentry/testutils/helpers/test_action_log.py +++ b/tests/sentry/testutils/helpers/test_action_log.py @@ -116,17 +116,17 @@ def test_nested_captures(self) -> None: class TestActionLogActivityEnabled(TestCase): def test_opens_and_closes_the_gate(self) -> None: - assert not should_serve_action_log_activity(self.project, self.user) + assert not should_serve_action_log_activity(self.project, self.user, endpoint="test") with action_log_activity_enabled(): - assert should_serve_action_log_activity(self.project, self.user) + assert should_serve_action_log_activity(self.project, self.user, endpoint="test") - assert not should_serve_action_log_activity(self.project, self.user) + assert not should_serve_action_log_activity(self.project, self.user, endpoint="test") @action_log_activity_enabled() def test_works_as_a_decorator(self) -> None: - assert should_serve_action_log_activity(self.project, self.user) + assert should_serve_action_log_activity(self.project, self.user, endpoint="test") @action_log_activity_enabled() def test_applies_to_every_project(self) -> None: - assert should_serve_action_log_activity(self.create_project(), self.user) + assert should_serve_action_log_activity(self.create_project(), self.user, endpoint="test")