Finish WebFlux transaction before popping scope #2724
Conversation
…roblems with popScope and transaction.finish
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46b1782 | 387.72 ms | 458.74 ms | 71.02 ms |
| 1707044 | 338.80 ms | 384.79 ms | 46.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 46b1782 | 1.72 MiB | 2.28 MiB | 570.44 KiB |
| 1707044 | 1.72 MiB | 2.28 MiB | 570.44 KiB |
Previous results on branch: fix/finish-webflux-transaction-before-scope-pop
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| cc386a9 | 339.47 ms | 460.14 ms | 120.67 ms |
| 1ba2a69 | 304.45 ms | 353.29 ms | 48.84 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| cc386a9 | 1.72 MiB | 2.28 MiB | 570.44 KiB |
| 1ba2a69 | 1.72 MiB | 2.28 MiB | 570.44 KiB |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2724 +/- ##
============================================
- Coverage 81.13% 81.11% -0.02%
- Complexity 4449 4451 +2
============================================
Files 345 343 -2
Lines 16402 16402
Branches 2226 2227 +1
============================================
- Hits 13307 13304 -3
- Misses 2168 2169 +1
- Partials 927 929 +2
☔ View full report in Codecov by Sentry. |
lbloder
left a comment
lbloder
left a comment
There was a problem hiding this comment.
Besides the questions about getting the Hub in SentryWebFilterWithThreadLocalAccessor, LGTM
| __ -> | ||
| doFinally( | ||
| serverWebExchange, | ||
| Sentry.getCurrentHub(), |
There was a problem hiding this comment.
Does Sentry.getCurrentHub() work correctly in this filter method? ReactorUtils.withSentryNewMainHubClone does not set the current hub, so shouldn't we get the hub from the context?
Or is this somehow handled by reactor behind the scenes?
There was a problem hiding this comment.
ThreadLocal is set via ThreadLocalAccessor and propagated via reactor context.
There was a problem hiding this comment.
Just tested again with debugger, works as expected (i.e. uses the correct hub).
| .doOnError(e -> doOnError(transactionContainer.transaction, e)) | ||
| .doFirst( | ||
| () -> { | ||
| doFirst(serverWebExchange, Sentry.getCurrentHub()); |
There was a problem hiding this comment.
See comment about Sentry.getCurrentHub()
| () -> { | ||
| doFirst(serverWebExchange, Sentry.getCurrentHub()); | ||
| transactionContainer.transaction = | ||
| maybeStartTransaction(Sentry.getCurrentHub(), serverWebExchange.getRequest()); |
There was a problem hiding this comment.
See comment about Sentry.getCurrentHub()
📜 Description
Merge
SentryWebTracingFilterintoSentryWebFilterto avoid ordering problems withpopScopeandtransaction.finish.💡 Motivation and Context
Fixes #2700
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps