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

feat(feedback): ignore feedbacks in issue search #57528

Closed
wants to merge 9 commits into from
Closed

feat(feedback): ignore feedbacks in issue search #57528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dfd94d7
feat(feedback): ignore feedbacks in issue search
JoshFerge Oct 4, 2023
da29ba1
use query parameter, add test
JoshFerge Oct 31, 2023
32ef7fd
add test
JoshFerge Nov 1, 2023
4f42cca
cleanup
JoshFerge Nov 1, 2023
42e519c
fix tests
JoshFerge Nov 1, 2023
79ceb6f
add start and end to search
JoshFerge Nov 1, 2023
42ea00f
add referrer to delete and put
JoshFerge Nov 1, 2023
9e1e886
fix type
JoshFerge Nov 1, 2023
3c1c023
Merge branch 'master' into jferg/ignore-feedbacks-in-search
JoshFerge Nov 6, 2023
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
18 changes: 15 additions & 3 deletions src/sentry/api/endpoints/organization_group_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
from sentry.utils.validators import normalize_event_id

ERR_INVALID_STATS_PERIOD = "Invalid stats_period. Valid choices are '', '24h', and '14d'"
DEFAULT_REFERRER = "search.group_index"
allowed_inbox_search_terms = frozenset(["date", "status", "for_review", "assigned_or_suggested"])


Expand Down Expand Up @@ -165,7 +166,13 @@ class OrganizationGroupIndexEndpoint(OrganizationEventsEndpointBase):
}

def _search(
self, request: Request, organization, projects, environments, extra_query_kwargs=None
self,
request: Request,
organization,
projects,
environments,
referrer,
extra_query_kwargs=None,
):
with start_span(op="_search"):
query_kwargs = build_query_params_from_request(
Expand All @@ -182,7 +189,7 @@ def _search(
query_kwargs.pop("sort_by")
result = inbox_search(**query_kwargs)
else:
query_kwargs["referrer"] = "search.group_index"
query_kwargs["referrer"] = referrer
Copy link
Member

Choose a reason for hiding this comment

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

Should we default to DEFAULT_REFERRER here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

referrer will always be passed in and defined here -- the default is set in the callers

result = search.query(**query_kwargs)
return result, query_kwargs

Expand Down Expand Up @@ -234,6 +241,7 @@ def get(self, request: Request, organization) -> Response:
:qparam list collapse: an optional list of strings to opt out of certain pieces of data. Supports `stats`, `lifetime`, `base`, `unhandled`
"""
stats_period = request.GET.get("groupStatsPeriod")
referrer = request.GET.get("referrer", DEFAULT_REFERRER)
try:
start, end = get_date_range_from_stats_period(request.GET)
except InvalidParams as e:
Expand Down Expand Up @@ -335,6 +343,7 @@ def get(self, request: Request, organization) -> Response:
organization,
projects,
environments,
referrer,
{"count_hits": True, "date_to": end, "date_from": start},
)
except (ValidationError, discover.InvalidSearchQuery) as exc:
Expand Down Expand Up @@ -440,6 +449,7 @@ def put(self, request: Request, organization) -> Response:
"""
projects = self.get_projects(request, organization)
is_fetching_replay_data = request.headers.get("X-Sentry-Replay-Request") == "1"
referrer = request.GET.get("referrer", DEFAULT_REFERRER)

if (
len(projects) > 1
Expand All @@ -456,6 +466,7 @@ def put(self, request: Request, organization) -> Response:
organization,
projects,
self.get_environments(request, organization),
referrer,
)

return update_groups(
Expand Down Expand Up @@ -486,7 +497,7 @@ def delete(self, request: Request, organization) -> Response:
:auth: required
"""
projects = self.get_projects(request, organization)

referrer = request.GET.get("referrer", DEFAULT_REFERRER)
is_fetching_replay_data = request.headers.get("X-Sentry-Replay-Request") == "1"

if (
Expand All @@ -504,6 +515,7 @@ def delete(self, request: Request, organization) -> Response:
organization,
projects,
self.get_environments(request, organization),
referrer,
)

return delete_groups(request, projects, organization.id, search_fn)
10 changes: 7 additions & 3 deletions src/sentry/issues/search.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ def _query_params_for_generic(
conditions: Sequence[Any],
actor: Optional[Any] = None,
categories: Optional[Sequence[GroupCategory]] = None,
referrer: str = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Optional if it's to accept None as a default value

) -> Optional[SnubaQueryParams]:
organization = Organization.objects.filter(id=organization_id).first()
if organization and features.has(
Expand All @@ -219,8 +220,9 @@ def _query_params_for_generic(
if categories is None:
logging.error("Category is required in _query_params_for_generic")
return None

category_ids = {gc.value for gc in categories}
if referrer != "api.feedback_index":
Copy link
Member

Choose a reason for hiding this comment

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

Are there other places in the codebase where we change behavior based on the referrer? To me it's a bit unexpected and I think I'd prefer a more explicit way of telling this code path you either do or don't want feedbacks. But if this is something we do elsewhere then this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think ideally we'd have a separate endpoint that only queried feedbacks, and feedbacks by default are hidden in this endpoint.

This would allow us to remove all of the referrer based behavior here. I'm not sure if we modify behavior by referrer elsewhere.

I agree that it is a bit weird -- I can attempt some refactors and making a separate endpoint as a follow up and then remove the referrer logic here -- what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

The followup with separate endpoints sounds like a great solution to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

okay sounds good!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on this feeling awkward, I'd rather see that as a specific param or different endpoint path.

category_ids.discard(GroupCategory.FEEDBACK.value)
Comment on lines +224 to +225
Copy link
Member

Choose a reason for hiding this comment

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

While we are here can we also apply the inverse:
If the referrer is feedback, replace any categories and ensure GroupCategory.FEEDBACK is present and the only value set.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can follow up with this -- probably want to slightly reorganize how we're doing this.

group_types = {
gt.type_id
for gt in grouptype.registry.get_visible(organization, actor)
Expand Down Expand Up @@ -248,13 +250,15 @@ def _query_params_for_generic(
return None


def get_search_strategies() -> Mapping[int, GroupSearchStrategy]:
def get_search_strategies(referrer: str) -> Mapping[int, GroupSearchStrategy]:
strategies = {}
for group_category in GroupCategory:
if group_category == GroupCategory.ERROR:
strategy = _query_params_for_error
else:
strategy = functools.partial(_query_params_for_generic, categories=[group_category])
strategy = functools.partial(
_query_params_for_generic, categories=[group_category], referrer=referrer
)
strategies[group_category.value] = strategy
return strategies

Expand Down
8 changes: 4 additions & 4 deletions src/sentry/search/snuba/executors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@ def _prepare_params_for_category(
get_sample: bool,
actor: Optional[Any] = None,
aggregate_kwargs: Optional[PrioritySortWeights] = None,
referrer: Optional[str] = None,
) -> SnubaQueryParams:
"""
:raises UnsupportedSearchQuery: when search_filters includes conditions on a dataset that doesn't support it
Expand Down Expand Up @@ -336,8 +337,7 @@ def _prepare_params_for_category(
orderby=orderby,
),
)

strategy = get_search_strategies()[group_category]
strategy = get_search_strategies(referrer)[group_category]
snuba_query_params = strategy(
pinned_query_partial,
selected_columns,
Expand Down Expand Up @@ -379,7 +379,6 @@ def snuba_search(
* the count of total results (rows) available for this query.
"""
filters = {"project_id": project_ids}

environments = None
if environment_ids is not None:
filters["environment"] = environment_ids
Expand Down Expand Up @@ -420,7 +419,7 @@ def snuba_search(
if not group_categories:
group_categories = {
gc
for gc in get_search_strategies().keys()
for gc in get_search_strategies(referrer).keys()
if gc != GroupCategory.PROFILE.value
or features.has("organizations:issue-platform", organization, actor=actor)
}
Expand Down Expand Up @@ -448,6 +447,7 @@ def snuba_search(
get_sample,
actor,
aggregate_kwargs,
referrer,
)
except UnsupportedSearchQuery:
pass
Expand Down
92 changes: 91 additions & 1 deletion tests/snuba/search/test_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from sentry.exceptions import InvalidSearchQuery
from sentry.issues.grouptype import (
ErrorGroupType,
FeedbackGroup,
NoiseConfig,
PerformanceNPlusOneGroupType,
PerformanceRenderBlockingAssetSpanGroupType,
Expand Down Expand Up @@ -73,6 +74,7 @@ def make_query(
date_from=None,
date_to=None,
cursor=None,
referrer=None,
aggregate_kwargs=None,
):
search_filters = []
Expand All @@ -87,7 +89,6 @@ def make_query(
kwargs["limit"] = limit
if aggregate_kwargs:
kwargs["aggregate_kwargs"] = {"priority": {**aggregate_kwargs}}

return self.backend.query(
projects,
search_filters=search_filters,
Expand All @@ -97,6 +98,7 @@ def make_query(
date_from=date_from,
date_to=date_to,
cursor=cursor,
referrer=referrer,
**kwargs,
)

Expand Down Expand Up @@ -3684,6 +3686,94 @@ def test_rejected_filters(self):
== []
)

def test_feedback_category_hidden_default(self):
with self.feature(
["organizations:issue-platform", FeedbackGroup.build_visible_feature_name()]
):
event_id_1 = uuid.uuid4().hex
_, group_info = process_event_and_issue_occurrence(
{
"id": uuid.uuid4().hex,
"project_id": 1,
"event_id": event_id_1,
"fingerprint": ["nonsense"],
"issue_title": "User Feedback",
"subtitle": "it was bad",
"culprit": "api/123",
"resource_id": "1234",
"evidence_data": {"Test": 123},
"evidence_display": [
{"name": "hi", "value": "bye", "important": True},
{"name": "what", "value": "where", "important": False},
],
"type": FeedbackGroup.type_id,
"detection_time": datetime.now().timestamp(),
"level": "info",
},
{
"event_id": event_id_1,
"project_id": self.project.id,
"title": "some problem",
"platform": "python",
"tags": {"my_tag": "1"},
"timestamp": before_now(minutes=1).isoformat(),
"received": before_now(minutes=1).isoformat(),
},
)
results = self.make_query(
search_filter_query="issue.category:feedback",
date_from=self.base_datetime,
date_to=self.base_datetime + timedelta(days=10),
)
assert set(results) == set()

def test_feedback_category_show_when_referrer_set(self):
with self.feature(
[
"organizations:issue-platform",
FeedbackGroup.build_visible_feature_name(),
FeedbackGroup.build_ingest_feature_name(),
]
):
event_id_1 = uuid.uuid4().hex
_, group_info = process_event_and_issue_occurrence(
{
"id": uuid.uuid4().hex,
"project_id": 1,
"event_id": event_id_1,
"fingerprint": ["nonsense"],
"issue_title": "User Feedback",
"subtitle": "it was bad",
"culprit": "api/123",
"resource_id": "1234",
"evidence_data": {"Test": 123},
"evidence_display": [
{"name": "hi", "value": "bye", "important": True},
{"name": "what", "value": "where", "important": False},
],
"type": FeedbackGroup.type_id,
"detection_time": datetime.now().timestamp(),
"level": "info",
},
{
"event_id": event_id_1,
"project_id": self.project.id,
"title": "some problem",
"platform": "python",
"tags": {"my_tag": "1"},
"timestamp": before_now(minutes=1).isoformat(),
"received": before_now(minutes=1).isoformat(),
},
)
results = self.make_query(
search_filter_query="issue.category:feedback",
referrer="api.feedback_index",
date_from=self.base_datetime,
date_to=self.base_datetime + timedelta(days=10),
)
assert group_info is not None
assert list(results) == [group_info.group]


class CdcEventsSnubaSearchTest(TestCase, SharedSnubaMixin):
@property
Expand Down
Loading