-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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 the feature of excluding unused URLs in CommonFilter (#287) #914
Conversation
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
============================================
- Coverage 42.58% 42.49% -0.09%
+ Complexity 1443 1440 -3
============================================
Files 310 310
Lines 8992 8993 +1
Branches 1221 1222 +1
============================================
- Hits 3829 3822 -7
- Misses 4696 4701 +5
- Partials 467 470 +3
Continue to review full report at Codecov.
|
...entinel-web-servlet/src/main/java/com/alibaba/csp/sentinel/adapter/servlet/CommonFilter.java
Outdated
Show resolved
Hide resolved
And could you please also update the document here: https://github.com/alibaba/Sentinel/blob/master/sentinel-adapter/sentinel-web-servlet/README.md |
...entinel-web-servlet/src/main/java/com/alibaba/csp/sentinel/adapter/servlet/CommonFilter.java
Outdated
Show resolved
Hide resolved
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
@lym-ifae The CI status indicates there's something wrong with your changes: https://travis-ci.org/alibaba/Sentinel/builds/559316648 |
...entinel-web-servlet/src/main/java/com/alibaba/csp/sentinel/adapter/servlet/CommonFilter.java
Outdated
Show resolved
Hide resolved
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
Thanks for contributing! 👍 |
(cherry picked from commit 7dd20dd)
Why we need it
We need to support filtering urls that don't want to be resource in
CommonFilter
.Does this pull request fix one issue?
Resolves #287
Describe how you did it
As there's already a UrlCleaner interface, we could just reuse the interface and improve the logic of unifying the resource name with UrlCleaner. If developers are going to exclude some URLs, they can convert the URLs to the empty string "" in their
UrlCleaner
implementation, and inCommonFilter
we could check whether the cleaned resource name is empty. If it's empty, then it should be excluded and won't go through the logic of SentinelSphU.entry
.Describe how to verify it
We can get the statistic node of the resource via
ClusterBuilderSlot.getClusterNode(res)
method, and if theClusterNode
of the target URL is absent, then we think it has been excluded.Special notes for reviews