Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix invalid assertion issues grouping #537
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
Fix invalid assertion issues grouping #537
Changes from all commits
470126f
8ce0dee
92226fd
7060c54
4c33ddc
7e58b47
93d6b75
3b0e59b
0e18e3f
9b8ca8b
a112ae9
cb34dfe
763abc2
a958f93
4eac75b
ffc1c3c
4cb0875
eabb15b
76a00fe
e328c84
75051cf
9d9f341
ff4e72a
2270015
7a3eb94
e9f2823
b243944
dd940b3
9d0ef3e
25227cd
9300a53
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We really have to reevaluate the use of tags in the SDK. As a general rule, the SDK should not set any tags and add that information in contexts instead. Relay then handles those and elevates specific ones to tags.
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.
I don't follow this.
CaptureException
does not actually capture an exception. Instead it adds a whole bunch of markers that are later getting picked up by the bridge?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.
Can these be put in
contexts
instead?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.
The problem with using context values here is that these are ignored by
sentry-native
when it captures a native crash on Android and thus there will be no way to identify whether the event is an assertion or not upon the next application launch.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.
I don't follow. Why can we call
CaptureException
from inCaptureAssertion
but we have to go straight to the bridge?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.
That's a good point. I've moved the assertion-specific logic from
CaptureException
toCaptureAssertion
. Also, in this case we can leave theCaptureException
interface stub empty since on Android assert/ensure handling flow is different from other platforms.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.
Where is this coming from?
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.
Previously, UE caused the app to crash whenever an assertion failed and the Crashpad captured these crashes for us. We've changed how assertions are handled so now when it fails a timeout is used to ensure a corresponding event is sent to Sentry within the same session before shutting down the app manually.