enhancement(regex_parser transform): Add RegexSet support to regex #2493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mre
changed the title
|
Very nice! Thanks for your work on this. I'd like to see if we can deprecate the previous |
Good idea! We could print a warning when The |
|
Agree! I say we start with a single-line |
mre
force-pushed
the
regexset
branch
3 times, most recently
from
0f5ee0f
to
3b92e9e
Compare
mre
changed the title
mre
force-pushed
the
regexset
branch
2 times, most recently
from
1891428
to
0fbb770
Compare
|
@binarylogic sounds good! Let me know if the wording should be changed. The CI pipeline is currently failing, but it doesn't look like it's related to the changes in this PR. |
mre
commented
May 4, 2020
src/transforms/regex_parser.rs
Outdated
| warn!( | ||
| "Usage of `regex` is deprecated and will be removed in a future version. \ | ||
| Please upgrade your config to use `regexes` instead: \ | ||
| `regexes = ['{}']`", |
There was a problem hiding this comment.
Shall we add a link to the regex transform documentation here?
lukesteensen
requested changes
May 4, 2020
There was a problem hiding this comment.
This is looking great, thank you @mre! I added notes about a couple of things we'll want to clean up before merging, and a couple more general things that I'm curious about.
|
Found some time to fix the tests. The PR should be good to go now. There's a caveat in the implementation: regexes = [
'(?P<id>\d+)',
'(?P<id>\d+) (?P<time>[\d.]+) (?P<check>\S+)',
]then the first pattern will match. That's because it matches the pattern anywhere in the input. It's expected behavior for regular expressions of course, but in the case of multiple patterns, this can be confusing. (At least I was confused by that, hence the failing test.) The proper solution is to capture the entire line like so: regexes = [
'^(?P<id>\d+)$',
'^(?P<id>\d+) (?P<time>[\d.]+) (?P<check>\S+)$',
]This works as "intended" and the second pattern matches. |
|
@mre I think a little warning in the docs is plenty. :) I don't think an extra option is necessary. |
lukesteensen
approved these changes
May 12, 2020
There was a problem hiding this comment.
New changes look great! Thank you so much for this @mre, we really appreciate it.
If @binarylogic and @Hoverbear agree, I'd like to go ahead with the s/regexes/patterns/ rename, but this seems otherwise just about ready to go.
|
Fine with me! |
…dev#2469) This allows to specify multiple regular expression patterns to be defined that will be matched on the input using `regex::RegexSet`. Signed-off-by: Matthias Endler <[email protected]>
|
Okay renamed @binarylogic is that something you could help me with? |
|
@mre It looks like the problem exists on master as well. Please give us a day or two to fix it. :) |
|
I think that's something we can sort on master, so I'm going to go ahead and merge this as-is. Thanks again @mre! We're super appreciative of the time and effort you've put in here to make Vector better. |
Hoverbear
changed the title
Hoverbear
changed the title
jszwedko
added a commit
that referenced
this pull request
Jul 22, 2020
Previously, was incorrectly mapping the capture indexes of the matched
pattern across the capture groups of all patterns so that, with
something like:
```toml
[sources.in]
type = "stdin"
[transforms.regex]
type = "regex_parser"
inputs = ["in"]
patterns = [
'^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)',
"^notblah (?P<close_while>.+)$",
]
[sinks.out]
inputs = ["regex"]
type = "console"
encoding.codec = "json"
```
And a line of:
```
'notblah something
``
Would end up setting both the `socket_code` and `close_while` fields:
```json
{
"close_while": "something",
"host": "jesse-thinkpad",
"socket_code": "something",
"source_type": "stdin",
"timestamp": "2020-07-22T19:30:12.647060371Z"
}
```
This change simply updates `RegexParser.capture_names` to also be a `Vec` of
the capture information for each pattern similar to `capture_logs` and uses the
same match index later to access it.
A couple of questions that came up while I was looking at this:
It looks like, if multiple patterns match, it simply chooses the first one. Is
this what we want? It was indirectly
discussed in #2493 but a preference
wasn't explicitly stated and it doesn't appear to be documented (in `master`)
for the new `patterns` field. Once we decide what the behavior should be, I can
document it and/or change the implementation if needed. I might have expected
it to apply each matching pattern.
I expected to still see the deprecated `regex` parameter in the, unrelased,
documentation; just marked as deprecated, but it appears to have been dropped
wholesale in
https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41
. I just wanted to clarify if this is the expected the process for deprecating
parameters.
Fixes: #3096
Signed-off-by: Jesse Szwedko <[email protected]>
jszwedko
added a commit
that referenced
this pull request
Jul 24, 2020
Previously, was incorrectly mapping the capture indexes of the matched
pattern across the capture groups of all patterns so that, with
something like:
```toml
[sources.in]
type = "stdin"
[transforms.regex]
type = "regex_parser"
inputs = ["in"]
patterns = [
'^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)',
"^notblah (?P<close_while>.+)$",
]
[sinks.out]
inputs = ["regex"]
type = "console"
encoding.codec = "json"
```
And a line of:
```
'notblah something
``
Would end up setting both the `socket_code` and `close_while` fields:
```json
{
"close_while": "something",
"host": "jesse-thinkpad",
"socket_code": "something",
"source_type": "stdin",
"timestamp": "2020-07-22T19:30:12.647060371Z"
}
```
This change simply updates `RegexParser.capture_names` to also be a `Vec` of
the capture information for each pattern similar to `capture_logs` and uses the
same match index later to access it.
A couple of questions that came up while I was looking at this:
It looks like, if multiple patterns match, it simply chooses the first one. Is
this what we want? It was indirectly
discussed in #2493 but a preference
wasn't explicitly stated and it doesn't appear to be documented (in `master`)
for the new `patterns` field. Once we decide what the behavior should be, I can
document it and/or change the implementation if needed. I might have expected
it to apply each matching pattern.
I expected to still see the deprecated `regex` parameter in the, unrelased,
documentation; just marked as deprecated, but it appears to have been dropped
wholesale in
https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41
. I just wanted to clarify if this is the expected the process for deprecating
parameters.
Fixes: #3096
Signed-off-by: Jesse Szwedko <[email protected]>
jszwedko
added a commit
that referenced
this pull request
Jul 24, 2020
Previously, was incorrectly mapping the capture indexes of the matched
pattern across the capture groups of all patterns so that, with
something like:
```toml
[sources.in]
type = "stdin"
[transforms.regex]
type = "regex_parser"
inputs = ["in"]
patterns = [
'^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)',
"^notblah (?P<close_while>.+)$",
]
[sinks.out]
inputs = ["regex"]
type = "console"
encoding.codec = "json"
```
And a line of:
```
'notblah something
``
Would end up setting both the `socket_code` and `close_while` fields:
```json
{
"close_while": "something",
"host": "jesse-thinkpad",
"socket_code": "something",
"source_type": "stdin",
"timestamp": "2020-07-22T19:30:12.647060371Z"
}
```
This change simply updates `RegexParser.capture_names` to also be a `Vec` of
the capture information for each pattern similar to `capture_logs` and uses the
same match index later to access it.
A couple of questions that came up while I was looking at this:
It looks like, if multiple patterns match, it simply chooses the first one. Is
this what we want? It was indirectly
discussed in #2493 but a preference
wasn't explicitly stated and it doesn't appear to be documented (in `master`)
for the new `patterns` field. Once we decide what the behavior should be, I can
document it and/or change the implementation if needed. I might have expected
it to apply each matching pattern.
I expected to still see the deprecated `regex` parameter in the, unrelased,
documentation; just marked as deprecated, but it appears to have been dropped
wholesale in
https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41
. I just wanted to clarify if this is the expected the process for deprecating
parameters.
Fixes: #3096
Signed-off-by: Jesse Szwedko <[email protected]>
jszwedko
added a commit
that referenced
this pull request
Jul 28, 2020
…3164) * fix(regex_parser transform): Correctly assign capture group fields Previously, was incorrectly mapping the capture indexes of the matched pattern across the capture groups of all patterns so that, with something like: ```toml [sources.in] type = "stdin" [transforms.regex] type = "regex_parser" inputs = ["in"] patterns = [ '^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)', "^notblah (?P<close_while>.+)$", ] [sinks.out] inputs = ["regex"] type = "console" encoding.codec = "json" ``` And a line of: ``` 'notblah something `` Would end up setting both the `socket_code` and `close_while` fields: ```json { "close_while": "something", "host": "jesse-thinkpad", "socket_code": "something", "source_type": "stdin", "timestamp": "2020-07-22T19:30:12.647060371Z" } ``` This change simply updates `RegexParser.capture_names` to also be a `Vec` of the capture information for each pattern similar to `capture_logs` and uses the same match index later to access it. A couple of questions that came up while I was looking at this: It looks like, if multiple patterns match, it simply chooses the first one. Is this what we want? It was indirectly discussed in #2493 but a preference wasn't explicitly stated and it doesn't appear to be documented (in `master`) for the new `patterns` field. Once we decide what the behavior should be, I can document it and/or change the implementation if needed. I might have expected it to apply each matching pattern. I expected to still see the deprecated `regex` parameter in the, unrelased, documentation; just marked as deprecated, but it appears to have been dropped wholesale in https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41 . I just wanted to clarify if this is the expected the process for deprecating parameters. Fixes: #3096
fanatid
pushed a commit
that referenced
this pull request
Jul 28, 2020
…3164) * fix(regex_parser transform): Correctly assign capture group fields Previously, was incorrectly mapping the capture indexes of the matched pattern across the capture groups of all patterns so that, with something like: ```toml [sources.in] type = "stdin" [transforms.regex] type = "regex_parser" inputs = ["in"] patterns = [ '^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)', "^notblah (?P<close_while>.+)$", ] [sinks.out] inputs = ["regex"] type = "console" encoding.codec = "json" ``` And a line of: ``` 'notblah something `` Would end up setting both the `socket_code` and `close_while` fields: ```json { "close_while": "something", "host": "jesse-thinkpad", "socket_code": "something", "source_type": "stdin", "timestamp": "2020-07-22T19:30:12.647060371Z" } ``` This change simply updates `RegexParser.capture_names` to also be a `Vec` of the capture information for each pattern similar to `capture_logs` and uses the same match index later to access it. A couple of questions that came up while I was looking at this: It looks like, if multiple patterns match, it simply chooses the first one. Is this what we want? It was indirectly discussed in #2493 but a preference wasn't explicitly stated and it doesn't appear to be documented (in `master`) for the new `patterns` field. Once we decide what the behavior should be, I can document it and/or change the implementation if needed. I might have expected it to apply each matching pattern. I expected to still see the deprecated `regex` parameter in the, unrelased, documentation; just marked as deprecated, but it appears to have been dropped wholesale in https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41 . I just wanted to clarify if this is the expected the process for deprecating parameters. Fixes: #3096
mengesb
pushed a commit
to jacobbraaten/vector
that referenced
this pull request
Dec 9, 2020
…ectordotdev#2493) Signed-off-by: Brian Menges <[email protected]>
mengesb
pushed a commit
to jacobbraaten/vector
that referenced
this pull request
Dec 9, 2020
…ectordotdev#3164) * fix(regex_parser transform): Correctly assign capture group fields Previously, was incorrectly mapping the capture indexes of the matched pattern across the capture groups of all patterns so that, with something like: ```toml [sources.in] type = "stdin" [transforms.regex] type = "regex_parser" inputs = ["in"] patterns = [ '^blah \((?P<socket_code>[0-9]+): (?:[^\)]+)\) while (?P<timeout_while>.)', "^notblah (?P<close_while>.+)$", ] [sinks.out] inputs = ["regex"] type = "console" encoding.codec = "json" ``` And a line of: ``` 'notblah something `` Would end up setting both the `socket_code` and `close_while` fields: ```json { "close_while": "something", "host": "jesse-thinkpad", "socket_code": "something", "source_type": "stdin", "timestamp": "2020-07-22T19:30:12.647060371Z" } ``` This change simply updates `RegexParser.capture_names` to also be a `Vec` of the capture information for each pattern similar to `capture_logs` and uses the same match index later to access it. A couple of questions that came up while I was looking at this: It looks like, if multiple patterns match, it simply chooses the first one. Is this what we want? It was indirectly discussed in vectordotdev#2493 but a preference wasn't explicitly stated and it doesn't appear to be documented (in `master`) for the new `patterns` field. Once we decide what the behavior should be, I can document it and/or change the implementation if needed. I might have expected it to apply each matching pattern. I expected to still see the deprecated `regex` parameter in the, unrelased, documentation; just marked as deprecated, but it appears to have been dropped wholesale in https://github.com/timberio/vector/pull/2493/files#diff-4d642800436bfa506ff51f7b75556d9dL41 . I just wanted to clarify if this is the expected the process for deprecating parameters. Fixes: vectordotdev#3096 Signed-off-by: Brian Menges <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows to specify multiple regular expressions to be defined
that will be matched on the input using
regex::RegexSet.Fixes #2469.
Note that this is a breaking change, as it requires all configs to be rewritten
to use
regexes = ["..."]instead ofregex = "...". Alternatively, we couldsupport both fields in the config; although I think this would lead to confusion down
the road.
Design decisions
regexes, whilethe actual
RegexSetis calledregexsetin the transform. The reason for keeping both is thatRegexSet::matches returns the set of regular expressions that match in the given text.
The set returned contains the index of each regular expression that matches in the given text.
The index is in correspondence with the order of regular expressions given to
RegexSet's constructor, which matches with the index in the vector.
The reasoning behind this was that types with the same name will probably be
used similarly further down the pipeline, no matter which pattern was matching.
Also, splitting up the coercions for each pattern would likely cause additional duplication
and make the configuration syntax harder to understand for beginners.
Open questions
clone(). Was considering to store referencesto the regular expressions inside the vector instead of the expressions themselves,
but I was hoping to avoid lifetimes to keep the code more maintainable. Maybe it's a trade-off,
worth considering, though - or there is a better way.
As discussed with @Hoverbear and @lukesteensen.