-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
There was a problem hiding this 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.
a5448ba
to
f95cb0c
Compare
b78ca4b
to
8cea7c2
Compare
f95cb0c
to
c8930d9
Compare
8cea7c2
to
dc0c2e4
Compare
991e9d1
to
9ff142e
Compare
src/transforms/remap.rs
Outdated
return; | ||
} | ||
|
||
output.push(event) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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: |
a5ead8a
to
6417766
Compare
1531931
to
182f038
Compare
6417766
to
911958d
Compare
182f038
to
b3b49fd
Compare
b3b49fd
to
7198885
Compare
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>
49ec811
to
d64082e
Compare
Dropping on error was previously possible, but undocumented.
This PR also adds an option to
drop_on_abort
, which defaults totrue
, and will instruct Vector to drop events when the program is manually aborted through theabort
statement.closes #6720