impr(sentry apps): Add Duration, Resource ids, Request id to the dashboard - #122930
impr(sentry apps): Add Duration, Resource ids, Request id to the dashboard#122930Christinarlong wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dbda0ff. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| SentryAppResourceType.ISSUE: SubjectSpec("group", [("issue", "id")]), | ||
| SentryAppResourceType.ACTIVITY_ALERT: SubjectSpec("group", [("issue", "id")]), | ||
| SentryAppResourceType.COMMENT: SubjectSpec("comment", [("comment_id",)]), | ||
| # TODO: AlertRule is a legacy model and should be migrated to Detector when we phase out the legacy payload |
There was a problem hiding this comment.
Should we just not support this to spare ourselves cleanup work later?
| @@ -386,6 +416,10 @@ def send_and_save_webhook_request( | |||
| if (p_id := response.headers.get("Sentry-Hook-Project")) and p_id.isdigit() | |||
| else None | |||
| ) | |||
| elapsed = getattr(response, "elapsed", None) | |||
| duration_ms = ( | |||
There was a problem hiding this comment.
We could also potentially record the duration here and above on connection errors as the difference between just before _send_webhook_request and now - not sure if that's meaningful but it would mean that we always have a duration on all requests (after 30d)
| @@ -361,6 +260,52 @@ def test_errors_only_filter(self) -> None: | |||
| assert response.status_code == 200 | |||
| assert len(response.data) == 2 | |||
|
|
|||
| def test_request_id_subject_and_duration_on_success_and_error_rows(self) -> None: | |||
| """request_id, subject, and duration are surfaced on both success and error rows.""" | |||
There was a problem hiding this comment.
| """request_id, subject, and duration are surfaced on both success and error rows.""" |
nit: this is just restating the line above it
| ) == ("456", "preprod_artifact") | ||
|
|
||
|
|
||
| def test_realistic_payload_fixtures_stay_within_buffer_limit() -> None: |
There was a problem hiding this comment.
I might be misunderstanding but this seems to not be testing anything at all?
| ) == ("d1e2f3a4-5678-4b9c-8d0e-1f2a3b4c5d6e", "autofix_run") | ||
|
|
||
|
|
||
| def test_seer_run_id_zero_is_not_treated_as_missing() -> None: |
There was a problem hiding this comment.
Curious when we'd expect this to happen?
2 participants
Adds 4 new fields to the request buffer
For the subject/resource fields, there's a mapping we do that first checks if the specific event_type (issue.created, seer.autofix_run_started etc.) has a resource override else we have default resources(e.g Event, Group etc.) for each of the webhook resources. Currently the mapping from webhook resource -> subject is 1:1 but I think that makes sense for most cases since webhooks are generally some model being serialized.
This PR also breaks out ConnectionError to be its own handling (kinda), mainly jsut add a new fake status code so we can tell in the UI