-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(events): Add has_stacktrace
property to BaseEvent
class
#79100
base: master
Are you sure you want to change the base?
Conversation
81d960a
to
d79899d
Compare
) | ||
|
||
threads = get_path(self.data, "threads", "values") or [] | ||
has_thread_frames = any(get_path(thread, "stacktrace", "frames") for thread in threads) |
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.
Do you have an example of where stack traces are extracted from threads?
So far, I have not seen stack traces being used to group mobile issues.
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.
There are a number of snapshot inputs where that's the case:
https://github.com/getsentry/sentry/blob/master/tests/sentry/grouping/grouping_inputs/cocoa_dispatch_client_callout.json
https://github.com/getsentry/sentry/blob/master/tests/sentry/grouping/grouping_inputs/block_invoke.json
https://github.com/getsentry/sentry/blob/master/tests/sentry/grouping/grouping_inputs/threads-compute-hashes.json
Also, it's part of the event schema:
https://develop.sentry.dev/sdk/data-model/event-payloads/threads/
https://github.com/getsentry/sentry-data-schemas/blob/main/relay/event.schema.json#L3844
@@ -439,6 +440,63 @@ def test_get_hashes_gets_hashes_and_variants_if_none_on_event(self): | |||
assert default_variant.component.values[0].id == "message" | |||
assert default_variant.component.values[0].values == ["Dogs are great!"] | |||
|
|||
def test_has_stacktrace(self): |
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.
Could you please split this into multiple cases?
Can you please pull these tests out of this class? I don't think we need DB support to run these tests and would make them faster.
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
This adds a new property,
has_stacktrace
, to theBaseEvent
class. It does exactly what you'd expect, given the name, handling the obvious cases (a single stacktrace in eitherexception
orthreads
) as well as the less obvious ones (top-level stacktraces, such as you might get by usingattach_stacktrace
withcapture_exception
, and cases where there are multiple stacktraces, such as with a chained exception).Note: We currently have a similar utility function,
event_content_has_stacktrace
, which we use when deciding whether or not to send a given event to Seer. The reasons I chose not to just move that function to a more neutral location and use it are a) that function doesn't handle top-level stacktraces, and b) in cases where there are multiple exceptions, it only looks at the last one in the chain. These are things we probably want to fix, but we can't do that without also changingget_stacktrace_string
and adding a bunch of new tests. At that point it felt like I would be straying too far afield from the core purpose of this PR, so instead I just left a TODO.