getsentry / sentry-dart

Sentry SDK for Dart and Flutter
https://sentry.io/for/flutter/
MIT License
766 stars 239 forks source link

Sentry.configureScope not synchronize in native #194

Closed nEdAy closed 2 years ago

nEdAy commented 4 years ago

sentry_flutter: ^4.0.0-alpha.2

Sentry.configureScope((Scope scope) async {
    scope.user = User(id: xxx);
});

The user information was set in the flutter and did not synchronize in native(Android).

Hope to add this feature, thanks

marandaneto commented 4 years ago

@nEdAy thanks for this.

True, we still don't synchronize the Scope to the native layer, but we'll definitely do it.

bruno-garcia commented 3 years ago

This is needed also in order to get the scope IP address to the session in native.

rayliverified commented 3 years ago

Is this related to a race condition that happens between the native and Flutter SDKs? If await is used on SentryFlutter.init(), only the Flutter scope details are sent. If await is not called on SentryFlutter.init(), only the native details are sent. I'm currently getting both the native and Flutter details to send by calling Sentry.configureScope before every error report call.

marandaneto commented 3 years ago

@searchy2 not sure if I follow, this is a feature request, not a bug, at least related to this issue. could you open an issue describing the problem? using the issue template would be very helpful too, thanks,

rayliverified commented 3 years ago

Will do, thanks!

mafreud commented 3 years ago

any update?

marandaneto commented 3 years ago

no, you could always call the Native SDKs yourself via Method channels, we'll eventually do it, just not a high priority right now, PRs are also welcome, happy to guide & review.

ueman commented 3 years ago

Do we want to implement it with ScopeObserver (like the Java implementation)?

marandaneto commented 3 years ago

@ueman it's not a unified behavior across SDK but usually helpful when you have multiple SDKs packaged together. it sounds like a good idea for me, you can experiment and see how it goes, obviously, we can adapt if necessary. when Dart is used with Flutter, it should propagate the changes (possible to turn on/off via options). when Dart is used alone, it should not, so that's why the Interface exists.

ueman commented 3 years ago

A few thoughts on this:

Option 1a:

The Java interface has only synchronus methods, which we can't use because MethodChannels are async. That would require use to make every synchronized property of a Scope to be an async method. This would be a pretty huge breaking change.

Option 1b:

Just like 1a but we don't await the MethodChannels. This is kinda a best effort approach. No breaking changes.

Option 2:

We could do something like a Sentry.configureNativeScope():

await SentryFlutter.configureNativeScope((scope){
// everything which is done in this callback is synced to the native SDKs
});

// No callback syncs the current scope to native
await SentryFlutter.configureNativeScope();

This method would then be responsible to sync everything to the native side.

What do you think @marandaneto ? Or do you have another idea?

marandaneto commented 3 years ago

@ueman let's take a look at the react-native-sdk, I believe they should have the same problem, let's see what they have done. I'd be fine not awaiting the scope to be set on native, but this has a downside, you set a user, the App. crashes right away and it didn't have time enough to sync to native, that crash report won't have the user.

ueman commented 3 years ago

ReactNative also uses option 1b. Or at least it looks like that to me. https://reactnative.dev/docs/native-modules-android mentions Promises for JS <-> Android communication which https://github.com/getsentry/sentry-react-native/blob/master/src/js/scope.ts does not use.

marandaneto commented 3 years ago

@jennmueng can help us out on that, how does it work on React-native? pros? cons?

bruno-garcia commented 3 years ago

If going with option 1b we need to consider back pressure. What happens if I call that in a tight loop? Is it going to overwhelm the process? If so we'd need to track how many futures are open and drop calls accordingly.

Honestly the Flutter SDK is doing things right. By having an API that returns Future across the board, allowing for async code to run on init, eventProcessors, etc. We should discuss this with a bigger audience and consider the implications of taking this approach on more SDKs.

jennmueng commented 3 years ago

@ueman Yes we use option 1b over in React Native. It does not use promises for syncing scope across the native layer, I believe it's the easiest and quickest way to implement scope sync across the native layer for 99% of cases unless a crash happens right away like @marandaneto's example.

But I'm not sure if I follow the discussion: If a crash does happen right away on the native layer with the scope info missing, wouldn't the crash just happen regardless and prolonging it on the upper (JS/Flutter) layer not have any effect? If it's a crash from the upper (JS/Flutter) layer, wouldn't it have the scope set already?

bruno-garcia commented 3 years ago

But I'm not sure if I follow the discussion: If a crash does happen right away on the native layer with the scope info missing, wouldn't the crash just happen regardless and prolonging it on the upper (JS/Flutter) layer not have any effect? If it's a crash from the upper (JS/Flutter) layer, wouldn't it have the scope set already?

The thought is that the code that will crash runs on the same thread/sequentially to changing scope. Imagine:

Sentry.init(..);
Sentry.setUser(user);
crash();

With setUser returning before the asynchornous code completes, the crash most likely will not contain the user.

The .NET SDK has a ConfigureScopeAsync method, similar to what @ueman suggested.

Changing the whole API such that all scope changes is something I assume we'll never do as it also becomes very cumbersome. I think this topic is worth a call. @marandaneto what do you think?

marandaneto commented 3 years ago

yes, it's already on our agenda for today :)

marandaneto commented 3 years ago

@ueman hold on on this one for a bit, we need to discuss the right way to do it, and yesterday we didn't have time enough to discuss this further, the decision would likely affect other SDKs like Cordova, React-native etc

marandaneto commented 3 years ago

@ueman we could not talk about it, we'd have a meeting about that today but I won't be able to make it, but @bruno-garcia can take the lead. let's do other tasks before getting to this one.

bruno-garcia commented 3 years ago

We discussed hybrid SDK integrations today and a lot of the design ideas and solutions we had involved synchronously passing data between layers. The way that came up then for Flutter is to call through Dart ffi into sentry-native and have Java/Kotlin also sync to sentry-native.

For iOS we would need a C ABI to access the scope too or bundle sentry-native.

ueman commented 3 years ago

That comes with its own set of drawbacks. On macOS for example you have to sign your application if you are using ffi. See https://dart.dev/guides/libraries/c-interop

Also you still need to think about JavaScript interop. See https://pub.dev/packages/js JavaScript interop is synchronus though.

bssughosh commented 3 years ago

Any updates regarding this?

marandaneto commented 3 years ago

@bssughosh not yet, would you like to contribute with a PR? happy to guide

bssughosh commented 3 years ago

@marandaneto I can surely contribute with a PR but haven't worked on flutter plugins before. So will need some guidance there

marandaneto commented 3 years ago

@bssughosh awesome, i will provide a draft next week so you have an idea on what to do

bssughosh commented 3 years ago

@marandaneto Is the draft ready?

marandaneto commented 3 years ago

@bssughosh unfortunately not yet due to some other tasks.

you can get an idea on how the React Native SDK does tho.

JS code -> https://github.com/getsentry/sentry-react-native/blob/master/src/js/scope.ts#L61 Wrapper JS Code to Native Code https://github.com/getsentry/sentry-react-native/blob/master/src/js/wrapper.ts#L383-L405 Android Code https://github.com/getsentry/sentry-react-native/blob/master/android/src/main/java/io/sentry/react/RNSentryModule.java#L342-L395 iOS Code https://github.com/getsentry/sentry-react-native/blob/master/ios/RNSentry.m#L274-L304

We can take the same approach using the MethodChannel from the sentry_flutter package. does that help?

bssughosh commented 3 years ago

@bssughosh unfortunately not yet due to some other tasks.

you can get an idea on how the React Native SDK does tho.

JS code -> https://github.com/getsentry/sentry-react-native/blob/master/src/js/scope.ts#L61 Wrapper JS Code to Native Code https://github.com/getsentry/sentry-react-native/blob/master/src/js/wrapper.ts#L383-L405 Android Code https://github.com/getsentry/sentry-react-native/blob/master/android/src/main/java/io/sentry/react/RNSentryModule.java#L342-L395 iOS Code https://github.com/getsentry/sentry-react-native/blob/master/ios/RNSentry.m#L274-L304

We can take the same approach using the MethodChannel from the sentry_flutter package. does that help?

@marandaneto Thank you for the resources. Will go through them and revert back

marandaneto commented 3 years ago

@bssughosh did u get the chance to look into it?

bssughosh commented 3 years ago

@bssughosh did u get the chance to look into it?

@marandaneto I went through the react code but was not able to figure out the place where I need to start in flutter plugin code

marandaneto commented 2 years ago

@bssughosh ok, I will try to draft an example within the next few days.

bruno-garcia commented 2 years ago

Example from Unity:

C# interface used to fire scope changes: https://github.com/getsentry/sentry-dotnet/blob/a2a921b55892238718271f91ea48422d08de69ca/src/Sentry/IScopeObserver.cs

C# base class for scope sync: https://github.com/getsentry/sentry-unity/blob/bcf9ce7daa90ff533a306a400758b73af67a5000/src/Sentry.Unity/ScopeObserver.cs#L9

C# -> Java (Android): https://github.com/getsentry/sentry-unity/blob/bcf9ce7daa90ff533a306a400758b73af67a5000/src/Sentry.Unity.Android/AndroidJavaScopeObserver.cs#L9

When initializing the SDK, we set the instance of the scope observer, and also opt-in for scope sync (users can opt-out on the init callback: https://github.com/getsentry/sentry-unity/blob/bcf9ce7daa90ff533a306a400758b73af67a5000/src/Sentry.Unity.Android/SentryNativeAndroid.cs#L19-L20

There's the equivalent for iOS through P/Invoke: https://github.com/getsentry/sentry-unity/tree/main/src/Sentry.Unity.iOS

marandaneto commented 2 years ago

https://github.com/getsentry/sentry-java/pull/937

marandaneto commented 2 years ago

https://github.com/getsentry/sentry-dart/releases/tag/6.6.0-beta.1

dotdoom commented 2 years ago

setUser is now async, but configureScope doesn't take an async method as a parameter. So there's an unavoidable context escape. Has anyone filed an issue for that yet?

ueman commented 2 years ago

@dotdoom no, but please do so.

marandaneto commented 2 years ago

@dotdoom @ueman https://github.com/getsentry/sentry-dart/issues/921

Artein commented 1 year ago

For me setting user with id via configureScope does not work.

My code: await Sentry.configureScope((scope) async => await scope.setUser(SentryUser(id: id!.toString())));

In this way in Sentry user id should be just number like (100, 123 etc), but I see something like that "user [id:08D1225F-F37A-4A81-A074-1DEE68D799A2]"

What am I doing wrong?

sentry_flutter: 7.8.0

marandaneto commented 1 year ago

@Artein Android or iOS?

Artein commented 1 year ago

@Artein Android or iOS?

iOS.

Interesting, that when I tested with 7.7.0 — the issue existed. But in Issues view on Sentry site some of the issues have valid userId's (from 7.7.0 as well).

For now, I don't know when it works, and when it doesn't.

marandaneto commented 1 year ago

@Artein Android or iOS?

iOS.

Interesting, that when I tested with 7.7.0 — the issue existed. But in Issues view on Sentry site some of the issues have valid userId's (from 7.7.0 as well).

For now, I don't know when it works, and when it doesn't.

Please raise a new issue since this one is closed, with a minimal reproducible example. I tested it here and it works just fine.

Could be that before you call await Sentry.configureScope((scope) async => await scope.setUser(SentryUser(id: id!.toString())));, some errors were sent, and in this case, those errors didn't have your own id yet.