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

Remove profiler main thread io #2348

Merged
merged 10 commits into from Nov 15, 2022

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Nov 8, 2022

📜 Description

Removed a useless file exist check in AndroidTransactionProfiler
Offloaded AndroidTransactionProfiler methods to background threads using executor service

💡 Motivation and Context

When using Android Strict Mode, the file io that was happening in the main thread (file existence checks) were generating warnings, as in this issue

Closes #2317

💚 How did you test it?

I updated the unit tests

📝 Checklist

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

🔮 Next steps

wrapped AndroidTransactionProfiler methods in executor service
fixed crash in sample app's ProfilingActivity
@stefanosiano stefanosiano changed the base branch from main to feat/profiling-frame-metrics November 8, 2022 16:36
wrapped AndroidTransactionProfiler methods in executor service
fixed crash in sample app's ProfilingActivity
@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 378.35 ms 453.73 ms 75.38 ms
Size 1.73 MiB 2.33 MiB 612.62 KiB

Baseline results on branch: feat/profiling-frame-metrics

Startup times

Revision Plain With Sentry Diff
e7e2fb1 317.90 ms 353.63 ms 35.73 ms
43860fd 297.76 ms 330.25 ms 32.49 ms
9385630 313.85 ms 319.06 ms 5.21 ms
5b4c2f3 319.28 ms 356.79 ms 37.52 ms
922e325 360.50 ms 405.19 ms 44.69 ms
b1b2eb0 315.10 ms 349.81 ms 34.71 ms
5711c14 298.11 ms 323.40 ms 25.30 ms
a6b6b98 370.26 ms 459.67 ms 89.41 ms

App size

Revision Plain With Sentry Diff
e7e2fb1 1.73 MiB 2.32 MiB 612.50 KiB
43860fd 1.73 MiB 2.33 MiB 612.62 KiB
9385630 1.73 MiB 2.32 MiB 612.50 KiB
5b4c2f3 1.73 MiB 2.32 MiB 612.52 KiB
922e325 1.73 MiB 2.32 MiB 612.55 KiB
b1b2eb0 1.73 MiB 2.32 MiB 612.50 KiB
5711c14 1.73 MiB 2.32 MiB 612.50 KiB
a6b6b98 1.73 MiB 2.32 MiB 612.52 KiB

Previous results on branch: fix/profiler-main-thread-io

Startup times

Revision Plain With Sentry Diff
9eb967e 364.30 ms 394.56 ms 30.26 ms
552b6d9 310.70 ms 381.00 ms 70.30 ms
1fea68e 328.90 ms 348.24 ms 19.34 ms
413eb4e 343.06 ms 381.49 ms 38.43 ms

App size

Revision Plain With Sentry Diff
9eb967e 1.73 MiB 2.33 MiB 612.62 KiB
552b6d9 1.73 MiB 2.32 MiB 612.23 KiB
1fea68e 1.73 MiB 2.32 MiB 612.23 KiB
413eb4e 1.73 MiB 2.32 MiB 612.23 KiB

@stefanosiano stefanosiano marked this pull request as ready for review November 9, 2022 17:01
@romtsn
Copy link
Member

romtsn commented Nov 10, 2022

Closes #2317

@codecov-commenter
Copy link

Codecov Report

Base: 80.26% // Head: 80.32% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (cf455ed) compared to base (cafe0f3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@                        Coverage Diff                         @@
##             feat/profiling-frame-metrics    #2348      +/-   ##
==================================================================
+ Coverage                           80.26%   80.32%   +0.05%     
  Complexity                           3687     3687              
==================================================================
  Files                                 292      292              
  Lines                               13840    13828      -12     
  Branches                             1832     1824       -8     
==================================================================
- Hits                                11109    11107       -2     
+ Misses                               2016     2012       -4     
+ Partials                              715      709       -6     
Impacted Files Coverage Δ
...sentry/spring/webflux/TransactionNameProvider.java 50.00% <0.00%> (-5.56%) ⬇️
...pring/jakarta/webflux/TransactionNameProvider.java 50.00% <0.00%> (-5.56%) ⬇️
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
...o/sentry/spring/webflux/SentryRequestResolver.java 73.68% <0.00%> (+3.68%) ⬆️
.../spring/jakarta/webflux/SentryRequestResolver.java 73.68% <0.00%> (+3.68%) ⬆️
...java/io/sentry/spring/webflux/SentryWebFilter.java 100.00% <0.00%> (+4.76%) ⬆️
...sentry/spring/jakarta/webflux/SentryWebFilter.java 100.00% <0.00%> (+4.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stefanosiano stefanosiano merged commit ace3c67 into feat/profiling-frame-metrics Nov 15, 2022
@stefanosiano stefanosiano deleted the fix/profiler-main-thread-io branch November 15, 2022 17:59
stefanosiano added a commit that referenced this pull request Nov 18, 2022
* added ProfileMeasurements to AndroidTransactionProfiler and ProfilingTraceData
* added frameMetrics (slow and frozen frames) and screen refresh rate
* added a SentryFrameMetricsCollector to collect frameMetrics
* updated espresso test dependency to stable to run on Android 13 devices
* increased benchmark cpu overhead threshold from 5% to 5.5%
* updated benchmark device from OnePlus Nord N200 to Google Pixel 3a
* added automatic activity transaction in ui test
* Remove profiler main thread io (#2348)
* fixed crash in sample app's ProfilingActivity
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.

None yet

4 participants