Experimental Spring Webflux integration #1529
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1529 +/- ##
============================================
- Coverage 76.03% 76.02% -0.02%
- Complexity 1955 1974 +19
============================================
Files 192 198 +6
Lines 6765 6837 +72
Branches 675 680 +5
============================================
+ Hits 5144 5198 +54
- Misses 1294 1306 +12
- Partials 327 333 +6
Continue to review full report at Codecov.
|
maciejwalkowiak
changed the title
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebExceptionHandler.java
Show resolved
Hide resolved
sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebExceptionHandler.java
Show resolved
Hide resolved
sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java
Outdated
Show resolved
Hide resolved
| mechanism.setType("SentryWebExceptionHandler"); | ||
| mechanism.setHandled(false); | ||
| final Throwable throwable = | ||
| new ExceptionMechanismException(mechanism, ex, Thread.currentThread()); |
There was a problem hiding this comment.
more like a question, is SentryWebExceptionHandler always executed in the calling thread? otherwise marking the current thread will be wrong due to the usage of Thread.currentThread().
Asking because of #1524 (comment)
in coroutines, the handler can get called in different threads
There was a problem hiding this comment.
I don't think there is a guarantee that its executed on the same thread. thread is a required property - what should be set there other than currentThread in this case?
There was a problem hiding this comment.
the side effect is that the exception.setThreadId will contain the id of the current thread, which could display the wrong stack trace in the sentry issues page if SentryOptions#attachThreads is enabled (all threads reported).
I'm wondering if we should make ExceptionMechanismException#getThread nullable, and add a new ctor to ExceptionMechanismException and just don't set the threadId if not available.
There was a problem hiding this comment.
If we include Thread stack traces + Exception stack trace, the UI will display the Exception stack trace on the threads list (and ignore the Thread one) by matching its ID. So if we don't send an ID this could be broken.
Since this is asynchronous, the "thread" is less relevant here in terms of "This thread has done all the work up to this point". But it's OK to be "this thread did the work at the time of capture" which is what this is represented here.
So I think keeping as Thread.currentThread() is fine here and arguably a better approach than making it nullable given how Sentry works.
There was a problem hiding this comment.
yep I am just afraid that the thread id may be wrong, and we'd be hiding the stack trace of that thread since it's going to be showing the exception stack trace, I don't know if it's a big problem though.
sentry-spring/src/main/java/io/sentry/spring/webflux/TransactionNameProvider.java
Outdated
Show resolved
Hide resolved
|
add new package entry to https://github.com/getsentry/sentry-java/blob/main/.craft.yml |
Instructions for the release registry are on the repo README (it's just a |
|
@marandaneto only the sample is a new package and as far as I can see we don't have other samples configured in |
oh, true, I thought it was a new package but indeed, its the sample, all good, thanks! |
|
I believe this closes #999 CodeQL is unhappy |
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
|
@maciejwalkowiak please open up the docs for it |
|
|
Hi. when is the release date of this PR? |
|
@pkgonan we don't have a release schedule, but we release very often: https://github.com/getsentry/sentry-java/releases I recommend watching this repo for releases and trying out next beta. |
|
@maciejwalkowiak Thank you. |
|
|
||
| /** Handles unhandled exceptions in Spring WebFlux integration. */ | ||
| @Order( | ||
| -2) // the DefaultErrorWebExceptionHandler provided by Spring Boot for error handling is ordered |
There was a problem hiding this comment.
Maybe use existing property for order tuning? sentry.exception-resolver-order. But, for some reason, it has default value 1 for MVC handler.
📜 Description
Experimental Integration for Spring Webflux with Project Reactor.
💡 Motivation and Context
Support reactive Spring stack. #640
💚 How did you test it?
Integration tests.
📝 Checklist
🔮 Next steps