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 event handlers don't report removals for Input::add_event() #583

Open
ckreibich opened this issue Sep 17, 2019 · 3 comments

Comments

@ckreibich
Copy link
Contributor

commented Sep 17, 2019

In the Input Framework, Input::add_event() supports events for changes to the data set. According to the docs, "event streams work exactly the same as table streams and support most of the options that are also supported for table streams". Perhaps I've identified a case of "most" here, but it looks like removals from the data set are not flagged via events with the Input::EVENT_REMOVED change type.

Consider the attached script. Run it sniffing e.g. on lo to keep it going. With data.txt in place, it will report addition of the contents of the file, as expected. For updates, no removal event is generated, and an Input::EVENT_NEW is generated for existing entries:

# zeek -i lo ./test-events.zeek
listening on lo

warning: data.txt/Input::READER_ASCII: Init: cannot open data.txt

Now copy data.txt into place:

ev Input::EVENT_NEW [name=hello, num=1]\x0a
ev Input::EVENT_NEW [name=there, num=2]\x0a

Now copy data-small.txt over data.txt:

ev Input::EVENT_NEW [name=hello, num=1]\x0a

This is a bug, no?

data.txt
data-small.txt
test-event.zeek.txt

@jsiwek

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Unqualified opinion: current behavior looks ok, docs may need improvement.

The event-based input stream implies there's no backing state or definition of what "key" to use for the dataset so every re-read has to report each entry as "new". e.g. if you have two entries, [name=hello, num=1] and [name=hello, num=2], is there a "change" or actually an "addition" going on with the data set ? To tell the difference, would need to know if you're expecting a set where the key is (name, num) or a table where the key is just name and storing num as associated values, and that's exactly what you specify with a table-based input stream.

So think the implication is "table-based streams track the state of the data set and so can provide more detailed change/removal information, but event-based streams do not store any state as the data set gets (re)read and can't report changes/removals".

@jsiwek jsiwek added the Area: Input label Sep 18, 2019
@ckreibich

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Yep, if the docs spell it out accordingly, that would be fine with me. Right now I feel the text suggests that it works the same as for the table-driven approach.
It's a pity that the change event handler receives an Input::Event if it's always EVENT_NEW in practice, because it had me thinking that the other types would be supported. Perhaps the thought originally was that we'd support this eventually.

@jsiwek

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Yep, if the docs spell it out accordingly, that would be fine with me

Ok, will leave the issue open as a documentation enhancement in case no one else chimes in to call it out as a bug.

It's a pity that the change event handler receives an Input::Event if it's always EVENT_NEW in practice, because it had me thinking that the other types would be supported.

Yeah, I was also confused by its presence.

Perhaps the thought originally was that we'd support this eventually.

Yeah, or placeholder in case we come up with other event types in the future that make sense, but my thinking was for change/remove events you end up implementing that by way of storing a table of entries, at which point it's the same thing as a table-based input stream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.