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

Android Profiler on calling thread #2691

Merged
merged 4 commits into from May 5, 2023

Conversation

stefanosiano
Copy link
Member

📜 Description

AndroidTransactionProfiler now starts/stops profiles without offloading to the executorService
removed any file checks due to the code running possibly on the main thread
wrapped Debug methods into try/catch to compensate lack of file checks

💡 Motivation and Context

If the executor service is very busy, the profiler would start with some delay, and when finishing it would wait for the executor to finish, possibly causing ANRs.
fixes #2649

💚 How did you test it?

Unit test

📝 Checklist

  • 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

…ng to the executorService

removed any file checks due to the code running possibly on the main thread
wrapped Debug methods into try/catch to compensate lack of file checks
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 323.46 ms 389.62 ms 66.16 ms
Size 1.72 MiB 2.28 MiB 565.45 KiB

Previous results on branch: fis/android-profiler-drop-executor-service

Startup times

Revision Plain With Sentry Diff
3481dd7 347.71 ms 407.70 ms 59.99 ms
aa0dbc4 283.43 ms 330.94 ms 47.51 ms

App size

Revision Plain With Sentry Diff
3481dd7 1.72 MiB 2.26 MiB 552.61 KiB
aa0dbc4 1.72 MiB 2.26 MiB 552.61 KiB

@stefanosiano stefanosiano marked this pull request as ready for review May 3, 2023 10:27
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (8e5929c) 81.05% compared to head (1dbf094) 81.05%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2691   +/-   ##
=========================================
  Coverage     81.05%   81.05%           
  Complexity     4427     4427           
=========================================
  Files           345      345           
  Lines         16356    16356           
  Branches       2219     2219           
=========================================
  Hits          13258    13258           
  Misses         2170     2170           
  Partials        928      928           

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

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM, but in the original issue this was somehow related to slow network connection. Have you managed to reproduce it? I wasn't sure how the slow connectivity can be the reason here, because our HttpConnection uses a different thread pool (also single-threaded), so in theory they should not conflict with SentryExecutorService

@stefanosiano
Copy link
Member Author

stefanosiano commented May 5, 2023

LGTM, but in the original issue this was somehow related to slow network connection. Have you managed to reproduce it? I wasn't sure how the slow connectivity can be the reason here, because our HttpConnection uses a different thread pool (also single-threaded), so in theory they should not conflict with SentryExecutorService

I reproduced it by spawning lots of transactions and having a fake dsn to have all the envelopes sending timeout.
The SentryExecutorService is used by SendCachedEnvelopeIntegration and SendCachedEnvelopeFireAndForgetIntegration.
I think in case of slow connection envelopes are cached to disk and sent, but there is a wait in the processing of the envelope file
Does it make sense or am i missing something?
At the moment the SentryExecutorService is not used for any critical things anymore (except scheduling auto finish of profiling, ttfd and cpu/mem collection, but that's more to cover edge cases), but we could think about using an executor only for the envelope handling.

@romtsn
Copy link
Member

romtsn commented May 5, 2023

@stefanosiano ah gotcha, that makes sense, yes! And then we were blocking on the profiler, although the executor was already busy. I think it's fine to use the executor as long as we don't call blocking .get on the main thread (except the startup crash case, where we block for 5s max), so this looks good

@stefanosiano stefanosiano merged commit 2011dda into main May 5, 2023
18 checks passed
@stefanosiano stefanosiano deleted the fis/android-profiler-drop-executor-service branch May 5, 2023 12:05
@helloword1
Copy link

When will the new version be released to fix the issue?

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.

In situations where the network is slow, ANR often occurs
3 participants