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.

Sign up for GitHub

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

Jump to bottom

enhancement(regex_parser transform): Add support for target_field #2023

Merged
merged 11 commits into from
Apr 1, 2020
Merged

enhancement(regex_parser transform): Add support for target_field #2023

merged 11 commits into from
Apr 1, 2020

Conversation

bruceg
Copy link
Member

@bruceg bruceg commented Mar 10, 2020

Closes #1812

@bruceg bruceg added type: enhancement A value-adding code change that enhances its existing functionality. transform: regex_parser labels Mar 10, 2020
@bruceg bruceg requested a review from LucioFranco March 10, 2020 18:49
Hoverbear
Hoverbear reviewed Mar 10, 2020
View reviewed changes
src/transforms/regex_parser.rs Outdated
#[serde(default, deny_unknown_fields)]
pub struct RegexParserConfig {
pub regex: String,
pub field: Option<Atom>,
#[derivative(Default(value = "false"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[derivative(Default(value = "false"))]
#[derivative(Default(value = "true"))]

Copy link
Member Author

Choose a reason for hiding this comment

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

This follows the lead of other parser transforms like json_parser that default to false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update https://github.com/timberio/vector/blob/8c252a2c32fda4317284be46b70cf2e64e13d651/.meta/transforms/regex_parser.toml#L18 then! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be the default for drop_field, which is a different setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but in your branch:

image

image
In master:

image

https://github.com/timberio/vector/blob/9cd0ba6fca6b577c4c76956fc550496f5031175f/.meta/transforms/regex_parser.toml#L18

https://github.com/timberio/vector/blob/9cd0ba6fca6b577c4c76956fc550496f5031175f/src/transforms/regex_parser.rs#L30

Copy link
Member Author

@bruceg bruceg Mar 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see it now, sorry. That was part of a lead up patch that I had forgotten about. Will fix. Odd that no test caught it, though.

@binarylogic
Copy link
Contributor

Nice work. Everything else, docs wise, looks good to me.

@bruceg bruceg removed the request for review from LucioFranco March 10, 2020 22:27
@bruceg bruceg self-assigned this Mar 10, 2020
lukesteensen
lukesteensen reviewed Mar 11, 2020
View reviewed changes
Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

I'm curious how people feel about the overwrite_target option. I'm not sure it's useful enough to warrant the additional implementation complexity, but there may be use cases I'm not thinking about.

(I could have sworn we talked about this somewhere but now I can't find it)

@binarylogic
Copy link
Contributor

I'm indifferent, but I tend to agree it adds confusion to the UX as well. My opinion is not strong.

Fix default value for drop_field
0ca23e9 
Signed-off-by: Bruce Guenter <[email protected]>
@bruceg bruceg changed the title feat(regex_parser transform): Add support for target_field enhancement(regex_parser transform): Add support for target_field Mar 11, 2020
@bruceg
Copy link
Member Author

bruceg commented Mar 16, 2020

So where are we with overwrite_target? Should I leave it the same as json_parser, make it default to true, or drop it entirely?

@binarylogic
Copy link
Contributor

Leave it and default it to true. It's fine as an advanced option. We should follow up by changing the default to true in the json_parser as well.

binarylogic
binarylogic approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@binarylogic binarylogic left a comment

Choose a reason for hiding this comment

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

Looks good!

@binarylogic
Copy link
Contributor

Merging so that we can get this in 0.9.0.

@binarylogic binarylogic merged commit ddd5478 into master Apr 1, 2020
@binarylogic binarylogic deleted the regex-parser-target branch April 1, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukesteensen lukesteensen lukesteensen left review comments

@binarylogic binarylogic binarylogic approved these changes

@Hoverbear Hoverbear Hoverbear approved these changes

Assignees

@bruceg bruceg

Labels
type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

regex_parser transform doesn't work with nested fields
4 participants
@bruceg @binarylogic @Hoverbear @lukesteensen