Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(search): hide feedback from issues search by default #76984

Merged
merged 9 commits into from
Sep 9, 2024
Merged

fix(search): hide feedback from issues search by default #76984

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e826fb4
fix(search): hide feedback from issues search by default
aliu39 Sep 5, 2024
4fa57db
Refactor to a separate where condition
aliu39 Sep 5, 2024
a4a77c2
Uncomment line in test_snuba_perf_issue
aliu39 Sep 5, 2024
9c6f53d
Merge branch 'master' of github.com:getsentry/sentry into aliu/filter…
aliu39 Sep 5, 2024
2a7314b
Use a new field in GroupType class
aliu39 Sep 5, 2024
5253fae
Fix get_by_type_id() call
aliu39 Sep 5, 2024
1da96c7
Parametrize tests on useGroupSnubaDataset
aliu39 Sep 5, 2024
0173912
Move in_default_search filter to before is_visible filter, in group_t…
aliu39 Sep 5, 2024
2c63a78
Fix snuba_perf_issue test
aliu39 Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/sentry/issues/grouptype.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ class GroupType:
# If True this group type should be released everywhere. If False, fall back to features to
# decide if this is released.
released: bool = False
# If False this group is excluded from default searches, when there are no filters on issue.category or issue.type.
in_default_search: bool = True

# Allow automatic resolution of an issue type, using the project-level option.
enable_auto_resolve: bool = True
Expand Down Expand Up @@ -580,6 +582,7 @@ class FeedbackGroup(GroupType):
creation_quota = Quota(3600, 60, 1000) # 1000 per hour, sliding window of 60 seconds
default_priority = PriorityLevel.MEDIUM
notification_config = NotificationConfig(context=[])
in_default_search = False # hide from issues stream


@dataclass(frozen=True)
Expand Down
24 changes: 23 additions & 1 deletion src/sentry/search/snuba/executors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@
from sentry.constants import ALLOWED_FUTURE_DELTA
from sentry.db.models.manager.base_query_set import BaseQuerySet
from sentry.issues import grouptype
from sentry.issues.grouptype import ErrorGroupType, GroupCategory, get_group_types_by_category
from sentry.issues.grouptype import (
ErrorGroupType,
GroupCategory,
GroupType,
get_group_types_by_category,
)
from sentry.issues.grouptype import registry as gt_registry
from sentry.issues.search import (
SEARCH_FILTER_UPDATERS,
IntermediateSearchQueryPartial,
Expand Down Expand Up @@ -1837,6 +1843,22 @@ def query(
where_conditions.append(
Condition(Column("occurrence_type_id", joined_entity), Op.IN, group_types)
)
else:
all_group_type_objs: list[GroupType] = [
gt_registry.get_by_type_id(id)
for id in gt_registry.get_all_group_type_ids()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do this like this, on line 1828

if not is_errors:
    visible_group_types = group_types_from(search_filters)
    if visible_group_types is None:
        # When no group types are specified, default to all default group types
        visible_group_types = [gt for gt in all_group_type_objs if not gt.in_default_search]
    # remove `raw_group_types is not None` check, since it'll never be none now, just potentially empty
    # Then just run the same logic we were running before

This keeps the logic shared which should reduce the chance of bugs. Since in your new branch you missed grouptype.registry.get_visible, and so we'd have started showing unreleased issue types to all users.

Reading this code, this might actually be a bug already... if no issue types are specified, we're not filtering to visible issue types only. So doing this will fix the bug

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, this line should be like this right?
[gt for gt in all_group_type_objs if not gt.in_default_search]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the visible filter seems to happen in the if not None case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the logic to the base case of group_types_from. For some reason this causes test_snuba_perf_issue to fail, not sure why, it worked once locally so could be flakey.

Copy link
Member Author

@aliu39 aliu39 Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the test, it's because the GroupType created by storing a transaction, 2001 ProfileFileIOGroupType, is not registered.

Not sure how GroupTypes are registered in prod, but this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change makes it so by default, issues with unregistered GroupTypes are never listed (before, we would just skip the filter).

Do we expect all group types to be registered in prod? Is it ok to manually register the 2001 type to pass this test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not totally sure what you mean by it not being registered? It might just be because the group type hasn't been marked as visible?

You can do that like

sentry/tests/snuba/search/test_backend.py

Line 3583 in 9d25078

with self.feature([ProfileFileIOGroupType.build_visible_feature_name()]):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh got it, that was it, thanks!

]
hidden_group_type_ids = [
gt.type_id for gt in all_group_type_objs if not gt.in_default_search
]
if hidden_group_type_ids:
where_conditions.append(
Condition(
Column("occurrence_type_id", joined_entity),
Op.NOT_IN,
hidden_group_type_ids,
)
)

sort_func = self.get_sort_defs(joined_entity)[sort_by]

Expand Down
37 changes: 37 additions & 0 deletions tests/sentry/feedback/usecases/test_create_feedback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

import time
from datetime import datetime
from typing import Any
from unittest.mock import Mock

Expand All @@ -16,6 +17,7 @@
)
from sentry.models.group import Group, GroupStatus
from sentry.testutils.helpers import Feature
from sentry.testutils.helpers.datetime import iso_format
from sentry.testutils.pytest.fixtures import django_db_all
from sentry.types.group import GroupSubStatus

Expand Down Expand Up @@ -70,6 +72,41 @@ def create_dummy_response(*args, **kwargs):
)


def mock_feedback_event(project_id: int, dt: datetime):
return {
"project_id": project_id,
"request": {
"url": "https://sentry.sentry.io/feedback/?statsPeriod=14d",
"headers": {
"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/118.0.0.0 Safari/537.36"
},
},
"event_id": "56b08cf7852c42cbb95e4a6998c66ad6",
"timestamp": dt.timestamp(),
"received": iso_format(dt),
"environment": "prod",
"release": "frontend@daf1316f209d961443664cd6eb4231ca154db502",
"user": {
"ip_address": "72.164.175.154",
"email": "josh.ferge@sentry.io",
"id": 880461,
"isStaff": False,
"name": "Josh Ferge",
},
"contexts": {
"feedback": {
"contact_email": "josh.ferge@sentry.io",
"name": "Josh Ferge",
"message": "Testing!!",
"replay_id": "3d621c61593c4ff9b43f8490a78ae18e",
"url": "https://sentry.sentry.io/feedback/?statsPeriod=14d",
},
},
"breadcrumbs": [],
"platform": "javascript",
}


def test_fix_for_issue_platform():
event: dict[str, Any] = {
"project_id": 1,
Expand Down
57 changes: 56 additions & 1 deletion tests/sentry/issues/endpoints/test_organization_group_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@
from django.utils import timezone

from sentry import options
from sentry.feedback.usecases.create_feedback import FeedbackCreationSource, create_feedback_issue
from sentry.integrations.models.external_issue import ExternalIssue
from sentry.integrations.models.organization_integration import OrganizationIntegration
from sentry.issues.grouptype import (
FeedbackGroup,
PerformanceNPlusOneGroupType,
PerformanceRenderBlockingAssetSpanGroupType,
PerformanceSlowDBQueryGroupType,
Expand Down Expand Up @@ -57,13 +59,14 @@
from sentry.testutils.cases import APITestCase, SnubaTestCase
from sentry.testutils.helpers import parse_link_header
from sentry.testutils.helpers.datetime import before_now, iso_format
from sentry.testutils.helpers.features import apply_feature_flag_on_cls, with_feature
from sentry.testutils.helpers.features import Feature, apply_feature_flag_on_cls, with_feature
from sentry.testutils.helpers.options import override_options
from sentry.testutils.silo import assume_test_silo_mode
from sentry.types.activity import ActivityType
from sentry.types.group import GroupSubStatus, PriorityLevel
from sentry.users.models.user_option import UserOption
from sentry.utils import json
from tests.sentry.feedback.usecases.test_create_feedback import mock_feedback_event
from tests.sentry.issues.test_utils import SearchIssueTestMixin


Expand Down Expand Up @@ -3917,6 +3920,58 @@ def test_snuba_heavy_error_handled_boolean(self, _: MagicMock) -> None:
assert len(response_handled_0.data) == 1
assert int(response_handled_0.data[0]["id"]) == handled_event.group.id

def test_feedback_filtered_by_default(self, _):
with Feature(
{
"organizations:feedback-ingest": True,
"organizations:feedback-visible": True,
}
):
event = self.store_event(
data={"event_id": uuid4().hex, "timestamp": iso_format(before_now(seconds=1))},
project_id=self.project.id,
)
assert event.group is not None

feedback_event = mock_feedback_event(self.project.id, before_now(seconds=1))
create_feedback_issue(
feedback_event, self.project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
)
self.login_as(user=self.user)
res = self.get_success_response()

assert len(res.data) == 1
issue = res.data[0]
feedback_group = Group.objects.get(type=FeedbackGroup.type_id)
assert int(issue["id"]) != feedback_group.id
aliu39 marked this conversation as resolved.
Show resolved Hide resolved
assert issue["issueCategory"] != "feedback"

def test_feedback_filter(self, _):
aliu39 marked this conversation as resolved.
Show resolved Hide resolved
with Feature(
{
"organizations:feedback-ingest": True,
"organizations:feedback-visible": True,
}
):
event = self.store_event(
data={"event_id": uuid4().hex, "timestamp": iso_format(before_now(seconds=1))},
project_id=self.project.id,
)
assert event.group is not None

feedback_event = mock_feedback_event(self.project.id, before_now(seconds=1))
create_feedback_issue(
feedback_event, self.project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
)
self.login_as(user=self.user)
res = self.get_success_response(query="issue.category:feedback")

assert len(res.data) == 1
issue = res.data[0]
feedback_group = Group.objects.get(type=FeedbackGroup.type_id)
assert int(issue["id"]) == feedback_group.id
aliu39 marked this conversation as resolved.
Show resolved Hide resolved
assert issue["issueCategory"] == "feedback"


class GroupUpdateTest(APITestCase, SnubaTestCase):
endpoint = "sentry-api-0-organization-group-index"
Expand Down
Loading