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 2022/06/02 10:06:25 UTC
[GitHub] [skywalking-java] xjs1919 opened a new pull request, #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
xjs1919 opened a new pull request, #191:
URL: https://github.com/apache/skywalking-java/pull/191
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== π Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ==== -->
### Enhance TracePathMatcher, "/eureka/*" or "/eureka/**" can match "/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 closed pull request #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng closed pull request #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
URL: https://github.com/apache/skywalking-java/pull/191
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144758098
By comparison, FastPathMatcher seems more intuitive?
Case 1:
`/eureka/*` vs `/eureka/test/`
* means zero or one path part, but a path part may have ends with / or not.
Case 2:
`/eureka/*/` vs `/eureka/`
* means zero or one path part, but if it means zero, the path should be `/eureka//`, can we omit the double /?
Case 3:
`/eureka/**` vs `/eureka`
/** means at least one /
Case 4:
`/eureka/**/` vs `/eureka`
**/ means path should ends with /
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144716060
If this does follow ant path match, I have concerns about this, whether this us reasonable.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144774865
@xjs1919 Please update with us, why do you think this behavior makes sense?
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144738988
@wu-sheng
No, I means the current behavior of AntPathMatcher is `/eureka/**` matches `/eureka`, but `/eureka/*` not.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144747700
Highlights for diff
```
matches("/eureka/*", "/eureka/test/") = false/true
matches("/eureka/*/", "/eureka/") = true/false
matches("/eureka/**", "/eureka") = true/false
matches("/eureka/**/", "/eureka") = true/false
```
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144698660
After my test, AntPathMatcher do not match this rule.
```Kotlin
AntPathMatcherAntPathMatcher().match("/eruka/*", "/eruka") // false
```
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144721296
> If this does follow ant path match, I have concerns about this, whether this is reasonable.
@wu-sheng `/eureka/**` matches `/eureka`, but `/eureka/*` not.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144773119
Spring's AntPathMatcher is copy from Apache Ant.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144752534
Are these diff from previous or PR version?
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144745820
There are my test result, use 'AntPathMatcher/FastPathMatcher'
```Kotlin
matches("/eureka/*", "/eureka") = false/false
matches("/eureka/*", "/eureka/") = true/true
matches("/eureka/*", "/eureka/test/") = false/true
matches("/eureka/*", "/eureka/test") = true/true
matches("/eureka/*/", "/eureka") = false/false
matches("/eureka/*/", "/eureka/") = true/false
matches("/eureka/*/", "/eureka/test") = false/false
matches("/eureka/*/", "/eureka/test/") = true/true
matches("/eureka/**", "/eureka") = true/false
matches("/eureka/**", "/eureka/") = true/true
matches("/eureka/**", "/eureka/test/") = true/true
matches("/eureka/**", "/eureka/test") = true/true
matches("/eureka/**/", "/eureka") = true/false
matches("/eureka/**/", "/eureka/") = true/true
matches("/eureka/**/", "/eureka/test") = true/true
matches("/eureka/**/", "/eureka/test/") = true/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] wu-sheng commented on pull request #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144736783
Is there other tools supporting in this way?
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144756848
The diff is about Spring's AntPathMatcher between my FastPathMatcher in current production version of skywalking.
I am not compare the PR version.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144765621
I feel like the current version is good. I am not sure why Spring works in a different way.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1147437991
Closing due to support @devkanro 's opinion(original author)
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144733894
OK, got your point. Both should support.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144736466
I just want to ask, whether 2 configs make more sense than supporting this kind of config? After all, /* means at least having a /.
My concern is about this.
--
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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
devkanro commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1144740356
I am don't sure my FastPathMatcher's behavior, I will get test 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] xjs1919 commented on pull request #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
xjs1919 commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1146920596
> @xjs1919 Please update with us, why do you think this behavior makes sense?
i want to ignore path contains "actuator", so i have to config like this :
trace.ignore_path=${SW_AGENT_TRACE_IGNORE_PATH:**/actuator/**,GET:/actuator}
it works, however a little redundant and not gracefulοΌand Spring's AntPathMatcher can match "/eureka" with pattern "/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 #191: string "/eureka" should match pattern "/eureka/*" or "/eureka/**"
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #191:
URL: https://github.com/apache/skywalking-java/pull/191#issuecomment-1147425632
I am not sure why AntPathMatcher works like that, but to be honest, I don't think it is the standard.
`/eureka/**` ignoring `/eureka` seems unreasonable. Maybe Spring team has their reasons, but logically, this is 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