-
-
Notifications
You must be signed in to change notification settings - Fork 440
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
Enable Backpressure Management by Default #3284
Conversation
|
@@ -474,7 +474,7 @@ private static boolean initConfigurations(final @NotNull SentryOptions options) | |||
options.addPerformanceCollector(new JavaMemoryCollector()); | |||
} | |||
|
|||
if (options.isEnableBackpressureHandling()) { | |||
if (options.isEnableBackpressureHandling() && Platform.isJvm()) { |
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.
Since there's currently no way of disabling backpressure for Android via Metadata I only enable it for backend. There usually shouldn't be as high a volume of transactions on Android.
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 decided in Backend Daily it's OK to disable backpressure management for Android. If we ever want it we'll have to make it configurable and probably do a separate test phase before enabling it by default there.
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
99a51e2 | 405.11 ms | 479.65 ms | 74.54 ms |
baaf637 | 375.71 ms | 441.58 ms | 65.87 ms |
95a98b5 | 379.14 ms | 420.80 ms | 41.66 ms |
8ff8fd0 | 432.77 ms | 495.18 ms | 62.41 ms |
937879e | 400.98 ms | 482.84 ms | 81.86 ms |
4e29063 | 376.38 ms | 390.98 ms | 14.60 ms |
28c9a83 | 366.20 ms | 433.88 ms | 67.67 ms |
0bd723b | 412.52 ms | 496.65 ms | 84.13 ms |
93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
a537f8a | 457.14 ms | 514.19 ms | 57.05 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
99a51e2 | 1.72 MiB | 2.29 MiB | 576.34 KiB |
baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
95a98b5 | 1.70 MiB | 2.27 MiB | 586.31 KiB |
8ff8fd0 | 1.72 MiB | 2.27 MiB | 558.15 KiB |
937879e | 1.72 MiB | 2.27 MiB | 558.42 KiB |
4e29063 | 1.72 MiB | 2.29 MiB | 578.38 KiB |
28c9a83 | 1.70 MiB | 2.28 MiB | 592.00 KiB |
0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
a537f8a | 1.70 MiB | 2.27 MiB | 584.74 KiB |
Previous results on branch: feat/backpressure-opt-out
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2b9eda9 | 340.57 ms | 404.43 ms | 63.86 ms |
395c5fd | 397.80 ms | 466.40 ms | 68.60 ms |
858f42e | 390.96 ms | 454.06 ms | 63.10 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2b9eda9 | 1.70 MiB | 2.28 MiB | 591.98 KiB |
395c5fd | 1.70 MiB | 2.28 MiB | 591.95 KiB |
858f42e | 1.70 MiB | 2.28 MiB | 591.98 KiB |
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.
Should we also reverse the enableBackPressureHandling
boolean in the two SentryAutoConfigurationTest
classes?
Otherwise LGTM 👍
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
📜 Description
Turn on backpressure management by default.
Implemented in #3072
Docs PR: getsentry/sentry-docs#9539
💡 Motivation and Context
To reduce overhead Sentry performance instrumentation causes in times of load.
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps