-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Add exceptionsToTrace configuration support in @SentinelResource annotation #543
Conversation
Codecov Report
@@ Coverage Diff @@
## master #543 +/- ##
============================================
+ Coverage 37.63% 38.74% +1.11%
- Complexity 1100 1216 +116
============================================
Files 259 275 +16
Lines 8171 8702 +531
Branches 1113 1163 +50
============================================
+ Hits 3075 3372 +297
- Misses 4697 4900 +203
- Partials 399 430 +31
Continue to review full report at Codecov.
|
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.
Plz also reformat your code to match Alibaba Java Code Guideline.
sentinel-core/src/main/java/com/alibaba/csp/sentinel/annotation/SentinelResource.java
Outdated
Show resolved
Hide resolved
return false; | ||
} | ||
for (Class exceptionToTrace : exceptionsToTrace) { | ||
if (ex.getClass().isAssignableFrom(exceptionToTrace)) { |
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.
It seems incorrect. For each exception to trace, it should be:
if (ex.getClass().isAssignableFrom(exceptionToTrace)) { | |
if (exceptionToTrace.isAssignableFrom(ex.getClass())) { |
Please be sure to add test cases and run the demo to verify it.
@@ -59,12 +59,33 @@ public Object invokeResourceWithSentinel(ProceedingJoinPoint pjp) throws Throwab | |||
} catch (BlockException ex) { | |||
return handleBlockException(pjp, annotation, ex); | |||
} catch (Throwable ex) { | |||
Tracer.trace(ex); | |||
if (isTrackedException(ex, annotation.exceptionsToTrace())) { |
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.
Maybe we can move the logic to AbstractSentinelAspectSupport
class to support extension for sub-classes. E.g.:
protected void traceException(Throwable ex, SentinelResource annotation)
Thanks for contributing. I'll help to update the test cases and refactor code later. Please note that every new feature or enhancement must carry corresponding test cases. |
Improved in 61c5250 |
Describe what this PR does / why we need it
In some scenarios,this resource will be degraded when it throws a specific exception.
So the configuration of exception class that should be traced (blocked), is needed.
Does this pull request fix one issue?
Fixes #540
Describe how you did it
Add the configuration of exception classes that would be traced.
Describe how to verify it
None
Special notes for reviews
None