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

feat(remap): support dropping events when remap fails or is aborted #6722

Merged
merged 9 commits into from Mar 31, 2021

Conversation

JeanMertz
Copy link
Contributor

@JeanMertz JeanMertz commented Mar 11, 2021

Dropping on error was previously possible, but undocumented.

This PR also adds an option to drop_on_abort, which defaults to true, and will instruct Vector to drop events when the program is manually aborted through the abort statement.

closes #6720

@JeanMertz JeanMertz requested review from a team, pablosichert and StephenWakely and removed request for a team March 11, 2021 15:23
@JeanMertz JeanMertz self-assigned this Mar 11, 2021
@JeanMertz JeanMertz added the domain: vrl Anything related to the Vector Remap Language label Mar 11, 2021
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

I like it. I think you need to update the benches to use the new struct field name.

@JeanMertz JeanMertz requested a review from a team March 12, 2021 12:28
@JeanMertz JeanMertz changed the base branch from master to jean/vrl-abort March 12, 2021 12:32
@JeanMertz JeanMertz changed the title feat(remap): support dropping events when remap fails feat(remap): support dropping events when remap fails or is aborted Mar 12, 2021
@JeanMertz JeanMertz assigned StephenWakely and unassigned JeanMertz Mar 12, 2021
return;
}

output.push(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this actually be pushing the original event? In other words, do we want an abort to abandon any changes made in the script, or should it keep any changes made up to the point abort was called (assuming drop_on_abort is false)?

If we do, I notice that the abort function as actually marked as infallible. Presumable, this would mean either we would have to abandon the check above and clone even if the script is infallible, or we need to mark abort as fallible.

Copy link
Member

Choose a reason for hiding this comment

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

This is a good question! My inkling would be to have abort not persist any modifications.

Maybe @binarylogic has an opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was it should. But I’ll defer to your decision

Copy link
Contributor

Choose a reason for hiding this comment

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

If it does keep the event, then it isn't really doing much more that an if statement.

Copy link
Member

@jszwedko jszwedko Mar 12, 2021

Choose a reason for hiding this comment

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

abort says to me that the entire script should be aborted and have similar similar effects as if there were an error: namely that any mutations are discarded.

If we called it return on the other hand, I could see preserving the modifications.

I do agree that an abort that preserves modifications is not significantly different from an if statement wrapping the parts of the script that the user would not like to be executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

The term "abort" feels like it should abort the script and not persist changes. Alternatively, something like "emit" (like we use in Lua) might be better served for preserving changes and exiting early.

src/transforms/remap.rs Outdated Show resolved Hide resolved
@jszwedko
Copy link
Member

Noting I mentioned this PR to Ben and he had the good question of whether we need two options for dropping events separately, or if we should just have one: drop_on_abort that applies to both errors and aborts.

JeanMertz and others added 9 commits March 31, 2021 09:47
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Stephen Wakely <fungus.humungus@gmail.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
Signed-off-by: Jean Mertz <git@jeanmertz.com>
@JeanMertz JeanMertz merged commit 6a35516 into master Mar 31, 2021
@JeanMertz JeanMertz deleted the jean/drop-on-err branch March 31, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: vrl Anything related to the Vector Remap Language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to drop events in the remap transform
5 participants