Skip to content
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

Align how we enrich the event with device context on Hybrid SDKs #11

Open
11 of 12 tasks
marandaneto opened this issue May 17, 2022 · 23 comments · Fixed by getsentry/sentry-react-native#3170
Open
11 of 12 tasks

Comments

@marandaneto
Copy link

marandaneto commented May 17, 2022

The way how we enrich the event with device context on Hybrid SDKs varies depending on the OS.

iOS: The device context lives on the scope, so we call the native bridge, serialize the scope, return a Map and build the event with all the device context, all the device context is available in the beforeSend callback.
https://github.com/getsentry/sentry-dart/blob/912b9205691837abdd546c62844bc9568b908495/flutter/ios/Classes/SentryFlutterPluginApple.swift#L84-L132
Once the event is built, we call captureEnvelope https://github.com/getsentry/sentry-dart/blob/912b9205691837abdd546c62844bc9568b908495/flutter/ios/Classes/SentryFlutterPluginApple.swift#L340

Android: The Hybrid SDK writes the envelope into the disk and a File observer that lives in the Android SDK picks it up, deserializes, and enriches the event with the device context via Event processors, all the device context is not available in the beforeSend callback.
https://github.com/getsentry/sentry-dart/blob/912b9205691837abdd546c62844bc9568b908495/flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt#L89

Both implementations have a trade-off but I believe we should follow the iOS way or find a better alternative.
For that, we'd need to change how the Android SDK enriches the event with device context.

What we want:
The device context should be visible in the beforeSend callback for Android and iOS, you should also be able to filter out the event.
We want to unify this behavior since this leads to different problems depending on the platform.

Ps: We cannot get rid of the File observer on Android due to sentry-native that cannot call the JNI bridge within the signal handler.

Hacks like this should go away.

Android SDK

Hybrid SDKs

  1. Platform: Dart
    denrase
  2. Android Platform: Capacitor Status: Backlog
    lucas-zimerman
@marandaneto
Copy link
Author

Related issue getsentry/sentry-java#2063

@philipphofmann
Copy link
Member

I guess the main reason for going with the iOS approach is having all the data available on beforeSend, right, @marandaneto?

@marandaneto
Copy link
Author

I guess the main reason for going with the iOS approach is having all the data available on beforeSend, right, @marandaneto?

Yes.

@adinauer
Copy link
Member

Android needs to provide a way of retrieving the info (eg. as a map), then hybrid SDKs can pick that up and create the event before passing it to the Android SDK.

@bruno-garcia
Copy link
Member

Ps: We cannot get rid of the File observer on Android due to sentry-native that cannot call the JNI bridge within the signal handler.

In the case of the signal handler, we will read the envelope after the app restarts. Do we have any other case to have the file observer? Maybe it could go otherwise?

@marandaneto
Copy link
Author

@bruno-garcia if the C layer calls Java back via JNI when it's not a signal handler to capture the event, the file watcher could go.

@marandaneto
Copy link
Author

marandaneto commented Sep 22, 2022

There's another edge case that causes a bug on React Native, on Android, we remove all the breadcrumbs because the Native layer adds them back (this is done to avoid duplication).
If the device is offline, the Android SDK sends the event on restart but does not add the breadcrumbs back because the event is cached and doesn't have the crumbs anymore.
Also getsentry/sentry-react-native#2146

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Oct 17, 2022

Assuming we are going the iOS way, we need

  • captureEnvelope, which exists on the hub in java already,
  • storeEnvelope for hard crashes,
  • and create SentryEnvelope.from(byteArray), I thought something like that might exist but I could not find it.

Is that right?

@marandaneto
Copy link
Author

storeEnvelope can be implemented in the plugin as well, no need to come from the Android SDK, this is technically already there.
SentryEnvelope.from(byteArray) you can do EnvelopeReader.read(stream), but yeah exposing a method that takes a byte array would be better.

@markushi
Copy link
Member

markushi commented Mar 1, 2023

We could refactor the DefaultAndroidEventProcessor to be able to provide the generated data to the hybrid SDKs as well. This is already done on iOS. ANRv2 already contains some ground work for this, as the scope serialization is done here already.

For View Hierarchies this is causing issues right now as well, as events and attachments are sent separated.

@markushi
Copy link
Member

markushi commented Mar 1, 2023

Lets wait for ANRv2 before starting work on this.

@marandaneto
Copy link
Author

Another one getsentry/sentry-react-native#2896

@expcapitaldev
Copy link

hi,guys, I am using sentry-cordova and have sometimes double breadcrumbs, can we do something with that ?

@philipphofmann
Copy link
Member

@expcapitaldev, please open an issue at the sentry-cordova for your problem.

@krystofwoldrich
Copy link
Member

Linking another related issue getsentry/sentry-react-native#3090

@krystofwoldrich
Copy link
Member

What will hybrid SDKs need from SentryAndroid?

  • Methods to get currently initialized SDK name and version. Hybrid SDKs can't save the information from the init method as this would not work for manually initialized apps.
  • Get native SDK data about user, device and app, contexts, tags, extra, fingerprint, severityLevel, environment and breadcrumbs. These data we currently retrieve from sentry-cocoa. We might be able to use Sentry.configureScope as in sentry-cocoa.
  • Way to store final envelope bytes (only sent_at should be updated) during app crash.
  • Way to send final envelope bytes (only send_at should be updated) during non-crash events.

Some of the mentioned items are possible today, this is meant to capture everything that needs to be available to the hybrid SDKs.

@marandaneto
Copy link
Author

I'd avoid the Sentry.configureScope approach, I'd rather expose an internal API that we have access to.

@markushi
Copy link
Member

markushi commented Jun 28, 2023

Required work:

Moved to the issue description.

Android:

  • [x] For event enriching: For collections (e.g. breadcrumbs) stop trying to merge values (use hybrid hint)
  • [ ] Provide a way to retrieve the scope data (pull based approach). Have a ScopeUtils.serialize(scope) class
  • [x] Provide a way to retrieve the sdk name/version
  • [x] Provide a way to retrieve dist + environment from options

RN:

  • [ ] Retrieve above data
  • [ ] Use HubAdapter.getInstance().captureEnvelope() for capturing the envelope

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Jun 29, 2023

@romtsn @markushi For these two points, will we just adjust the captureEnvelope for hybrid SDKs or would it make sense to create a new method that just updates the sent_at but otherwise sends as is?

HubAdapter.getInstance().captureEnvelope() Does not enrich data apart from sent_at.

  • Way to store final envelope bytes (only sent_at should be updated) during app crash.
  • Way to send final envelope bytes (only send_at should be updated) during non-crash events.

@krystofwoldrich
Copy link
Member

Because of the changes in Profiles ingestion, getsentry/sentry-react-native#3057 is blocked by this.

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Jul 7, 2023

I've checked how to call captureEnvelope, and it works but I think we could have a simpler interface for creating envelopes from bytes.

        final HubAdapter hubAdapter = HubAdapter.getInstance();
        IEnvelopeReader envelopeReader = hubAdapter.getOptions().getEnvelopeReader();
        final InputStream byteStream = new ByteArrayInputStream(bytes)) {
        final SentryEnvelope sentryEnvelope = envelopeReader.read(byteStream);
        if (sentryEnvelope != null) {
            hubAdapter.captureEnvelope(sentryEnvelope);
        }

@markushi Would something like SentryEnvelope.from(bytes) make sense?

        byte[] bytes = new byte[rawBytes.size()];
        final SentryEnvelope sentryEnvelope = SentryEnvelope.from(bytes);
        if (sentryEnvelope != null) {
            hubAdapter.captureEnvelope(sentryEnvelope);
        }

@marandaneto I'm not sure what about Flutter, would it make sense to have a constructor from(bytes)?

@marandaneto
Copy link
Author

Dart issue getsentry/sentry-dart#1556

@marandaneto
Copy link
Author

@krystofwoldrich keep the meta issue open till all the SDKs implement it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
8 participants