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:

• feat(ndk): handle native init failure code sentry-java#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)

sentry-native/ndk/lib/src/main/jni/sentry.c

Lines 299 to 302 in 9895a5c

	ENSURE_OR_FAIL(outbox_path);
	transport = sentry_transport_new(send_envelope);
	ENSURE_OR_FAIL(transport);

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:

sentry-native/src/sentry_core.c

Lines 180 to 184 in 9895a5c

	if (sentry__path_create_dir_all(options->database_path)) {
	SENTRY_WARN("failed to create database directory or there is no write "
	"access to this directory");
	goto fail;
	}

?

// 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)

[markushi]

Thanks again you for contributing, much appreciated! I left a comment on how far the return code should be propagated up. If something is unclear, just let me know.

[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 🙏

[supervacuus]

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)?

[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.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.46%. Comparing base (67a9f8e) to head (e52ffb9).
⚠️ Report is 54 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1430       +/-   ##
===========================================
- Coverage   84.66%   66.46%   -18.20%     
===========================================
  Files          58       64        +6     
  Lines        7960    11187     +3227     
  Branches     1556     1897      +341     
===========================================
+ Hits         6739     7435      +696     
- Misses       1058     3524     +2466     
- Partials      163      228       +65     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.