Skip to content
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

Support regex path matching for autoLogging.ignorePaths option #100

Merged
merged 2 commits into from
Sep 13, 2020

Conversation

jafern14
Copy link

@jafern14 jafern14 commented Apr 28, 2020

I was using the latest version and have a use-case where I want to ignore a dynamic range of paths that can change over time. In order to support this I thought that adding RegExp support to the ignoring mechanism might be an option.

Let me know what you think. Thanks for taking a look.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "pino-http",
"version": "5.1.0",
"version": "5.2.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rever this.

shouldLogSuccess = !autoLoggingIgnorePaths.includes(url.pathname)
shouldLogSuccess = !autoLoggingIgnorePaths.find(ignorePath => {
return new RegExp(ignorePath).test(url.pathname)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be generating a new regexp for strings that are anot already regexp.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina let me know if you prefer some other approach. I thought this would work as well and be a bit more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mcollina
Copy link
Member

@jsumners @davidmarkclements wdyt?

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, we initially discussed regexes being used in this path at #77 (comment)

I think the test for if the item is a regex instance is a good compromise. Some sort of find-my-way-like test would probably be better, but that's just a guess.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jsumners
Copy link
Member

ping: @davidmarkclements

@DanHulton
Copy link

Is there any intention of merging this? I can always target a branch to get this functionality, but it would be a lot cleaner to just be able to use the base package.

I'm not clear on the governance of this project, does @davidmarkclements need to +1 it, because it's been many months waiting on him to check in.

Copy link
Member

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@davidmarkclements davidmarkclements merged commit 81482cc into pinojs:master Sep 13, 2020
@davidmarkclements
Copy link
Member

published as 5.3.0 sorry for the delay everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants