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

HintUtils.isFromHybridSdk and HintUtils.setIsFromHybridSdk should use hints instead #2525

Closed
marandaneto opened this issue Feb 7, 2023 · 9 comments
Labels
Platform: Android Type: Bug Something isn't working

Comments

@marandaneto
Copy link
Contributor

Integration

sentry-android

Build System

Gradle

AGP Version

7.0.0

Proguard

Disabled

Version

6.13.1

Steps to Reproduce

Events captured on Hybrid SDKs directly in the native layer (Java/Kotlin) will not add extra context on the DefaultAndroidEventProcessor, ScreenshotEventProcessor, etc.
The reason is that the HintUtils.isFromHybridSdk method checks if the event was created by a certain SDK prefix.

After those changes:

val sdkVersion = SdkVersion(name, version)
options.setSentryClientName(name)
options.setSdkVersion(sdkVersion)

Where the name and version are actually the name and version of the Hybrid SDK, every event will be considered coming from the Hybrid SDK even though they were captured directly in the native layer.

Expected Result

Events will be enriched with extra context

Actual Result

Events won't be enriched with extra context

@marandaneto
Copy link
Contributor Author

marandaneto commented Feb 7, 2023

@krystofwoldrich I believe this method was introduced by you, have you considered this use case?
Did RN already migrate to calling setSentryClientName and setSdkVersion?
Related issue getsentry/team-mobile#48

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Feb 7, 2023

@marandaneto RN Did not migrate to calling setSentryClientName and setSdkVersion yet.

Maybe we can use the contexts event.contexts.platform_context or event.platform?

What do you mean by should use hints?

@marandaneto
Copy link
Contributor Author

The SDK already had something similar called HintUtils.shouldApplyScopeData(hints) and other options.
With hints, if we pass something specific when capturing events, it's possible to know where the events come from (Hybrid SDKs or not).

event.platform is very limited, for Dart, for example, is other.
I don't think SDKs have platform_context.

I don't know the solution for this as of now, would need to think thru it.

@krystofwoldrich
Copy link
Member

krystofwoldrich commented Feb 8, 2023

I've tested this with RN and the native crashes are not marked as hybrid even after setting the RN SDK name and version. I've also tested regular Sentry.captureError in the Java part of the RN app and it also worked correctly. I've also tested it with the Flutter example app and it also seems to work correctly. Events from dart go thru OutboxSender and Java errors don't.

Event hint isHybrid is added in OutboxSender. I don't know enough about the Java SDK to say in what case this will fail.
I guess it comes down to knowing when OutboxSender is used and then it will be clear if the current workaround is okay or not.

HintUtils.setIsFromHybridSdk(hint, event.getSdk().getName());

@krystofwoldrich
Copy link
Member

@romtsn @markushi Do you know when is OutboxSender used besides Hybrid SDKs?
I guess when an error comes from a native?

@marandaneto
Copy link
Contributor Author

@romtsn @markushi Do you know when is OutboxSender used besides Hybrid SDKs? I guess when an error comes from a native?

EnvelopeFileObserverIntegration so Hybrid SDKs and sentry-native events.
SendCachedEnvelopeFireAndForgetIntegration sending of cached/offline events when the above failed to send right away (after restarting the app).
I believe those 2 use cases.

@krystofwoldrich
Copy link
Member

Thanks for the details.

So SendCachedEnvelopeFireAndForgetIntegration is also used for cached/offline java events, right?
Cached and offline don't go thru the regular pipeline anyway (they already have viewHierarchy and screenshots and other context). So even tho they will get marked as hybrid, it won't cause issues at the moment.

Or am I missing something.

@krystofwoldrich
Copy link
Member

Based on the latest updates from getsentry/team-mobile#48

This won't be an issue as the native SDKs shipped with Hybrids will be named sentry.java.android.hybrid-name.

@markushi
Copy link
Member

@krystofwoldrich could we close this issue in favor of getsentry/team-mobile#48?
cc @marandaneto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform: Android Type: Bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

3 participants