-
-
Notifications
You must be signed in to change notification settings - Fork 461
Automatically downsample transaction when system is under load (backpressure) #3072
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
Co-authored-by: Sentry Github Bot <[email protected]> Co-authored-by: Roman Zavarnitsyn <[email protected]>
* reduced timeout of AsyncHttpTransport close to flushTimeoutMillis (default 4s in Android and 15s in Java) to avoid ANRs in Android
* Add deadline timeout for automatic transactions * Update Changelog * Address PR comments * Fix formatting * Add missing test, improve naming * Ensure deadline timeout is only set once
* updated minimum Android SDK version to 19 * removed few workarounds for version < 19
Co-authored-by: Roman Zavarnitsyn <[email protected]> Co-authored-by: Sentry Github Bot <[email protected]>
sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java
Outdated
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d6d2b2e | 392.55 ms | 467.50 ms | 74.95 ms |
| 99a51e2 | 405.11 ms | 479.65 ms | 74.54 ms |
| 0bd723b | 375.20 ms | 452.41 ms | 77.20 ms |
| c7e2fbc | 377.85 ms | 426.35 ms | 48.50 ms |
| b172d4e | 412.60 ms | 492.68 ms | 80.08 ms |
| 4e29063 | 364.08 ms | 445.51 ms | 81.43 ms |
| bc4be3b | 360.40 ms | 435.04 ms | 74.64 ms |
| 93a76ca | 397.30 ms | 455.16 ms | 57.87 ms |
| 8fd337b | 349.16 ms | 459.22 ms | 110.06 ms |
| 93a76ca | 377.41 ms | 448.22 ms | 70.81 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
| 99a51e2 | 1.72 MiB | 2.29 MiB | 576.34 KiB |
| 0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
| c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
| b172d4e | 1.72 MiB | 2.29 MiB | 578.09 KiB |
| 4e29063 | 1.72 MiB | 2.29 MiB | 578.38 KiB |
| bc4be3b | 1.72 MiB | 2.29 MiB | 576.53 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
| 8fd337b | 1.72 MiB | 2.27 MiB | 555.00 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
Previous results on branch: feat/backpressure
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8d081e4 | 446.34 ms | 535.84 ms | 89.50 ms |
| 9f06b6e | 361.10 ms | 430.00 ms | 68.90 ms |
| 95f8fd0 | 415.82 ms | 470.54 ms | 54.72 ms |
| 3b8dee7 | 436.04 ms | 526.52 ms | 90.48 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 8d081e4 | 1.72 MiB | 2.27 MiB | 556.60 KiB |
| 9f06b6e | 1.72 MiB | 2.27 MiB | 556.27 KiB |
| 95f8fd0 | 1.72 MiB | 2.27 MiB | 556.38 KiB |
| 3b8dee7 | 1.72 MiB | 2.27 MiB | 556.30 KiB |
|
one thing you're missing - a new client report ( sentry-java/sentry/src/main/java/io/sentry/Hub.java Lines 668 to 670 in 1ad36f1
|
sentry/src/main/java/io/sentry/transport/QueuedThreadPoolExecutor.java
Outdated
Show resolved
Hide resolved
lbloder
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.
Left comments on some minor things. Looks good otherwise 👍
Do we want to add the configuration also to the ExternalOptions to make this available to other integrations too?
As far as I can see, enabling backpressureHandling is currently only possible via Spring and the SentryProperties or by supplying SentryOptions in code, correct?
sentry/src/main/java/io/sentry/backpressure/BackpressureMonitor.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/transport/AsyncHttpTransport.java
Outdated
Show resolved
Hide resolved
…check is run against it
lbloder
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, one minor nitpick :)
📜 Description
Automatically downsample transaction when system is under load by checking:
This check is performed every 10 seconds. With each negative health check we halve
tracesSampleRateup to 10 times, meaning the originaltracesSampleRateis multiplied by 1, 1/2, 1/4, ... up to 1/1024 (~ 0.001%). Any positive health check resets to the originaltracesSampleRateset inSentryOptions.Also see python PRs for reference.
💡 Motivation and Context
Closes #2829
💚 How did you test it?
Unit tests, manually using Spring Boot sample.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps