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

Change the filter API to avoid negative lookahead #188

Closed
colmsnowplow opened this issue Aug 8, 2022 · 2 comments
Closed

Change the filter API to avoid negative lookahead #188

colmsnowplow opened this issue Aug 8, 2022 · 2 comments

Comments

@colmsnowplow
Copy link
Collaborator

When we refactored the filter config for v1 release, we arrived upon the decision that using regex removes lots of complexity, since it does the work of handing matching logic for us.

Since the native go regex package doesn't support negative lookaheads, we decided to use regexp2, which does.

In hindsight, I can see now that this leads to two slightly problematic consequences:

  1. Negative filters are actually hard to configure - configuration of a negative lookahead in regex isn't intuitive, and is hard to test. Since we expect the user to configure this, and we deploy to non-technical users sometimes, it's slightly more to ask of them than I'd like it to be.
  2. There's a performance penalty involved - negative lookaheads have a high performance cost, but regexp2 is also slightly slower even for simple patterns.

The solution: We add a configuration option along the lines of DiscardOnMatch, which defaults to false. All it does is shouldKeepMessage = !shouldKeepMessage. Then we just use the built in regexp package, and we disallow negaitve lookaheads in configuration.

I don't believe this leads to much additional complexity in the code, and it solves both of the above problems.

@colmsnowplow
Copy link
Collaborator Author

While we're at it, we should reconsider the field names in the transformation config - let's try to make them as declarative as possible.

@TiganeteaRobert
Copy link
Contributor

I agree with your point here, I've personally lost a lot of time to figure out negative lookaheads so for a non-technical user it could be a nightmare. A configuration option sounds like the best solution to me and should be fairly easy to implement and document.

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

No branches or pull requests

2 participants