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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concurrent profiling 1 - added envelope payload data format #2216

Merged
merged 6 commits into from
Sep 27, 2022

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Aug 11, 2022

馃摐 Description

We want to support concurrent transactions. In order to do it, we are going to add the list of transactions occurred during a profile.
This pr just adds the data format of the transaction list that will be sent in the envelope payload, inserting the profiled transaction in it.
This is the first part of concurrent profiling support. Next part is this pr

馃挕 Motivation and Context

We want to avoid situations where the user cannot profile his transaction due to automatic transactions occurring at the same time, as pointed in this issue
The strategy we will follow is described here

馃挌 How did you test it?

Updated unit test to check the new format
Updated the ui test to check the current profiled transaction is added to the transaction list

馃摑 Checklist

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

馃敭 Next steps

Another pr will contain the logic to add other transactions occurring during profiling

@codecov-commenter
Copy link

Codecov Report

Merging #2216 (4abe8f5) into main (7dd32c0) will decrease coverage by 0.20%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #2216      +/-   ##
============================================
- Coverage     80.65%   80.45%   -0.21%     
- Complexity     3355     3366      +11     
============================================
  Files           240      241       +1     
  Lines         12324    12416      +92     
  Branches       1633     1652      +19     
============================================
+ Hits           9940     9989      +49     
- Misses         1778     1804      +26     
- Partials        606      623      +17     
Impacted Files Coverage 螖
.../main/java/io/sentry/ProfilingTransactionData.java 48.80% <48.80%> (酶)
...ry/src/main/java/io/sentry/ProfilingTraceData.java 77.23% <62.50%> (+0.84%) 猬嗭笍

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

@stefanosiano stefanosiano changed the title added list of ProfilingTransactionData to the profiling envelope payload added list of ProfilingTransactionData to the profiling envelope payload data format Aug 12, 2022
@stefanosiano stefanosiano changed the title added list of ProfilingTransactionData to the profiling envelope payload data format added concurrent profiling envelope payload data format Aug 12, 2022
@stefanosiano stefanosiano changed the title added concurrent profiling envelope payload data format Concurrent profiling 1 - added envelope payload data format Aug 12, 2022
@stefanosiano stefanosiano marked this pull request as ready for review August 19, 2022 13:16
@philipphofmann
Copy link
Member

This PR has been sitting here for a while. @stefanosiano, how can we move this forward? Do you still need a review?

@stefanosiano
Copy link
Member Author

This PR has been sitting here for a while. @stefanosiano, how can we move this forward? Do you still need a review?

In order to merge it i'm waiting for relay to be updated.
However, yeah, It'd need a review, as the pr is finished. Same with this other
I'll ping someone

* added detection and recording of transactions occurred in a profile
* added unit/ui tests
* added CollectionUtils.map method
@stefanosiano
Copy link
Member Author

I'm going to wait for relay pr to be merged before merging this into master

)

* add cpu start/end timestamps to transactions in profile envelope payloads
* added ui test to check times are different
* sorted transaction list in ui test for better checks
* added profile truncation reasons, "normal" by default
* added uitest to check default profile truncation reason is "normal"
* added unit test for truncation reason and updated another to remove a testOnly useless method
@github-actions
Copy link
Contributor

github-actions bot commented Sep 19, 2022

Performance metrics 馃殌

Plain With Sentry Diff
Startup time 298.57 ms 334.35 ms 35.78 ms
Size 1.74 MiB 2.33 MiB 607.50 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3d89dea 345.59 ms 364.06 ms 18.47 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
3d89dea 322.38 ms 350.82 ms 28.45 ms
2f079a1 296.91 ms 337.43 ms 40.51 ms

App size

Revision Plain With Sentry Diff
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
2f079a1 1.74 MiB 2.33 MiB 605.56 KiB

@github-actions
Copy link
Contributor

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

Generated by 馃毇 dangerJS against 830669b

@stefanosiano stefanosiano merged commit 743134a into main Sep 27, 2022
@stefanosiano stefanosiano deleted the feat/concurrent-profiling-data branch September 27, 2022 11:23
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