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

Disable Android concurrent profiling #2434

Merged
merged 10 commits into from
Dec 22, 2022

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Dec 16, 2022

📜 Description

SentryTracer sends profile inside transaction envelope
AndroidTransactionProfiler now profiles only one transaction at a time
Removed @deprecated annotations to methods in SentryClient to capture a profile with a transaction
Added methods in Hub to capture a profile with a transaction

💡 Motivation and Context

Concurrent profiling create issues with dynamic sampling. We have to send the profile in the same envelope of the transaction to have dynamic sampling work with both of them.
Since we don't have control over the Android profiler internals, we currently cannot adapt the profiler to create a profile for each concurrent transaction, so we are disabling it.

💚 How did you test it?

Unit and ui 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

AndroidTransactionProfiler now profiles only one transaction at a time
Removed @deprecated annotations to methods in SentryClient to capture a profile with a transaction
Added methods in Hub to capture a profile with a transaction
@stefanosiano stefanosiano changed the title Disable concurrent profiling in Disable Android concurrent profiling Dec 16, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 16, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 369.54 ms 397.92 ms 28.38 ms
Size 1.73 MiB 2.33 MiB 616.48 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f1150bc 352.62 ms 453.27 ms 100.65 ms
a04f788 321.78 ms 354.12 ms 32.35 ms
4a9c176 319.77 ms 363.20 ms 43.43 ms
90e9745 314.68 ms 357.28 ms 42.60 ms
3695453 314.63 ms 353.10 ms 38.47 ms
16371c5 314.02 ms 394.54 ms 80.52 ms
507f924 342.51 ms 402.65 ms 60.14 ms
703d523 330.27 ms 377.66 ms 47.39 ms
4a9c176 320.62 ms 334.68 ms 14.06 ms
87598a5 310.41 ms 373.27 ms 62.86 ms

App size

Revision Plain With Sentry Diff
f1150bc 1.73 MiB 2.33 MiB 615.66 KiB
a04f788 1.73 MiB 2.32 MiB 609.88 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
90e9745 1.73 MiB 2.32 MiB 608.63 KiB
3695453 1.73 MiB 2.32 MiB 611.62 KiB
16371c5 1.73 MiB 2.32 MiB 611.62 KiB
507f924 1.73 MiB 2.32 MiB 609.95 KiB
703d523 1.73 MiB 2.33 MiB 613.23 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
87598a5 1.73 MiB 2.33 MiB 614.63 KiB

Previous results on branch: android/revert-concurrent-profiling

Startup times

Revision Plain With Sentry Diff
7bdc3d5 321.85 ms 358.76 ms 36.90 ms
7792c74 323.35 ms 376.27 ms 52.91 ms
7feb887 341.53 ms 381.33 ms 39.80 ms
37e37b0 314.21 ms 343.14 ms 28.93 ms
3502262 295.67 ms 347.65 ms 51.98 ms
755f6f3 327.00 ms 361.27 ms 34.27 ms

App size

Revision Plain With Sentry Diff
7bdc3d5 1.73 MiB 2.33 MiB 616.48 KiB
7792c74 1.73 MiB 2.33 MiB 616.40 KiB
7feb887 1.73 MiB 2.33 MiB 616.00 KiB
37e37b0 1.73 MiB 2.33 MiB 616.41 KiB
3502262 1.73 MiB 2.33 MiB 615.78 KiB
755f6f3 1.73 MiB 2.33 MiB 615.78 KiB

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2022

Codecov Report

Base: 80.03% // Head: 80.08% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (fcc20dc) compared to base (93fbd16).
Patch coverage: 69.23% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2434      +/-   ##
============================================
+ Coverage     80.03%   80.08%   +0.04%     
- Complexity     3806     3807       +1     
============================================
  Files           306      306              
  Lines         14402    14386      -16     
  Branches       1904     1904              
============================================
- Hits          11527    11521       -6     
+ Misses         2127     2118       -9     
+ Partials        748      747       -1     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java 9.52% <0.00%> (+1.30%) ⬆️
sentry/src/main/java/io/sentry/NoOpHub.java 46.51% <ø> (+1.05%) ⬆️
...ntry/src/main/java/io/sentry/NoOpSentryClient.java 70.00% <ø> (ø)
...main/java/io/sentry/NoOpSentryExecutorService.java 33.33% <0.00%> (-6.67%) ⬇️
...src/main/java/io/sentry/SentryExecutorService.java 94.73% <0.00%> (-5.27%) ⬇️
sentry/src/main/java/io/sentry/Hub.java 77.99% <100.00%> (+0.05%) ⬆️
sentry/src/main/java/io/sentry/IHub.java 92.00% <100.00%> (+0.33%) ⬆️
sentry/src/main/java/io/sentry/ISentryClient.java 95.45% <100.00%> (+0.21%) ⬆️
...c/main/java/io/sentry/NoOpTransactionProfiler.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/SentryClient.java 86.48% <100.00%> (+0.36%) ⬆️
... and 3 more

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 marked this pull request as ready for review December 19, 2022 10:12
@stefanosiano
Copy link
Member Author

just realized a possible race condition when starting/finishing a transaction. Gonna make it a draft and fix it

@stefanosiano stefanosiano marked this pull request as draft December 19, 2022 17:01
… a time can be profiled

AndroidTransactionProfiler.onTransactionFinish now waits for executor service thread to finish and returns its data instead of finishing profile in another thread
added ISentryExecutorService method Future<T> submit(Callable<T>)
@stefanosiano stefanosiano marked this pull request as ready for review December 20, 2022 17:32
return options
.getExecutorService()
.submit(() -> onTransactionFinish(transaction, false))
.get();
Copy link
Member

Choose a reason for hiding this comment

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

hmm, why do we need to use ExecutorService here if we are anyway awaiting for it in a blocking way?

Copy link
Member Author

@stefanosiano stefanosiano Dec 21, 2022

Choose a reason for hiding this comment

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

to avoid calling onTransactionStart in the executor service thread and onTransactionFinish in a different thread. In case these methods are called very fast there could be a race condition. It's an edge case, tough

@stefanosiano stefanosiano merged commit 5610867 into main Dec 22, 2022
@stefanosiano stefanosiano deleted the android/revert-concurrent-profiling branch December 22, 2022 12:17
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.

4 participants