-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(related_issues): Support passing an event_id to the endpoint #70878
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,5 +35,12 @@ def get(self, request: Request, group: Group) -> Response: | |
""" | ||
# The type of related issues to retrieve. Can be either `same_root_cause` or `trace_connected`. | ||
related_type = request.query_params["type"] | ||
data, meta = RELATED_ISSUES_ALGORITHMS[related_type](group) | ||
return Response({"type": related_type, "data": data, "meta": meta}) | ||
extra_args = { | ||
"event_id": request.query_params.get("event_id"), | ||
"project_id": request.query_params.get("project_id"), | ||
} | ||
try: | ||
data, meta = RELATED_ISSUES_ALGORITHMS[related_type](group, extra_args) | ||
return Response({"type": related_type, "data": data, "meta": meta}) | ||
except AssertionError: | ||
return Response({}, status=400) | ||
Comment on lines
+45
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ideally it would say which field, but this is fine. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,9 @@ | ||
# Module to evaluate if other errors happened in the same trace. | ||
# | ||
# Refer to README in module for more details. | ||
from sentry import eventstore | ||
from sentry.api.utils import default_start_end_dates | ||
from sentry.eventstore.models import GroupEvent | ||
from sentry.models.group import Group | ||
from sentry.models.project import Project | ||
from sentry.search.events.builder import QueryBuilder | ||
|
@@ -11,12 +13,47 @@ | |
from sentry.utils.snuba import bulk_snuba_queries | ||
|
||
|
||
def trace_connected_analysis(group: Group) -> tuple[list[int], dict[str, str]]: | ||
# If we drop trace connected issues from similar issues we can stop using the group | ||
def trace_connected_analysis( | ||
group: Group, extra_args: dict[str, str | None] | None = None | ||
) -> tuple[list[int], dict[str, str]]: | ||
"""Determine if the group has a trace connected to it and return other issues that were part of it.""" | ||
event = group.get_recommended_event_for_environments() | ||
if not event or event.trace_id is None: | ||
return [], {} | ||
if extra_args is None: | ||
extra_args = {} | ||
|
||
issues: list[int] = [] | ||
meta: dict[str, str] = {} | ||
event_id = extra_args.get("event_id") | ||
project_id = extra_args.get("project_id") | ||
if event_id: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new approach we want to support. |
||
# If we are passing an specific event_id, we need to get the project_id | ||
assert project_id is not None | ||
event = eventstore.backend.get_event_by_id(project_id, event_id, group_id=group.id) | ||
# If we are requesting an specific event, we want to be notified with an error | ||
assert event is not None | ||
# This ensures that the event is actually part of the group and we are notified | ||
assert event.group_id == group.id | ||
Comment on lines
+30
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AssertionError produced here would bubble up as generic |
||
else: | ||
# If we drop trace connected issues from similar issues we can remove this | ||
event = group.get_recommended_event_for_environments() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old approach. |
||
|
||
if event: | ||
issues, meta = trace_connected_issues(event) | ||
else: | ||
meta["error"] = "No event found for group." | ||
|
||
return issues, meta | ||
|
||
|
||
def trace_connected_issues(event: GroupEvent) -> tuple[list[int], dict[str, str]]: | ||
meta = {"event_id": event.event_id} | ||
if event.trace_id: | ||
meta["trace_id"] = event.trace_id | ||
else: | ||
meta["error"] = "No trace_id found in event." | ||
return [], meta | ||
|
||
group = event.group | ||
org_id = group.project.organization_id | ||
# XXX: Test without a list and validate the data type | ||
project_ids = list(Project.objects.filter(organization_id=org_id).values_list("id", flat=True)) | ||
|
@@ -41,4 +78,4 @@ def trace_connected_analysis(group: Group) -> tuple[list[int], dict[str, str]]: | |
if datum["issue.id"] != group.id # Exclude itself | ||
} | ||
) | ||
return transformed_results, {"event_id": event.event_id, "trace_id": event.trace_id} | ||
return transformed_results, meta |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3406,7 +3406,11 @@ def convert_event_data_to_span(self, event: Event) -> dict[str, Any]: | |
|
||
return span_data | ||
|
||
def load_errors(self, project: Project, span_id: str) -> list[Event]: | ||
def load_errors( | ||
self, | ||
project: Project, | ||
span_id: str | None = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unrelated change that makes calling this code less weird. |
||
) -> list[Event]: | ||
"""Generates trace with errors across two projects.""" | ||
start, _ = self.get_start_end_from_day_ago(1000) | ||
error_data = load_data( | ||
|
@@ -3416,7 +3420,7 @@ def load_errors(self, project: Project, span_id: str) -> list[Event]: | |
error_data["contexts"]["trace"] = { | ||
"type": "trace", | ||
"trace_id": self.trace_id, | ||
"span_id": span_id, | ||
"span_id": span_id or uuid4().hex[:16], | ||
} | ||
error_data["level"] = "fatal" | ||
error = self.store_event(error_data, project_id=project.id) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
from uuid import uuid4 | ||
|
||
from django.urls import reverse | ||
|
||
from sentry.testutils.cases import APITestCase, SnubaTestCase, TraceTestCase | ||
|
@@ -47,20 +45,49 @@ def test_same_root_related_issues(self) -> None: | |
assert response.json() == {"type": "same_root_cause", "data": [5], "meta": {}} | ||
|
||
def test_trace_connected_errors(self) -> None: | ||
error_event, _, another_proj_event = self.load_errors(self.project, uuid4().hex[:16]) | ||
error_event, _, another_proj_event = self.load_errors(self.project) | ||
group = error_event.group | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to pass the |
||
self.group_id = error_event.group_id # type: ignore[assignment] | ||
recommended_event = group.get_recommended_event_for_environments() # type: ignore[union-attr] | ||
assert recommended_event is not None # It helps with typing | ||
# This assertion ensures that the behaviour is different from the next test | ||
assert recommended_event.event_id != another_proj_event.event_id | ||
|
||
# This asserts that there are two issues which belong to the same trace | ||
assert error_event.group_id != another_proj_event.group_id | ||
assert error_event.project.id != another_proj_event.project.id | ||
assert error_event.trace_id == another_proj_event.trace_id | ||
|
||
# This sets the group_id to the one we want to query about | ||
self.group_id = error_event.group_id # type: ignore[assignment] | ||
response = self.get_success_response(qs_params={"type": "trace_connected"}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this line closer to the call to the API since it is related. |
||
assert response.json() == { | ||
"type": "trace_connected", | ||
# This is the other issue in the trace that it is not itself | ||
"data": [another_proj_event.group_id], | ||
"meta": {"event_id": recommended_event.event_id, "trace_id": error_event.trace_id}, | ||
"meta": { | ||
"event_id": recommended_event.event_id, | ||
"trace_id": error_event.trace_id, | ||
}, | ||
} | ||
|
||
def test_trace_connected_errors_specific_event(self) -> None: | ||
error_event, _, another_proj_event = self.load_errors(self.project) | ||
|
||
# This sets the group_id to the one we want to query about | ||
self.group_id = another_proj_event.group_id # type: ignore[assignment] | ||
response = self.get_success_response( | ||
qs_params={ | ||
"type": "trace_connected", | ||
"event_id": another_proj_event.event_id, | ||
"project_id": another_proj_event.project_id, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We test in here that we pass |
||
) | ||
assert response.json() == { | ||
"type": "trace_connected", | ||
# This is the other issue in the trace that it is not itself | ||
"data": [error_event.group_id], | ||
"meta": { | ||
"event_id": another_proj_event.event_id, | ||
"trace_id": another_proj_event.trace_id, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project_id
is a required parameter for querying an event from Snuba.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's required, shouldn't this validate that it is not none?