Skip to content

Improved ignore path regex #210

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

Merged
merged 2 commits into from
May 30, 2022
Merged

Improved ignore path regex #210

merged 2 commits into from
May 30, 2022

Conversation

tom-pytel
Copy link
Contributor

This is the same as skywalking-nodejs PR #81:

Minor enhancement/fix? Previously the config.trace_ignore_path pattern /a/**/* would match /a/b/, /a/b/c and /a/b/c/, etc... but not /a/ or /a/b. Now it does.

@tom-pytel tom-pytel added the enhancement New feature or request label May 30, 2022
@tom-pytel tom-pytel added this to the 1.0.0 milestone May 30, 2022
@tom-pytel tom-pytel requested a review from Superskyyy May 30, 2022 16:35
@tom-pytel
Copy link
Contributor Author

Update test case, /**/ should match any number of path segments OR NONE, test case was expecting to match at least one. Correct pattern to match AT LEAST ONE is /*/**/.

@Superskyyy
Copy link
Member

Superskyyy commented May 30, 2022

I think that a /a/**/* pattern should not match /a/ because there must be another / after the **.

Just want to point out the potential mismatch with a common perception of glob syntax. What do you think?

@tom-pytel
Copy link
Contributor Author

tom-pytel commented May 30, 2022

I think that a /a/**/* pattern should not match /a/ because there must be another / after the **.

Just want to point out the potential mismatch with a common perception of glob syntax. What do you think?

Could be mismatch with standard glob but the idea is to allow /**/* to be a wildcard that matches anything, including /, /a, /a/, /a/b, etc... Otherwise it won't and you will need two patterns. Your call, if you want I can revert to standard behavior in the node agent as well.

EDIT: Previous behavior was a little weird with no single wildcard pattern possible, for example /** had the following match matrix:

/        match
/a       match
/a/      no match
/a/b     match
/a/b/    no match
/a/b/c   match

Now, /**/* gives:

/        match
/a       match
/a/      match
/a/b     match
/a/b/    match
/a/b/c   match

EDIT2: Or should /** match everything above?

@Superskyyy
Copy link
Member

I think you are right, this is indeed more robust. Probably adding a small FAQ documentation will be nice, so it eliminates user confusion.

@Superskyyy
Copy link
Member

Other than this concern all LGTM.

@tom-pytel
Copy link
Contributor Author

tom-pytel commented May 30, 2022

I think you are right, this is indeed more robust. Probably adding a small FAQ documentation will be nice, so it eliminates user confusion.

I was looking for a place to document this and found the following in /docs/en/setup/EnvVars.md:

"SW_TRACE_IGNORE_PATH: You can setup multiple URL path patterns, The endpoints match these patterns wouldn't be traced. the current matching rules follow Ant Path match style , like /path/*, /path/**, /path/?"

Then, looking up Ant Path match style:

** matches zero or more 'directories' in a path

And:

com/**/test.jsp - matches all test.jsp files underneath the com path

And:

org/springframework/**/*.jsp - matches all .jsp files underneath the org/springframework path

Which implies the previous behavior and test below did not actually follow the specification. Since now it does, there is nothing extra to document I think?

pattern = 'eureka/**/test/**'
path = 'eureka/test/list'
self.assertFalse(fast_path_match(pattern, path))

@Superskyyy
Copy link
Member

I see, I didn't take a careful look ealier. Then everything LGTM :)

@Superskyyy Superskyyy changed the title improved ignore path regex Improved ignore path regex May 30, 2022
@tom-pytel tom-pytel merged commit a69041f into apache:master May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants