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

Collect memory usage in transactions #2445

Merged
merged 5 commits into from
Jan 3, 2023
Merged

Conversation

stefanosiano
Copy link
Member

📜 Description

Added a MemoryCollector, with different implementations for Java and Android, set in SentryOptions
Added TransactionPerformanceCollector, that uses the MemoryCollector, to collect performance data while a transaction is running.
Collected data is a list of objects containing a timestamp, java heap memory usage and native memory usage (only on Android)

💡 Motivation and Context

We want to build a POC to get the memory usage data as a time series while a transaction runs.

💚 How did you test it?

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

…ndroid set in SentryOptions

Added TransactionPerformanceCollector to collect performance data while a transaction is running
@github-actions
Copy link
Contributor

github-actions bot commented Dec 28, 2022

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

Generated by 🚫 dangerJS against 47c6487

public class AndroidMemoryCollector implements IMemoryCollector {
@Override
public MemoryCollectionData collect() {
long now = System.currentTimeMillis();
Copy link
Member Author

Choose a reason for hiding this comment

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

Is System.currentTimeMillis() good enough as timestamp? Or should I go with DateUtils.getCurrentDateTime().getTime()?

Copy link
Member

Choose a reason for hiding this comment

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

DateUtils is using Calendar under the hood which itself seems to be using System.currentTimeMillis(). Thus I'd stick to System.currentTimeMillis().

@github-actions
Copy link
Contributor

github-actions bot commented Dec 28, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 393.70 ms 450.45 ms 56.75 ms
Size 1.73 MiB 2.33 MiB 616.87 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4a9c176 319.77 ms 363.20 ms 43.43 ms
f1150bc 352.62 ms 453.27 ms 100.65 ms
f809aac 301.51 ms 346.60 ms 45.09 ms
4a9c176 336.33 ms 384.73 ms 48.41 ms
5fd37fa 334.02 ms 401.10 ms 67.08 ms
703d523 358.00 ms 369.94 ms 11.94 ms
81a1a6c 328.73 ms 421.28 ms 92.55 ms
a04f788 321.78 ms 354.12 ms 32.35 ms
95647bd 349.76 ms 397.92 ms 48.16 ms
ecf9680 303.12 ms 346.35 ms 43.23 ms

App size

Revision Plain With Sentry Diff
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
f1150bc 1.73 MiB 2.33 MiB 615.66 KiB
f809aac 1.73 MiB 2.32 MiB 608.63 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
5fd37fa 1.73 MiB 2.33 MiB 616.48 KiB
703d523 1.73 MiB 2.33 MiB 613.23 KiB
81a1a6c 1.73 MiB 2.32 MiB 612.47 KiB
a04f788 1.73 MiB 2.32 MiB 609.88 KiB
95647bd 1.73 MiB 2.33 MiB 616.54 KiB
ecf9680 1.73 MiB 2.32 MiB 612.39 KiB

Previous results on branch: feat/memory-usage-collection

Startup times

Revision Plain With Sentry Diff
0cf2bf4 363.81 ms 390.04 ms 26.23 ms
bd06028 320.02 ms 368.52 ms 48.50 ms

App size

Revision Plain With Sentry Diff
0cf2bf4 1.73 MiB 2.33 MiB 616.76 KiB
bd06028 1.73 MiB 2.33 MiB 616.69 KiB

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2022

Codecov Report

Base: 80.09% // Head: 80.15% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (a70ace8) compared to base (b3704c8).
Patch coverage: 93.24% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2445      +/-   ##
============================================
+ Coverage     80.09%   80.15%   +0.06%     
- Complexity     3807     3828      +21     
============================================
  Files           306      310       +4     
  Lines         14390    14461      +71     
  Branches       1904     1913       +9     
============================================
+ Hits          11525    11591      +66     
- Misses         2118     2121       +3     
- Partials        747      749       +2     
Impacted Files Coverage Δ
.../src/main/java/io/sentry/MemoryCollectionData.java 70.00% <70.00%> (ø)
...ava/io/sentry/TransactionPerformanceCollector.java 95.00% <95.00%> (ø)
sentry/src/main/java/io/sentry/Hub.java 78.04% <100.00%> (+0.05%) ⬆️
...y/src/main/java/io/sentry/JavaMemoryCollector.java 100.00% <100.00%> (ø)
...y/src/main/java/io/sentry/NoOpMemoryCollector.java 100.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Sentry.java 55.73% <100.00%> (+0.48%) ⬆️
sentry/src/main/java/io/sentry/SentryOptions.java 80.04% <100.00%> (+0.23%) ⬆️
sentry/src/main/java/io/sentry/SentryTracer.java 90.76% <100.00%> (+0.18%) ⬆️

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 28, 2022 21:18
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice one, looks good! Please have a look at my comments

public class AndroidMemoryCollector implements IMemoryCollector {
@Override
public MemoryCollectionData collect() {
long now = System.currentTimeMillis();
Copy link
Member

Choose a reason for hiding this comment

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

DateUtils is using Calendar under the hood which itself seems to be using System.currentTimeMillis(). Thus I'd stick to System.currentTimeMillis().

added a synchronization mechanism in TransactionPerformanceCollector
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.

3 participants