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.

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(related_issues): Support passing an event_id to the endpoint #70878

Merged
merged 5 commits into from
May 15, 2024

Conversation

armenzg
Copy link
Member

@armenzg armenzg commented May 14, 2024

This enables showing trace connected issues from the issue details page for the event we're currently viewing.

This enables showing trace connected issues from the issue details page for the event we're currently viewing.
@armenzg armenzg self-assigned this May 14, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label May 14, 2024
"event_id": request.query_params.get("event_id"),
"project_id": request.query_params.get("project_id"),
}
data, meta = RELATED_ISSUES_ALGORITHMS[related_type](group, extra_args)
Copy link
Member Author

Choose a reason for hiding this comment

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

One of the two algorithms will now use the extra two query parameters.

data, meta = RELATED_ISSUES_ALGORITHMS[related_type](group)
extra_args = {
"event_id": request.query_params.get("event_id"),
"project_id": request.query_params.get("project_id"),
Copy link
Member Author

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.

Copy link
Member

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?

meta: dict[str, str] = {}
event_id = extra_args.get("event_id")
project_id = extra_args.get("project_id")
if event_id:
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 new approach we want to support.

assert event.group_id == group.id
else:
# If we drop trace connected issues from similar issues we can remove this
event = group.get_recommended_event_for_environments()
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 old approach.

def load_errors(
self,
project: Project,
span_id: str | None = None,
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 is an unrelated change that makes calling this code less weird.

@@ -47,20 +45,43 @@ 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)
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to pass the span_id anymore.

recommended_event = group.get_recommended_event_for_environments() # type: ignore[union-attr]
assert recommended_event is not None # It helps with typing

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]
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

qs_params={
"type": "trace_connected",
"event_id": another_proj_event.event_id,
"project_id": another_proj_event.project_id,
Copy link
Member Author

Choose a reason for hiding this comment

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

We test in here that we pass event_id and project_id unlike the previous test.

@armenzg armenzg marked this pull request as ready for review May 14, 2024 19:10
@armenzg armenzg requested a review from a team as a code owner May 14, 2024 19:10
Comment on lines +30 to +35
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
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError produced here would bubble up as generic 500 Internal Server Error, but it should be caught in the front end and return 400 Bad Request

Comment on lines +45 to +46
except AssertionError:
return Response({}, status=400)
Copy link
Member

Choose a reason for hiding this comment

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

ideally it would say which field, but this is fine.

@armenzg armenzg merged commit e34e412 into master May 15, 2024
47 of 48 checks passed
@armenzg armenzg deleted the feat/support-trace-id-in-endpoint/armenzg branch May 15, 2024 15:58
Copy link

sentry-io bot commented May 16, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ SnubaError: HTTPConnectionPool(host='127.0.0.1', port=10006): Read timed out. (read timeout=30) /api/0/issues/{issue_id}/related-issues/ View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants