Skip to content
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

chore: Logic for full blocking app hangs #4282

Merged
merged 23 commits into from
Aug 16, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Aug 14, 2024

📜 Description

Change the logic for detecting app hangs in the ANRTrackerV2 to use frames delay to detect app hangs. The first step is to only report a fully blocking app hang if the app didn't render a single frame in the timeoutInterval of the ANRTrackerV2.

It's worth noting that the SentryANRTrackingIntegrationV2 doesn't exist in the options yet, and further
work is required to surface this new feature to our users. Still, we should give this a thorough review. The diff looks huge, but it's mostly tests.

This PR is based on #4279.

#skip-changelog

💡 Motivation and Context

This is required for #3492.

💚 How did you test it?

Unit tests and on a real device.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Fix a race condition in the SentryFramesTracker and
SentryDelayedFramesTracker that sometimes leads to frame delay durations
longer than the queried interval of start and end time. This is fixed by
moving the previousFrameSystemTimestamp down to the
SentryDelayedFramesTracker so we can adequately synchronize it without
acquiring extra locks on the main thread.
Add SentryFramesDelayResult containing framesContributingToDelayCount,
which is the count for the frames that contributed to the frames delay
count. This is required for GH-3492.
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 99.64413% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.614%. Comparing base (8c4ac55) to head (c569694).
Report is 1 commits behind head on main.

Files Patch % Lines
Sources/Sentry/SentryANRTrackerV2.m 98.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4282       +/-   ##
=============================================
+ Coverage   91.609%   91.614%   +0.004%     
=============================================
  Files          617       616        -1     
  Lines        50046     50096       +50     
  Branches     18097     18010       -87     
=============================================
+ Hits         45847     45895       +48     
- Misses        4105      4110        +5     
+ Partials        94        91        -3     
Files Coverage Δ
Sources/Sentry/SentryANRTrackingIntegrationV2.m 97.368% <ø> (-0.132%) ⬇️
Sources/Sentry/SentryDelayedFramesTracker.m 99.074% <100.000%> (+0.017%) ⬆️
Sources/Sentry/SentryDependencyContainer.m 96.202% <100.000%> (+0.048%) ⬆️
...tions/FramesTracking/SentryFramesDelayResult.swift 100.000% <100.000%> (ø)
...sts/Integrations/ANR/SentryANRTrackerV2Tests.swift 100.000% <100.000%> (+1.481%) ⬆️
...ions/ANR/SentryANRTrackingIntegrationV2Tests.swift 95.620% <ø> (ø)
Sources/Sentry/SentryANRTrackerV2.m 99.065% <98.000%> (-0.935%) ⬇️

... and 18 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

The code change does not reflect the PR description.
Do you still plan do add changes to this PR?

Copy link

github-actions bot commented Aug 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1237.27 ms 1257.23 ms 19.96 ms
Size 21.58 KiB 706.54 KiB 684.96 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
01a28a9 1237.24 ms 1253.24 ms 16.00 ms
e072ad1 1232.40 ms 1253.19 ms 20.80 ms
43aa39d 1239.16 ms 1270.42 ms 31.26 ms
2f9c5f9 1218.78 ms 1239.58 ms 20.80 ms
f1b1dec 1234.27 ms 1249.10 ms 14.84 ms
11b2ffa 1204.86 ms 1218.16 ms 13.31 ms
94e1968 1218.20 ms 1230.57 ms 12.37 ms
bb5dc7d 1240.44 ms 1266.45 ms 26.01 ms
15b8c61 1223.16 ms 1244.83 ms 21.67 ms
881a955 1209.47 ms 1225.94 ms 16.47 ms

App size

Revision Plain With Sentry Diff
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
e072ad1 21.58 KiB 625.83 KiB 604.24 KiB
43aa39d 20.76 KiB 432.82 KiB 412.06 KiB
2f9c5f9 21.58 KiB 418.82 KiB 397.24 KiB
f1b1dec 21.58 KiB 616.14 KiB 594.56 KiB
11b2ffa 22.85 KiB 412.67 KiB 389.82 KiB
94e1968 21.58 KiB 614.73 KiB 593.15 KiB
bb5dc7d 22.85 KiB 412.98 KiB 390.13 KiB
15b8c61 20.76 KiB 419.67 KiB 398.91 KiB
881a955 22.85 KiB 407.63 KiB 384.78 KiB

Previous results on branch: feat/fully-blocking-app-hangs

Startup times

Revision Plain With Sentry Diff
8e92619 1230.45 ms 1250.06 ms 19.61 ms

App size

Revision Plain With Sentry Diff
8e92619 21.58 KiB 706.83 KiB 685.25 KiB

Base automatically changed from ref/add-frames-delay-result to main August 14, 2024 11:03
@philipphofmann
Copy link
Member Author

philipphofmann commented Aug 14, 2024

The code change does not reflect the PR description. Do you still plan do add changes to this PR?

Sorry, the stacked PRs messed up the diff. I need to merge main back into here. Then it should be ready for review.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

The new algorithm looks good to me.

P.S.: Anything that prevents us from written the V2 in swift already?

Sources/Sentry/SentryANRTrackingIntegrationV2.m Outdated Show resolved Hide resolved
@philipphofmann philipphofmann merged commit 279351b into main Aug 16, 2024
64 of 65 checks passed
@philipphofmann philipphofmann deleted the feat/fully-blocking-app-hangs branch August 16, 2024 06:00
@philipphofmann philipphofmann self-assigned this Sep 23, 2024
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.

2 participants