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

Support InAppIncludes #472

Closed
1 of 3 tasks
marandaneto opened this issue Feb 2, 2021 · 5 comments
Closed
1 of 3 tasks

Support InAppIncludes #472

marandaneto opened this issue Feb 2, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@marandaneto
Copy link
Contributor

Description

Support InAppIncludes for Android and mark frames as inApp if part of the frame's function or package

When does the problem happen

Frames are always inApp=false on Android events and grouping is suboptimal

  • During build
  • During run-time
  • When capturing a hard crash

Environment

any Android version

Steps To Reproduce

Just capture an event and see in_app in the raw JSON

@marandaneto marandaneto added the enhancement New feature or request label Feb 2, 2021
@marandaneto
Copy link
Contributor Author

marandaneto commented Feb 2, 2021

a workaround would be getsentry/sentry-java#1220 but this seems very wrong.
ideally, the Android bits pass the app's package to sentry-native init and adds it as inAppInclude.

Android events look like this:

"function": "Java_io_sentry_samples_android_NativeSample_crash",
"package": "/data/app/~~PI_9s6SKRF3ilWeN39_ocg==/io.sentry.samples.android-ZM0JaA9G1o6oI8i0m34KBA==/lib/arm64/libnative-sample.so",

Art frames look like this:

"function": "art::ArtMethod::Invoke",
"raw_function": "art::ArtMethod::Invoke(art::Thread*, unsigned int*, unsigned int, art::JValue*, char const*)",
"symbol": "_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc",
"package": "/apex/com.android.art/lib64/libart.so",

So I guess it's reasonable to mark them as inApp if the inAppIncludes is within the function or package.

@Swatinem
Copy link
Member

After the first round of feedback I’m really unsure how this is supposed to work.

  • inAppIncludes is supposed to be a prefix

This means that neither Java_**io_sentry_samples_android**_NativeSample_crash nor /data/app/~~PI_9s6SKRF3ilWeN39_ocg==/**io.sentry.samples.android**-ZM0JaA9G1o6oI8i0m34KBA==/lib/arm64/libnative-sample.so would match here.
Also, how are you supposed to figure out that path prefix in the first place?
Same for other third party libraries, which can be all over the place, not necessarily in well known places.

  • inAppIncludes takes precedence over inAppExcludes

So if we for example make sentry_ a default excludes, the fact that you bundle that into your APK, it would be flagged as in-app still.

@mitsuhiko
Copy link
Member

I'm generally not sure how in-app prefixes is supposed to work with the obfuscation on one hand and debug symbols on the other. Why do we not let people do this via server side enhancements and shipping better defaults there?

@marandaneto
Copy link
Contributor Author

marandaneto commented Apr 19, 2021

JNI methods are not obfuscated via proguard as they need to match in Java and C code.
The suggestion was using a contains and not a startsWith exactly because of the prefix gargabe, maybe the switch from startsWith or contains could be hidden by a flag that Android sets it.
No need for debug symbols in this case, we do on-device symbolication.
Doing this on the server-side is suboptimal, every customer would need to do it manually (that's how it is today btw), we can do it automatically on client-side as we do with Android and has way better inApp classification.

@marandaneto
Copy link
Contributor Author

turns out this would be still a bad inApp classification and it'd be better done on the server with default grouping rules, a DACI and sentry issue is the output of this change, hence closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants