You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/11/18 03:28:15 UTC
[GitHub] [skywalking-java] newboy2004 opened a new pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
newboy2004 opened a new pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72
### Fix <TracePathMatcher should match pattern "**" with paths end by "/">
- [ ] Add a unit test to verify that the fix works.
- [ ] Use "Spring AntPathMatcher' instead of ”FastPathMatcher“
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r753798447
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -83,6 +90,10 @@ private boolean normalMatch(String pat, int p, String str, int s) {
private boolean wildcardMatch(String pat, int p, String str, int s) {
char pc = safeCharAt(pat, p);
+ //if pat already arrival end,and str in position of s is not '/',then return true
+ if (pc == '\u0000' && safeCharAt(str, s) != '/') {
+ return true;
Review comment:
I think this change will cause a bug that 'abc/*' matching 'abc/foo/bar'
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +135,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
Review comment:
I think we can remove this check after we remove the last '/' both of pattern and value,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752141885
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
Ummm...I can't remember what I thought at the time...
Maybe it's because `/eureka/` doesn't match `/eureka`, I want to make it same as `/eureka/**` case?
I have changed my mind, the `/eureka/client/` should belong to `/eureka/**`.
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-972814817
@newboy2004 Please follow the @devkanro 's comments to polish/enhance this fix and update the changes.md in the root.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975066979
> > All operation names, such as typically `/eureka/` is collected by a HTTP client plugin. If that plugin says removing a `/` as suffix, I think it is generally easy.
>
> The code test mentioned above should return false，please @devkanro check
The point here is, `returning false` breaks the expectation when this PR raised.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975073155
> > > All operation names, such as typically `/eureka/` is collected by a HTTP client plugin. If that plugin says removing a `/` as suffix, I think it is generally easy.
> >
> >
> > The code test mentioned above should return false，please @devkanro check
>
> The point here is, `returning false` breaks the expectation when this PR raised.
so remvoing the last '/' suffix, it's not a good idea, it breaks the expectation.
or is there a logical problem with the modification of the code after the removal / modification
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973873798
I think the conclusion is very clear. The recommended way to fix is removing the last `/` in the operation name, before running `match`. Such as in `TraceIgnoreExtendService#trySampling`. What do you think? @newboy2004
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973711212
> Agree, this kind of change seems better and easier to understand.
Continue to repair * matching, and remove the last / of pattern and string before?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752041255
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
Maybe the comment need be changed.
```Java
// End of pattern, make matching success quickly.
if (p >= pat.length() && s < str.length()) {
return true;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752144713
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
And maybe the rule of `wildcardMatch` also should be changed?
Should `/eureka/*` match with `/eureka/client/` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro edited a comment on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975165175
We should rearrange the rules so that we can analyze the edge conditions.
Basic (from spring ant matcher):
1. `?` matches any one char.
2. `*` matches zero or more chars except '/'.
3. `**` matches zero or more path parts.
Edge(for *):
1. `/eureka/*` matches `/eureka/`?
2. `/eureka/*` matches `/eureka/test/`?
3. `/eureka/*/` matches `/eureka/`?
4. `/eureka/*/` matches `/eureka/test`?
5. Is `/eureka/*` same as `/eureka/*/`?
Edge(for **):
1. `/eureka/**` matches `/eureka/`?
2. `/eureka/**` matches `/eureka/test/`?
3. `/eureka/**/` matches `/eureka/`?
4. `/eureka/**/` matches `/eureka/test`?
5. Is `/eureka/**` same as `/eureka/**/`?
Edge(for normal):
1. `/eureka/` matches `/eureka`?
2. `/eureka` matches `/eureka/`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975042380
All operation names, such as typically `/eureka/` is collected by a HTTP client plugin. If that plugin says removing a `/` as suffix, I think it is generally easy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975040034
> edge
We can consider not supporting this so-called bug，so I'll consider claiming other tasks?haha
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973716531
Remove / before the match, I think. Then no need to change matching core, fromy understanding.
Please correct me if I am wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973928582
> conclusion
ok，i understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752825902
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
what is final dicussion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975064543
> All operation names, such as typically `/eureka/` is collected by a HTTP client plugin. If that plugin says removing a `/` as suffix, I think it is generally easy.
The code test mentioned above should return false，please @devkanro check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973792250
> i think remove / after,Assert should return true.
please check my opinion,is it right?
Yes, `/eureka/apps/` should be captured by `/eureka/*` expression. That is why @devkanro proposed a change about removing `/` in `/eureka/apps/`. Then the current FastPathMatcher should return true, too. Right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r753798447
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -83,6 +90,10 @@ private boolean normalMatch(String pat, int p, String str, int s) {
private boolean wildcardMatch(String pat, int p, String str, int s) {
char pc = safeCharAt(pat, p);
+ //if pat already arrival end,and str in position of s is not '/',then return true
+ if (pc == '\u0000' && safeCharAt(str, s) != '/') {
+ return true;
Review comment:
I think this change will cause a bug that 'abc/*' matching 'abc/foo/bar'
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +135,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
Review comment:
I think we can remove this check after we remove the last '/' both of pattern and value,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro edited a comment on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975170969
My answer(all false for current behavior):
Edge(for *):
1. [x] `/eureka/*` matches `/eureka/`?
2. [x] `/eureka/*` matches `/eureka/test/`?
3. [ ] `/eureka/*/` matches `/eureka/`?
4. [ ] `/eureka/*/` matches `/eureka/test`?
5. [ ] Is `/eureka/*` same as `/eureka/*/`?
Edge(for **):
1. [x] `/eureka/**` matches `/eureka/`?
2. [x] `/eureka/**` matches `/eureka/test/`?
3. [ ] `/eureka/**/` matches `/eureka/`?
4. [ ] `/eureka/**/` matches `/eureka/test`?
5. [ ] Is `/eureka/**` same as `/eureka/**/`?
Edge(for normal):
1. [ ] `/eureka/` matches `/eureka`?
2. [ ] `/eureka` matches `/eureka/`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r753916270
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +135,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
Review comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975157355
Removing suffix is not a good idea. And changing logic makes me concerns about new bugs.
After all, this is just an enhancement for this kind of case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752149394
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
>/eureka/ doesn't match /eureka
I think they should be same, do you know any different in some use cases?
I feel they are same.
> And maybe the rule of wildcardMatch also should be changed?
Should /eureka/* match with /eureka/client/ ?
Yes, it should be supported, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975036763
OK, I can see this becomes a tricky point. From this point of view, removing `/` and changing matching rules are both not a very good idea, as these kinds of edge conditions.
I think we should consider whether changing plugin's operation rules fits more cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975029711
> > I think the conclusion is very clear. The recommended way to fix is removing the last `/` in the operation name, before running `match`. Such as in `TraceIgnoreExtendService#trySampling`. What do you think? @newboy2004
>
> I think the current doesn't follow this conclusion. We just need to slightly change the input.
ok,but if i remove the last '/' in the operation name,please confirm blow code,return true or false:
String patten = "/eureka/*";
path = "/eureka/";
match = pathMatcher.match(patten, path);
Assert.assertTrue(match);
i think '*' match zero or one char,but remove the last '/' after,pattern='/eureka/*' path='/eureka',accroding Ant Rule,result should return false,do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-974816125
> I think the conclusion is very clear. The recommended way to fix is removing the last `/` in the operation name, before running `match`. Such as in `TraceIgnoreExtendService#trySampling`. What do you think? @newboy2004
I think the current doesn't follow this conclusion. We just need to slightly change the input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-972525972
If you want to add this, you need to enhance the existing one, rather than use Spring to replace.
We knew Spring can do from beginning, but we determined this is not suitable, due to package size and License risk.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973697700
The simple resolution is remove the last '/' for the pattern and string.
If this change is done during the matching process, many states will be introduced, which will complicate the state machine of the matcher.
I think it is a good choice to normalize(remove the last '/') the pattern and value before entering the matcher.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng closed pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng closed pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-972649243
@devkanro Could you recheck this? I think you wrote this originally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973699585
Agree, this kind of change seems better and easier to understand.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973867688
> > i think remove / after,Assert should return true.
> > please check my opinion,is it right?
>
> Yes, `/eureka/apps/` should be captured by `/eureka/*` expression. That is why @devkanro proposed a change about removing `/` in `/eureka/apps/`. Then the current FastPathMatcher should return true, too. Right?
i wait for final opinion? my local code repo have already finish what * rule match and unit test according before discussion
waiting for you final opionion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r751894702
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/pom.xml
##########
@@ -41,5 +42,11 @@
<version>${ststem-rules.version}</version>
<scope>test</scope>
</dependency>
+
+ <dependency>
+ <groupId>org.springframework</groupId>
+ <artifactId>spring-core</artifactId>
Review comment:
We can't accept Spring core, sorry. Because we have to include all codes inside the agent plugin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r753916305
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -83,6 +90,10 @@ private boolean normalMatch(String pat, int p, String str, int s) {
private boolean wildcardMatch(String pat, int p, String str, int s) {
char pc = safeCharAt(pat, p);
+ //if pat already arrival end,and str in position of s is not '/',then return true
+ if (pc == '\u0000' && safeCharAt(str, s) != '/') {
+ return true;
Review comment:
i review it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro edited a comment on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro edited a comment on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975165175
We should rearrange the rules so that we can analyze the edge conditions.
Basic:
1. `?` matches any one char.
2. `*` matches zero or more chars except '/'.
3. `**` matches one or more path part.
Edge(for *):
1. `/eureka/*` matches `/eureka/`?
2. `/eureka/*` matches `/eureka/test/`?
3. `/eureka/*/` matches `/eureka/`?
4. `/eureka/*/` matches `/eureka/test`?
5. Is `/eureka/*` same as `/eureka/*/`?
Edge(for **):
1. `/eureka/**` matches `/eureka/`?
2. `/eureka/**` matches `/eureka/test/`?
3. `/eureka/**/` matches `/eureka/`?
4. `/eureka/**/` matches `/eureka/test`?
5. Is `/eureka/**` same as `/eureka/**/`?
Edge(for normal):
1. `/eureka/` matches `/eureka`?
2. `/eureka` matches `/eureka/`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975165175
We should rearrange the rules so that we can analyze the edge conditions.
Basic:
1. `?` matches any one char.
2. `*` matches one path part.
3. `**` matches one or more path part.
Edge(for *):
1. `/eureka/*` matches `/eureka/`?
2. `/eureka/*` matches `/eureka/test/`?
3. `/eureka/*/` matches `/eureka/`?
4. `/eureka/*/` matches `/eureka/test`?
5. Is `/eureka/*` same as `/eureka/*/`?
Edge(for **):
1. `/eureka/**` matches `/eureka/`?
2. `/eureka/**` matches `/eureka/test/`?
3. `/eureka/**/` matches `/eureka/`?
4. `/eureka/**/` matches `/eureka/test`?
5. Is `/eureka/**` same as `/eureka/**/`?
Edge(for normal):
1. `/eureka/` matches `/eureka`?
2. `/eureka` matches `/eureka/`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-975170969
My answer(all false for current behavior):
Edge(for *):
1. [x] `/eureka/*` matches `/eureka/`?
2. [x] `/eureka/*` matches `/eureka/test/`?
3. [ ] `/eureka/*/` matches `/eureka/`?
4. [ ] `/eureka/*/` matches `/eureka/test`?
5. [ ] Is `/eureka/*` same as `/eureka/*/`?
Edge(for **):
1. [x] `/eureka/**` matches `/eureka/`?
2. [x] `/eureka/**` matches `/eureka/test/`?
3. [ ] `/eureka/**/` matches `/eureka/`?
4. [ ] `/eureka/**/` matches `/eureka/test`?
5. [ ] Is `/eureka/**` same as `/eureka/**/`?
Edge(for normal):
1. [ ] `/eureka/` matches `/eureka`?
2. [x] `/eureka` matches `/eureka/`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-986603030
No update in 2 weeks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-972664932
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752044809
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
In my originally design, the `/eureka/**` not match the `/eureka/client/` is a feature, but I have no preference for this, I can accept it regardless of whether it matches or does not match.
Could you show your use cases and benefits for it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752083218
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
@devkanro Could you share a little more about why `/eureka/client/` doesn't belong to `/eureka/**`? I hope we could have a fully evaluation considering this was being asked more than once.(This is the first fix)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752061900
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
I think because some users think all things under `/eureka/` belong to this match rule.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752829132
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
The current resolution is, what your proposal is correct. Just follow the polish requirements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752836938
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
@wu-sheng
> /eureka/ doesn't match /eureka
Ummm, The reason I do this is to simplify the state machine. There are no other special considerations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] devkanro commented on a change in pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
devkanro commented on a change in pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#discussion_r752836938
##########
File path: apm-sniffer/optional-plugins/trace-ignore-plugin/src/main/java/org/apache/skywalking/apm/plugin/trace/ignore/matcher/FastPathMatcher.java
##########
@@ -124,7 +124,7 @@ private boolean wildcardMatch(String pat, int p, String str, int s) {
private boolean multiWildcardMatch(String pat, int p, String str, int s) {
// End of pattern, just check the end of string is '/' quickly.
if (p >= pat.length() && s < str.length()) {
- return str.charAt(str.length() - 1) != '/';
+ return true;
}
Review comment:
@wu-sheng
> /eureka/ doesn't match /eureka
Ummm, The reason I do this is to simplify the state machine. There are no other special considerations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973790658
> Remove / before the match, I think. Then no need to change matching core, fromy understanding. Please correct me if I am wrong.
please see blow origin code:
String patten = "/eureka/*";
path = "/eureka/apps/";
match = pathMatcher.match(patten, path);
Assert.assertFalse(match);
i think remove / after,Assert should return true.
please check my opinion,is it right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] newboy2004 commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
newboy2004 commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-973697240
> @newboy2004 Please follow the @devkanro 's comments to polish/enhance this fix and update the changes.md in the root.
ok,i continue repair it.
By the way, accessing githubcom is slow. Is there any good solution？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #72: TracePathMatcher should match pattern "**" with paths end by "/" (#7875)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #72:
URL: https://github.com/apache/skywalking-java/pull/72#issuecomment-974816125
> I think the conclusion is very clear. The recommended way to fix is removing the last `/` in the operation name, before running `match`. Such as in `TraceIgnoreExtendService#trySampling`. What do you think? @newboy2004
I think the current doesn't follow this conclusion. We just need to slightly change the input.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org