module: skb: add more IP fields

[amorenoz]

My only comment is about the semantics/readability of the resulting json. Having:

"skb": { "ip": { "version":  { "v4":  { "tos" : 0 } } } }

seems like a bit ugly and redundant (the version field only has one member that starts with "v").

I don't have a clear alternative though, I've tried adding #[serde(flatten)] to version so the result is:

"skb": { "ip": { "v4":  { "tos" : 0 } } }

which imho is only sligthly better. If we were to handle the json object (or resulting python dict) we would have to test for the existence of a field. This would of course be fixed by a helper function at the Event level.

Another alternative would be to have something like:

"skb": { "ip": { "version": "v4", "version_flags":  { "tos" : 0 } } }

But I'm not 100% convinced either.

Any thoughts?

[atenart]

My only comment is about the semantics/readability of the resulting json. Having:

"skb": { "ip": { "version":  { "v4":  { "tos" : 0 } } } }

seems like a bit ugly and redundant (the version field only has one member that starts with "v").

Agreed.

I don't have a clear alternative though, I've tried adding #[serde(flatten)] to version so the result is:

"skb": { "ip": { "v4":  { "tos" : 0 } } }

which imho is only sligthly better. If we were to handle the json object (or resulting python dict) we would have to test for the existence of a field. This would of course be fixed by a helper function at the Event level.

Something like:

impl SkbIpEvent {
    fn v4 -> Option<SkbIpv4Event> {}
    fn v6 -> Option<SkbIpv6Event> {}
}

?

Another alternative would be to have something like:

"skb": { "ip": { "version": "v4", "version_flags":  { "tos" : 0 } } }

But I'm not 100% convinced either.

Any thoughts?

I'd prefer not to split the version and its specific fields.

[amorenoz]

My only comment is about the semantics/readability of the resulting json. Having:

"skb": { "ip": { "version":  { "v4":  { "tos" : 0 } } } }

seems like a bit ugly and redundant (the version field only has one member that starts with "v").

Agreed.

I don't have a clear alternative though, I've tried adding #[serde(flatten)] to version so the result is:

"skb": { "ip": { "v4":  { "tos" : 0 } } }

which imho is only sligthly better. If we were to handle the json object (or resulting python dict) we would have to test for the existence of a field. This would of course be fixed by a helper function at the Event level.

Something like:

impl SkbIpEvent {
    fn v4 -> Option<SkbIpv4Event> {}
    fn v6 -> Option<SkbIpv6Event> {}
}

?

For example, yes. So you think flattening the version struct is OK?

[atenart]

For example, yes. So you think flattening the version struct is OK?

Yes, I think that's fine, it helps removing one redundant layer.

[atenart]

Flattened the version field of SkbIpEvent as suggested (& tested re-importing events into Retis).

Did not add the v4/v6 helpers for now as it's not 100% clear if we'll need them: it's quite easy to use if let ... or match ... construction and for example I could not convert the two existing users of the version field to this as it was less efficient. So let's wait and see if/when we need such helpers.