-
-
Notifications
You must be signed in to change notification settings - Fork 342
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
feat(experimental): Initialize Android SDK from json configuration #4451
base: capture-app-start-errors
Are you sure you want to change the base?
Conversation
|
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af | 439.23 ms | 427.31 ms | -11.92 ms |
b75148e | 440.04 ms | 421.36 ms | -18.68 ms |
555070f | 438.67 ms | 428.30 ms | -10.37 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af | 17.75 MiB | 20.11 MiB | 2.37 MiB |
b75148e | 17.75 MiB | 20.11 MiB | 2.37 MiB |
555070f | 17.75 MiB | 20.11 MiB | 2.37 MiB |
Previous results on branch: antonis/init-from-json
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36893e3 | 416.35 ms | 420.88 ms | 4.53 ms |
1646f54 | 351.27 ms | 335.28 ms | -15.99 ms |
d0ffabe | 497.57 ms | 478.45 ms | -19.12 ms |
69b6731 | 475.62 ms | 477.30 ms | 1.67 ms |
f74989d | 480.30 ms | 505.59 ms | 25.29 ms |
236a90a | 417.78 ms | 432.00 ms | 14.22 ms |
ee5a152 | 455.06 ms | 471.58 ms | 16.52 ms |
4ea1d1e | 473.92 ms | 473.94 ms | 0.02 ms |
66c8b49 | 430.41 ms | 442.98 ms | 12.57 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36893e3 | 17.75 MiB | 20.11 MiB | 2.37 MiB |
1646f54 | 17.75 MiB | 20.11 MiB | 2.37 MiB |
d0ffabe | 17.75 MiB | 20.11 MiB | 2.37 MiB |
69b6731 | 17.75 MiB | 20.11 MiB | 2.37 MiB |
f74989d | 17.75 MiB | 20.11 MiB | 2.37 MiB |
236a90a | 17.75 MiB | 20.11 MiB | 2.37 MiB |
ee5a152 | 17.75 MiB | 20.11 MiB | 2.37 MiB |
4ea1d1e | 17.75 MiB | 20.11 MiB | 2.37 MiB |
66c8b49 | 17.75 MiB | 20.11 MiB | 2.37 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af+dirty | 1209.44 ms | 1217.13 ms | 7.70 ms |
b75148e+dirty | 1221.53 ms | 1220.85 ms | -0.68 ms |
555070f+dirty | 1213.59 ms | 1217.79 ms | 4.20 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
b75148e+dirty | 2.63 MiB | 3.69 MiB | 1.06 MiB |
555070f+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
Previous results on branch: antonis/init-from-json
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69b6731+dirty | 1218.08 ms | 1223.96 ms | 5.88 ms |
ee5a152+dirty | 1233.22 ms | 1227.27 ms | -5.96 ms |
1646f54+dirty | 1237.86 ms | 1238.08 ms | 0.22 ms |
4ea1d1e+dirty | 1231.00 ms | 1231.73 ms | 0.73 ms |
236a90a+dirty | 1222.65 ms | 1221.98 ms | -0.67 ms |
66c8b49+dirty | 1225.84 ms | 1217.93 ms | -7.90 ms |
d0ffabe+dirty | 1210.75 ms | 1212.22 ms | 1.47 ms |
36893e3+dirty | 1211.35 ms | 1212.33 ms | 0.97 ms |
f74989d+dirty | 1220.98 ms | 1219.00 ms | -1.98 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69b6731+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
ee5a152+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
1646f54+dirty | 2.63 MiB | 3.68 MiB | 1.05 MiB |
4ea1d1e+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
236a90a+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
66c8b49+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
d0ffabe+dirty | 2.63 MiB | 3.69 MiB | 1.05 MiB |
36893e3+dirty | 2.63 MiB | 3.68 MiB | 1.05 MiB |
f74989d+dirty | 2.63 MiB | 3.68 MiB | 1.05 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af+dirty | 1213.08 ms | 1223.82 ms | 10.73 ms |
b75148e+dirty | 1202.72 ms | 1212.04 ms | 9.32 ms |
555070f+dirty | 1223.61 ms | 1227.57 ms | 3.96 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
208f4af+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
b75148e+dirty | 3.19 MiB | 4.25 MiB | 1.07 MiB |
555070f+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
Previous results on branch: antonis/init-from-json
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69b6731+dirty | 1221.39 ms | 1219.30 ms | -2.08 ms |
ee5a152+dirty | 1209.06 ms | 1204.49 ms | -4.57 ms |
1646f54+dirty | 1217.25 ms | 1210.30 ms | -6.95 ms |
4ea1d1e+dirty | 1240.67 ms | 1246.71 ms | 6.04 ms |
236a90a+dirty | 1212.67 ms | 1214.40 ms | 1.73 ms |
66c8b49+dirty | 1226.60 ms | 1226.08 ms | -0.52 ms |
d0ffabe+dirty | 1219.33 ms | 1223.08 ms | 3.76 ms |
36893e3+dirty | 1231.54 ms | 1231.40 ms | -0.15 ms |
f74989d+dirty | 1246.20 ms | 1245.12 ms | -1.08 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
69b6731+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
ee5a152+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
1646f54+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
4ea1d1e+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
236a90a+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
66c8b49+dirty | 3.19 MiB | 4.25 MiB | 1.07 MiB |
d0ffabe+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
36893e3+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
f74989d+dirty | 3.19 MiB | 4.25 MiB | 1.06 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b75148e+dirty | 428.91 ms | 461.26 ms | 32.35 ms |
208f4af+dirty | 346.93 ms | 402.77 ms | 55.84 ms |
555070f+dirty | 388.25 ms | 424.44 ms | 36.19 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b75148e+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
208f4af+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
555070f+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
Previous results on branch: antonis/init-from-json
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36893e3+dirty | 380.00 ms | 431.02 ms | 51.02 ms |
236a90a+dirty | 384.20 ms | 419.90 ms | 35.69 ms |
1646f54+dirty | 399.30 ms | 453.04 ms | 53.74 ms |
4ea1d1e+dirty | 384.08 ms | 403.66 ms | 19.58 ms |
d0ffabe+dirty | 395.28 ms | 436.64 ms | 41.36 ms |
ee5a152+dirty | 393.11 ms | 458.70 ms | 65.59 ms |
69b6731+dirty | 363.50 ms | 409.44 ms | 45.94 ms |
f74989d+dirty | 265.81 ms | 320.56 ms | 54.75 ms |
66c8b49+dirty | 384.27 ms | 429.34 ms | 45.07 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
36893e3+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
236a90a+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
1646f54+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
4ea1d1e+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
d0ffabe+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
ee5a152+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
69b6731+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
f74989d+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
66c8b49+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
public static void startWithOptions( | ||
@NotNull final Context context, @NotNull final Map<String, Object> options) { |
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.
Let's match the SentryAndroid
init signature, so the RN manual init can be a drop in replacement.
This public interface should allow user to overwrite/extend the options specified in the sentry.options.json
file using the configuration handler.
I would avoid exposing the untyped options map to users as it might be error prone and hard to use. Ofc same is the json file, but there we can't control it besides introducing a json scheme.
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.
This public interface should allow user to overwrite/extend the options specified in the sentry.options.json file using the configuration handler.
I think this is a great idea and makes the exposed api really powerful allowing for dynamic configurations that a json file cannot provide. Updated with 25d142a
* | ||
* @param context Android Context | ||
*/ | ||
public static void start(@NotNull final Context context) { |
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.
I'm thinking about using init here to match the SentryAndroid
. Cocoa uses start
because init
is reserved for constructors in Obj-C.
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.
Updated with 7788b81
packages/core/android/src/main/java/io/sentry/react/RNSentryMapConverter.java
Outdated
Show resolved
Hide resolved
// We are not directly using `convertToWritable` since `Arguments.createArray()` | ||
// fails before bridge initialisation |
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.
Can you share more details? I didn't know that, is there an issue that we could link? Or is that expected behaviour?
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.
The implementation would have been simpler if we could replace the mapToReadableMap(map)
above with (WritableMap) convertToWritable(map)
but this results in an ExceptionInInitializerError
when called from the Android Application onCreate
method before RN initialises. I think the root of the issue is similar with this old RN issue or this one.
Full stack trace
java.lang.ExceptionInInitializerError
at com.facebook.react.bridge.Arguments.createMap(Arguments.java:163)
at io.sentry.react.RNSentryMapConverter.convertToWritable(RNSentryMapConverter.java:39)
at io.sentry.react.RNSentryMapConverter.jsonObjectToReadableMap(RNSentryMapConverter.java:142)
at io.sentry.react.RNSentrySDK.start(RNSentrySDK.java:50)
at io.sentry.reactnative.sample.MainApplication.initializeSentry(MainApplication.kt:59)
at io.sentry.reactnative.sample.MainApplication.onCreate(MainApplication.kt:42)
at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1388)
at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7586)
at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2449)
at android.os.Handler.dispatchMessage(Handler.java:109)
at android.os.Looper.loopOnce(Looper.java:232)
at android.os.Looper.loop(Looper.java:317)
at android.app.ActivityThread.main(ActivityThread.java:8787)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:591)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:871)
Caused by: java.lang.IllegalStateException: SoLoader.init() not yet called
at com.facebook.soloader.SoLoader.assertInitialized(SoLoader.java:1273)
at com.facebook.soloader.SoLoader.loadLibraryOnNonAndroid(SoLoader.java:880)
at com.facebook.soloader.SoLoader.loadLibrary(SoLoader.java:825)
at com.facebook.soloader.SoLoader.loadLibrary(SoLoader.java:812)
at com.facebook.react.bridge.ReactBridge.staticInit(ReactBridge.java:34)
at com.facebook.react.bridge.NativeMap.<clinit>(NativeMap.kt:20)
at com.facebook.react.bridge.Arguments.createMap(Arguments.java:163)
at io.sentry.react.RNSentryMapConverter.convertToWritable(RNSentryMapConverter.java:39)
at io.sentry.react.RNSentryMapConverter.jsonObjectToReadableMap(RNSentryMapConverter.java:142)
at io.sentry.react.RNSentrySDK.start(RNSentrySDK.java:50)
at io.sentry.reactnative.sample.MainApplication.initializeSentry(MainApplication.kt:59)
at io.sentry.reactnative.sample.MainApplication.onCreate(MainApplication.kt:42)
at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1388)
at android.app.ActivityThread.handleBindApplication(ActivityThread.java:7586)
at android.app.ActivityThread.-$$Nest$mhandleBindApplication(Unknown Source:0)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2449)
at android.os.Handler.dispatchMessage(Handler.java:109)
at android.os.Looper.loopOnce(Looper.java:232)
at android.os.Looper.loop(Looper.java:317)
at android.app.ActivityThread.main(ActivityThread.java:8787)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:591)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:871)
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.
Iterating on this calling initializeSentry()
after SoLoader.init
fixes the above issue.
Lines 41 to 50 in 2dc8f22
override fun onCreate() { | |
super.onCreate() | |
// When the native init is enabled the `autoInitializeNativeSdk` | |
// in JS has to be set to `false` | |
// this.initializeSentry() | |
SoLoader.init(this, OpenSourceMergedSoMapping) | |
if (BuildConfig.IS_NEW_ARCHITECTURE_ENABLED) { | |
// If you opted-in for the New Architecture, we load the native entry point for this app. | |
load() | |
} |
We can probably drop the custom implementation in favour of reusing our existing convertToWritable
documenting the requirement/limitation 🤔 Wdyt @krystofwoldrich ?
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.
We can't do that, because we need to be able to initialize before any RN code. To capture any potential errors/crashes.
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.
because we need to be able to initialize before any RN code
Makes sense. I'll stick with the new mapToReadableMap
implementation that does not depend on RN.
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.
But I think we can refactor convertToWritable
to be more reusable.
We could create convertToNativeWritable
and convertToJavaWritable
.
Which internally use adjusted convertToWritable
which depending on the need uses Arguments.createArray()
or new JavaOnlyArray
.
Arguments.createArray()
and new JavaOnlyArray
implement the same interface, but the createArray
instantiates the cpp implemented array which can be passed over the bridge to JS. The JavaOnly*
classes can't be passed to JS.
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.
Sounds like a good idea 🙇 I'll pursue this direction and iterate back.
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.
Since this change involved many modifications, I spun it off into a separate PR #4469
@Test | ||
fun testJsonObjectToReadableMap() { | ||
val json = | ||
JSONObject().apply { | ||
put("stringKey", "stringValue") | ||
put("booleanKey", true) | ||
put("intKey", 123) | ||
} | ||
|
||
val result = RNSentryMapConverter.jsonObjectToReadableMap(json) | ||
|
||
assertNotNull(result) | ||
assertTrue(result is JavaOnlyMap) | ||
assertEquals("stringValue", result.getString("stringKey")) | ||
assertEquals(true, result.getBoolean("booleanKey")) | ||
assertEquals(123, result.getInt("intKey")) | ||
} | ||
|
||
@Test | ||
fun testMapToReadableMap() { | ||
val map = | ||
mapOf( | ||
"stringKey" to "stringValue", | ||
"booleanKey" to true, | ||
"intKey" to 123, | ||
) | ||
|
||
val result = RNSentryMapConverter.mapToReadableMap(map) | ||
|
||
assertNotNull(result) | ||
assertTrue(result is JavaOnlyMap) | ||
assertEquals("stringValue", result.getString("stringKey")) | ||
assertEquals(true, result.getBoolean("booleanKey")) | ||
assertEquals(123, result.getInt("intKey")) | ||
} |
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.
We should also ensure nested maps and arrays are handled because some Sentry init options require those types.
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.
Makes sense 👍
The nested maps/arrays should be handled with the #4469 enhancement/refactoring discussed above #4451 (comment)
I'll also extend the json handling to support nested structures with #4470
JSONObject jsonObject = getOptionsFromConfigurationFile(context); | ||
ReadableMap rnOptions = RNSentryMapConverter.jsonObjectToReadableMap(jsonObject); |
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.
Our current lint doesn't enforce it, but let's use the @NotNull/@Nullable
annotations here.
Let's add tests to ensure we gracefully continue if the file is not present or contains broken JSON and the init with configure callback is used.
If the start()
is used I agree it make sense to fail as there is no other way to add the options.
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.
Good point 👍
I've handled this in #4474
# Conflicts: # packages/core/android/src/main/java/io/sentry/react/RNSentryStart.java
Taking a note to recheck the following separately from this PR: |
// Only options set here will apply to the Android SDK | ||
// Options from JS are not passed to the Android SDK when initialized manually | ||
RNSentrySDK.init(this) { options -> | ||
// Options set here will apply to the Android SDK overriding the ones from `sentry.options.json` | ||
options.dsn = "https://[email protected]/5428561" | ||
options.isDebug = true | ||
|
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.
The beforeSend
callback should not be needed anymore, the RNSentrySDK.init
should handle the RN defaults internally.
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.
Removed with 7522fce
androidAssetsDir.mkdirs() | ||
} | ||
|
||
copy { |
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.
Just checking does this overwrite the file, if it already exists in the assets?
It should, to ensure we always bundle the up to date config.
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.
Yep, it always replaces any existing file with that name in the assets. Note that after the build the file is deleted from assets.
OptionsConfiguration<SentryAndroidOptions> { options -> | ||
options.dsn = "https://[email protected]" | ||
options.isDebug = false | ||
options.release = "some-release" |
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.
I think options.environment = "base"
is missing here.
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.
Yep, I could add this. I was mainly aiming for an option that does not exist in baseConfig so I could test three cases:
- overridden values existing in both base and overriding config
- values present only in base that make it to the final config
- values present only in the overriding config that make it to the final config
Thinking it again probably the above could have been separate test case
# Conflicts: # CHANGELOG.md
Hey @krystofwoldrich 👋 |
📢 Type of change
Depends on #4445 and #4479
📜 Description
Adds the following methods that allow initialisation of Sentry Android in RN applications:
RNSentrySDK.init(Context context)
: Initialises using thesentry.options.json
file located in the root of the RN project.RNSentrySDK.init(Context context, Sentry.OptionsConfiguration<SentryAndroidOptions> configuration)
: Initialises with the passedconfiguration
and thesentry.options.json
if it exists.💡 Motivation and Context
Part of #3608
💚 How did you test it?
CI, Manual testing
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps