Browse Source

feat(metric-issues): Add a new endpoint for open periods (#83797)

Refactors the group details backend to call a new helper function to get
the open periods for a group. The helper is also used in a new endpoint,
similar to the GroupEvents endpoint. This will be used to display a list
of open period in the issue details UI.

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Snigdha Sharma 1 month ago
parent
commit
c6f36c9bbc

+ 6 - 0
src/sentry/api/urls.py

@@ -175,6 +175,7 @@ from sentry.issues.endpoints import (
     GroupHashesEndpoint,
     GroupNotesDetailsEndpoint,
     GroupNotesEndpoint,
+    GroupOpenPeriodsEndpoint,
     GroupSimilarIssuesEmbeddingsEndpoint,
     GroupSimilarIssuesEndpoint,
     GroupTombstoneDetailsEndpoint,
@@ -697,6 +698,11 @@ def create_group_urls(name_prefix: str) -> list[URLPattern | URLResolver]:
             GroupEventsEndpoint.as_view(),
             name=f"{name_prefix}-group-events",
         ),
+        re_path(
+            r"^(?P<issue_id>[^\/]+)/open-periods/$",
+            GroupOpenPeriodsEndpoint.as_view(),
+            name=f"{name_prefix}-group-open-periods",
+        ),
         re_path(
             r"^(?P<issue_id>[^\/]+)/events/(?P<event_id>(?:latest|oldest|recommended|\d+|[A-Fa-f0-9-]{32,36}))/$",
             GroupEventDetailsEndpoint.as_view(),

+ 2 - 0
src/sentry/issues/endpoints/__init__.py

@@ -6,6 +6,7 @@ from .group_events import GroupEventsEndpoint
 from .group_hashes import GroupHashesEndpoint
 from .group_notes import GroupNotesEndpoint
 from .group_notes_details import GroupNotesDetailsEndpoint
+from .group_open_periods import GroupOpenPeriodsEndpoint
 from .group_similar_issues import GroupSimilarIssuesEndpoint
 from .group_similar_issues_embeddings import GroupSimilarIssuesEmbeddingsEndpoint
 from .group_tombstone import GroupTombstoneEndpoint
@@ -40,6 +41,7 @@ __all__ = (
     "GroupHashesEndpoint",
     "GroupNotesDetailsEndpoint",
     "GroupNotesEndpoint",
+    "GroupOpenPeriodsEndpoint",
     "GroupSimilarIssuesEmbeddingsEndpoint",
     "GroupSimilarIssuesEndpoint",
     "GroupTombstoneDetailsEndpoint",

+ 4 - 61
src/sentry/issues/endpoints/group_details.py

@@ -1,7 +1,7 @@
 import functools
 import logging
 from collections.abc import Sequence
-from datetime import datetime, timedelta
+from datetime import timedelta
 
 from django.utils import timezone
 from rest_framework.exceptions import ValidationError
@@ -24,8 +24,6 @@ from sentry.api.serializers import GroupSerializer, GroupSerializerSnuba, serial
 from sentry.api.serializers.models.group_stream import get_actions, get_available_issue_plugins
 from sentry.api.serializers.models.plugin import PluginSerializer
 from sentry.api.serializers.models.team import TeamSerializer
-from sentry.incidents.models.alert_rule import AlertRule
-from sentry.incidents.utils.metric_issue_poc import OpenPeriod
 from sentry.integrations.api.serializers.models.external_issue import ExternalIssueSerializer
 from sentry.integrations.models.external_issue import ExternalIssue
 from sentry.issues.constants import get_issue_tsdb_group_model
@@ -33,7 +31,7 @@ from sentry.issues.escalating_group_forecast import EscalatingGroupForecast
 from sentry.issues.grouptype import GroupCategory
 from sentry.models.activity import Activity
 from sentry.models.eventattachment import EventAttachment
-from sentry.models.group import Group
+from sentry.models.group import Group, get_open_periods_for_group
 from sentry.models.groupinbox import get_inbox_details
 from sentry.models.grouplink import GroupLink
 from sentry.models.groupowner import get_owner_details
@@ -47,12 +45,12 @@ from sentry.sentry_apps.api.serializers.platform_external_issue import (
 )
 from sentry.sentry_apps.models.platformexternalissue import PlatformExternalIssue
 from sentry.tasks.post_process import fetch_buffered_group_stats
-from sentry.types.activity import ActivityType
 from sentry.types.ratelimit import RateLimit, RateLimitCategory
 from sentry.users.services.user.service import user_service
 from sentry.utils import metrics
 
 delete_logger = logging.getLogger("sentry.deletions.api")
+OPEN_PERIOD_LIMIT = 50
 
 
 def get_group_global_count(group: Group) -> str:
@@ -90,61 +88,6 @@ class GroupDetailsEndpoint(GroupEndpoint, EnvironmentMixin):
         seen_by = list(GroupSeen.objects.filter(group=group).order_by("-last_seen"))
         return [seen for seen in serialize(seen_by, request.user) if seen is not None]
 
-    def _get_open_periods_for_group(self, group: Group) -> list[OpenPeriod]:
-        if not features.has("organizations:issue-open-periods", group.organization):
-            return []
-
-        activities = Activity.objects.filter(
-            group=group,
-            type__in=[ActivityType.SET_UNRESOLVED.value, ActivityType.SET_RESOLVED.value],
-            datetime__gte=timezone.now() - timedelta(days=90),
-        ).order_by("datetime")
-
-        open_periods = []
-        start: datetime | None = group.first_seen
-
-        for activity in activities:
-            if activity.type == ActivityType.SET_RESOLVED.value and start:
-                open_periods.append(
-                    OpenPeriod(
-                        start=start,
-                        end=activity.datetime,
-                        duration=activity.datetime - start,
-                        is_open=False,
-                        last_checked=activity.datetime,
-                    )
-                )
-                start = None
-            elif activity.type == ActivityType.SET_UNRESOLVED.value and not start:
-                start = activity.datetime
-
-        if start:
-            event = group.get_latest_event()
-            last_checked = group.last_seen
-            if event:
-                alert_rule_id = (
-                    event.data.get("contexts", {}).get("metric_alert", {}).get("alert_rule_id")
-                )
-                if alert_rule_id:
-                    try:
-                        alert_rule = AlertRule.objects.get(id=alert_rule_id)
-                        now = timezone.now()
-                        last_checked = now - timedelta(seconds=alert_rule.snuba_query.time_window)
-                    except AlertRule.DoesNotExist:
-                        pass
-
-            open_periods.append(
-                OpenPeriod(
-                    start=start,
-                    end=None,
-                    duration=None,
-                    is_open=True,
-                    last_checked=last_checked,
-                )
-            )
-
-        return open_periods[::-1]
-
     def _get_context_plugins(self, request: Request, group: Group):
         project = group.project
         return serialize(
@@ -222,7 +165,7 @@ class GroupDetailsEndpoint(GroupEndpoint, EnvironmentMixin):
 
             # TODO: these probably should be another endpoint
             activity = Activity.objects.get_activities_for_group(group, 100)
-            open_periods = self._get_open_periods_for_group(group)
+            open_periods = get_open_periods_for_group(group, limit=OPEN_PERIOD_LIMIT)
             seen_by = self._get_seen_by(request, group)
 
             if "release" not in collapse:

+ 45 - 0
src/sentry/issues/endpoints/group_open_periods.py

@@ -0,0 +1,45 @@
+from __future__ import annotations
+
+from typing import TYPE_CHECKING, Any
+
+from rest_framework.exceptions import ParseError
+from rest_framework.request import Request
+from rest_framework.response import Response
+
+from sentry.api.api_owners import ApiOwner
+from sentry.api.api_publish_status import ApiPublishStatus
+from sentry.api.base import region_silo_endpoint
+from sentry.api.bases import GroupEndpoint
+from sentry.api.paginator import GenericOffsetPaginator
+from sentry.api.utils import get_date_range_from_params
+from sentry.exceptions import InvalidParams
+from sentry.models.group import get_open_periods_for_group
+
+if TYPE_CHECKING:
+    from sentry.models.group import Group
+
+
+@region_silo_endpoint
+class GroupOpenPeriodsEndpoint(GroupEndpoint):
+    publish_status = {
+        "GET": ApiPublishStatus.PRIVATE,
+    }
+    owner = ApiOwner.ISSUES
+
+    def get(self, request: Request, group: Group) -> Response:
+        """
+        Return a list of open periods for an issue
+        """
+        try:
+            start, end = get_date_range_from_params(request.GET, optional=True)
+        except InvalidParams:
+            raise ParseError(detail="Invalid date range")
+
+        def data_fn(offset: int, limit: int) -> Any:
+            return get_open_periods_for_group(group, start, end, offset, limit)
+
+        return self.paginate(
+            request=request,
+            on_results=lambda results: [result.to_dict() for result in results],
+            paginator=GenericOffsetPaginator(data_fn=data_fn),
+        )

+ 101 - 1
src/sentry/models/group.py

@@ -21,7 +21,7 @@ from django.utils.http import urlencode
 from django.utils.translation import gettext_lazy as _
 from snuba_sdk import Column, Condition, Op
 
-from sentry import eventstore, eventtypes, options, tagstore
+from sentry import eventstore, eventtypes, features, options, tagstore
 from sentry.backup.scopes import RelocationScope
 from sentry.constants import DEFAULT_LOGGER_NAME, LOG_LEVELS, MAX_CULPRIT_LENGTH
 from sentry.db.models import (
@@ -42,6 +42,7 @@ from sentry.issues.priority import (
     PriorityChangeReason,
     get_priority_for_ongoing_group,
 )
+from sentry.models.activity import Activity
 from sentry.models.commit import Commit
 from sentry.models.grouphistory import record_group_history, record_group_history_from_activity_type
 from sentry.models.organization import Organization
@@ -60,6 +61,7 @@ from sentry.utils.numbers import base32_decode, base32_encode
 from sentry.utils.strings import strip, truncatechars
 
 if TYPE_CHECKING:
+    from sentry.incidents.utils.metric_issue_poc import OpenPeriod
     from sentry.integrations.services.integration import RpcIntegration
     from sentry.models.environment import Environment
     from sentry.models.team import Team
@@ -1050,3 +1052,101 @@ def pre_save_group_default_substatus(instance, sender, *args, **kwargs):
                 "No substatus allowed for group",
                 extra={"status": instance.status, "substatus": instance.substatus},
             )
+
+
+def get_last_checked_for_open_period(group: Group) -> datetime:
+    from sentry.incidents.models.alert_rule import AlertRule
+    from sentry.issues.grouptype import MetricIssuePOC
+
+    event = group.get_latest_event()
+    last_checked = group.last_seen
+    if event and group.type == MetricIssuePOC.type_id:
+        alert_rule_id = event.data.get("contexts", {}).get("metric_alert", {}).get("alert_rule_id")
+        if alert_rule_id:
+            try:
+                alert_rule = AlertRule.objects.get(id=alert_rule_id)
+                now = timezone.now()
+                last_checked = now - timedelta(seconds=alert_rule.snuba_query.time_window)
+            except AlertRule.DoesNotExist:
+                pass
+
+    return last_checked
+
+
+def get_open_periods_for_group(
+    group: Group,
+    query_start: datetime | None = None,
+    query_end: datetime | None = None,
+    offset: int | None = None,
+    limit: int | None = None,
+) -> list[OpenPeriod]:
+    from sentry.incidents.utils.metric_issue_poc import OpenPeriod
+
+    if not features.has("organizations:issue-open-periods", group.organization):
+        return []
+
+    if query_start is None or query_end is None:
+        query_start = timezone.now() - timedelta(days=90)
+        query_end = timezone.now()
+
+    query_limit = limit * 2 if limit else None
+    activities = Activity.objects.filter(
+        group=group,
+        type__in=[ActivityType.SET_UNRESOLVED.value, ActivityType.SET_RESOLVED.value],
+        datetime__gte=query_start,
+        datetime__lte=query_end,
+    ).order_by("-datetime")[:query_limit]
+
+    open_periods = []
+    start: datetime | None = None
+    end: datetime | None = None
+    last_checked = get_last_checked_for_open_period(group)
+
+    # Handle currently open period
+    if group.status == GroupStatus.UNRESOLVED and len(activities) > 0:
+        open_periods.append(
+            OpenPeriod(
+                start=activities[0].datetime,
+                end=None,
+                duration=None,
+                is_open=True,
+                last_checked=last_checked,
+            )
+        )
+        activities = activities[1:]
+
+    for activity in activities:
+        if activity.type == ActivityType.SET_RESOLVED.value:
+            end = activity.datetime
+        elif activity.type == ActivityType.SET_UNRESOLVED.value:
+            start = activity.datetime
+            if end is not None:
+                open_periods.append(
+                    OpenPeriod(
+                        start=start,
+                        end=end,
+                        duration=end - start,
+                        is_open=False,
+                        last_checked=end,
+                    )
+                )
+                end = None
+
+    # Add the very first open period, which has no UNRESOLVED activity for the group creation
+    open_periods.append(
+        OpenPeriod(
+            start=group.first_seen,
+            end=end if end else None,
+            duration=end - group.first_seen if end else None,
+            is_open=False if end else True,
+            last_checked=end if end else last_checked,
+        )
+    )
+
+    if offset and limit:
+        return open_periods[offset : offset + limit]
+
+    if limit:
+        return open_periods[:limit]
+
+    return open_periods

+ 1 - 107
tests/sentry/issues/endpoints/test_group_details.py

@@ -310,18 +310,8 @@ class GroupDetailsTest(APITestCase, SnubaTestCase):
             assert response.data["id"] == str(group.id)
             assert response.data["count"] == "16"
 
-    def test_open_periods_flag_off(self) -> None:
-        self.login_as(user=self.user)
-        group = self.create_group()
-        url = f"/api/0/issues/{group.id}/"
-
-        # open periods are not supported for non-metric issue groups
-        response = self.client.get(url, format="json")
-        assert response.status_code == 200, response.content
-        assert response.data["openPeriods"] == []
-
     @with_feature("organizations:issue-open-periods")
-    def test_open_periods_new_group(self) -> None:
+    def test_open_periods(self) -> None:
         self.login_as(user=self.user)
         group = self.create_group()
         url = f"/api/0/issues/{group.id}/"
@@ -348,102 +338,6 @@ class GroupDetailsTest(APITestCase, SnubaTestCase):
         assert open_period["isOpen"] is True
         assert open_period["lastChecked"] > time
 
-    @with_feature("organizations:issue-open-periods")
-    def test_open_periods_resolved_group(self) -> None:
-        self.login_as(user=self.user)
-        group = self.create_group()
-        url = f"/api/0/issues/{group.id}/"
-
-        # test a new group has an open period
-        group.type = MetricIssuePOC.type_id
-        group.save()
-
-        group.status = GroupStatus.RESOLVED
-        group.save()
-        resolved_time = timezone.now()
-        Activity.objects.create(
-            group=group,
-            project=group.project,
-            type=ActivityType.SET_RESOLVED.value,
-            datetime=resolved_time,
-        )
-
-        alert_rule = self.create_alert_rule(
-            organization=self.organization,
-            projects=[self.project],
-            name="Test Alert Rule",
-        )
-        time = timezone.now() - timedelta(seconds=alert_rule.snuba_query.time_window)
-
-        response = self.client.get(url, format="json")
-        assert response.status_code == 200, response.content
-        open_periods = response.data["openPeriods"]
-        assert len(open_periods) == 1
-        open_period = open_periods[0]
-        assert open_period["start"] == group.first_seen
-        assert open_period["end"] == resolved_time
-        assert open_period["duration"] == resolved_time - group.first_seen
-        assert open_period["isOpen"] is False
-        assert open_period["lastChecked"] > time
-
-    @with_feature("organizations:issue-open-periods")
-    def test_open_periods_unresolved_group(self) -> None:
-        self.login_as(user=self.user)
-        group = self.create_group()
-        url = f"/api/0/issues/{group.id}/"
-
-        group.type = MetricIssuePOC.type_id
-        group.save()
-
-        group.status = GroupStatus.RESOLVED
-        group.save()
-        resolved_time = timezone.now()
-        first_resolved_activity = Activity.objects.create(
-            group=group,
-            project=group.project,
-            type=ActivityType.SET_RESOLVED.value,
-            datetime=resolved_time,
-        )
-
-        # test that another open period is created
-        unresolved_time = timezone.now()
-        group.status = GroupStatus.UNRESOLVED
-        group.save()
-        Activity.objects.create(
-            group=group,
-            project=group.project,
-            type=ActivityType.SET_UNRESOLVED.value,
-            datetime=unresolved_time,
-        )
-
-        second_resolved_time = timezone.now()
-        group.status = GroupStatus.RESOLVED
-        group.save()
-        second_resolved_activity = Activity.objects.create(
-            group=group,
-            project=group.project,
-            type=ActivityType.SET_RESOLVED.value,
-            datetime=second_resolved_time,
-        )
-        response = self.client.get(url, format="json")
-        assert response.status_code == 200, response.content
-        assert response.data["openPeriods"] == [
-            {
-                "start": unresolved_time,
-                "end": second_resolved_time,
-                "duration": second_resolved_time - unresolved_time,
-                "isOpen": False,
-                "lastChecked": second_resolved_activity.datetime,
-            },
-            {
-                "start": group.first_seen,
-                "end": resolved_time,
-                "duration": resolved_time - group.first_seen,
-                "isOpen": False,
-                "lastChecked": first_resolved_activity.datetime,
-            },
-        ]
-
 
 class GroupUpdateTest(APITestCase):
     def test_resolve(self) -> None:

+ 171 - 0
tests/sentry/issues/endpoints/test_group_open_periods.py

@@ -0,0 +1,171 @@
+from datetime import timedelta
+
+from django.utils import timezone
+
+from sentry.issues.grouptype import MetricIssuePOC, ProfileFileIOGroupType
+from sentry.models.activity import Activity
+from sentry.models.group import GroupStatus, get_open_periods_for_group
+from sentry.testutils.cases import APITestCase
+from sentry.testutils.helpers.features import with_feature
+from sentry.types.activity import ActivityType
+
+
+class GroupOpenPeriodsTest(APITestCase):
+    def setUp(self) -> None:
+        super().setUp()
+
+        self.login_as(user=self.user)
+        self.group = self.create_group()
+        # test a new group has an open period
+        self.group.type = MetricIssuePOC.type_id
+        self.group.save()
+
+        self.alert_rule = self.create_alert_rule(
+            organization=self.organization,
+            projects=[self.project],
+            name="Test Alert Rule",
+        )
+        self.last_checked = timezone.now() - timedelta(
+            seconds=self.alert_rule.snuba_query.time_window
+        )
+
+        self.url = f"/api/0/issues/{self.group.id}/open-periods/"
+
+    def test_open_periods_flag_off(self) -> None:
+        self.url = f"/api/0/issues/{self.group.id}/open-periods/"
+        # open periods are not supported for non-metric issue groups
+        self.group.type = ProfileFileIOGroupType.type_id
+        self.group.save()
+
+        response = self.client.get(self.url, format="json")
+        assert response.status_code == 200, response.content
+        assert response.data == []
+
+    @with_feature("organizations:issue-open-periods")
+    def test_open_periods_new_group(self) -> None:
+        response = self.client.get(self.url, format="json")
+        assert response.status_code == 200, response.content
+        assert len(response.data) == 1
+        open_period = response.data[0]
+        assert open_period["start"] == self.group.first_seen
+        assert open_period["end"] is None
+        assert open_period["duration"] is None
+        assert open_period["isOpen"] is True
+        assert open_period["lastChecked"] >= self.last_checked
+
+    @with_feature("organizations:issue-open-periods")
+    def test_open_periods_resolved_group(self) -> None:
+        self.group.status = GroupStatus.RESOLVED
+        self.group.save()
+        resolved_time = timezone.now()
+        activity = Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_RESOLVED.value,
+            datetime=resolved_time,
+        )
+
+        response = self.client.get(self.url, format="json")
+        assert response.status_code == 200, response.content
+        assert response.data == [
+            {
+                "start": self.group.first_seen,
+                "end": resolved_time,
+                "duration": resolved_time - self.group.first_seen,
+                "isOpen": False,
+                "lastChecked": activity.datetime,
+            }
+        ]
+
+    @with_feature("organizations:issue-open-periods")
+    def test_open_periods_unresolved_group(self) -> None:
+        self.group.status = GroupStatus.RESOLVED
+        self.group.save()
+        resolved_time = timezone.now()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_RESOLVED.value,
+            datetime=resolved_time,
+        )
+
+        # test that another open period is created
+        unresolved_time = timezone.now()
+        self.group.status = GroupStatus.UNRESOLVED
+        self.group.save()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_UNRESOLVED.value,
+            datetime=unresolved_time,
+        )
+
+        self.group.status = GroupStatus.RESOLVED
+        self.group.save()
+        second_resolved_time = timezone.now()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_RESOLVED.value,
+            datetime=second_resolved_time,
+        )
+        response = self.client.get(self.url, format="json")
+        assert response.status_code == 200, response.content
+        assert response.data == [
+            {
+                "start": unresolved_time,
+                "end": second_resolved_time,
+                "duration": second_resolved_time - unresolved_time,
+                "isOpen": False,
+                "lastChecked": second_resolved_time,
+            },
+            {
+                "start": self.group.first_seen,
+                "end": resolved_time,
+                "duration": resolved_time - self.group.first_seen,
+                "isOpen": False,
+                "lastChecked": resolved_time,
+            },
+        ]
+
+    @with_feature("organizations:issue-open-periods")
+    def test_open_periods_limit(self) -> None:
+        self.group.status = GroupStatus.RESOLVED
+        self.group.save()
+        resolved_time = timezone.now()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_RESOLVED.value,
+            datetime=resolved_time,
+        )
+
+        # test that another open period is created
+        unresolved_time = timezone.now()
+        self.group.status = GroupStatus.UNRESOLVED
+        self.group.save()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_UNRESOLVED.value,
+            datetime=unresolved_time,
+        )
+
+        second_resolved_time = timezone.now()
+        self.group.status = GroupStatus.RESOLVED
+        self.group.save()
+        Activity.objects.create(
+            group=self.group,
+            project=self.group.project,
+            type=ActivityType.SET_RESOLVED.value,
+            datetime=second_resolved_time,
+        )
+        open_periods = get_open_periods_for_group(self.group, limit=1)
+        assert len(open_periods) == 1
+        assert open_periods[0].to_dict() == {
+            "start": unresolved_time,
+            "end": second_resolved_time,
+            "duration": second_resolved_time - unresolved_time,
+            "isOpen": False,
+            "lastChecked": second_resolved_time,
+        }