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

Add captureProfile method to hub and client #2290

Merged
merged 12 commits into from Oct 13, 2022
Merged

Conversation

stefanosiano
Copy link
Member

📜 Description

I added the method captureProfile to IHub and ISentryClient, so the profiles can be sent in separate envelopes instead of being attached to transactions.
Since the methods that send transaction and profile together were already exposed e.g. in HubAdapter or SentryClient, they were deprecated to avoid breaking changes.

💡 Motivation and Context

We want to separate Profiling and Performance.
The first step to do so is to send separate envelopes for them.
This will also allows to send timed out profiles without waiting for a transaction to be attached to (more on this on another pr)

💚 How did you test it?

I added few unit tests for the new methods and added one to check the deprecated method calls the new ones.

📝 Checklist

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

🔮 Next steps

The profiler will send the profile envelope by itself, instead of relying on SentryTracer, but will be done in another pr

…lient

removed internal captureTransaction(..., ProfilingTraceData) methods from hub and client
deprecated public captureTransaction(..., ProfilingTraceData) methods in hub and client
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

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

Generated by 🚫 dangerJS against cd1f302

@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 332.65 ms 380.96 ms 48.31 ms
Size 1.73 MiB 2.29 MiB 580.01 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3d89dea 322.38 ms 350.82 ms 28.45 ms
7300956 324.20 ms 353.79 ms 29.58 ms
4ca1d7b 331.33 ms 335.78 ms 4.45 ms
4dd88fe 306.88 ms 391.58 ms 84.70 ms
7300956 337.57 ms 384.21 ms 46.64 ms
54cebc8 331.12 ms 385.14 ms 54.02 ms
c5ccd8a 329.98 ms 365.52 ms 35.54 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
d4087ee 278.00 ms 313.86 ms 35.86 ms
4dd88fe 302.12 ms 331.17 ms 29.04 ms

App size

Revision Plain With Sentry Diff
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
4ca1d7b 1.73 MiB 2.29 MiB 579.88 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
54cebc8 1.73 MiB 2.29 MiB 579.43 KiB
c5ccd8a 1.74 MiB 2.33 MiB 607.44 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
d4087ee 1.73 MiB 2.29 MiB 579.50 KiB
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB

Previous results on branch: feat/hub-capture-profile

Startup times

Revision Plain With Sentry Diff
485764c 292.65 ms 384.36 ms 91.71 ms
3fe771f 313.41 ms 356.24 ms 42.83 ms
77ab44a 295.33 ms 331.33 ms 36.01 ms
56b8edf 329.37 ms 369.98 ms 40.61 ms
abd3232 364.24 ms 375.19 ms 10.94 ms

App size

Revision Plain With Sentry Diff
485764c 1.73 MiB 2.29 MiB 580.01 KiB
3fe771f 1.73 MiB 2.29 MiB 580.00 KiB
77ab44a 1.73 MiB 2.29 MiB 580.00 KiB
56b8edf 1.73 MiB 2.29 MiB 580.00 KiB
abd3232 1.73 MiB 2.29 MiB 580.01 KiB

@stefanosiano stefanosiano marked this pull request as ready for review October 11, 2022 16:29
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Just some comments about the tests, apart from that LGTM

sentry/src/test/java/io/sentry/SentryClientTest.kt Outdated Show resolved Hide resolved
sentry/src/test/java/io/sentry/HubTest.kt Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2022

Codecov Report

Base: 80.11% // Head: 79.96% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (711db82) compared to base (2be62a1).
Patch coverage: 59.57% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2290      +/-   ##
============================================
- Coverage     80.11%   79.96%   -0.16%     
+ Complexity     3426     3425       -1     
============================================
  Files           242      242              
  Lines         12696    12734      +38     
  Branches       1698     1702       +4     
============================================
+ Hits          10172    10183      +11     
- Misses         1881     1901      +20     
- Partials        643      650       +7     
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java 9.09% <0.00%> (-0.44%) ⬇️
sentry/src/main/java/io/sentry/IHub.java 87.50% <ø> (-0.50%) ⬇️
...ntry/src/main/java/io/sentry/NoOpSentryClient.java 63.63% <0.00%> (-6.37%) ⬇️
sentry/src/main/java/io/sentry/NoOpHub.java 44.44% <50.00%> (-2.07%) ⬇️
sentry/src/main/java/io/sentry/SentryClient.java 86.25% <61.11%> (-1.25%) ⬇️
sentry/src/main/java/io/sentry/Hub.java 74.45% <70.58%> (-0.30%) ⬇️
sentry/src/main/java/io/sentry/ISentryClient.java 95.23% <100.00%> (-0.22%) ⬇️
sentry/src/main/java/io/sentry/SentryTracer.java 90.49% <100.00%> (+0.07%) ⬆️
...a/io/sentry/clientreport/ClientReportRecorder.java 64.86% <0.00%> (-9.46%) ⬇️
... and 2 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 merged commit 7d87f22 into main Oct 13, 2022
@stefanosiano stefanosiano deleted the feat/hub-capture-profile branch October 13, 2022 15:59
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