-
-
Notifications
You must be signed in to change notification settings - Fork 462
feat(events): Detect oversized events and reduce their size #4903
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
Conversation
|
sentry/src/main/java/io/sentry/util/EventSizeLimitingUtils.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 357.79 ms | 421.84 ms | 64.05 ms |
| ae7fed0 | 293.84 ms | 380.22 ms | 86.38 ms |
| 9fbb112 | 404.51 ms | 475.65 ms | 71.14 ms |
| 27d7cf8 | 397.90 ms | 498.65 ms | 100.75 ms |
| a416a65 | 333.78 ms | 410.37 ms | 76.59 ms |
| d217708 | 411.22 ms | 430.86 ms | 19.63 ms |
| 539ca63 | 313.51 ms | 355.43 ms | 41.92 ms |
| 23d6b12 | 354.10 ms | 408.38 ms | 54.28 ms |
| 14ff5ee | 419.75 ms | 495.73 ms | 75.98 ms |
| ce0a49e | 532.00 ms | 609.96 ms | 77.96 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| ae7fed0 | 1.58 MiB | 2.12 MiB | 551.77 KiB |
| 9fbb112 | 1.58 MiB | 2.11 MiB | 539.18 KiB |
| 27d7cf8 | 1.58 MiB | 2.12 MiB | 549.42 KiB |
| a416a65 | 1.58 MiB | 2.12 MiB | 555.26 KiB |
| d217708 | 1.58 MiB | 2.10 MiB | 532.97 KiB |
| 539ca63 | 1.58 MiB | 2.12 MiB | 551.41 KiB |
| 23d6b12 | 1.58 MiB | 2.10 MiB | 532.31 KiB |
| 14ff5ee | 1.58 MiB | 2.10 MiB | 535.08 KiB |
| ce0a49e | 1.58 MiB | 2.10 MiB | 532.94 KiB |
Previous results on branch: feat/detect-oversized-error-events
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 238dc90 | 318.98 ms | 361.46 ms | 42.48 ms |
| 73f341a | 312.72 ms | 354.90 ms | 42.18 ms |
| 0c31c0a | 310.15 ms | 354.83 ms | 44.68 ms |
| 21ae207 | 310.42 ms | 355.09 ms | 44.67 ms |
| 606e793 | 329.40 ms | 395.16 ms | 65.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 238dc90 | 1.58 MiB | 2.13 MiB | 556.43 KiB |
| 73f341a | 1.58 MiB | 2.13 MiB | 556.24 KiB |
| 0c31c0a | 1.58 MiB | 2.13 MiB | 556.43 KiB |
| 21ae207 | 1.58 MiB | 2.13 MiB | 556.42 KiB |
| 606e793 | 1.58 MiB | 2.13 MiB | 556.48 KiB |
| * Enables event size limiting with {@link EventSizeLimitingEventProcessor}. When enabled, events | ||
| * exceeding 1MB will have breadcrumbs and stack frames reduced to stay under the limit. | ||
| */ | ||
| private boolean enableEventSizeLimiting = false; |
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.
Kepping this opt-in for now, we could turn it on by default in the next major.
| * @return the modified event (should ideally be reduced in size) | ||
| */ | ||
| @NotNull | ||
| SentryEvent execute(@NotNull SentryEvent event, @NotNull Hint hint); |
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.
IMO running this between beforeSend and passing this on to the transport is the best place to check since customers might be adding large amounts of data in beforeSend, so first reducing size and then adding back to it could easily cause the event to become too large again.
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.
Looks good to me! I'm a bit worried about the extra overhead of serializing to determine final size on mobile, but as this is opt-in for now, all good 👍
sentry/src/main/java/io/sentry/util/EventSizeLimitingUtils.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/util/EventSizeLimitingUtils.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/util/EventSizeLimitingUtils.java
Outdated
Show resolved
Hide resolved
| final @NotNull List<SentryStackFrame> truncatedFrames = new ArrayList<>(); | ||
| truncatedFrames.addAll(frames.subList(0, FRAMES_PER_SIDE)); | ||
| truncatedFrames.addAll(frames.subList(frames.size() - FRAMES_PER_SIDE, frames.size())); | ||
| stacktrace.setFrames(truncatedFrames); |
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.
We could extract this into a truncateFrames(frames) method and re-use it.
Co-authored-by: Markus Hintersteiner <markus.hintersteiner@sentry.io>
📜 Description
Detect oversized events and reduce their size by removing breadcrumbs and reducing stack trace size.
Also add
onOversizedErrorcallback that allows customers to customize what's dropped.💡 Motivation and Context
So we can be more lenient with truncation in SDK and only perform it if required to reduce event size.
Closes #4822
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps