-
-
Notifications
You must be signed in to change notification settings - Fork 465
proposal for streaming NativeEventCollector #5065
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
proposal for streaming NativeEventCollector #5065
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d091211 | 325.47 ms | 371.58 ms | 46.11 ms |
| 6ad3f8e | 313.98 ms | 362.27 ms | 48.29 ms |
| 14ebc28 | 320.09 ms | 398.66 ms | 78.57 ms |
| dc79daa | 333.69 ms | 382.49 ms | 48.80 ms |
| 7aef08b | 286.72 ms | 361.82 ms | 75.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d091211 | 1.58 MiB | 2.19 MiB | 622.38 KiB |
| 6ad3f8e | 1.58 MiB | 2.19 MiB | 622.25 KiB |
| 14ebc28 | 1.58 MiB | 2.20 MiB | 637.07 KiB |
| dc79daa | 1.58 MiB | 2.19 MiB | 622.86 KiB |
| 7aef08b | 1.58 MiB | 2.19 MiB | 622.25 KiB |
markushi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| * remainder of the section on close to position the stream at the next envelope item. Does not | ||
| * close the underlying stream. | ||
| */ | ||
| private static final class BoundedInputStream extends InputStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's elegant!
df83d57
into
feat/tombstone_native_sdk_merge
* feat: merge tombstone and native sdk events * add preliminary change log * add preliminary change log * apply review+sync feedback * add tombstone manifest flag * remove tombstone-native correlation via processStateSummary * 2-phase streaming NativeEventCollector (#5065) * extract inner inApp check into a reusable static method * reduce I/O in the collect() method of the NativeEventCollector. * add native attachments to TombstoneHint. * introduce VMA -> module coalescing via ModuleAccumulator * ensure native crash survives the merge * handle null nativeLibraryDir in TombstoneParser * clarify inApp vs nativeLibraryDir usage in code comment * ignore stack frames from anonymous VMAs that don't resolve to a function name * use the right nativeLibraryDir for the tombstone test fixture * add proguard rule for protobuf-lite * make BoundedInputStream safer wrt double closes * override empty function name behavior from SentryStackTraceFactory.isInApp() because it applies to class-names generically whereas we use it only for function name prefixes. * pre merge preps. --------- Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
Proposal for an adaptation to the
NativeEventCollectorintroduced in #5037:extractNativeEventFromFile()to load the full envelope and event.I used the envelopes from the core test resources, copied them to the
sentry-android-coretest resource folder, and added a couple of native-specific envelopes for the tests.This limits memory usage as much as possible since there are at best 10
ApplicationExitInfowithEXIT_REASON_NATIVE(likely fewer), which act as an upper bound to the number of envelopes/events we fully decode.It should also be much fairer on the CPU, since much less decoding/deserialization is required. I/O will be the same during collection; however, with this approach, we must read the matching envelopes from disk again during full decode.
#skip-changelog