BPF<>Rust event reporting and events internal representation

retis-org / retis

Tracing packets in the Linux networking stack & friends

https://retis.readthedocs.io/en/stable/

100 stars 14 forks source link

BPF<>Rust event reporting and events internal representation #53

Closed atenart closed 1 year ago

atenart commented 2 years ago

Following our discussion, I think we kind of agreed on at least the BPF <> Rust event logic (dynamic, variable length, tlv-ish). I implemented a first version to at least validate this would work, and it surprisingly does :)

Things we hadn't settled on and can change:

I went for using per-collector structures to format the event sections instead of only using generic types. Changing this would be quite easy.
Raw events are put in an HashMap<String, String> when unmarshaling.

Events are also currently not propagated after unmarshaling. Also One thing I'm not happy with for now is the naming in the event module.

Based on #51.

atenart commented 2 years ago

Updated the PR with the other event part: internal representation. Also refactored some parts.

atenart commented 1 year ago

Updated the PR taking into account @amorenoz comments, in addition to:

Unit tests for the raw event parsing (moved the logic to its own function).
Add an extra test in the unmarshalers (checking the data size to unmarshal).
Added an interface to retrieve field values from an event.

amorenoz commented 1 year ago

Having concluded that regardless of the decision we make on https://github.com/net-trace/packet-tracer/pull/53#discussion_r1048290385 it won't change the infrastructure significantly, I think the PR is in a good shape.

vlrpl commented 1 year ago

Having concluded that regardless of the decision we make on #53 (comment) it won't change the infrastructure significantly, I think the PR is in a good shape.

Agreed. The discussion was quite interesting, but I have the feeling that a preference will stand out based on the level of flexibility/consistency we'd need. Looked at the PR from a formal point of view as well and LGTM too. Any potential overlook (if any) may perfectly be addressed in subsequent patches, considering this series is needed for multiple reasons. Going to merge it. <-- small correction, just marking it as approved until the conversations are "officially" marked as resolved :) Feel free to merge the series straight away after marking them