-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Use the same hub in WebFlux exception handler as we do in WebFilter #2566
Use the same hub in WebFlux exception handler as we do in WebFilter #2566
Conversation
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a98c90a | 293.96 ms | 333.90 ms | 39.94 ms |
a864e2f | 353.66 ms | 392.94 ms | 39.28 ms |
b300cf4 | 371.66 ms | 401.00 ms | 29.34 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a98c90a | 1.73 MiB | 2.34 MiB | 626.31 KiB |
a864e2f | 1.73 MiB | 2.34 MiB | 626.31 KiB |
b300cf4 | 1.73 MiB | 2.34 MiB | 626.31 KiB |
Previous results on branch: fix/use-request-hub-in-webflux-exception-handler
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
165ee8a | 336.26 ms | 378.39 ms | 42.13 ms |
8da9be7 | 321.31 ms | 385.62 ms | 64.31 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
165ee8a | 1.73 MiB | 2.34 MiB | 626.31 KiB |
8da9be7 | 1.73 MiB | 2.34 MiB | 626.31 KiB |
…e-request-hub-in-webflux-exception-handler
Codecov ReportBase: 80.41% // Head: 80.41% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## feat/reactor-with-sentry-continue-or-fresh #2566 +/- ##
=============================================================================
Coverage 80.41% 80.41%
- Complexity 4020 4022 +2
=============================================================================
Files 332 332
Lines 15155 15161 +6
Branches 1979 1980 +1
=============================================================================
+ Hits 12187 12192 +5
Misses 2189 2189
- Partials 779 780 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@@ -33,22 +34,29 @@ public SentryWebExceptionHandler(final @NotNull IHub hub) { | |||
@Override |
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.
This should probably also be done for Spring Boot 2
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.
Let's not do it in this PR. I'm afraid adding it there might make things worse. If we clone a new hub from main hub we might end up with static calls during requests that end up in different (non parent - child) hubs. Maybe ThreadLocalAccessor
is added to Spring Boot 2 at some point.
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.
Load test confirms problems: WARNING: Attempt to pop the root scope.
📜 Description
Pass hub from
WebFilter
toSentryWebExceptionHandler
viaServerWebExchange
attributes
.💡 Motivation and Context
We just used whatever hub is there which could possibly bleed into other requests.
💚 How did you test it?
Manually with debugger
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps