-
-
Notifications
You must be signed in to change notification settings - Fork 434
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
Fix ensure all options are processed before integrations are loaded #2377
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f809aac | 301.51 ms | 346.60 ms | 45.09 ms |
a04f788 | 321.78 ms | 354.12 ms | 32.35 ms |
38e4f11 | 358.20 ms | 433.73 ms | 75.53 ms |
507f924 | 342.51 ms | 402.65 ms | 60.14 ms |
90e9745 | 314.68 ms | 357.28 ms | 42.60 ms |
7597ded | 289.60 ms | 339.69 ms | 50.09 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
f809aac | 1.73 MiB | 2.32 MiB | 608.63 KiB |
a04f788 | 1.73 MiB | 2.32 MiB | 609.88 KiB |
38e4f11 | 1.73 MiB | 2.32 MiB | 609.82 KiB |
507f924 | 1.73 MiB | 2.32 MiB | 609.95 KiB |
90e9745 | 1.73 MiB | 2.32 MiB | 608.63 KiB |
7597ded | 1.73 MiB | 2.32 MiB | 609.88 KiB |
Previous results on branch: fix/sentry-init-cache-folder
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
629a327 | 339.80 ms | 385.59 ms | 45.79 ms |
42efe66 | 384.22 ms | 440.48 ms | 56.26 ms |
8b13643 | 341.57 ms | 396.72 ms | 55.15 ms |
423b573 | 305.68 ms | 354.96 ms | 49.28 ms |
aba9a75 | 317.06 ms | 367.67 ms | 50.61 ms |
fbefdb1 | 333.34 ms | 369.27 ms | 35.93 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
629a327 | 1.73 MiB | 2.32 MiB | 609.99 KiB |
42efe66 | 1.73 MiB | 2.32 MiB | 609.99 KiB |
8b13643 | 1.73 MiB | 2.32 MiB | 609.99 KiB |
423b573 | 1.73 MiB | 2.33 MiB | 612.79 KiB |
aba9a75 | 1.73 MiB | 2.32 MiB | 609.99 KiB |
fbefdb1 | 1.73 MiB | 2.32 MiB | 609.99 KiB |
Codecov ReportBase: 80.28% // Head: 80.28% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #2377 +/- ##
=========================================
Coverage 80.28% 80.28%
Complexity 3690 3690
=========================================
Files 292 292
Lines 13845 13845
Branches 1833 1833
=========================================
Hits 11116 11116
Misses 2014 2014
Partials 715 715 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. |
sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but two points:
- Can we add a test that tests specifically this problem (manually init and check that the cacheDir of
EnvelopeCache
is the correct one with the dsn hash in the path)? - Can we switch to manual init in the sample app with this PR as well pls? thanks
📜 Description
Right now some integrations are created between default/auto-init and the custom
SentryOptions.configure
callback. If an integration copies references fromSentryOptions
it can hold a stale configuration, as theSentryOptions.configure
block can change any option afterwards.This is a quick fix, I guess in the long run we should take a look into streamlining the overall SDK init process to avoid these kind of issues.
This PR splits up the
AndroidOptionsInitializer.init
method into two parts:loadDefaultAndMetadataOptions()
initializeIntegrationsAndProcessors()
This allows us to execute execute the custom
OptionsConfiguration.configure()
callback in-between, which ensures all options are set before any integrations and processors are created.💡 Motivation and Context
Fixes #2373
💚 How did you test it?
📝 Checklist
🔮 Next steps