feat: Add App Hangs tracking #1906
Conversation
Co-authored-by: Ivan Dlugos <[email protected]>
Co-authored-by: Philipp Hofmann <[email protected]>
Co-authored-by: Philipp Hofmann <[email protected]>
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Good job, I maybe found a few issues to address.
| if (result == amount) | ||
| break; |
There was a problem hiding this comment.
m: Shouldn't we somehow handle this edge case? I'm not sure how likely it is that stacktraces will be cut off, but I already see complaints that the stacktrace is incomplete.
There was a problem hiding this comment.
We can't alloc more memory here. What do you suggest?
There was a problem hiding this comment.
Hmm, I'm not sure. @Swatinem, can you maybe help us here? We allocate a struct with a maximum of 100 stack entries before suspending the threads. Here we are walking the stack cursor after suspending the threads. Do you have any recommendations on what to do when the stack has more than 100 entries? Now we ignore this, and the stacktrace would miss a few entries. We can't allocate memory either.
There was a problem hiding this comment.
@armcknight, do you maybe have any ideas on how to solve the issue above ⬆️ ?
There was a problem hiding this comment.
@brustolin, what about merging this PR and creating an issue for this edge case? This is the only issue left, I believe.
Co-authored-by: Philipp Hofmann <[email protected]>
| if (result == amount) | ||
| break; |
There was a problem hiding this comment.
Hmm, I'm not sure. @Swatinem, can you maybe help us here? We allocate a struct with a maximum of 100 stack entries before suspending the threads. Here we are walking the stack cursor after suspending the threads. Do you have any recommendations on what to do when the stack has more than 100 entries? Now we ignore this, and the stacktrace would miss a few entries. We can't allocate memory either.
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
Almost LGTM, thanks @brustolin 🥇
Co-authored-by: Philipp Hofmann <[email protected]>
| // Function used for tests | ||
| - (void)clear; |
There was a problem hiding this comment.
Recommend moving to a private header.
There was a problem hiding this comment.
This is not public anyways. Only headers in /Sources/Sentry/Public are.
philipphofmann
left a comment
philipphofmann
left a comment
There was a problem hiding this comment.
LGTM 🚀, great job @brustolin 👏 . Let's create an issue for the edge case that a stacktrace has more than 100 entries.
Added app hangs tracking Co-authored-by: Dhiogo Brustolin <[email protected]>
Added app hangs tracking Co-authored-by: Dhiogo Brustolin <[email protected]>
With #1906, we have our own solution of detecting app hangs. We can remove the unused SentryCrash deadlock detection.
With #1906, we have our own solution for detecting app hangs. We can remove the unused SentryCrash deadlock detection.
The original PR was reverted with #1905 because
testStacktraceHasFrames_forEveryThreadis sometimes flaky and we need to figure out what causes this before merging this again.Resolves: #1052