TracePathMatcher should match pattern "**" with paths end by "/" (#7875) by newboy2004 · Pull Request #72 · apache/skywalking-java

newboy2004 · 2021-11-18T03:27:57Z

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“

wu-sheng · 2021-11-18T04:22:46Z

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.

wu-sheng · 2021-11-18T08:40:08Z

@devkanro Could you recheck this? I think you wrote this originally.

devkanro · 2021-11-18T09:02:00Z

ok

devkanro · 2021-11-18T09:14:30Z

        // 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;
        }


Maybe the comment need be changed.

// End of pattern, make matching success quickly. if (p >= pat.length() && s < str.length()) { return true; }

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?

I think because some users think all things under /eureka/ belong to this match rule.

@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)

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

And maybe the rule of wildcardMatch also should be changed?
Should /eureka/* match with /eureka/client/ ?

/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.

what is final dicussion?

The current resolution is, what your proposal is correct. Just follow the polish requirements.

@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.

wu-sheng · 2021-11-18T12:18:48Z

@newboy2004 Please follow the @devkanro 's comments to polish/enhance this fix and update the changes.md in the root.

newboy2004 · 2021-11-19T03:21:31Z

@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？

devkanro · 2021-11-19T03:22:40Z

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.

wu-sheng · 2021-11-19T03:28:16Z

Agree, this kind of change seems better and easier to understand.

newboy2004 · 2021-11-19T03:56:24Z

Agree, this kind of change seems better and easier to understand.

Continue to repair * matching, and remove the last / of pattern and string before?

wu-sheng · 2021-11-19T04:06:14Z

Remove / before the match, I think. Then no need to change matching core, fromy understanding.
Please correct me if I am wrong.

newboy2004 · 2021-11-19T06:29:10Z

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?

wu-sheng · 2021-11-19T06:33:19Z

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?

newboy2004 · 2021-11-19T08:37:52Z

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

wu-sheng · 2021-11-19T08:48:00Z

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

newboy2004 · 2021-11-19T10:05:43Z

conclusion

ok，i understand.

devkanro · 2021-11-21T13:13:13Z

        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;


I think this change will cause a bug that 'abc/*' matching 'abc/foo/bar'

i review it

devkanro · 2021-11-21T13:14:16Z

        // 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;


I think we can remove this check after we remove the last '/' both of pattern and value,

wu-sheng · 2021-11-21T13:17:10Z

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.

newboy2004 · 2021-11-22T03:09:26Z

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?

wu-sheng · 2021-11-22T03:23:58Z

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.

newboy2004 · 2021-11-22T03:32:26Z

edge

We can consider not supporting this so-called bug，so I'll consider claiming other tasks?haha

wu-sheng · 2021-11-22T03:39:56Z

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.

newboy2004 · 2021-11-22T04:02:17Z

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

wu-sheng · 2021-11-22T04:04:38Z

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.

newboy2004 · 2021-11-22T04:13:30Z

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

wu-sheng · 2021-11-22T06:05:06Z

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.

devkanro · 2021-11-22T06:17:58Z

We should rearrange the rules so that we can analyze the edge conditions.

Basic (from spring ant matcher):

? matches any one char.
* matches zero or more chars except '/'.
** matches zero or more path parts.

Edge(for *):

/eureka/* matches /eureka/?
/eureka/* matches /eureka/test/?
/eureka/*/ matches /eureka/?
/eureka/*/ matches /eureka/test?
Is /eureka/* same as /eureka/*/?

Edge(for **):

/eureka/** matches /eureka/?
/eureka/** matches /eureka/test/?
/eureka/**/ matches /eureka/?
/eureka/**/ matches /eureka/test?
Is /eureka/** same as /eureka/**/?

Edge(for normal):

/eureka/ matches /eureka?
/eureka matches /eureka/?

devkanro · 2021-11-22T06:26:29Z

wu-sheng · 2021-12-06T09:40:35Z

No update in 2 weeks.

lincyang720 added 4 commits November 17, 2021 18:11

TracePathMatcher should match pattern "**" with paths end by "/"

310c9b9

TracePathMatcher should match pattern "**" with paths end by "/"

6655ccb

TracePathMatcher should match pattern "**" with paths end by "/" (#7875)

775b399

Merge remote-tracking branch 'origin/main' into main

25c8426

wu-sheng requested changes Nov 18, 2021

View reviewed changes

Comment thread apm-sniffer/optional-plugins/trace-ignore-plugin/pom.xml Outdated

wu-sheng added the invalid This doesn't seem right label Nov 18, 2021

lincyang720 added 2 commits November 18, 2021 16:31

TracePathMatcher should match pattern "**" with paths end by "/" (#7875)

72e190e

Merge branch 'issue_7875' into main

cda48dc

wu-sheng added enhancement New feature or request plugin and removed invalid This doesn't seem right labels Nov 18, 2021

devkanro suggested changes Nov 18, 2021

View reviewed changes

lincyang720 added 2 commits November 20, 2021 13:05

TracePathMatcher should match pattern "**" with paths end by "/" (#7875)

3a792ee

Merge branch 'issue_7875' into main

1eb68c5

devkanro suggested changes Nov 21, 2021

View reviewed changes

wu-sheng closed this Dec 6, 2021

This was referenced Dec 6, 2021

[Bug] TracePathMatcher should match pattern "**" with paths end by "/" apache/skywalking#7875

Closed

Fix <TracePathMatcher should match pattern "**" with paths end by "/"> #81

Merged

GuoHaoZai pushed a commit to GuoHaoZai/skywalking-java that referenced this pull request Apr 24, 2025

Add Envoy sidecar request/response filter time metrics (apache#72)

ad4a02c

Conversation

newboy2004 commented Nov 18, 2021

Fix <TracePathMatcher should match pattern "**" with paths end by "/">

Uh oh!

Uh oh!

wu-sheng commented Nov 18, 2021

Uh oh!

wu-sheng commented Nov 18, 2021

Uh oh!

devkanro commented Nov 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devkanro Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Nov 18, 2021

Uh oh!

newboy2004 commented Nov 19, 2021

Uh oh!

devkanro commented Nov 19, 2021

Uh oh!

wu-sheng commented Nov 19, 2021

Uh oh!

newboy2004 commented Nov 19, 2021

Uh oh!

wu-sheng commented Nov 19, 2021

Uh oh!

newboy2004 commented Nov 19, 2021

Uh oh!

wu-sheng commented Nov 19, 2021

Uh oh!

newboy2004 commented Nov 19, 2021

Uh oh!

wu-sheng commented Nov 19, 2021

Uh oh!

newboy2004 commented Nov 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Nov 21, 2021

Uh oh!

newboy2004 commented Nov 22, 2021

Uh oh!

wu-sheng commented Nov 22, 2021

Uh oh!

newboy2004 commented Nov 22, 2021

Uh oh!

wu-sheng commented Nov 22, 2021

Uh oh!

newboy2004 commented Nov 22, 2021

Uh oh!

wu-sheng commented Nov 22, 2021

Uh oh!

newboy2004 commented Nov 22, 2021

Uh oh!

wu-sheng commented Nov 22, 2021

devkanro Nov 19, 2021 •

edited

Loading

devkanro commented Nov 22, 2021 •

edited

Loading

devkanro commented Nov 22, 2021 •

edited

Loading