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.

Sign up for GitHub

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

Jump to bottom

Startup Profiling 3 - Add ContentProvider and start profile #3128

Merged
merged 8 commits into from
Jan 12, 2024
Merged

Startup Profiling 3 - Add ContentProvider and start profile #3128

merged 8 commits into from
Jan 12, 2024

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Jan 4, 2024 •
edited
Loading

📜 Description

Added SentryStartupProfilingProvider, which reads the config file from disk, sample the trace and profile, and starts the profiler. Then it stores the startup profiler and sampling decision in AppStartMetrics:

  • The sampling decision is inherited by the first activity transaction, if available
  • The startup profiler is reused in AndroidOptionsInitializer, instead of creating a new one, if available
    Startup profile is bound to the startup transaction:
  • Added ITransactionProfiler.isRunning to check if profiler is already running
    Added io.sentry.profiling.enable-startup manifest option (forgot to do it in previous PR)
    Moved profilingTracesHz from SentryAndroidOptions to SentryOptions (needed to get access in Sentry.init to save profiling config to disk)

Example profile: we can see the 98ms of our SentryInitProvider 😅

💡 Motivation and Context

This is the last PR to enable startup profiling

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Docs

@stefanosiano
first activity transaction now inherits startup sampling decision, if…
fe75ded 
… available

if a startup profiler was instantiated, it will be reused in AndroidOptionsInitializer, instead of creating a new one
added ITransactionProfiler.isRunning
startup profiler and sampling decision is stored in AppStartMetrics
startup profile is bound to the startup transaction
added io.sentry.profiling.enable-startup manifest option
moved profilingTracesHz from SentryAndroidOptions to SentryOptions
added SentryStartupProfilingProvider
@stefanosiano
updated changelog
186c8df
@stefanosiano stefanosiano marked this pull request as ready for review January 4, 2024 16:58
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 4, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against 693c1da

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 4, 2024 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 410.86 ms 481.52 ms 70.66 ms
Size 1.70 MiB 2.27 MiB 584.06 KiB

Baseline results on branch: feat/startup-profiling2-add-options

Startup times

Revision Plain With Sentry Diff
f19450a 394.36 ms 473.10 ms 78.74 ms
47df090 386.30 ms 467.52 ms 81.22 ms

App size

Revision Plain With Sentry Diff
f19450a 1.70 MiB 2.27 MiB 583.38 KiB
47df090 1.72 MiB 2.27 MiB 561.22 KiB

Previous results on branch: feat/startup-profiling3-start-profiler

Startup times

Revision Plain With Sentry Diff
691b9d3 576.60 ms 706.33 ms 129.73 ms
9a38f08 396.55 ms 478.92 ms 82.37 ms
aa1cf14 420.06 ms 475.65 ms 55.58 ms
42c03a5 375.61 ms 435.12 ms 59.51 ms

App size

Revision Plain With Sentry Diff
691b9d3 1.72 MiB 2.27 MiB 562.86 KiB
9a38f08 1.70 MiB 2.27 MiB 584.03 KiB
aa1cf14 1.70 MiB 2.27 MiB 584.14 KiB
42c03a5 1.72 MiB 2.27 MiB 562.90 KiB

@bruno-garcia bruno-garcia mentioned this pull request Jan 4, 2024
Open
@stefanosiano
Merge branch 'feat/startup-profiling2-add-options' into feat/startup-…
6691f71 
…profiling3-start-profiler

# Conflicts:
#	sentry/src/main/java/io/sentry/Sentry.java
romtsn
romtsn approved these changes Jan 11, 2024
View reviewed changes
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

some comments, but overall looks great, amazing stuff! Let's slim down that SentryInitProvider time

@stefanosiano
moved isStartupTransaction into TransactionOptions, instead of a new …
64660b3 
…method in IHub

added synchronized blocks for startup profiling management
added log for not sampled startup profile
@stefanosiano
merged SentryStartupProfilingProvider into SentryPerformanceProvider
7f690ef
@stefanosiano
Copy link
Member Author

@romtsn Want to have a last look before merging? i should have updated it with all your feedback

@romtsn
Copy link
Member

romtsn commented Jan 12, 2024

@romtsn Want to have a last look before merging? i should have updated it with all your feedback

LGTM now, thanks for addressing!

@stefanosiano stefanosiano merged commit 7d13d65 into feat/startup-profiling2-add-options Jan 12, 2024
13 of 14 checks passed
@stefanosiano stefanosiano deleted the feat/startup-profiling3-start-profiler branch January 12, 2024 16:07
stefanosiano added a commit that referenced this pull request Jan 15, 2024
@stefanosiano
Startup Profiling 2 - Add options and sampling logic (#3121)
02f6aa7 
* added TransactionContext.isForNextStartup
* added SentryOptions.enableStartupProfiling
* added SentryStartupProfilingOptions class with Json ser/deser
* added startupProfilingConfigFile deletion and creation on init
* added sampling decision on SDK init with isForNextStartup flag set to true
* added SentryOptions.getCacheDirPathWithoutDsn for startupProfiling config file

* Startup Profiling 3 - Add ContentProvider and start profile (#3128)
* first activity transaction now inherits startup sampling decision, if available
* if a startup profiler was instantiated, it will be reused in AndroidOptionsInitializer, instead of creating a new one
* added ITransactionProfiler.isRunning
* startup profiler and sampling decision is stored in AppStartMetrics
* startup profile is bound to the startup transaction
* added io.sentry.profiling.enable-startup manifest option
* moved profilingTracesHz from SentryAndroidOptions to SentryOptions
* added startup profiling launch in SentryPerformanceProvider
* added isStartupTransaction to TransactionOptions
stefanosiano added a commit that referenced this pull request Jan 15, 2024
@stefanosiano
Startup Profiling 1 - Decouple Profiler from Transaction (#3101)
5784ed8 
* Startup Profiling 1 - Decouple Profiler from Transaction (#3101)
* removed the need of a transaction to start a profile in AndroidTransactionProfiler, allowing to bind one later
* added a constructor to AndroidTransactionProfiler without options
* added options as param to AndroidTransactionProfiler.onTransactionFinish

* Startup Profiling 2 - Add options and sampling logic (#3121)
* added TransactionContext.isForNextStartup
* added SentryOptions.enableStartupProfiling
* added SentryStartupProfilingOptions class with Json ser/deser
* added startupProfilingConfigFile deletion and creation on init
* added sampling decision on SDK init with isForNextStartup flag set to true
* added SentryOptions.getCacheDirPathWithoutDsn for startupProfiling config file

* Startup Profiling 3 - Add ContentProvider and start profile (#3128)
* first activity transaction now inherits startup sampling decision, if available
* if a startup profiler was instantiated, it will be reused in AndroidOptionsInitializer, instead of creating a new one
* added ITransactionProfiler.isRunning
* startup profiler and sampling decision is stored in AppStartMetrics
* startup profile is bound to the startup transaction
* added io.sentry.profiling.enable-startup manifest option
* moved profilingTracesHz from SentryAndroidOptions to SentryOptions
* added startup profiling launch in SentryPerformanceProvider
* added isStartupTransaction to TransactionOptions
@sdzhong sdzhong mentioned this pull request Jan 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romtsn romtsn romtsn approved these changes

@adinauer adinauer Awaiting requested review from adinauer

@markushi markushi Awaiting requested review from markushi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stefanosiano @romtsn