Profile envelopes sent directly from profiler by stefanosiano · Pull Request #2298 · getsentry/sentry-java

stefanosiano · 2022-10-14T09:09:46Z

📜 Description

The AndroidTransactionProfiler now directly sends profile envelopes. This means we don't have to pass the ProfileTraceData anymore anyway.
So we could drop the old timed out profiles handling, as they are directly sent when they time out

💡 Motivation and Context

This change allows profiles to be sent independently from transactions.
It also fixes and simplify timed out profiles handling. Previously we had to wait for a transaction to finish to attach the timed out profile to it and send the envelope. Now the envelope is sent as soon as the profile times out.

💚 How did you test it?

I added a ui test to check the profile is sent on timeout without finishing the transaction
I added few unit tests to check the captureProfile method of the hub is called by the profiler itself.

📝 Checklist

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

🔮 Next steps

Dropped old timed out profiles handling. Now they are directly sent when they time out AndroidTransactionProfiler does not return the ProfileTraceData anymore Added ui test for timed out profiles

github-actions · 2022-10-14T09:11:01Z

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

Generated by 🚫 dangerJS against 00571fc

stefanosiano · 2022-10-14T09:15:16Z

Do I need a changelog entry? The ITransactionProfiler class is internal, and so should be all the changes in this pr

github-actions · 2022-10-14T09:24:21Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	312.63 ms	349.02 ms	36.39 ms
Size	1.73 MiB	2.32 MiB	607.11 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`3d89dea`	322.38 ms	350.82 ms	28.45 ms
`3d89dea`	345.59 ms	364.06 ms	18.47 ms
`7300956`	324.20 ms	353.79 ms	29.58 ms
`221a5df`	350.61 ms	391.58 ms	40.97 ms
`4dd88fe`	306.88 ms	391.58 ms	84.70 ms
`2f079a1`	296.91 ms	337.43 ms	40.51 ms
`2c5f172`	289.18 ms	307.56 ms	18.38 ms
`1e4690d`	354.69 ms	387.88 ms	33.19 ms
`2c5f172`	310.20 ms	357.16 ms	46.96 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
`3d89dea`	1.74 MiB	2.33 MiB	604.92 KiB
`7300956`	1.73 MiB	2.29 MiB	578.69 KiB
`221a5df`	1.73 MiB	2.29 MiB	581.39 KiB
`4dd88fe`	1.73 MiB	2.29 MiB	579.50 KiB
`2f079a1`	1.74 MiB	2.33 MiB	605.56 KiB
`2c5f172`	1.73 MiB	2.29 MiB	580.10 KiB
`1e4690d`	1.74 MiB	2.33 MiB	604.92 KiB
`2c5f172`	1.73 MiB	2.29 MiB	580.10 KiB
`4dd88fe`	1.73 MiB	2.29 MiB	579.50 KiB

Previous results on branch: feat/android-profiler-sends-envelopes

Startup times

Revision	Plain	With Sentry	Diff
`192a9f0`	270.92 ms	313.15 ms	42.23 ms
`f0a7fca`	338.82 ms	383.54 ms	44.72 ms
`cd0ffed`	344.89 ms	392.55 ms	47.66 ms
`c6d0735`	359.24 ms	397.53 ms	38.29 ms
`3f263a8`	310.81 ms	361.34 ms	50.53 ms
`adfb80f`	329.92 ms	390.77 ms	60.85 ms

App size

Revision	Plain	With Sentry	Diff
`192a9f0`	1.73 MiB	2.29 MiB	580.27 KiB
`f0a7fca`	1.73 MiB	2.29 MiB	580.27 KiB
`cd0ffed`	1.73 MiB	2.29 MiB	580.20 KiB
`c6d0735`	1.73 MiB	2.29 MiB	581.16 KiB
`3f263a8`	1.73 MiB	2.29 MiB	580.27 KiB
`adfb80f`	1.73 MiB	2.29 MiB	580.27 KiB

codecov-commenter · 2022-10-14T09:30:32Z

Codecov Report

Base: 80.11% // Head: 80.08% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (3adb5db) compared to base (afa17ae).
Patch coverage: 56.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2298      +/-   ##
============================================
- Coverage     80.11%   80.08%   -0.03%     
+ Complexity     3433     3429       -4     
============================================
  Files           242      242              
  Lines         12765    12752      -13     
  Branches       1703     1700       -3     
============================================
- Hits          10227    10213      -14     
- Misses         1889     1890       +1     
  Partials        649      649

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/Hub.java	`75.81% <ø> (+0.27%)`	⬆️
sentry/src/main/java/io/sentry/HubAdapter.java	`8.21% <0.00%> (-0.88%)`	⬇️
sentry/src/main/java/io/sentry/IHub.java	`91.66% <ø> (ø)`
sentry/src/main/java/io/sentry/ISentryClient.java	`95.23% <ø> (ø)`
sentry/src/main/java/io/sentry/NoOpHub.java	`45.45% <ø> (+1.01%)`	⬆️
...ntry/src/main/java/io/sentry/NoOpSentryClient.java	`70.00% <ø> (+6.36%)`	⬆️
sentry/src/main/java/io/sentry/SentryClient.java	`85.34% <77.77%> (-0.92%)`	⬇️
...c/main/java/io/sentry/NoOpTransactionProfiler.java	`100.00% <100.00%> (ø)`
sentry/src/main/java/io/sentry/SentryEnvelope.java	`100.00% <100.00%> (ø)`
sentry/src/main/java/io/sentry/SentryTracer.java	`90.37% <100.00%> (-0.12%)`	⬇️
... 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.

markushi · 2022-10-17T06:22:52Z

Do I need a changelog entry? The ITransactionProfiler class is internal, and so should be all the changes in this pr

Good question! @romtsn how do we usually deal with internal changes?

markushi · 2022-10-17T06:23:10Z

Looks good to me! 👍

romtsn · 2022-10-17T09:22:31Z

I guess the changelog is not only for the users but also for us, right? So I'd probably add an entry there. Also, this sounds like a behavior change (profiles are sent without transactions now), which makes it reasonable to add to the changelog again.

romtsn · 2022-10-17T10:04:01Z

+                ? ProfilingTraceData.TRUNCATION_REASON_TIMEOUT
+                : ProfilingTraceData.TRUNCATION_REASON_NORMAL);
+
+    HubAdapter.getInstance().captureProfile(profilingTraceData);


How about we move this into the constructor? Then we should be also able to test it properly without overriding Sentry.setCurrentHub

Perhaps, we could even make AndroidTransactionProfiler an integration, but I'm not sure if it was ever discussed before @marandaneto

Maybe create an envelope out of the profilingTraceData and call captureEnvelope instead?
Your call here @romtsn
I don't recall any discussion related to AndroidTransactionProfiler as an integration.

Out of curiosity, why is it preferred to create an Envelope and call captureEnvelope instead of calling captureProfile/captureTransaction/captureWhatever?
Is it for consistency or are there other reasons? @marandaneto
I can quickly create the envelope and drop the captureProfile methods, if it's better

Because this expands the public API and is not needed.

Does this mean we want to delay the 6.6.0 release until we removed captureProfile from the public API again in main?

Ah it's marked internal anyways, nevermind.

@adinauer Actually, HubAdapter is public, and it has a captureProfile method

HubAdapter is just to make Sentry static class testable.

That's fine, but HubAdapter isn't marked as internal, so technically there's nothing preventing users to actually use it.
Just wanted to make sure we are aware of this

@marandaneto i made the change. Would you take another look before merging?

adinauer

LGTM, agree with roman on the changelog. I too think it should be listed.

removed captureProfile methods profiles are sent creating an envelope and using captureEnvelope added SentryEnvelope.from(... ProfilingTraceData ...)

romtsn

@stefanosiano I'm planning to cut a release today, would you like to merge it so it gets in?

stefanosiano added 2 commits October 14, 2022 11:03

AndroidTransactionProfiler now directly sends profile envelopes

c333c48

Dropped old timed out profiles handling. Now they are directly sent when they time out AndroidTransactionProfiler does not return the ProfileTraceData anymore Added ui test for timed out profiles

Merge branch 'main' into feat/android-profiler-sends-envelopes

1287e69

stefanosiano marked this pull request as ready for review October 14, 2022 09:10

stefanosiano requested review from adinauer, markushi and romtsn as code owners October 14, 2022 09:10

stefanosiano marked this pull request as draft October 14, 2022 09:12

stefanosiano marked this pull request as ready for review October 14, 2022 09:15

stefanosiano marked this pull request as draft October 14, 2022 11:14

stefanosiano added 2 commits October 14, 2022 13:18

Fixed ui tests not clearing unasserted requests after running test

0ebc5ae

Fixed dogfood ui test sending data after some time to mock relay

103e4a9

stefanosiano marked this pull request as ready for review October 14, 2022 16:09

Merge branch 'main' into feat/android-profiler-sends-envelopes

4a7b8ef

romtsn reviewed Oct 17, 2022

View reviewed changes

Comment thread sentry/src/main/java/io/sentry/SentryTracer.java

romtsn reviewed Oct 17, 2022

View reviewed changes

adinauer approved these changes Oct 17, 2022

View reviewed changes

stefanosiano added 3 commits October 17, 2022 20:31

added hub parameter to AndroidTransactionProfiler ctor

e858a65

removed captureProfile methods profiles are sent creating an envelope and using captureEnvelope added SentryEnvelope.from(... ProfilingTraceData ...)

Merge branch 'main' into feat/android-profiler-sends-envelopes

3adb5db

Merge branch 'main' into feat/android-profiler-sends-envelopes

314686f

romtsn approved these changes Oct 19, 2022

View reviewed changes

Merge branch 'main' into feat/android-profiler-sends-envelopes

00571fc

stefanosiano merged commit f50fb16 into main Oct 19, 2022

stefanosiano deleted the feat/android-profiler-sends-envelopes branch October 19, 2022 11:15

gslevinkov mentioned this pull request Oct 27, 2022

NO-TICKET - update Sentry version to 6.6.0 p2p-org/key-app-android#924

Merged

Uh oh!

Uh oh!

Conversation

stefanosiano commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanosiano commented Oct 14, 2022

Uh oh!

github-actions Bot commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/android-profiler-sends-envelopes

Startup times

App size

Uh oh!

codecov-commenter commented Oct 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markushi commented Oct 17, 2022

Uh oh!

markushi commented Oct 17, 2022

Uh oh!

romtsn commented Oct 17, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

romtsn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stefanosiano commented Oct 14, 2022 •

edited

Loading

github-actions Bot commented Oct 14, 2022 •

edited

Loading

github-actions Bot commented Oct 14, 2022 •

edited

Loading

codecov-commenter commented Oct 14, 2022 •

edited

Loading

adinauer Oct 17, 2022 •

edited

Loading