Skip to content

Conversation

@brustolin
Copy link
Contributor

@brustolin brustolin commented Jun 3, 2022

📜 Description

When the app stops responding we should create an error event reporting it.

image

💡 Motivation and Context

Close #1052

💚 How did you test it?

Unit test

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@brustolin brustolin requested a review from philipphofmann as a code owner June 3, 2022 13:46
@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 3c6bcad

@brustolin brustolin changed the title feat: Create ANR error event. feat: Add ANR tracking Jun 3, 2022
@bruno-garcia bruno-garcia requested review from marandaneto and vaind June 3, 2022 14:32
@bruno-garcia
Copy link
Member

@vaind and @marandaneto as they know about ANR on Android and Unity

Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

Some notes below, I'm not confident in my understanding of sentry-cocoa to fully review this.

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2022

Codecov Report

Merging #1861 (2737889) into master (472d146) will increase coverage by 0.06%.
The diff coverage is 97.26%.

❗ Current head 2737889 differs from pull request most recent head 3c6bcad. Consider uploading reports for the commit 3c6bcad to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1861      +/-   ##
==========================================
+ Coverage   91.91%   91.98%   +0.06%     
==========================================
  Files         200      200              
  Lines        9450     9557     +107     
==========================================
+ Hits         8686     8791     +105     
- Misses        764      766       +2     
Impacted Files Coverage Δ
Sources/Sentry/SentryClient.m 99.15% <ø> (ø)
Sources/Sentry/include/SentryDependencyContainer.h 30.00% <0.00%> (-3.34%) ⬇️
Sources/Sentry/SentryOptions.m 99.19% <83.33%> (-0.40%) ⬇️
Sources/Sentry/SentryANRTrackingIntegration.m 95.55% <96.66%> (-4.45%) ⬇️
Sources/Sentry/SentryANRTracker.m 98.93% <97.87%> (-1.07%) ⬇️
Sources/Sentry/Public/SentryOptions.h 100.00% <100.00%> (ø)
...s/Sentry/SentryCrashDefaultMachineContextWrapper.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryDependencyContainer.m 92.68% <100.00%> (+1.13%) ⬆️
...rces/Sentry/SentryOutOfMemoryTrackingIntegration.m 100.00% <100.00%> (+2.27%) ⬆️
Sources/Sentry/SentryStacktraceBuilder.m 89.18% <100.00%> (+2.98%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa49e72...3c6bcad. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

First pass, still have plenty of files to review. Thanks for doing this @brustolin.

@brustolin
Copy link
Contributor Author

An alternative would to look into each thread for the one that contains the main function as first frame

I'm not sure if this would also work for release builds.

AFAIK, for iOS, the app entry point is a main function, created manually or by the compiler, does not matter the build configuration. For MacOS it may be called start

But I like this idea better

SentryCrashThread mainThreadID = pthread_mach_thread_np(pthread_self());

@bruno-garcia
Copy link
Member

Flaky test?

REF_NAME: 1861/merge
CONFIGURATION: Test
2022-06-14 13:[5](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:6)0:21.730 xcodebuild[3[6](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:7)51:14285] [MT] DVTAssertions: Warning in /Library/Caches/com.apple.xbs/Sources/IDEFrameworks/IDEFrameworks-18212/IDEFoundation/Testing/IDETestRunSpecificationBuilder.m:6[7](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:8)8
Details:  Failed to compute path to baseline file during test run spec construction: <XCTHTestRunSpecification: 0x7fd69f9ae740>
Object:   <IDETestRunSpecificationBuilder>
Method:   +testRunSpecificationsForTestingSpecifiers:scheme:buildables:withBuildParameters:additionalEnvironmentVariables:additionalCommandLineArguments:testRerunPolicy:includeClangProfileParameters:shouldDebugAppExtensions:error:
Thread:   <NSThread: 0x7fd69c41a920>{number = 1, name = main}
Please file a bug at https://feedbackassistant.apple.com with this warning message and any useful information you can provide.
...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................T......................T......................T................T..........................................................................................................................................................................................................................................................................................................................................................................................................................T.T.................................................................................................................................................................................................................................................................................................................................................................
2022-06-14 13:57:12.370 xcodebuild[3651:142[8](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:9)5] [MT] IDETestOperationsObserverDebug: [10](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:11)8.000 elapsed -- Testing started completed.
2022-06-14 13:57:[12](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:13).370 xcodebuild[3651:14285] [MT] IDETestOperationsObserverDebug: 0.000 sec, +0.000 sec -- start
2022-06-14 [13](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:14):57:12.370 xcodebuild[3651:[14](https://github.com/getsentry/sentry-cocoa/runs/6881692740?check_suite_focus=true#step:8:15)285] [MT] IDETestOperationsObserverDebug: 108.000 sec, +108.000 sec -- end
Failing tests:
	SentryTests:
		SentryThreadInspectorTests.testStacktraceHasFrames_forEveryThread()

@brustolin
Copy link
Contributor Author

Flaky test?

But only on Catalyst.

@philipphofmann
Copy link
Member

@bruno-garcia, made a valid point that we should maybe turn this off by default for now to get some feedback before turning it on by default. We can add it to the list of new features in the wizard.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, expect we should disable this per default. When you disable App Hang tracking, we still track them for OOMs to avoid false OOMs right?

@brustolin
Copy link
Contributor Author

When you disable App Hang tracking, we still track them for OOMs to avoid false OOMs right?

Yes.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@brustolin brustolin merged commit 2339bee into master Jun 20, 2022
@brustolin brustolin deleted the feat/anr-report branch June 20, 2022 11:56
philipphofmann added a commit that referenced this pull request Jun 20, 2022
@philipphofmann philipphofmann restored the feat/anr-report branch June 20, 2022 15:33
philipphofmann added a commit that referenced this pull request Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Report App Hangs

7 participants