sync Java to C #937

bruno-garcia · 2020-09-22T01:17:59Z

PoC Java -> native scope sync

TODO:

Write tests
Measure perf
Add the other scope data

So far:

Crumbs:

user:

Tags:

Swatinem

I bet this is only the beginning and you will add a lot more hooks?

sentry-android-ndk/src/main/jni/sentry.c

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java

sentry-android-ndk/src/main/jni/sentry.c

sentry-samples/sentry-samples-android/src/main/java/io/sentry/sample/MainActivity.java

...amples/sentry-samples-servlet/src/main/java/io/sentry/samples/servlet/SentryInitializer.java

marandaneto · 2020-09-22T07:59:47Z

sentry/src/main/java/io/sentry/IScopeObserver.java

+
+import io.sentry.protocol.User;
+
+public interface IScopeObserver {


M: we'd need to implement the other fields too, like extra, fingerprint... and also to remove them, like removeTag...

The customer only asked about user,tag and breadcrumbs but yeah, for completion we'd have to add a bunch more stuff

or to document pretty well what we sync and what we don't, I'm fine either way.

Could be an option. Specially if it'll require lots of JNI calls to sync stuff.

tags start with the min. necessary to unblock folks and see how it goes, we can implement the others later then.
starting with tags, extras, user, and breadcrumbs.

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java

…eObserver.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>

sentry-android-ndk/src/main/jni/sentry.c

Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>

...amples/sentry-samples-servlet/src/main/java/io/sentry/samples/servlet/SentryInitializer.java

sentry-android-ndk/src/main/jni/sentry.c

marandaneto · 2020-09-23T12:35:26Z

sentry-android-ndk/src/main/jni/sentry.c

+    }
+
+    // TODO: make this a method
+    if (data) {


@Swatinem do you know an easier way to do this? from Map<String, Object> on java to sentry-native?

damn, I have no idea. But if you put this into a method, you can at least goto fail and have a few finalizers (those DeleteLocalRef) there. That should at least take care of the nesting.

Those jclass/jmethodID things can probably be done someplace else and cached inside a struct. I hope the pointers returned from there are stable across GC?

I can imagine something like:

struct { jclass map; jmethodID map_entrySet; jclass set; jmethodID set_iterator; … }

Also, why do you need entrySet, can’t you get an iterator from Map directly?

for the last question, no, https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#entrySet--

the suggestions above really make sense, I will experiment with just parsing the map as a String, let's see how it goes, last case I'd do this way, thanks

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java

…eObserver.java

Swatinem · 2020-09-23T19:11:18Z

sentry-android-ndk/src/main/jni/sentry.c

+                                                    }
+                                                    const char* valueStr = (*env)->GetStringUTFChars(env, value, NULL);
+                                                    if (!valueStr) {  // Out of memory
+                                                        (*env)->ReleaseStringUTFChars(env, key, keyStr);


We never called this for any of the other cases. should we?

it'd not be a bad idea, but those values are set by us and quite small Strs length, here could be arbitrary data set by customers like a full JSON response, that's why it's so important, good catch.

sentry-android-ndk/src/main/jni/sentry.c

Swatinem · 2020-09-23T19:19:39Z

sentry-android-ndk/src/main/jni/sentry.c

+    }
+
+    // TODO: make this a method
+    if (data) {


damn, I have no idea. But if you put this into a method, you can at least goto fail and have a few finalizers (those DeleteLocalRef) there. That should at least take care of the nesting.

Those jclass/jmethodID things can probably be done someplace else and cached inside a struct. I hope the pointers returned from there are stable across GC?

I can imagine something like:

struct { jclass map; jmethodID map_entrySet; jclass set; jmethodID set_iterator; … }

Also, why do you need entrySet, can’t you get an iterator from Map directly?

…ntry-java into feat/java-native-scope-sync

marandaneto · 2020-09-25T13:19:28Z

sentry-android-ndk/src/main/jni/sentry.c

@@ -11,8 +11,257 @@ struct transport_options {

 struct transport_options g_transport_options;

+JNIEXPORT void JNICALL


we don't need to check NPEs here, because on Java the thread-safe list dont accept null keys and values

sentry-android-ndk/src/main/jni/sentry.c

marandaneto · 2020-09-25T13:21:01Z

sentry-android-ndk/src/main/jni/sentry.c

@@ -25,27 +274,35 @@ static void send_envelope(sentry_envelope_t *envelope, void *unused_data) {
    sentry_envelope_write_to_file(envelope, outbox_path);
 }

-JNIEXPORT void JNICALL Java_io_sentry_android_ndk_SentryNdk_initSentryNative(JNIEnv *env, jclass cls, jobject sentry_sdk_options) {
+JNIEXPORT void JNICALL
+Java_io_sentry_android_ndk_SentryNdk_initSentryNative(JNIEnv *env, jclass cls,


just indentation, ignore it

marandaneto · 2020-09-25T13:21:39Z

sentry/src/main/java/io/sentry/SentryOptions.java

+  private final @NotNull List<IScopeObserver> observers = new ArrayList<>();
+
+  /** Enable the Java -> NDK Scope sync */
+  private boolean enableScopeSync;


opt-in by default

bruno-garcia

?

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java

sentry/src/main/java/io/sentry/SentryOptions.java

codecov-commenter · 2020-09-25T14:18:12Z

Codecov Report

❗ No coverage uploaded for pull request base (main@77ee2aa). Click here to learn what that means.
The diff coverage is 97.05%.

@@           Coverage Diff           @@
##             main     #937   +/-   ##
=======================================
  Coverage        ?   71.23%           
  Complexity      ?     1245           
=======================================
  Files           ?      126           
  Lines           ?     4575           
  Branches        ?      472           
=======================================
  Hits            ?     3259           
  Misses          ?     1067           
  Partials        ?      249

Impacted Files	Coverage Δ	Complexity Δ
sentry/src/main/java/io/sentry/Breadcrumb.java	`93.18% <ø> (ø)`	`19.00 <0.00> (?)`
sentry/src/main/java/io/sentry/NoOpSerializer.java	`44.44% <0.00%> (ø)`	`4.00 <0.00> (?)`
sentry/src/main/java/io/sentry/GsonSerializer.java	`100.00% <100.00%> (ø)`	`12.00 <1.00> (?)`
sentry/src/main/java/io/sentry/Scope.java	`91.66% <100.00%> (ø)`	`52.00 <0.00> (?)`
sentry/src/main/java/io/sentry/SentryOptions.java	`78.88% <100.00%> (ø)`	`79.00 <4.00> (?)`
...y/src/main/java/io/sentry/protocol/DebugImage.java	`3.33% <0.00%> (ø)`	`1.00% <0.00%> (?%)`
...src/main/java/io/sentry/SentryExecutorService.java	`100.00% <0.00%> (ø)`	`6.00% <0.00%> (?%)`
...rc/main/java/io/sentry/SentryExceptionFactory.java	`92.68% <0.00%> (ø)`	`8.00% <0.00%> (?%)`
...ry/src/main/java/io/sentry/protocol/Mechanism.java	`37.93% <0.00%> (ø)`	`6.00% <0.00%> (?%)`
...c/main/java/io/sentry/transport/NoOpTransport.java	`40.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
... and 121 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77ee2aa...1c685e6. Read the comment docs.

…ntry-java into feat/java-native-scope-sync

marandaneto · 2020-09-28T12:09:21Z

sentry-android-ndk/src/main/jni/sentry.c

+        // we create an object because the Java layer parses it as a Map
+        sentry_value_t dataObject = sentry_value_new_object();
+        sentry_value_set_by_key(dataObject, "data", sentry_value_new_string(charData));
+
+        sentry_value_set_by_key(crumb, "data", dataObject);


this could be improved if we expose something like sentry_value_from_json
cc @Swatinem

marandaneto

I approve myself!

bruno-garcia · 2020-09-28T15:50:11Z

Thanks for building this @marandaneto !!:rocket:

Swatinem · 2020-09-29T08:31:06Z

sentry/src/main/java/io/sentry/GsonSerializer.java

+   */
+  @Override
+  public String serialize(final @NotNull Map<String, Object> data) throws Exception {
+    Objects.requireNonNull(data, "The SentryEnvelope object is required.");


I guess the exception needs better wording :-D

Swatinem · 2020-09-29T08:34:21Z

sentry-android-ndk/src/main/jni/sentry.c

+    if (data) {
+        const char *charData = (*env)->GetStringUTFChars(env, data, 0);
+
+        // we create an object because the Java layer parses it as a Map


hm, was that there already? I don’t see it in this PR

sync Java to C

2b98250

bruno-garcia requested review from mitsuhiko, Swatinem and marandaneto September 22, 2020 01:17

bruno-garcia marked this pull request as draft September 22, 2020 01:18

Swatinem reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-samples/sentry-samples-android/src/main/java/io/sentry/sample/MainActivity.java Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

...amples/sentry-samples-servlet/src/main/java/io/sentry/samples/servlet/SentryInitializer.java Outdated Show resolved Hide resolved

marandaneto reviewed Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java Outdated Show resolved Hide resolved

Update sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScop…

fb26d8f

…eObserver.java Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>

bruno-garcia commented Sep 22, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

bruno-garcia and others added 2 commits September 22, 2020 07:58

Update sentry-android-ndk/src/main/jni/sentry.c

1b6f8d4

Update sentry-android-ndk/src/main/jni/sentry.c

dd5d2d4

Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>

bruno-garcia commented Sep 22, 2020

View reviewed changes

...amples/sentry-samples-servlet/src/main/java/io/sentry/samples/servlet/SentryInitializer.java Outdated Show resolved Hide resolved

marandaneto added 4 commits September 23, 2020 10:50

spotless

e313cf8

Merge branch 'main' into feat/java-native-scope-sync

20f624f

draft

5011b31

draft crumbs data

7d1484c

marandaneto reviewed Sep 23, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 23, 2020

View reviewed changes

refactor

a8adbc9

bruno-garcia commented Sep 23, 2020

View reviewed changes

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java Outdated Show resolved Hide resolved

Update sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScop…

8595cff

…eObserver.java

Swatinem reviewed Sep 23, 2020

View reviewed changes

marandaneto added 3 commits September 25, 2020 13:52

add options for scope sync and sync breadcrumb data

a75f0ac

Merge branch 'feat/java-native-scope-sync' of github.com:getsentry/se…

bf08284

…ntry-java into feat/java-native-scope-sync

java docs

a931184

marandaneto reviewed Sep 25, 2020

View reviewed changes

sentry-android-ndk/src/main/jni/sentry.c Outdated Show resolved Hide resolved

marandaneto reviewed Sep 25, 2020

View reviewed changes

bruno-garcia commented Sep 25, 2020

View reviewed changes

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NdkScopeObserver.java Show resolved Hide resolved

bruno-garcia marked this pull request as ready for review September 25, 2020 13:50

bruno-garcia commented Sep 25, 2020

View reviewed changes

sentry/src/main/java/io/sentry/SentryOptions.java Outdated Show resolved Hide resolved

bruno-garcia commented Sep 25, 2020

View reviewed changes

sentry/src/main/java/io/sentry/SentryOptions.java Outdated Show resolved Hide resolved

bruno-garcia commented Sep 25, 2020

View reviewed changes

sentry/src/main/java/io/sentry/SentryOptions.java Outdated Show resolved Hide resolved

Apply suggestions from code review

beb337f

marandaneto added 3 commits September 28, 2020 09:36

add tests

9da437d

Merge branch 'feat/java-native-scope-sync' of github.com:getsentry/se…

a3ff9e0

…ntry-java into feat/java-native-scope-sync

add tests

1c685e6

marandaneto requested a review from a team September 28, 2020 08:52

marandaneto reviewed Sep 28, 2020

View reviewed changes

marandaneto requested a review from Swatinem September 28, 2020 12:09

marandaneto approved these changes Sep 28, 2020

View reviewed changes

bruno-garcia merged commit a87e484 into main Sep 28, 2020

bruno-garcia deleted the feat/java-native-scope-sync branch September 28, 2020 12:21

Swatinem reviewed Sep 29, 2020

View reviewed changes

marandaneto mentioned this pull request May 4, 2022

Sentry.configureScope not synchronize in native getsentry/sentry-dart#194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync Java to C #937

sync Java to C #937

bruno-garcia commented Sep 22, 2020 •

edited

Loading

Swatinem left a comment

marandaneto Sep 22, 2020

bruno-garcia Sep 22, 2020

marandaneto Sep 22, 2020

bruno-garcia Sep 23, 2020

marandaneto Sep 25, 2020

marandaneto Sep 23, 2020

Swatinem Sep 23, 2020

marandaneto Sep 25, 2020

Swatinem Sep 23, 2020

marandaneto Sep 25, 2020

Swatinem Sep 23, 2020

marandaneto Sep 25, 2020

marandaneto Sep 25, 2020

marandaneto Sep 25, 2020

bruno-garcia left a comment

codecov-commenter commented Sep 25, 2020 •

edited

Loading

marandaneto Sep 28, 2020

marandaneto left a comment

bruno-garcia commented Sep 28, 2020

Swatinem Sep 29, 2020

Swatinem Sep 29, 2020


		import io.sentry.protocol.User;

		public interface IScopeObserver {

		@@ -11,8 +11,257 @@ struct transport_options {

		struct transport_options g_transport_options;

		JNIEXPORT void JNICALL

sync Java to C #937

sync Java to C #937

Conversation

bruno-garcia commented Sep 22, 2020 • edited Loading

Swatinem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

marandaneto left a comment

Choose a reason for hiding this comment

bruno-garcia commented Sep 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia commented Sep 22, 2020 •

edited

Loading

codecov-commenter commented Sep 25, 2020 •

edited

Loading