Closed amorenoz closed 1 year ago
Currently on top of #133
Overall lots of great additions, and except one suggestion(below) regarding the tracking info, I think it's a nice PR and I didn't have lots of important inlined comments.
About the tracking information: the current logic is to add an extra global tracking event section which merges information from different sections, including the skb tracking one. This means the events end up with multiple tracking related sections and either all of them are printed which can be a bit confusing (and takes space) or we'll have to exclude some sections at the print level.
I'm thinking we could rework the skb-tracking section to be a generic tracking one (defined in the core), which could look like:
enum TrackingEvent { Id(u128), Raw(Vec<Box<dyn EventSection>>), } // or struct TrackingEvent { id: Option<u128>, raw: Vec<Box<dyn EventSection>>, }
Factories adding tracking information (currently
SkbTrackingFactory
[1]) could push to theTrackingEvent::raw
part and the sort command could set theTrackingEvent::id
field.Then we can implement the
EventFmt
trait so that ifid
is set it is printed, otherwise we print all raw tracking sections (ordered).Following this, I would also suggest to move
AddTracking
tocore::tracking
.Note that I can help in implementing this
TrackingEvent
or even do this as part of a following up series, if that is easier for you.WDYT?
[1] Maybe one could be set by OvS, but I'm not 100% sure if it should be there or kept in the OvS section.
Totally agree. In fact I was planning to do that exactly but it didn't look small so I decided to post this half-cooked version. Ideally we should have a single tracking section, however, currently each EventFactory
defines the EventSection
and there is one per ModuleId
, so merging both OvS and skb tracking info into a single section is not very clean. We would have to open up the APIs a bit.
A possible short-term approach could be to have all TrackingInfo
, SkbTrackingEvent
and OvsEvent
not print their tracking information (at least in the default format) and have a global helper Event::tracking_id()
that inspects the available sections and prints whatever is reasonable in each case.
Another possible approach (not that short-term I guess) would be to create a tracking info API internally so EventSections can report them as they decode data and a global entity can combine them.
Actually, following the line of "move tracking to the core...", what if:
struct Event {
tracking_id: u128,
sections: HashMap<ModuleId, Box<dyn EventSection>>,
}
impl Event {
fn tracking_id() -> Result<u128> {
// Figure out what sections are available and compute the tracking id out of them.
}
fn event_fmt(..) -> Result<()>{
// Also squash all tracking information into a single string
}
}
Totally agree. In fact I was planning to do that exactly but it didn't look small so I decided to post this half-cooked version.
I think that's fine and the unique tracking section could come as a follow up, to ease the merge of this big PR.
Ideally we should have a single tracking section, however, currently each
EventFactory
defines theEventSection
and there is one perModuleId
, so merging both OvS and skb tracking info into a single section is not very clean. We would have to open up the APIs a bit.
Or have a TrackingSection
the same way we have some core sections. We then already have an helper to retrieve a specific section (get_section
) and this TrackingSection
could offer some helpers to add EventSections
to it. We might need to have a list of tracking section to index them, but not much more.
A possible short-term approach could be to have all
TrackingInfo
,SkbTrackingEvent
andOvsEvent
not print their tracking information (at least in the default format) and have a global helperEvent::tracking_id()
that inspects the available sections and prints whatever is reasonable in each case.
If possible I'd like to avoid having runtime checks in the Event
about its sections. Same as the formatting comment, we should treat sections all the same and let them decide what to do.
Latest version changelog:
clap::Command
-to-retis::SubCommand
conversionsort
write events series to a file.Aparently BTreeMap::pop_first
is unstable in Ubuntu Jammy, rewrote it using another (much uglier) way.
This PR adds the basic post-processing infrastructure:
src/process
src/process/cli
Before doing so, it also reworks cli Subcommands a bit so that they require less boilerplate.
It adds a processing helper that is able to inject a new EventSection called TrackingInfo with the combined (skb+ovs) tracking information.
Finally it adds two processing commands:
Both events print json because I've intentionally left all formatting off this PR. This does not make a lot of sense right now but a follow up fix when formatting code is merged can print them in a nicer format.