feat(ndk): expose init return code

[hannojg]

Return int from SentryNdk.init reflecting sentry_init result. This can then be used by sentry-java/android to double check if the Ndk integration could be initialized successfully (which currently isn't happening, we just call the native method and assume it went successful).

Upstream PR in getsentry/sentry-java for android ndk:

• https://github.com/getsentry/sentry-java/pull/4842

[supervacuus]

@markushi, I think we discussed this some time ago, but I can't remember whether return code or Exception was the choice back then.

[hannojg]

Just as info, I made it so that an exception will be thrown by the ndk integration in getsentry/sentry-java if the return code is not 0 for success (as there it seems to be the place where we throw exception if init fails)

[hannojg]

But yeah, interestingly i think these ENSURE_OR_FAIL calls will currently go as silent errors (as it doesn't throw or log or return an error code)

https://github.com/getsentry/sentry-native/blob/9895a5c3ffab4e59e0c8020484cdd5dbc648666d/ndk/lib/src/main/jni/sentry.c#L299-L302

With this change its also sub-optimal, since the developer might turn on debug logging, check their logs but doesn't get any valuable information from it. I am actually wondering why we are doing that checking here, given that sentry_core.c already seems to do the same checking + provide proper error logging:

https://github.com/getsentry/sentry-native/blob/9895a5c3ffab4e59e0c8020484cdd5dbc648666d/src/sentry_core.c#L180-L184

?

// Edit: okay i don't know if sentry__path_create_dir_all would crash with an invalid string, so potentially we'd want to either throw or also log warnings here?

[supervacuus]

I think that was the main reason exceptions were raised back then: it allowed clients to differentiate between a native sentry_init failure and an error from the JNI layer, where the JNI layer could also provide more detailed feedback on which stage failed. What I cannot remember: whether there was a reason we didn't raise exceptions.

[markushi]

@sentry review

[markushi]

@hannojg thanks for opening this up! This is definitely an area of improvement and I think using a return code is the way to go right now. It would also allow some flexibility, e.g. returning some positive integer > 1 when init succeeds but feature XY from options could not be enabled. In the long run we should improve this even further, as we're e.g. not checking for Java Exceptions within the C code (ExceptionOccurred() as outlined in https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/design.html#java_exceptions)

[supervacuus]

Just my 2c here:

• I am fine if we don't throw from the JNI layer, however
• if we create a new protocol, then we should make it clear what things mean, especially the negative range currently does not provide any insight into the cause (we could use an enum to represent the failure states from the JNI layer)
• there is not a lot of overlap between the checks done in the JNI layer and the one in sentry_init, since all the checks here are classic client side checks, since for many option interfaces a NULL can be a valid value, meaning only the client knows whether that is an error (for instance, setting a transport to NULL is a valid configuration from the POV of the native SDK)

[markushi]

@hannojg it's been a while, do you want to address the PR feedback or should I take care of that?

[hannojg]

@hannojg it's been a while, do you want to address the PR feedback or should I take care of that?

Hey, thanks for asking! Would love to contribute but realistically I won't have time for it :( would appreciate it very much if you guys can take care, thanks 🙏

[markushi]

LGTM! But can we hold off on merging this so we can add it to the also-breaking inproc changes from #1446 for a single breaking-bump release (I hope we can do both this week)?

Sure, sounds good to me! I'll keep my "requested change" reviewer status for now then, so it doesn't get merged accidentally.